[v2,00/20] Omnivision OV4689 refactoring and improvements

  Hi,

This series contains refactoring and new features implementation for
the Omnivision OV4689 sensor driver. Specifically, patches 1, 2, 3, 5,
6, 10, 15, 16, 18, and 19 are refactorings, and are not supposed to
introduce any functional change. Patches 4 and 7 perform migration to
CCI helpers and subdevice active state respectively, and should not
introduce any hardware- and/or user-visible change either. Patch 8
fixes a possible race condition due to v4l2_async_register_subdev_sensor
being called too early in ov4689_probe, and patch 9 migrates power
management to PM autosuspend.

Patches 11-14 expose more sensor controls to the userspace, such as
(read-write) HBLANK, VFLIP/HFLIP, digital gain, and color
balance. Patch 17 implements configurable analogue crop rectangle via
.set_selection callback. And finally, patch 20 enables 2x2 binning
option. It should be noted that publicly available sensor
documentation is lacking description of many registers and their value
ranges, so a lot of values had to be found by experimentation.

Changes in v2:
- collect Laurent's r-b's
- squash together "CCI conversion" and "Set gain in one 16 bit write"
- use ctrl->val in ov4689_set_ctrl
- rename try_fmt to fmt in ov4689_init_cfg and drop corresponding comment
- rebase on top of media-stage and rename init_cfg->init_state
- sort register definitions by address throughout the whole series
- fix number of controls hint in v4l2_ctrl_handler_init
- make all hexadecimal constants lowercase
- disable runtime pm in probe error path
- implement pm autosuspend

Mikhail Rudenko (20):
  media: i2c: ov4689: Clean up and annotate the register table
  media: i2c: ov4689: Sort register definitions by address
  media: i2c: ov4689: Fix typo in a comment
  media: i2c: ov4689: CCI conversion
  media: i2c: ov4689: Remove i2c_client from ov4689 struct
  media: i2c: ov4689: Refactor ov4689_set_ctrl
  media: i2c: ov4689: Use sub-device active state
  media: i2c: ov4689: Enable runtime PM before registering sub-device
  media: i2c: ov4689: Use runtime PM autosuspend
  media: i2c: ov4689: Remove max_fps field from struct ov4689_mode
  media: i2c: ov4689: Make horizontal blanking configurable
  media: i2c: ov4689: Implement vflip/hflip controls
  media: i2c: ov4689: Implement digital gain control
  media: i2c: ov4689: Implement manual color balance controls
  media: i2c: ov4689: Move pixel array size out of struct ov4689_mode
  media: i2c: ov4689: Set timing registers programmatically
  media: i2c: ov4689: Configurable analogue crop
  media: i2c: ov4689: Eliminate struct ov4689_mode
  media: i2c: ov4689: Refactor ov4689_s_stream
  media: i2c: ov4689: Implement 2x2 binning

 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/ov4689.c | 964 +++++++++++++++++++++++--------------
 2 files changed, 592 insertions(+), 373 deletions(-)

--
2.43.0
  

Hi,

On 2023-12-18 at 20:40 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:

> Hi,
>
> This series contains refactoring and new features implementation for
> the Omnivision OV4689 sensor driver. Specifically, patches 1, 2, 3, 5,
> 6, 10, 15, 16, 18, and 19 are refactorings, and are not supposed to
> introduce any functional change. Patches 4 and 7 perform migration to
> CCI helpers and subdevice active state respectively, and should not
> introduce any hardware- and/or user-visible change either. Patch 8
> fixes a possible race condition due to v4l2_async_register_subdev_sensor
> being called too early in ov4689_probe, and patch 9 migrates power
> management to PM autosuspend.
>
> Patches 11-14 expose more sensor controls to the userspace, such as
> (read-write) HBLANK, VFLIP/HFLIP, digital gain, and color
> balance. Patch 17 implements configurable analogue crop rectangle via
> .set_selection callback. And finally, patch 20 enables 2x2 binning
> option. It should be noted that publicly available sensor
> documentation is lacking description of many registers and their value
> ranges, so a lot of values had to be found by experimentation.

Gentle ping on this series. Anything I can do to help getting it
reviewed and merged? Maybe split patches 15-20 which implement cropping
and binning and change the driver away from register list based model
into a separate series? Anyone?

