Re: [PATCH] media: gp8psk: initialize stats at power control logic

[VDRU VDRU]

Hi Mauro,

After many attempts ret was always 0. Please let me know if further
testing is needed.

Best regards,
Derek

On Mon, Nov 30, 2020 at 9:09 AM Mauro Carvalho Chehab
<mchehab+huawei@xxxxxxxxxx> wrote:
>
> Hi Derek,
>
> Em Mon, 30 Nov 2020 08:04:31 -0800
> VDRU VDRU <user.vdr@xxxxxxxxx> escreveu:
>
> > I have hardware that uses this driver and can conduct a test if it
> > will help resolve any confusion/assumption. I'd also like to suggest
> > that making changes to drivers with no means of testing those changes
> > is bad. This has happened in the past and resulted in unnecessarily
> > breaking drivers for those who use it. No patch should be merged
> > without testing!
>
> It helps a lot if you could test it.
>
> The current situation is that, if the I2C read fails, the
> driver will randomly power up only partially, which could
> cause issues.
>
> The original proposed approach:
>
>         https://lore.kernel.org/linux-media/20190627222020.45909-1-willemdebruijn.kernel@xxxxxxxxx/
>
> Will just give up trying to powering it up, while the
> patch I'm proposing will force the device to power up
> all parts of it. So, it seems safer than the original
> one.
>
> Please test with the enclosed patch. It is basically
> the same as the one I proposed, although this one will
> print a message at dlog, due to this:
>
>         pr_info("ret returned %d\n", ret);
>
> This could happen when the device got plugged and/or if
> you put the machine into suspend mode, when resuming it
> while streaming[1]
>
> Regards,
> Mauro
>
> [1] not sure if dvb-usb supports it. One of the rationales
> behind dvb-usb-v2 were to be able of properly do
> suspend/resumes.
>
>
>
> diff --git a/drivers/media/usb/dvb-usb/gp8psk.c b/drivers/media/usb/dvb-usb/gp8psk.c
> index c07f46f5176e..be55496cc717 100644
> --- a/drivers/media/usb/dvb-usb/gp8psk.c
> +++ b/drivers/media/usb/dvb-usb/gp8psk.c
> @@ -182,11 +182,16 @@ static int gp8psk_load_bcm4500fw(struct dvb_usb_device *d)
>
>  static int gp8psk_power_ctrl(struct dvb_usb_device *d, int onoff)
>  {
> -       u8 status, buf;
> +       u8 status = 0, buf;
> +       int ret;
>         int gp_product_id = le16_to_cpu(d->udev->descriptor.idProduct);
>
>         if (onoff) {
> -               gp8psk_usb_in_op(d, GET_8PSK_CONFIG,0,0,&status,1);
> +               ret = gp8psk_usb_in_op(d, GET_8PSK_CONFIG,0,0,&status,1);
> +               // Just to check if the condition happens in practice
> +               if (ret < 0)
> +                       pr_info("ret returned %d\n", ret);
> +
>                 if (! (status & bm8pskStarted)) {  /* started */
>                         if(gp_product_id == USB_PID_GENPIX_SKYWALKER_CW3K)
>                                 gp8psk_usb_out_op(d, CW3K_INIT, 1, 0, NULL, 0);
>