[v2] media: cx18: Remove unused cx18_reset_ir_gpio

[PATCH v2] media: cx18: Remove unused cx18_reset_ir_gpio

Posted by linux@treblig.org 1 year, 3 months ago

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.

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

Re: [PATCH v2] media: cx18: Remove unused cx18_reset_ir_gpio

Posted by Sean Young 1 year, 3 months ago

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
>

Re: [PATCH v2] media: cx18: Remove unused cx18_reset_ir_gpio

Posted by Hans Verkuil 1 year, 3 months ago

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
>>
>

Re: [PATCH v2] media: cx18: Remove unused cx18_reset_ir_gpio

Posted by Sean Young 1 year, 3 months ago

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

Re: [PATCH v2] media: cx18: Remove unused cx18_reset_ir_gpio

Posted by Andy Walls 1 year, 3 months ago

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);