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
