On 02/09/18 13:44, Sakari Ailus wrote:
> 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
>>>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
>>>> ---
>>>>  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.

You are correct in one respect: we use this API, but with video devices.
So subdevices support the g/s_register ops, and they are called via /dev/videoX.

We can remove the ioctl support from v4l2-subdev.c (not the g/s_register ops!).
Without VIDIOC_DBG_G_CHIP_INFO I don't think v4l2-dbg is usable. Although it
is always possible to call the ioctl directly, of course.

So if Mauro would agree to this, the DBG ioctl support in v4l2-subdev can be

But either remove them, or add this ioctl. Don't leave it in a zombie state.

Personally I see no harm whatsoever in just adding VIDIOC_DBG_G_CHIP_INFO.
If someone ever makes a patch to switch over to debugfs then these ioctls
can be removed.

BTW, how would new drivers use debugfs for this? Does regmap provide such



Reply via email to