[PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()
Marek Vasut
marex at denx.de
Wed Jun 29 04:04:39 PDT 2022
On 6/29/22 11:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,
Hi,
[...]
>> drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
>> 1 file changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> index c604d672c2156..c68d32931b277 100644
>> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
>> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
>> struct dcmi_framesize *sd_framesize)
>> {
>> const struct dcmi_format *sd_fmt;
>> + static struct lock_class_key key;
>> struct dcmi_framesize sd_fsize;
>> struct v4l2_pix_format *pix = &f->fmt.pix;
>> - struct v4l2_subdev_pad_config pad_cfg;
>> - struct v4l2_subdev_state pad_state = {
>> - .pads = &pad_cfg
>> - };
>> + struct v4l2_subdev_state *sd_state;
>> struct v4l2_subdev_format format = {
>> .which = V4L2_SUBDEV_FORMAT_TRY,
>> };
>> bool do_crop;
>> int ret;
>>
>> + /*
>> + * FIXME: Drop this call, drivers are not supposed to use
>> + * __v4l2_subdev_state_alloc().
>> + */
>> + sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
>> + if (IS_ERR(sd_state))
>> + return PTR_ERR(sd_state);
>> +
>
> I've been reading the discussion for the v1 patch, and I seriously do not like this.
>
> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.
>
> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.
>
> I think a helper function might be beneficial, but the real problem is with the
> comment: it does not explain why you shouldn't use it and what needs to be done
> to fix it.
>
> My suggestion would be to document that in the kerneldoc for this function in
> media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> in the other drivers that use this).
Would it be OK if I left the core rework/documentation to Tomi as a
subsequent patch to this one ?
> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?
Probably, I only ran into it with the DCMI so far.
More information about the linux-arm-kernel
mailing list