[0/7] media: v4l2-subdev: Rename pad config 'try_*' fields

Message ID 20231023214011.17730-1-laurent.pinchart@ideasonboard.com (mailing list archive)
Headers
Series media: v4l2-subdev: Rename pad config 'try_*' fields |

Message

Laurent Pinchart Oct. 23, 2023, 9:40 p.m. UTC
  Hello,

This series is the result of me getting bothered by the following note
in the documentation of the v4l2_subdev_pad_config structure:

 * Note: This struct is also used in active state, and the 'try' prefix is
 * historical and to be removed.

So I decided to drop the prefix.

Patches 1/7 to 6/7 replace direct usage of the fields in drivers with
the corresponding accessor functions. There was a relatively large
number of them in sensor drivers (in 6/7), but more worryingly, the
atmel-isi (1/7), microchip-isc (2/7) and tegra-video (5/7) should really
not have messed up with creating a v4l2_subdev_pad_config structure
manually. I urge the maintainers of those drivers to address the issue.

Finally, patch 7/7 renames the fields, which becomes easy after
addressing all the drivers.

The patches have been compile-tested only.

Sakari, this conflicts with your "[PATCH v3 0/8] Unify sub-device state
access functions" series. I don't mind rebasing on top if it gets merged
first.

Laurent Pinchart (7):
  media: atmel-isi: Use accessors for pad config 'try_*' fields
  media: microchip-isc: Use accessors for pad config 'try_*' fields
  media: atmel-isc: Use accessors for pad config 'try_*' fields
  media: atomisp: Use accessors for pad config 'try_*' fields
  media: tegra-video: Use accessors for pad config 'try_*' fields
  media: i2c: Use accessors for pad config 'try_*' fields
  media: v4l2-subdev: Rename pad config 'try_*' fields

 drivers/media/i2c/adv7183.c                   |  2 +-
 drivers/media/i2c/imx274.c                    | 12 +++----
 drivers/media/i2c/mt9m001.c                   |  2 +-
 drivers/media/i2c/mt9m111.c                   |  2 +-
 drivers/media/i2c/mt9t112.c                   |  2 +-
 drivers/media/i2c/mt9v011.c                   |  2 +-
 drivers/media/i2c/mt9v111.c                   |  2 +-
 drivers/media/i2c/ov2640.c                    |  2 +-
 drivers/media/i2c/ov2680.c                    |  4 +--
 drivers/media/i2c/ov6650.c                    | 34 ++++++++++++-------
 drivers/media/i2c/ov772x.c                    |  2 +-
 drivers/media/i2c/ov9640.c                    |  2 +-
 drivers/media/i2c/rj54n1cb0c.c                |  2 +-
 drivers/media/i2c/saa6752hs.c                 |  2 +-
 drivers/media/i2c/tw9910.c                    |  2 +-
 drivers/media/platform/atmel/atmel-isi.c      | 12 ++++---
 .../platform/microchip/microchip-isc-base.c   | 10 +++---
 .../media/atomisp/i2c/atomisp-gc2235.c        |  2 +-
 .../media/atomisp/i2c/atomisp-mt9m114.c       |  2 +-
 .../media/atomisp/i2c/atomisp-ov2722.c        |  2 +-
 .../staging/media/atomisp/pci/atomisp_tpg.c   |  2 +-
 .../media/deprecated/atmel/atmel-isc-base.c   | 10 +++---
 drivers/staging/media/tegra-video/vi.c        | 14 ++++----
 include/media/v4l2-subdev.h                   | 33 ++++++++----------
 24 files changed, 87 insertions(+), 74 deletions(-)


base-commit: 94e27fbeca27d8c772fc2bc807730aaee5886055
  

Comments

Eugen Hristev Oct. 24, 2023, 6:55 a.m. UTC | #1
On 10/24/23 00:40, Laurent Pinchart wrote:
> Hello,

Hello Laurent,

> 
> This series is the result of me getting bothered by the following note
> in the documentation of the v4l2_subdev_pad_config structure:
> 
>   * Note: This struct is also used in active state, and the 'try' prefix is
>   * historical and to be removed.
> 
> So I decided to drop the prefix.
> 
> Patches 1/7 to 6/7 replace direct usage of the fields in drivers with
> the corresponding accessor functions. There was a relatively large
> number of them in sensor drivers (in 6/7), but more worryingly, the
> atmel-isi (1/7), microchip-isc (2/7) and tegra-video (5/7) should really
> not have messed up with creating a v4l2_subdev_pad_config structure
> manually. I urge the maintainers of those drivers to address the issue.

