| Message ID | 20231023214011.17730-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: v4l2-subdev: Rename pad config 'try_*' fields | expand |
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
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
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.