On 02/22/18 10:30, Alexandre Courbot wrote:
> On Wed, Feb 21, 2018 at 4:29 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> On 02/21/2018 07:01 AM, Alexandre Courbot wrote:
>>> Hi Hans,
>>>
>>> On Tue, Feb 20, 2018 at 7:36 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>>> On 02/20/18 05:44, Alexandre Courbot wrote:
>>
>> <snip>
>>
>>>>> +#define MEDIA_REQUEST_IOC(__cmd, func) \
>>>>> + [_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = { \
>>>>> + .cmd = MEDIA_REQUEST_IOC_##__cmd, \
>>>>> + .fn = func, \
>>>>> + }
>>>>> +
>>>>> +struct media_request_ioctl_info {
>>>>> + unsigned int cmd;
>>>>> + long (*fn)(struct media_request *req);
>>>>> +};
>>>>> +
>>>>> +static const struct media_request_ioctl_info ioctl_info[] = {
>>>>> + MEDIA_REQUEST_IOC(SUBMIT, media_request_ioctl_submit),
>>>>> + MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
>>>>
>>>> There are only two ioctls, so there is really no need for the
>>>> MEDIA_REQUEST_IOC define. Just keep it simple.
>>>
>>> The number of times it is used doesn't change the fact that it helps
>>> with readability IMHO.
>>
>> But this macro just boils down to:
>>
>> static const struct media_request_ioctl_info ioctl_info[] = {
>> { MEDIA_REQUEST_IOC_SUBMIT, media_request_ioctl_submit },
>> { MEDIA_REQUEST_IOC_REINIT, media_request_ioctl_reinit },
>> };
>>
>> It's absolutely identical! So it seems senseless to me.
>
> This expands to more than that - the index needs to be offset by 0x80,
> something we probably don't want to repeat every line.
>
>>
>>>
>>>>
>>>>> +};
>>>>> +
>>>>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>>>>> + unsigned long __arg)
>>>>> +{
>>>>> + struct media_request *req = filp->private_data;
>>>>> + const struct media_request_ioctl_info *info;
>>>>> +
>>>>> + if ((_IOC_NR(cmd) < 0x80) ||
>>>>
>>>> Why start the ioctl number at 0x80? Why not just 0?
>>>> It avoids all this hassle with the 0x80 offset.
>>
>> There is no clash with the MC ioctls, so I really don't believe the 0x80
>> offset is needed.
>
> I suppose your comment in patch 16 supersedes this one. :)
Yes, it does. I realized later why this was done like this.
That said, I don't like the magic value. Something like this might be
cleaner:
const unsigned int first_ioc_nr = _IOC_NR(MEDIA_REQUEST_IOC_SUBMIT);
Then use first_ioc_nr (or nr_offset or whatever) instead of 0x80.
Regards,
Hans
