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

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

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.

-- 
Regards,

Laurent Pinchart