Re: [PATCH v5 04/10] media: imx219: Make control handler ops for PIXEL_RATE NULL

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Laurent

On Mon, Jun 08, 2026 at 05:42:00PM +0300, Laurent Pinchart wrote:
> On Mon, Jun 08, 2026 at 04:47:03PM +0300, Sakari Ailus wrote:
> > On Mon, Jun 08, 2026 at 01:27:55PM +0300, Laurent Pinchart wrote:
> > > On Mon, Jun 08, 2026 at 01:21:22PM +0300, Sakari Ailus wrote:
> > > > On Mon, Jun 08, 2026 at 11:24:26AM +0300, Laurent Pinchart wrote:
> > > > > On Mon, Jun 08, 2026 at 11:14:32AM +0300, Sakari Ailus wrote:
> > > > > > On Mon, Jun 08, 2026 at 11:03:38AM +0300, Laurent Pinchart wrote:
> > > > > > > On Mon, Jun 08, 2026 at 09:53:17AM +0200, Jacopo Mondi wrote:
> > > > > > > > Hi Laurent
> > > > > > > >   sorry if I reply in place of Sakari but I got this fresh
> > > > > > >
> > > > > > > Thanks :-)
> > > > > > >
> > > > > > > > On Mon, Jun 08, 2026 at 10:36:53AM +0300, Laurent Pinchart wrote:
> > > > > > > > > On Mon, Jun 08, 2026 at 12:53:50AM +0300, Sakari Ailus wrote:
> > > > > > > > > > The PIXEL_RATE control exists to convey the value to the userspace and has
> > > > > > > > > > no configuration that would need to be programmed to the sensor. Make the
> > > > > > > > > > control handler ops for the PIXEL_RATE control NULL and avoid a warning
> > > > > > > > > > (as well as returning an error) from the driver.
> > > > > > > > >
> > > > > > > > > I thought the standard way to handle pixel rate being read only was to
> > > > > > > > > set the V4L2_CTRL_FLAG_READ_ONLY flag, like we do for e.g.
> > > > > > > > > V4L2_CID_LINK_FREQ. Is that not correct ?
> > > > > > > >
> > > > > > > > PIXEL_RATE is RO by default
> > > > > > > >
> > > > > > > > drivers/media/v4l2-core/v4l2-ctrls-defs.c:      case V4L2_CID_PIXEL_RATE:
> > > > > > > > drivers/media/v4l2-core/v4l2-ctrls-defs.c-              *type = V4L2_CTRL_TYPE_INTEGER64;
> > > > > > > > drivers/media/v4l2-core/v4l2-ctrls-defs.c-              *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > > > > > drivers/media/v4l2-core/v4l2-ctrls-defs.c-              break;
> > > > > > > >
> > > > > > > > The purpose of setting the ctrl_ops member to NULL is to avoid having
> > > > > > > > to handle RO controls in the driver implementation of .s_ctrl().
> > > > > > >
> > > > > > > Shouldn't the V4L2 control framework avoid .s_ctrl() calls for read-only
> > > > > > > controls ? I thought it did already.
> > > > > >
> > > > > > The control may be read-only on the UAPI but the driver could still do
> > > > > > something about it in its s_ctrl() callback. I don't know if any driver
> > > > > > depends on this though.
> > > > >
> > > > > It seems to be one of the many areas where control handling should be
> > > > > simplified for drivers.
> > > > >
> > > > > In any case, the imx219 driver creates the V4L2_CID_LINK_FREQ control
> > > > > with a non-NULL ops pointer, sets the V4L2_CTRL_FLAG_READ_ONLY flag, and
> > > > > does not handle V4L2_CID_LINK_FREQ in imx219_set_ctrl(). If there's an
> > > > > issue for V4L2_CID_PIXEL_RATE there is also an issue for
> > > > > V4L2_CID_LINK_FREQ.
> > > >
> > > > The ops should be set to NULL for link_freq as well.
> > > >
> > > > > Maybe the best short term fix would be to drop the dev_info() in the
> > > > > default case of the ctrl->id switch in imx219_set_ctrl() ?
> > > >
> > > > Any reason why not to set ops NULL instead?
> > >
> > > Because that seems to be a hack. Drivers shouldn't have to set a NULL
> > > ops pointer for read-only controls, when there's already a read-only
> > > flag. I'd like to simplify the code on the driver side and handle this
> > > in the control framework, not adding yet another arcane rule that most
> > > driver authors will not be aware of.
> >
> > I don't think I'd necessarily call it a hack.
>
> It's still yet another undocumented behaviour to will be copied through
> cargo-cult in a subset of drivers, making the code base more difficult
> to understand and maintain. The fact that this patch addressed the
> PIXEL_RATE control but not the LINK_FREQUENCY control proves my concerns
> are valid :-)
>
> I'd like to see one scheme clearly documented, and used by all drivers.
> Let's first focus on selecting one scheme and documenting it. Hans'
> opinion would be useful.
>

We discussed this very same matter a few months ago.

Before having the framework handling this, by not calling into the
driver's s_ctrl for RO controls, all users in-tree shall be checked to
make sure they're actually not doing something with those RO controls.

I even started a branch to check all drivers one-by-one and first set
they're ops to NULL. I quickly got discouraged by the amount of work
required and gave up.

> > The control may be changeable, but not by the user. If the driver is just
> > setting the value without going through the control framework, control
> > events will be omitted.
>
> --
> Regards,
>
> Laurent Pinchart