> Changes in v2:
> - collect Laurent's r-b's
> - squash together "CCI conversion" and "Set gain in one 16 bit write"
> - use ctrl->val in ov4689_set_ctrl
> - rename try_fmt to fmt in ov4689_init_cfg and drop corresponding comment
> - rebase on top of media-stage and rename init_cfg->init_state
> - sort register definitions by address throughout the whole series
> - fix number of controls hint in v4l2_ctrl_handler_init
> - make all hexadecimal constants lowercase
> - disable runtime pm in probe error path
> - implement pm autosuspend
>
> Mikhail Rudenko (20):
>   media: i2c: ov4689: Clean up and annotate the register table
>   media: i2c: ov4689: Sort register definitions by address
>   media: i2c: ov4689: Fix typo in a comment
>   media: i2c: ov4689: CCI conversion
>   media: i2c: ov4689: Remove i2c_client from ov4689 struct
>   media: i2c: ov4689: Refactor ov4689_set_ctrl
>   media: i2c: ov4689: Use sub-device active state
>   media: i2c: ov4689: Enable runtime PM before registering sub-device
>   media: i2c: ov4689: Use runtime PM autosuspend
>   media: i2c: ov4689: Remove max_fps field from struct ov4689_mode
>   media: i2c: ov4689: Make horizontal blanking configurable
>   media: i2c: ov4689: Implement vflip/hflip controls
>   media: i2c: ov4689: Implement digital gain control
>   media: i2c: ov4689: Implement manual color balance controls
>   media: i2c: ov4689: Move pixel array size out of struct ov4689_mode
>   media: i2c: ov4689: Set timing registers programmatically
>   media: i2c: ov4689: Configurable analogue crop
>   media: i2c: ov4689: Eliminate struct ov4689_mode
>   media: i2c: ov4689: Refactor ov4689_s_stream
>   media: i2c: ov4689: Implement 2x2 binning
>
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/ov4689.c | 964 +++++++++++++++++++++++--------------
>  2 files changed, 592 insertions(+), 373 deletions(-)


--
Best regards,
Mikhail Rudenko
  

Hi Mikhail,

On Wed, Feb 21, 2024 at 06:02:15PM +0300, Mikhail Rudenko wrote:
> 
> Hi,
> 
> On 2023-12-18 at 20:40 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:
> 
> > Hi,
> >
> > This series contains refactoring and new features implementation for
> > the Omnivision OV4689 sensor driver. Specifically, patches 1, 2, 3, 5,
> > 6, 10, 15, 16, 18, and 19 are refactorings, and are not supposed to
> > introduce any functional change. Patches 4 and 7 perform migration to
> > CCI helpers and subdevice active state respectively, and should not
> > introduce any hardware- and/or user-visible change either. Patch 8
> > fixes a possible race condition due to v4l2_async_register_subdev_sensor
> > being called too early in ov4689_probe, and patch 9 migrates power
> > management to PM autosuspend.
> >
> > Patches 11-14 expose more sensor controls to the userspace, such as
> > (read-write) HBLANK, VFLIP/HFLIP, digital gain, and color
> > balance. Patch 17 implements configurable analogue crop rectangle via
> > .set_selection callback. And finally, patch 20 enables 2x2 binning
> > option. It should be noted that publicly available sensor
> > documentation is lacking description of many registers and their value
> > ranges, so a lot of values had to be found by experimentation.
> 
> Gentle ping on this series. Anything I can do to help getting it
> reviewed and merged? Maybe split patches 15-20 which implement cropping
> and binning and change the driver away from register list based model
> into a separate series? Anyone?

Oops, my bad. I'll review these shortly. I can already tell there's not
much to do though.
  

