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

[Hans de Goede <hdegoede@redhat.com>]

Hi Laurent,

On 6/7/23 17:51, Laurent Pinchart wrote:
> 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 prefer to port over drivers which I can actually test,
at least for now.

I already have converting ov5693.c (which also already has macros
to encode to width) on my TODO list. I'll convert that for v2
of the series.

And I also have a conversion of the "main" drivers/media/i2c/ov2680.c
ready.

I'll post that conversion as part of my big main ov2680 changes series
which I'll post in a couple of minutes (just need to write
a cover letter and then its ready).

Regards,

Hans



 
>> 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.
>