Thanks Tomi for details,
OK for me with a FIXME on top, for the sake of uniformity with other
drivers.
BR,
Hugues.
On 6/27/22 15:30, Tomi Valkeinen wrote:
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