Could you hint a bit about how the issue should be addressed ?
Instead of creating a `v4l2_subdev_pad_config`, one should use 
v4l2_subdev_lock_and_get_active_state() ? Is this the right way to do it ?

Thanks for looking into this,
Eugen

> 
> Finally, patch 7/7 renames the fields, which becomes easy after
> addressing all the drivers.
> 
> The patches have been compile-tested only.
> 
> Sakari, this conflicts with your "[PATCH v3 0/8] Unify sub-device state
> access functions" series. I don't mind rebasing on top if it gets merged
> first.
> 
> Laurent Pinchart (7):
>    media: atmel-isi: Use accessors for pad config 'try_*' fields
>    media: microchip-isc: Use accessors for pad config 'try_*' fields
>    media: atmel-isc: Use accessors for pad config 'try_*' fields
>    media: atomisp: Use accessors for pad config 'try_*' fields
>    media: tegra-video: Use accessors for pad config 'try_*' fields
>    media: i2c: Use accessors for pad config 'try_*' fields
>    media: v4l2-subdev: Rename pad config 'try_*' fields
> 
>   drivers/media/i2c/adv7183.c                   |  2 +-
>   drivers/media/i2c/imx274.c                    | 12 +++----
>   drivers/media/i2c/mt9m001.c                   |  2 +-
>   drivers/media/i2c/mt9m111.c                   |  2 +-
>   drivers/media/i2c/mt9t112.c                   |  2 +-
>   drivers/media/i2c/mt9v011.c                   |  2 +-
>   drivers/media/i2c/mt9v111.c                   |  2 +-
>   drivers/media/i2c/ov2640.c                    |  2 +-
>   drivers/media/i2c/ov2680.c                    |  4 +--
>   drivers/media/i2c/ov6650.c                    | 34 ++++++++++++-------
>   drivers/media/i2c/ov772x.c                    |  2 +-
>   drivers/media/i2c/ov9640.c                    |  2 +-
>   drivers/media/i2c/rj54n1cb0c.c                |  2 +-
>   drivers/media/i2c/saa6752hs.c                 |  2 +-
>   drivers/media/i2c/tw9910.c                    |  2 +-
>   drivers/media/platform/atmel/atmel-isi.c      | 12 ++++---
>   .../platform/microchip/microchip-isc-base.c   | 10 +++---
>   .../media/atomisp/i2c/atomisp-gc2235.c        |  2 +-
>   .../media/atomisp/i2c/atomisp-mt9m114.c       |  2 +-
>   .../media/atomisp/i2c/atomisp-ov2722.c        |  2 +-
>   .../staging/media/atomisp/pci/atomisp_tpg.c   |  2 +-
>   .../media/deprecated/atmel/atmel-isc-base.c   | 10 +++---
>   drivers/staging/media/tegra-video/vi.c        | 14 ++++----
>   include/media/v4l2-subdev.h                   | 33 ++++++++----------
>   24 files changed, 87 insertions(+), 74 deletions(-)
> 
> 
> base-commit: 94e27fbeca27d8c772fc2bc807730aaee5886055
  
Tomi Valkeinen Oct. 24, 2023, 8:41 a.m. UTC | #2
On 24/10/2023 09:55, Eugen Hristev wrote:
> On 10/24/23 00:40, Laurent Pinchart wrote:
>> Hello,
> 
> Hello Laurent,
> 
>>
>> This series is the result of me getting bothered by the following note
>> in the documentation of the v4l2_subdev_pad_config structure:
>>
>>   * Note: This struct is also used in active state, and the 'try' 
>> prefix is
>>   * historical and to be removed.
>>
>> So I decided to drop the prefix.
>>
>> Patches 1/7 to 6/7 replace direct usage of the fields in drivers with
>> the corresponding accessor functions. There was a relatively large
>> number of them in sensor drivers (in 6/7), but more worryingly, the
>> atmel-isi (1/7), microchip-isc (2/7) and tegra-video (5/7) should really
>> not have messed up with creating a v4l2_subdev_pad_config structure
>> manually. I urge the maintainers of those drivers to address the issue.
> 
> Could you hint a bit about how the issue should be addressed ?
> Instead of creating a `v4l2_subdev_pad_config`, one should use 
> v4l2_subdev_lock_and_get_active_state() ? Is this the right way to do it ?

