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
>>>> 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.
>
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
removed.
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
access?
Regards,
Hans
