Re: [PATCH 2/3] media: atomisp: ov2680: Convert to new CCI register access helpers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Wed, Jun 07, 2023 at 10:53:54AM +0200, Hans de Goede wrote:
> On 6/6/23 22:53, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2023 at 7:58 PM Hans de Goede wrote:
> >>
> >> Use the new comon CCI register access helpers to replace the private
> >> register access helpers in the ov2680 driver.
> >>
> >> While at it also switch to using the same register address defines
> >> as the standard drivers/media/i2c/ov2680.c driver to make merging
> >> the 2 drivers simpler.
> > 
> > ...
> > 
> >> +       cci_write(sensor->regmap, OV2680_REG_SENSOR_CTRL_0A, sensor_ctrl_0a, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_START, sensor->mode.h_start, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_VERTICAL_START, sensor->mode.v_start, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_END, sensor->mode.h_end, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_VERTICAL_END, sensor->mode.v_end, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_HORIZONTAL_OUTPUT_SIZE,
> >> +                 sensor->mode.h_output_size, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_VERTICAL_OUTPUT_SIZE,
> >> +                 sensor->mode.v_output_size, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_TIMING_HTS, sensor->mode.hts, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_TIMING_VTS, sensor->mode.vts, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_ISP_X_WIN, 0, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_ISP_Y_WIN, 0, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_X_INC, inc, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_Y_INC, inc, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_X_WIN, sensor->mode.h_output_size, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_Y_WIN, sensor->mode.v_output_size, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_FORMAT1, fmt1, &ret);
> >> +       cci_write(sensor->regmap, OV2680_REG_FORMAT2, fmt2, &ret);
> > 
> > I know that &ret thingy was discussed before and Laurent is keen to
> > have this, but has anybody actually tested how bad or not at all the
> > code generation becomes?
> 
> The cci_write function is in another module, so it won't be inlined
> and as such I don't see how the code generation can become bad. We
> loose all the if (ret) return ret; checks here, so the code should
> become smaller.
> 
> Or are you worried about having to pass the 1 extra parameter ?
> 
> > ...
> > 
> >> +       struct device *dev;
> >> +       struct regmap *regmap;
> > 
> > Isn't the same device associated with regmap? If so, one of them
> > probably duplicates the other.
> 
> You are right, but the entire atomisp-ov2680.c file is going away real
> soon now. I plan to post a series to get drivers/media/i2c/ov2680.c
> ready to replace it later today.
> 
> So I'm not even sure if this patch should be merged, as I mentioned in
> the cover letter this one is mostly here to illustrate use of the new
> helpers.

How about porting drivers/media/i2c/imx290.c ? That's a real-life
example that can be merged, which is good to serve as an example
showcasing the API usage in mainline. It will also help ensuring that
these helpers are a good fit for drivers that already encode the
register width in the macros.

> I also wrote this patch to make porting recent atomisp-ov2680.c
> changes over to drivers/media/i2c/ov2680.c easier. Part of the series
> to get drivers/media/i2c/ov2680.c into shape is converting it to the
> new CCI helpers so that I could then easily copy over bits from the
> also converted atomisp-ov2680.c.
> 
> So it might be interesting to still merge this so that the latest
> state of atomisp-ov2680.c is easier to compare to
> drivers/media/i2c/ov2680.c if the need arises.

-- 
Regards,

Laurent Pinchart