On Fri, Feb 23, 2024 at 08:15:41AM +0000, Sakari Ailus wrote:
> Hi Mikhail,
> 
> On Wed, Feb 21, 2024 at 06:02:15PM +0300, Mikhail Rudenko wrote:
> > 
> > Hi,
> > 
> > On 2023-12-18 at 20:40 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:
> > 
> > > Hi,
> > >
> > > This series contains refactoring and new features implementation for
> > > the Omnivision OV4689 sensor driver. Specifically, patches 1, 2, 3, 5,
> > > 6, 10, 15, 16, 18, and 19 are refactorings, and are not supposed to
> > > introduce any functional change. Patches 4 and 7 perform migration to
> > > CCI helpers and subdevice active state respectively, and should not
> > > introduce any hardware- and/or user-visible change either. Patch 8
> > > fixes a possible race condition due to v4l2_async_register_subdev_sensor
> > > being called too early in ov4689_probe, and patch 9 migrates power
> > > management to PM autosuspend.
> > >
> > > Patches 11-14 expose more sensor controls to the userspace, such as
> > > (read-write) HBLANK, VFLIP/HFLIP, digital gain, and color
> > > balance. Patch 17 implements configurable analogue crop rectangle via
> > > .set_selection callback. And finally, patch 20 enables 2x2 binning
> > > option. It should be noted that publicly available sensor
> > > documentation is lacking description of many registers and their value
> > > ranges, so a lot of values had to be found by experimentation.
> > 
> > Gentle ping on this series. Anything I can do to help getting it
> > reviewed and merged? Maybe split patches 15-20 which implement cropping
> > and binning and change the driver away from register list based model
> > into a separate series? Anyone?
> 
> Oops, my bad. I'll review these shortly. I can already tell there's not
> much to do though.

Done.
  

Hi Mikhail,

Thank you for the patch.

On Mon, Dec 18, 2023 at 08:40:40PM +0300, Mikhail Rudenko wrote:
> Remove repetitive pm_runtime_put calls in ov4689_s_stream function,
> and call pm_runtime_put once at the end of the "on" branch if any
> error occurred.
> 
> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
> ---
>  drivers/media/i2c/ov4689.c | 29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
> index e997c3231e85..884761d02119 100644
> --- a/drivers/media/i2c/ov4689.c
> +++ b/drivers/media/i2c/ov4689.c
> @@ -555,35 +555,26 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
>  					  ov4689_common_regs,
>  					  ARRAY_SIZE(ov4689_common_regs),
>  					  NULL);
> -		if (ret) {
> -			pm_runtime_put_sync(dev);
> -			goto unlock_and_return;
> -		}
> +		if (ret)
> +			goto cleanup_pm;
>  
>  		ret = ov4689_setup_timings(ov4689, sd_state);
> -		if (ret) {
> -			pm_runtime_put(dev);
> -			goto unlock_and_return;
> -		}
> +		if (ret)
> +			goto cleanup_pm;
>  
>  		ret = ov4689_setup_blc_anchors(ov4689, sd_state);
> -		if (ret) {
> -			pm_runtime_put(dev);
> -			goto unlock_and_return;
> -		}
> +		if (ret)
> +			goto cleanup_pm;
>  
>  		ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler);
> -		if (ret) {
> -			pm_runtime_put_sync(dev);
> -			goto unlock_and_return;
> -		}
> +		if (ret)
> +			goto cleanup_pm;
>  
>  		ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
>  				OV4689_MODE_STREAMING, NULL);
> -		if (ret) {
> +cleanup_pm:

A label within an if branch isn't great, readability-wise :-S Could we
maybe split the ov4687_s_stream() function in two (streamon and
streamoff, or similar names) ? You would then have a single
pm_runtime_put_sync() call in ov4689_s_stream(), in the error handling
path for the streamon function call.

> +		if (ret)
>  			pm_runtime_put_sync(dev);
> -			goto unlock_and_return;
> -		}
>  	} else {
>  		cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
>  			  OV4689_MODE_SW_STANDBY, NULL);
  

Hi Sakari,

On 2024-02-23 at 08:43 GMT, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:

