On Fri, Feb 09, 2018 at 01:18:18PM +0100, Hans Verkuil wrote:
> On 02/09/18 13:01, Sakari Ailus wrote:
> > Hi Hans,
> >
> > On Thu, Feb 08, 2018 at 09:36:46AM +0100, Hans Verkuil wrote:
> >> The VIDIOC_DBG_G/S_REGISTER ioctls imply that VIDIOC_DBG_G_CHIP_INFO is also
> >> present, since without that you cannot use v4l2-dbg.
> >>
> >> Just like the implementation in v4l2-ioctl.c this can be implemented in the
> >> core and no drivers need to be modified.
> >>
> >> It also makes it possible for v4l2-compliance to properly test the
> >> VIDIOC_DBG_G/S_REGISTER ioctls.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >> ---
> >> drivers/media/v4l2-core/v4l2-subdev.c | 13 +++++++++++++
> >> 1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 6cabfa32d2ed..2a5b5a3fa7a3 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -255,6 +255,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >> return -EPERM;
> >> return v4l2_subdev_call(sd, core, s_register, p);
> >> }
> >> + case VIDIOC_DBG_G_CHIP_INFO:
> >> + {
> >> + struct v4l2_dbg_chip_info *p = arg;
> >> +
> >> + if (p->match.type != V4L2_CHIP_MATCH_SUBDEV || p->match.addr)
> >> + return -EINVAL;
> >> + if (sd->ops->core && sd->ops->core->s_register)
> >> + p->flags |= V4L2_CHIP_FL_WRITABLE;
> >> + if (sd->ops->core && sd->ops->core->g_register)
> >> + p->flags |= V4L2_CHIP_FL_READABLE;
> >> + strlcpy(p->name, sd->name, sizeof(p->name));
> >> + return 0;
> >> + }
> >
> > This is effectively doing the same as debugfs except that it's specific to
> > V4L2. I don't think we should endorse its use, and especially not without a
> > real use case.
>
> We (Cisco) use it all the time. Furthermore, this works for any bus, not just
> i2c. Also spi, internal register busses, etc.
>
> It's been in use for many years. More importantly, there is no excuse to have
> only half the API implemented.
>
> It's all fine to talk about debugfs, but are you going to make that? This API
> works, it's supported by v4l2-dbg, it's in use. Now, let's at least make it
> pass v4l2-compliance.
>
> I agree, if we would redesign it, we would use debugfs. But I think it didn't
> even exist when this was made. So this API is here to stay and all it takes
> is this ioctl of code to add the missing piece for subdevs.
>
> Nobody is going to make a replacement for this using debugfs. Why spend effort
> on it if we already have an API for this?
It's not the first case when a more generic API replaces a subsystem
specific one. We have another conversion to make, switching from
implementing s_power() callback in drivers to runtime PM for instance.
I simply want to point out that this patch is endorsing something which is
obsolete and not needed: no-one has complained about the lack of this for
sub-devices, haven't they?
I'd just remove the check from v4l-compliance or make it optional. New
drivers should use debugfs instead if something like that is needed.
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
