[PATCH] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Tomi Valkeinen]

On 27/06/2022 16:01, Marek Vasut wrote:
> On 6/27/22 14:53, Hugues FRUCHET wrote:
>> Hi Marek,
> 
> Hi,
> 
>> Thanks for explanation, I understand now.
>>
>> Please note that dcmi is not atmel-isi.c has same code structure, 
>> hence same problem:
>>
>> static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>      struct v4l2_subdev_state pad_state = {
>>          .pads = &pad_cfg
>>          };
>> [...]
>>      ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>
>>
>> Moreover, searching for __v4l2_subdev_state_alloc() I see those "FIXME":
>>
>> drivers/media/platform/renesas/vsp1/vsp1_entity.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      entity->config = __v4l2_subdev_state_alloc(&entity->subdev,
>>                             "vsp1:config->lock", &key);
>>
>>
>> drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c
>>      /*
>>       * FIXME: Drop this call, drivers are not supposed to use
>>       * __v4l2_subdev_state_alloc().
>>       */
>>      sd_state = __v4l2_subdev_state_alloc(sd, "rvin:state->lock", &key);
>>
>>
>> So I wonder about introducing this new change in dcmi while it is 
>> marked as "FIXME" in other camera interface drivers ?
> 
> This is probably something Tomi/Laurent can answer better. It should be 
> OK for this driver as far as I understand the discussion in this thread.

Yes and no. We shouldn't use __ funcs in the drivers. 
__v4l2_subdev_state_alloc() calls exist in the current drivers as it 
wasn't trivial to change the driver to do it otherwise while adding the 
subdev state feature.

If I recall right, the other users (at least some of them) were storing 
internal state in the state, not passing it forward. And, of course, the 
drivers were themselves interested in the state stored there.

Here, we only need to allocate the state so that the driver is able to 
call set_fmt on another subdev, so it's a bit different case.

Anyway, I think it's _not_ ok to add __v4l2_subdev_state_alloc() without 
a FIXME comment. However, I think it's ok to add a helper func, similar 
to v4l2_subdev_call_state_active(), which in turn uses 
__v4l2_subdev_state_alloc.

However, if we end up in a situation where we think it's "normal" for 
drivers to call __v4l2_subdev_state_alloc, we need to rename it and drop 
the two underscores. But I think we're not there yet (and hopefully never).

  Tomi