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

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

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.

-- 
Regards,

Laurent Pinchart