> On Fri, Feb 23, 2024 at 08:15:41AM +0000, Sakari Ailus wrote:
>> Hi Mikhail,
>>
>> On Wed, Feb 21, 2024 at 06:02:15PM +0300, Mikhail Rudenko wrote:
>> >
>> > Hi,
>> >
>> > On 2023-12-18 at 20:40 +03, Mikhail Rudenko <mike.rudenko@gmail.com> wrote:
>> >
>> > > Hi,
>> > >
>> > > This series contains refactoring and new features implementation for
>> > > the Omnivision OV4689 sensor driver. Specifically, patches 1, 2, 3, 5,
>> > > 6, 10, 15, 16, 18, and 19 are refactorings, and are not supposed to
>> > > introduce any functional change. Patches 4 and 7 perform migration to
>> > > CCI helpers and subdevice active state respectively, and should not
>> > > introduce any hardware- and/or user-visible change either. Patch 8
>> > > fixes a possible race condition due to v4l2_async_register_subdev_sensor
>> > > being called too early in ov4689_probe, and patch 9 migrates power
>> > > management to PM autosuspend.
>> > >
>> > > Patches 11-14 expose more sensor controls to the userspace, such as
>> > > (read-write) HBLANK, VFLIP/HFLIP, digital gain, and color
>> > > balance. Patch 17 implements configurable analogue crop rectangle via
>> > > .set_selection callback. And finally, patch 20 enables 2x2 binning
>> > > option. It should be noted that publicly available sensor
>> > > documentation is lacking description of many registers and their value
>> > > ranges, so a lot of values had to be found by experimentation.
>> >
>> > Gentle ping on this series. Anything I can do to help getting it
>> > reviewed and merged? Maybe split patches 15-20 which implement cropping
>> > and binning and change the driver away from register list based model
>> > into a separate series? Anyone?
>>
>> Oops, my bad. I'll review these shortly. I can already tell there's not
>> much to do though.
>
> Done.

Thanks for the prompt review! I'll wait a few days to collect more
comments (especially on patches 17, 18, 20) and then post v3.


--
Best regards,
Mikhail Rudenko
  

Hi Laurent,

and thanks for the review!

On 2024-02-23 at 13:48 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mikhail,
>
> Thank you for the patch.
>
> On Mon, Dec 18, 2023 at 08:40:40PM +0300, Mikhail Rudenko wrote:
>> Remove repetitive pm_runtime_put calls in ov4689_s_stream function,
>> and call pm_runtime_put once at the end of the "on" branch if any
>> error occurred.
>>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>> ---
>>  drivers/media/i2c/ov4689.c | 29 ++++++++++-------------------
>>  1 file changed, 10 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
>> index e997c3231e85..884761d02119 100644
>> --- a/drivers/media/i2c/ov4689.c
>> +++ b/drivers/media/i2c/ov4689.c
>> @@ -555,35 +555,26 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
>>  					  ov4689_common_regs,
>>  					  ARRAY_SIZE(ov4689_common_regs),
>>  					  NULL);
>> -		if (ret) {
>> -			pm_runtime_put_sync(dev);
>> -			goto unlock_and_return;
>> -		}
>> +		if (ret)
>> +			goto cleanup_pm;
>>
>>  		ret = ov4689_setup_timings(ov4689, sd_state);
>> -		if (ret) {
>> -			pm_runtime_put(dev);
>> -			goto unlock_and_return;
>> -		}
>> +		if (ret)
>> +			goto cleanup_pm;
>>
>>  		ret = ov4689_setup_blc_anchors(ov4689, sd_state);
>> -		if (ret) {
>> -			pm_runtime_put(dev);
>> -			goto unlock_and_return;
>> -		}
>> +		if (ret)
>> +			goto cleanup_pm;
>>
>>  		ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler);
>> -		if (ret) {
>> -			pm_runtime_put_sync(dev);
>> -			goto unlock_and_return;
>> -		}
>> +		if (ret)
>> +			goto cleanup_pm;
>>
>>  		ret = cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
>>  				OV4689_MODE_STREAMING, NULL);
>> -		if (ret) {
>> +cleanup_pm:
>
> A label within an if branch isn't great, readability-wise :-S Could we
> maybe split the ov4687_s_stream() function in two (streamon and
> streamoff, or similar names) ? You would then have a single
> pm_runtime_put_sync() call in ov4689_s_stream(), in the error handling
> path for the streamon function call.

Okay, will split it in v3.

>> +		if (ret)
>>  			pm_runtime_put_sync(dev);
>> -			goto unlock_and_return;
>> -		}
>>  	} else {
>>  		cci_write(ov4689->regmap, OV4689_REG_CTRL_MODE,
>>  			  OV4689_MODE_SW_STANDBY, NULL);


--
Best regards,
Mikhail Rudenko