Hi Philipp,
Thank you for the patch.
On Tuesday 03 June 2014 11:35:55 Philipp Zabel wrote:
> This switches all register accesses to use regmap. It allows to
> use the regmap cache, tracing, and debug register dump facilities,
> and removes the need to open code read-modify-writes.
>
> Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
This looks good to me, but I have two small questions:
- How does regmap handle endianness ? It seems to hardcode a big endian byte
order, which is fortunately what we need here. I suppose you've successfully
tested this patch :-)
- How does regmap handle the register cache ? Will it try to populate it when
initialized, or will it only read registers on demand due to a read or an
update bits operation ?
> ---
> This patch was not included before.
> ---
> drivers/media/i2c/Kconfig | 1 +
> drivers/media/i2c/mt9v032.c | 112 +++++++++++++++++------------------------
> 2 files changed, 46 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 441053b..f40b4cf 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -551,6 +551,7 @@ config VIDEO_MT9V032
> tristate "Micron MT9V032 sensor support"
> depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> depends on MEDIA_CAMERA_SUPPORT
> + select REGMAP_I2C
> ---help---
> This is a Video4Linux2 sensor-level driver for the Micron
> MT9V032 752x480 CMOS sensor.
> diff --git a/drivers/media/i2c/mt9v032.c b/drivers/media/i2c/mt9v032.c
> index cb7c6df..e756d50 100644
> --- a/drivers/media/i2c/mt9v032.c
> +++ b/drivers/media/i2c/mt9v032.c
> @@ -17,6 +17,7 @@
> #include <linux/i2c.h>
> #include <linux/log2.h>
> #include <linux/mutex.h>
> +#include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/videodev2.h>
> #include <linux/v4l2-mediabus.h>
> @@ -245,6 +246,7 @@ struct mt9v032 {
> struct mutex power_lock;
> int power_count;
>
> + struct regmap *regmap;
> struct clk *clk;
>
> struct mt9v032_platform_data *pdata;
> @@ -252,7 +254,6 @@ struct mt9v032 {
> const struct mt9v032_model_version *version;
>
> u32 sysclk;
> - u16 chip_control;
> u16 aec_agc;
> u16 hblank;
> struct {
> @@ -266,40 +267,10 @@ static struct mt9v032 *to_mt9v032(struct v4l2_subdev
> *sd) return container_of(sd, struct mt9v032, subdev);
> }
>
> -static int mt9v032_read(struct i2c_client *client, const u8 reg)
> -{
> - s32 data = i2c_smbus_read_word_swapped(client, reg);
> - dev_dbg(&client->dev, "%s: read 0x%04x from 0x%02x\n", __func__,
> - data, reg);
> - return data;
> -}
> -
> -static int mt9v032_write(struct i2c_client *client, const u8 reg,
> - const u16 data)
> -{
> - dev_dbg(&client->dev, "%s: writing 0x%04x to 0x%02x\n", __func__,
> - data, reg);
> - return i2c_smbus_write_word_swapped(client, reg, data);
> -}
> -
> -static int mt9v032_set_chip_control(struct mt9v032 *mt9v032, u16 clear, u16
> set) -{
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> - u16 value = (mt9v032->chip_control & ~clear) | set;
> - int ret;
> -
> - ret = mt9v032_write(client, MT9V032_CHIP_CONTROL, value);
> - if (ret < 0)
> - return ret;
> -
> - mt9v032->chip_control = value;
> - return 0;
> -}
> -
> static int
> mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16 which, int enable)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> + struct regmap *map = mt9v032->regmap;
> u16 value = mt9v032->aec_agc;
> int ret;
>
> @@ -308,7 +279,7 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> which, int enable) else
> value &= ~which;
>
> - ret = mt9v032_write(client, MT9V032_AEC_AGC_ENABLE, value);
> + ret = regmap_write(map, MT9V032_AEC_AGC_ENABLE, value);
> if (ret < 0)
> return ret;
>
> @@ -319,7 +290,6 @@ mt9v032_update_aec_agc(struct mt9v032 *mt9v032, u16
> which, int enable) static int
> mt9v032_update_hblank(struct mt9v032 *mt9v032)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> struct v4l2_rect *crop = &mt9v032->crop;
> unsigned int min_hblank = mt9v032->model->data->min_hblank;
> unsigned int hblank;
> @@ -330,12 +300,13 @@ mt9v032_update_hblank(struct mt9v032 *mt9v032)
> min_hblank);
> hblank = max_t(unsigned int, mt9v032->hblank, min_hblank);
>
> - return mt9v032_write(client, MT9V032_HORIZONTAL_BLANKING, hblank);
> + return regmap_write(mt9v032->regmap, MT9V032_HORIZONTAL_BLANKING,
> + hblank);
> }
>
> static int mt9v032_power_on(struct mt9v032 *mt9v032)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> + struct regmap *map = mt9v032->regmap;
> unsigned long rate;
> int ret;
>
> @@ -350,7 +321,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
> udelay(1);
>
> /* Reset the chip and stop data read out */
> - ret = mt9v032_write(client, MT9V032_RESET, 1);
> + ret = regmap_write(map, MT9V032_RESET, 1);
> if (ret < 0)
> return ret;
>
> @@ -358,7 +329,7 @@ static int mt9v032_power_on(struct mt9v032 *mt9v032)
> rate = clk_get_rate(mt9v032->clk);
> ndelay(DIV_ROUND_UP(15 * 125000000, rate >> 3));
>
> - return mt9v032_write(client, MT9V032_CHIP_CONTROL, 0);
> + return regmap_write(map, MT9V032_CHIP_CONTROL, 0);
> }
>
> static void mt9v032_power_off(struct mt9v032 *mt9v032)
> @@ -368,7 +339,7 @@ static void mt9v032_power_off(struct mt9v032 *mt9v032)
>
> static int __mt9v032_set_power(struct mt9v032 *mt9v032, bool on)
> {
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> + struct regmap *map = mt9v032->regmap;
> int ret;
>
> if (!on) {
> @@ -382,14 +353,14 @@ static int __mt9v032_set_power(struct mt9v032
> *mt9v032, bool on)
>
> /* Configure the pixel clock polarity */
> if (mt9v032->pdata && mt9v032->pdata->clk_pol) {
> - ret = mt9v032_write(client, mt9v032->model->data->pclk_reg,
> + ret = regmap_write(map, mt9v032->model->data->pclk_reg,
> MT9V032_PIXEL_CLOCK_INV_PXL_CLK);
> if (ret < 0)
> return ret;
> }
>
> /* Disable the noise correction algorithm and restore the controls. */
> - ret = mt9v032_write(client, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
> + ret = regmap_write(map, MT9V032_ROW_NOISE_CORR_CONTROL, 0);
> if (ret < 0)
> return ret;
>
> @@ -433,43 +404,39 @@ static int mt9v032_s_stream(struct v4l2_subdev
> *subdev, int enable) const u16 mode = MT9V032_CHIP_CONTROL_MASTER_MODE
>
> | MT9V032_CHIP_CONTROL_DOUT_ENABLE
> | MT9V032_CHIP_CONTROL_SEQUENTIAL;
>
> - struct i2c_client *client = v4l2_get_subdevdata(subdev);
> struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> struct v4l2_rect *crop = &mt9v032->crop;
> - unsigned int read_mode;
> + struct regmap *map = mt9v032->regmap;
> unsigned int hbin;
> unsigned int vbin;
> int ret;
>
> if (!enable)
> - return mt9v032_set_chip_control(mt9v032, mode, 0);
> + return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, 0);
>
> /* Configure the window size and row/column bin */
> hbin = fls(mt9v032->hratio) - 1;
> vbin = fls(mt9v032->vratio) - 1;
> - read_mode = mt9v032_read(client, MT9V032_READ_MODE);
> - if (read_mode < 0)
> - return read_mode;
> - read_mode &= MT9V032_READ_MODE_RESERVED;
> - read_mode |= hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> - vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT;
> - ret = mt9v032_write(client, MT9V032_READ_MODE, read_mode);
> + ret = regmap_update_bits(map, MT9V032_READ_MODE,
> + ~MT9V032_READ_MODE_RESERVED,
> + hbin << MT9V032_READ_MODE_COLUMN_BIN_SHIFT |
> + vbin << MT9V032_READ_MODE_ROW_BIN_SHIFT);
> if (ret < 0)
> return ret;
>
> - ret = mt9v032_write(client, MT9V032_COLUMN_START, crop->left);
> + ret = regmap_write(map, MT9V032_COLUMN_START, crop->left);
> if (ret < 0)
> return ret;
>
> - ret = mt9v032_write(client, MT9V032_ROW_START, crop->top);
> + ret = regmap_write(map, MT9V032_ROW_START, crop->top);
> if (ret < 0)
> return ret;
>
> - ret = mt9v032_write(client, MT9V032_WINDOW_WIDTH, crop->width);
> + ret = regmap_write(map, MT9V032_WINDOW_WIDTH, crop->width);
> if (ret < 0)
> return ret;
>
> - ret = mt9v032_write(client, MT9V032_WINDOW_HEIGHT, crop->height);
> + ret = regmap_write(map, MT9V032_WINDOW_HEIGHT, crop->height);
> if (ret < 0)
> return ret;
>
> @@ -478,7 +445,7 @@ static int mt9v032_s_stream(struct v4l2_subdev *subdev,
> int enable) return ret;
>
> /* Switch to master "normal" mode */
> - return mt9v032_set_chip_control(mt9v032, 0, mode);
> + return regmap_update_bits(map, MT9V032_CHIP_CONTROL, mode, mode);
> }
>
> static int mt9v032_enum_mbus_code(struct v4l2_subdev *subdev,
> @@ -660,7 +627,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct mt9v032 *mt9v032 =
> container_of(ctrl->handler, struct mt9v032, ctrls);
> - struct i2c_client *client = v4l2_get_subdevdata(&mt9v032->subdev);
> + struct regmap *map = mt9v032->regmap;
> u32 freq;
> u16 data;
>
> @@ -670,23 +637,23 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
> ctrl->val);
>
> case V4L2_CID_GAIN:
> - return mt9v032_write(client, MT9V032_ANALOG_GAIN, ctrl->val);
> + return regmap_write(map, MT9V032_ANALOG_GAIN, ctrl->val);
>
> case V4L2_CID_EXPOSURE_AUTO:
> return mt9v032_update_aec_agc(mt9v032, MT9V032_AEC_ENABLE,
> !ctrl->val);
>
> case V4L2_CID_EXPOSURE:
> - return mt9v032_write(client, MT9V032_TOTAL_SHUTTER_WIDTH,
> - ctrl->val);
> + return regmap_write(map, MT9V032_TOTAL_SHUTTER_WIDTH,
> + ctrl->val);
>
> case V4L2_CID_HBLANK:
> mt9v032->hblank = ctrl->val;
> return mt9v032_update_hblank(mt9v032);
>
> case V4L2_CID_VBLANK:
> - return mt9v032_write(client, MT9V032_VERTICAL_BLANKING,
> - ctrl->val);
> + return regmap_write(map, MT9V032_VERTICAL_BLANKING,
> + ctrl->val);
>
> case V4L2_CID_PIXEL_RATE:
> case V4L2_CID_LINK_FREQ:
> @@ -723,7 +690,7 @@ static int mt9v032_s_ctrl(struct v4l2_ctrl *ctrl)
>
> | MT9V032_TEST_PATTERN_FLIP;
>
> break;
> }
> - return mt9v032_write(client, MT9V032_TEST_PATTERN, data);
> + return regmap_write(map, MT9V032_TEST_PATTERN, data);
> }
>
> return 0;
> @@ -791,7 +758,7 @@ static int mt9v032_registered(struct v4l2_subdev
> *subdev) struct i2c_client *client = v4l2_get_subdevdata(subdev);
> struct mt9v032 *mt9v032 = to_mt9v032(subdev);
> unsigned int i;
> - s32 version;
> + u32 version;
> int ret;
>
> dev_info(&client->dev, "Probing MT9V032 at address 0x%02x\n",
> @@ -804,10 +771,10 @@ static int mt9v032_registered(struct v4l2_subdev
> *subdev) }
>
> /* Read and check the sensor version */
> - version = mt9v032_read(client, MT9V032_CHIP_VERSION);
> - if (version < 0) {
> + ret = regmap_read(mt9v032->regmap, MT9V032_CHIP_VERSION, &version);
> + if (ret < 0) {
> dev_err(&client->dev, "Failed reading chip version\n");
> - return version;
> + return ret;
> }
>
> for (i = 0; i < ARRAY_SIZE(mt9v032_versions); ++i) {
> @@ -894,6 +861,13 @@ static const struct v4l2_subdev_internal_ops
> mt9v032_subdev_internal_ops = { .close = mt9v032_close,
> };
>
> +static const struct regmap_config mt9v032_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .max_register = 0xff,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> /*
> ---------------------------------------------------------------------------
> -- * Driver initialization and probing
> */
> @@ -917,6 +891,10 @@ static int mt9v032_probe(struct i2c_client *client,
> if (!mt9v032)
> return -ENOMEM;
>
> + mt9v032->regmap = devm_regmap_init_i2c(client, &mt9v032_regmap_config);
> + if (IS_ERR(mt9v032->regmap))
> + return PTR_ERR(mt9v032->regmap);
> +
> mt9v032->clk = devm_clk_get(&client->dev, NULL);
> if (IS_ERR(mt9v032->clk))
> return PTR_ERR(mt9v032->clk);
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