I had a quick look at atmel-isi. If I understand it right, it does not 
expose the subdevs to the userspace, and 'isi->entity.subdev' refers to 
the sensor.

In that case I think using v4l2_subdev_call_state_active() and 
v4l2_subdev_call_state_try() should usually work.

If there are cases where the same try state needs to be used for 
multiple calls, then the state management has to be done manually with 
__v4l2_subdev_state_alloc() and __v4l2_subdev_state_free() (e.g. 
drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c).

  Tomi
  
Laurent Pinchart Oct. 24, 2023, 8:47 a.m. UTC | #3
On Tue, Oct 24, 2023 at 11:41:57AM +0300, Tomi Valkeinen wrote:
> On 24/10/2023 09:55, Eugen Hristev wrote:
> > On 10/24/23 00:40, Laurent Pinchart wrote:
> >> Hello,
> > 
> > Hello Laurent,
> > 
> >>
> >> This series is the result of me getting bothered by the following note
> >> in the documentation of the v4l2_subdev_pad_config structure:
> >>
> >>   * Note: This struct is also used in active state, and the 'try' 
> >> prefix is
> >>   * historical and to be removed.
> >>
> >> So I decided to drop the prefix.
> >>
> >> Patches 1/7 to 6/7 replace direct usage of the fields in drivers with
> >> the corresponding accessor functions. There was a relatively large
> >> number of them in sensor drivers (in 6/7), but more worryingly, the
> >> atmel-isi (1/7), microchip-isc (2/7) and tegra-video (5/7) should really
> >> not have messed up with creating a v4l2_subdev_pad_config structure
> >> manually. I urge the maintainers of those drivers to address the issue.
> > 
> > Could you hint a bit about how the issue should be addressed ?
> > Instead of creating a `v4l2_subdev_pad_config`, one should use 
> > v4l2_subdev_lock_and_get_active_state() ? Is this the right way to do it ?
> 
> I had a quick look at atmel-isi. If I understand it right, it does not 
> expose the subdevs to the userspace, and 'isi->entity.subdev' refers to 
> the sensor.
> 
> In that case I think using v4l2_subdev_call_state_active() and 
> v4l2_subdev_call_state_try() should usually work.
> 
> If there are cases where the same try state needs to be used for 
> multiple calls, then the state management has to be done manually with 
> __v4l2_subdev_state_alloc() and __v4l2_subdev_state_free() (e.g. 
> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c).

And for the microchip-isc driver, my understanding is that it's an
MC-centric driver. It should therefore not call the sensor's
.enum_frame_size() operation, which would remove the stack-allocated
state in the isc_validate() function.
  
Sakari Ailus Oct. 27, 2023, 3:11 p.m. UTC | #4
Hi Laurent,

On Tue, Oct 24, 2023 at 12:40:04AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> This series is the result of me getting bothered by the following note
> in the documentation of the v4l2_subdev_pad_config structure:
> 
>  * Note: This struct is also used in active state, and the 'try' prefix is
>  * historical and to be removed.
> 
> So I decided to drop the prefix.
> 
> Patches 1/7 to 6/7 replace direct usage of the fields in drivers with
> the corresponding accessor functions. There was a relatively large
> number of them in sensor drivers (in 6/7), but more worryingly, the
> atmel-isi (1/7), microchip-isc (2/7) and tegra-video (5/7) should really
> not have messed up with creating a v4l2_subdev_pad_config structure
> manually. I urge the maintainers of those drivers to address the issue.
> 
> Finally, patch 7/7 renames the fields, which becomes easy after
> addressing all the drivers.
> 
> The patches have been compile-tested only.
> 
> Sakari, this conflicts with your "[PATCH v3 0/8] Unify sub-device state
> access functions" series. I don't mind rebasing on top if it gets merged
> first.

Thanks for the set, it's really nice. I missed there were so many drivers
still accessing this information directly --- I rather thought we always
had the access functions to do that, and all drivers used those.

I see you've already rebased v2 on top of the cleanups, I'll take these
then.