Re: [PATCH 15/16] lirc_dev: remove name from struct lirc_driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, May 02, 2017 at 06:04:18PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:04:52PM +0200, David Härdeman wrote:
>> The name is only used for a few debug messages and the name of the parent
>> device as well as the name of the lirc device (e.g. "lirc0") are sufficient
>> anyway.
>>...
>> @@ -207,8 +204,7 @@ lirc_register_driver(struct lirc_driver *d)
>>  	if (err)
>>  		goto out_cdev;
>>  
>> -	dev_info(ir->d.dev, "lirc_dev: driver %s registered at minor = %d\n",
>> -		 ir->d.name, ir->d.minor);
>> +	dev_info(ir->d.dev, "lirc device registered as lirc%d\n", minor);
>
>I'm not so sure this is a good idea. First of all, the documentation says
>you can use "dmesg |grep lirc_dev" to find your lirc devices and you've
>just replaced lirc_dev with lirc.

Sure, no strong preferences here, you could change the line to say
"lirc_dev device...", or drop the patch.

>https://linuxtv.org/downloads/v4l-dvb-apis/uapi/rc/lirc-dev-intro.html
>
>It's useful having the driver name in the message. For example, I have
>two rc devices connected usually:
>
>[sean@bigcore ~]$ dmesg | grep lirc_dev
>[    5.938284] lirc_dev: IR Remote Control driver registered, major 239
>[    5.945324] rc rc0: lirc_dev: driver ir-lirc-codec (winbond-cir) registered at minor = 0
>[ 5111.830118] rc rc1: lirc_dev: driver ir-lirc-codec (mceusb) registered at minor = 1

winbond-cir....good man :)

How about "dmesg | grep lirc -A2 -B2"?

I don't think the situation is that different from how you'd know which
input dev is allocated to any given rc_dev? With this patch applied the
relevant output will be:

[    0.393494] rc rc0: rc-core loopback device as /devices/virtual/rc/rc0
[    0.394458] input: rc-core loopback device as /devices/virtual/rc/rc0/input2
[    0.395717] rc rc0: lirc device registered as lirc0
[   12.612313] rc rc1: mceusb device as /devices/virtual/rc/rc1
[   12.612768] input: mceusb device as /devices/virtual/rc/rc1/input4
[   12.613112] rc rc1: lirc device registered as lirc1

(and we might want to change the lirc line to include the sysfs path?)

But realistically, how much dmesg grepping are we expecting normal
end-users to be doing?

Anyway, as I said, this patch isn't crucial, and we can revisit printk's
later (I'm looking at the ioctl locking right now and I think an
ir-lirc-codec and lirc_dev merger might be a good idea once the fate of
lirc_zilog has been decided).

>With the driver name I know which one is which.
>
>Maybe lirc_driver.name should be a "const char*" so no strcpy is needed
>(the ir-lirc-codec does not seem necessary).

Not that it really pertains to whether d->name should be kept or not,
but I think that lirc_dev shouldn't copy the lirc_driver struct into an
irctl struct internal copy at all, but just keep a normal pointer. I
haven't gotten around to vetting the (ab)use of the lirc_driver struct
yet though.

Regards,
David




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux