| Message ID | 20241012233932.255211-1-linux@treblig.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] media: cx18: Remove unused cx18_reset_ir_gpio | expand |
On Sun, 2024-10-13 at 00:39 +0100, linux@treblig.org wrote: > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > cx18_reset_ir_gpio() has been unused in tree since 2009 > commit eefe1010a465 ("V4L/DVB (10759): cx18: Convert GPIO connected > functions to act as v4l2_subdevices") > > It has a comment saying it's exported for use by 'lirc_pvr150' but I > don't > see any sign of it in the lirc git, and I see it removed support > for lirc_i2c.c 'Flavors of the Hauppage PVR-150...' in 2014. > The IR Tx functionality of lirc_zilog (aka lirc_pvr150) used this when it decided the Zilog 8051 microcontroller and firmware had wedged itself. If lirc_zilog is gone from staging and lirc_zilog/lirc_pvr150 is gone from out of tree LIRC, then I guess there's no current caller of this function. Removing it prevents any current or future module from bringing the IR chip out of a bad/stuck state via software. So that's the trade off for removing this function. -Andy > Remove it. > > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org> > --- > drivers/media/pci/cx18/cx18-gpio.c | 15 --------------- > drivers/media/pci/cx18/cx18-gpio.h | 1 - > 2 files changed, 16 deletions(-) > > diff --git a/drivers/media/pci/cx18/cx18-gpio.c > b/drivers/media/pci/cx18/cx18-gpio.c > index c85eb8d25837..485a6cbeb15a 100644 > --- a/drivers/media/pci/cx18/cx18-gpio.c > +++ b/drivers/media/pci/cx18/cx18-gpio.c > @@ -305,21 +305,6 @@ int cx18_gpio_register(struct cx18 *cx, u32 hw) > return v4l2_device_register_subdev(&cx->v4l2_dev, sd); > } > > -void cx18_reset_ir_gpio(void *data) > -{ > - struct cx18 *cx = to_cx18(data); > - > - if (cx->card->gpio_i2c_slave_reset.ir_reset_mask == 0) > - return; > - > - CX18_DEBUG_INFO("Resetting IR microcontroller\n"); > - > - v4l2_subdev_call(&cx->sd_resetctrl, > - core, reset, CX18_GPIO_RESET_Z8F0811); > -} > -EXPORT_SYMBOL(cx18_reset_ir_gpio); > -/* This symbol is exported for use by lirc_pvr150 for the IR-blaster > */ > - > /* Xceive tuner reset function */ > int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int > value) > { > diff --git a/drivers/media/pci/cx18/cx18-gpio.h > b/drivers/media/pci/cx18/cx18-gpio.h > index 0fa4c7ad2286..8d5797dea7f5 100644 > --- a/drivers/media/pci/cx18/cx18-gpio.h > +++ b/drivers/media/pci/cx18/cx18-gpio.h > @@ -17,5 +17,4 @@ enum cx18_gpio_reset_type { > CX18_GPIO_RESET_XC2028 = 2, > }; > > -void cx18_reset_ir_gpio(void *data); > int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int > value);
On Sun, Oct 13, 2024 at 12:39:32AM +0100, linux@treblig.org wrote: > From: "Dr. David Alan Gilbert" <linux@treblig.org> > > cx18_reset_ir_gpio() has been unused in tree since 2009 > commit eefe1010a465 ("V4L/DVB (10759): cx18: Convert GPIO connected > functions to act as v4l2_subdevices") > > It has a comment saying it's exported for use by 'lirc_pvr150' but I don't > see any sign of it in the lirc git, and I see it removed support > for lirc_i2c.c 'Flavors of the Hauppage PVR-150...' in 2014. > > Remove it. Interesting, I can't find any call site either. The ir-i2c-kbd driver could potentially use this, but it would to know the correct v4l2_dev for the device; also there are devices other than the cx18 which use the same IR module, so they would not have a way to force a reset or need a different mechanism (e.g. ivtv driver). So I don't understand how this could be wired up or how it was ever wired up. This could be great because if done correctly, we could remove the VIDIOC_INT_RESET ioctl completely. Then again, I don't know how often the device hangs. With the current driver the IR module I don't know of any hangs -- maybe the ioctl could just go anyway. Sean > > Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org> > --- > drivers/media/pci/cx18/cx18-gpio.c | 15 --------------- > drivers/media/pci/cx18/cx18-gpio.h | 1 - > 2 files changed, 16 deletions(-) > > diff --git a/drivers/media/pci/cx18/cx18-gpio.c b/drivers/media/pci/cx18/cx18-gpio.c > index c85eb8d25837..485a6cbeb15a 100644 > --- a/drivers/media/pci/cx18/cx18-gpio.c > +++ b/drivers/media/pci/cx18/cx18-gpio.c > @@ -305,21 +305,6 @@ int cx18_gpio_register(struct cx18 *cx, u32 hw) > return v4l2_device_register_subdev(&cx->v4l2_dev, sd); > } > > -void cx18_reset_ir_gpio(void *data) > -{ > - struct cx18 *cx = to_cx18(data); > - > - if (cx->card->gpio_i2c_slave_reset.ir_reset_mask == 0) > - return; > - > - CX18_DEBUG_INFO("Resetting IR microcontroller\n"); > - > - v4l2_subdev_call(&cx->sd_resetctrl, > - core, reset, CX18_GPIO_RESET_Z8F0811); > -} > -EXPORT_SYMBOL(cx18_reset_ir_gpio); > -/* This symbol is exported for use by lirc_pvr150 for the IR-blaster */ > - > /* Xceive tuner reset function */ > int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value) > { > diff --git a/drivers/media/pci/cx18/cx18-gpio.h b/drivers/media/pci/cx18/cx18-gpio.h > index 0fa4c7ad2286..8d5797dea7f5 100644 > --- a/drivers/media/pci/cx18/cx18-gpio.h > +++ b/drivers/media/pci/cx18/cx18-gpio.h > @@ -17,5 +17,4 @@ enum cx18_gpio_reset_type { > CX18_GPIO_RESET_XC2028 = 2, > }; > > -void cx18_reset_ir_gpio(void *data); > int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value); > -- > 2.47.0 >
On 10/28/24 20:58, Sean Young wrote: > On Sun, Oct 13, 2024 at 12:39:32AM +0100, linux@treblig.org wrote: >> From: "Dr. David Alan Gilbert" <linux@treblig.org> >> >> cx18_reset_ir_gpio() has been unused in tree since 2009 >> commit eefe1010a465 ("V4L/DVB (10759): cx18: Convert GPIO connected >> functions to act as v4l2_subdevices") >> >> It has a comment saying it's exported for use by 'lirc_pvr150' but I don't >> see any sign of it in the lirc git, and I see it removed support >> for lirc_i2c.c 'Flavors of the Hauppage PVR-150...' in 2014. >> >> Remove it. > > Interesting, I can't find any call site either. The ir-i2c-kbd driver could > potentially use this, but it would to know the correct v4l2_dev for the > device; also there are devices other than the cx18 which use the same IR > module, so they would not have a way to force a reset or need a different > mechanism (e.g. ivtv driver). > > So I don't understand how this could be wired up or how it was ever wired > up. > > This could be great because if done correctly, we could remove the > VIDIOC_INT_RESET ioctl completely. Then again, I don't know how often the > device hangs. With the current driver the IR module I don't know of any > hangs -- maybe the ioctl could just go anyway. > > > Sean > >> >> Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org> >> --- >> drivers/media/pci/cx18/cx18-gpio.c | 15 --------------- >> drivers/media/pci/cx18/cx18-gpio.h | 1 - >> 2 files changed, 16 deletions(-) >> >> diff --git a/drivers/media/pci/cx18/cx18-gpio.c b/drivers/media/pci/cx18/cx18-gpio.c >> index c85eb8d25837..485a6cbeb15a 100644 >> --- a/drivers/media/pci/cx18/cx18-gpio.c >> +++ b/drivers/media/pci/cx18/cx18-gpio.c >> @@ -305,21 +305,6 @@ int cx18_gpio_register(struct cx18 *cx, u32 hw) >> return v4l2_device_register_subdev(&cx->v4l2_dev, sd); >> } >> >> -void cx18_reset_ir_gpio(void *data) >> -{ >> - struct cx18 *cx = to_cx18(data); >> - >> - if (cx->card->gpio_i2c_slave_reset.ir_reset_mask == 0) >> - return; >> - >> - CX18_DEBUG_INFO("Resetting IR microcontroller\n"); >> - >> - v4l2_subdev_call(&cx->sd_resetctrl, >> - core, reset, CX18_GPIO_RESET_Z8F0811); Ah, this calls core.reset. But VIDIOC_INT_RESET in cx18_default() does the same (actually, it calls this for all subdevs). So dropping this code should be fine since you can still do the same thing with cx18-ctl --reset. This function was probably used a long time ago by lirc_pvr150, but I can't even find the source for that anymore. I assume it has a modern replacement. This is the original commit adding this function: commit 02fa272fcb6edda9059d6dbaab20dfe919f4f4d2 Author: Andy Walls <awalls@radix.net> Date: Sun Jul 13 19:30:15 2008 -0300 V4L/DVB (8332): cx18: Suport external reset of the Z8F0811 IR controller on HVR-1600 for lirc cx18: added in cx18_ir_reset_gpio function for lirc_pvr150 like module. Also added the ability to reset the IR chip via ioctl like ivtv. This needs the mutex to protect gpio_dir and gpio_val in struct cx18 as gpio changes can come from a few different asynchronous sources now. Signed-off-by: Andy Walls <awalls@radix.net> Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org> Perhaps Andy remembers how frequent the lock ups were. Regards, Hans >> -} >> -EXPORT_SYMBOL(cx18_reset_ir_gpio); >> -/* This symbol is exported for use by lirc_pvr150 for the IR-blaster */ >> - >> /* Xceive tuner reset function */ >> int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value) >> { >> diff --git a/drivers/media/pci/cx18/cx18-gpio.h b/drivers/media/pci/cx18/cx18-gpio.h >> index 0fa4c7ad2286..8d5797dea7f5 100644 >> --- a/drivers/media/pci/cx18/cx18-gpio.h >> +++ b/drivers/media/pci/cx18/cx18-gpio.h >> @@ -17,5 +17,4 @@ enum cx18_gpio_reset_type { >> CX18_GPIO_RESET_XC2028 = 2, >> }; >> >> -void cx18_reset_ir_gpio(void *data); >> int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value); >> -- >> 2.47.0 >> >
On Tue, Oct 29, 2024 at 11:57:16AM +0100, Hans Verkuil wrote: > On 10/28/24 20:58, Sean Young wrote: > > On Sun, Oct 13, 2024 at 12:39:32AM +0100, linux@treblig.org wrote: > >> From: "Dr. David Alan Gilbert" <linux@treblig.org> > >> > >> cx18_reset_ir_gpio() has been unused in tree since 2009 > >> commit eefe1010a465 ("V4L/DVB (10759): cx18: Convert GPIO connected > >> functions to act as v4l2_subdevices") > >> > >> It has a comment saying it's exported for use by 'lirc_pvr150' but I don't > >> see any sign of it in the lirc git, and I see it removed support > >> for lirc_i2c.c 'Flavors of the Hauppage PVR-150...' in 2014. > >> > >> Remove it. > > > > Interesting, I can't find any call site either. The ir-i2c-kbd driver could > > potentially use this, but it would to know the correct v4l2_dev for the > > device; also there are devices other than the cx18 which use the same IR > > module, so they would not have a way to force a reset or need a different > > mechanism (e.g. ivtv driver). > > > > So I don't understand how this could be wired up or how it was ever wired > > up. > > > > This could be great because if done correctly, we could remove the > > VIDIOC_INT_RESET ioctl completely. Then again, I don't know how often the > > device hangs. With the current driver the IR module I don't know of any > > hangs -- maybe the ioctl could just go anyway. > > > > > > Sean > > > >> > >> Signed-off-by: Dr. David Alan Gilbert <linux@treblig.org> > >> --- > >> drivers/media/pci/cx18/cx18-gpio.c | 15 --------------- > >> drivers/media/pci/cx18/cx18-gpio.h | 1 - > >> 2 files changed, 16 deletions(-) > >> > >> diff --git a/drivers/media/pci/cx18/cx18-gpio.c b/drivers/media/pci/cx18/cx18-gpio.c > >> index c85eb8d25837..485a6cbeb15a 100644 > >> --- a/drivers/media/pci/cx18/cx18-gpio.c > >> +++ b/drivers/media/pci/cx18/cx18-gpio.c > >> @@ -305,21 +305,6 @@ int cx18_gpio_register(struct cx18 *cx, u32 hw) > >> return v4l2_device_register_subdev(&cx->v4l2_dev, sd); > >> } > >> > >> -void cx18_reset_ir_gpio(void *data) > >> -{ > >> - struct cx18 *cx = to_cx18(data); > >> - > >> - if (cx->card->gpio_i2c_slave_reset.ir_reset_mask == 0) > >> - return; > >> - > >> - CX18_DEBUG_INFO("Resetting IR microcontroller\n"); > >> - > >> - v4l2_subdev_call(&cx->sd_resetctrl, > >> - core, reset, CX18_GPIO_RESET_Z8F0811); > > Ah, this calls core.reset. But VIDIOC_INT_RESET in cx18_default() does the > same (actually, it calls this for all subdevs). So dropping this code should > be fine since you can still do the same thing with cx18-ctl --reset. That's true. > This function was probably used a long time ago by lirc_pvr150, but I can't > even find the source for that anymore. I assume it has a modern replacement. ir-kbd-i2c.c handles this now, both receiving and sending. I've done some work on this driver and done a fair amount of testing. It certainly is possible to make the firmware hang - by sending it the wrong commands. With the current driver I've never seen any hangs or heard of reports of hangs. > This is the original commit adding this function: > > commit 02fa272fcb6edda9059d6dbaab20dfe919f4f4d2 > Author: Andy Walls <awalls@radix.net> > Date: Sun Jul 13 19:30:15 2008 -0300 > > V4L/DVB (8332): cx18: Suport external reset of the Z8F0811 IR controller on HVR-1600 for lirc > > cx18: added in cx18_ir_reset_gpio function for lirc_pvr150 like module. Also > added the ability to reset the IR chip via ioctl like ivtv. This needs the > mutex to protect gpio_dir and gpio_val in struct cx18 as gpio changes can > come from a few different asynchronous sources now. > > Signed-off-by: Andy Walls <awalls@radix.net> > Signed-off-by: Mauro Carvalho Chehab <mchehab@infradead.org> > > Perhaps Andy remembers how frequent the lock ups were. > > Regards, > > Hans > > >> -} > >> -EXPORT_SYMBOL(cx18_reset_ir_gpio); So I've been looking everywhere including the lirc cvs repo, and I can't where this function is used. Looking at the history with cvs is awful of course, so I may have missed something. I think since we don't know of any lockups and we don't know of any users, we can safely remove the function, i.e. let's apply this patch. The reason I was looking for users of this function is that I thought it would be nice if ir-kbd-i2c could use this function directly, and therefore the cx18-ctl --reset is no longer needed (and no need for the ioctl either). Something for another day. Sean
diff --git a/drivers/media/pci/cx18/cx18-gpio.c b/drivers/media/pci/cx18/cx18-gpio.c index c85eb8d25837..485a6cbeb15a 100644 --- a/drivers/media/pci/cx18/cx18-gpio.c +++ b/drivers/media/pci/cx18/cx18-gpio.c @@ -305,21 +305,6 @@ int cx18_gpio_register(struct cx18 *cx, u32 hw) return v4l2_device_register_subdev(&cx->v4l2_dev, sd); } -void cx18_reset_ir_gpio(void *data) -{ - struct cx18 *cx = to_cx18(data); - - if (cx->card->gpio_i2c_slave_reset.ir_reset_mask == 0) - return; - - CX18_DEBUG_INFO("Resetting IR microcontroller\n"); - - v4l2_subdev_call(&cx->sd_resetctrl, - core, reset, CX18_GPIO_RESET_Z8F0811); -} -EXPORT_SYMBOL(cx18_reset_ir_gpio); -/* This symbol is exported for use by lirc_pvr150 for the IR-blaster */ - /* Xceive tuner reset function */ int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value) { diff --git a/drivers/media/pci/cx18/cx18-gpio.h b/drivers/media/pci/cx18/cx18-gpio.h index 0fa4c7ad2286..8d5797dea7f5 100644 --- a/drivers/media/pci/cx18/cx18-gpio.h +++ b/drivers/media/pci/cx18/cx18-gpio.h @@ -17,5 +17,4 @@ enum cx18_gpio_reset_type { CX18_GPIO_RESET_XC2028 = 2, }; -void cx18_reset_ir_gpio(void *data); int cx18_reset_tuner_gpio(void *dev, int component, int cmd, int value);