Re: [PATCH] si2157: Add support for Logilink VG0022A.

[Mauro Carvalho Chehab]

Em Fri, 4 Oct 2019 15:50:18 +0200
JP <jp@xxxxxxx> escreveu:

> On 10/4/19 2:08 PM, Mauro Carvalho Chehab wrote:
> > Em Fri, 4 Oct 2019 13:50:43 +0200
> > JP <jp@xxxxxxx> escreveu:
> >  
> >> On 10/3/19 10:03 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Thu, 3 Oct 2019 21:51:35 +0200
> >>> Gonsolo <gonsolo@xxxxxxxxx> escreveu:
> >>>     
> >>>>> 1) The firmware file is likely at the Windows driver for this device
> >>>>> (probably using a different format). It should be possible to get
> >>>>> it from there.  
> >>>> If you tell me how I'm willing to do this. :)  
> >>> I don't know. I was not the one that extracted the firmware. I guess
> >>> Antti did it.
> >>>
> >>> I suspect that there are some comments about that in the past at the
> >>> ML. seek at lore.kernel.org.
> >>>     
> >>>>> 2) Another possibility would be to add a way to tell the si2168 driver
> >>>>> to not try to load a firmware, using the original one. That would
> >>>>> require adding a field at si2168_config to allow signalizing to it
> >>>>> that it should not try to load a firmware file, and add a quirk at
> >>>>> the af9035 that would set such flag for Logilink VG0022A.  
> >>>> I don't get this. Which firmware, si2168 or si2157?  
> >>> The one that it is causing the problem. If I understood well, the
> >>> culprit was the si2168 firmware.
> >>>     
> >>>> I'm still for option 3: If there is a bogus chip revision number it's
> >>>> likely the VG0022A and we can safely set fw to NULL, in which case
> >>>> everything works.
> >>>> All already working devices will continue to work as before.
> >>>> With a low probability there are other devices that will return 0xffff
> >>>> but a) they didn't work until now and b) they receive a clear message
> >>>> that they return bogus numbers and this works just for the VG0022A, in
> >>>> which case this hardware can be tested.
> >>>> At last, *my* VG0022A will work without a custom kernel which I'm a
> >>>> big fan of. :))
> >>>>
> >>>> Are there any counterarguments except that it is not the cleanest
> >>>> solution in the universe? ;)  
> >>> That's a really bad solution. Returning 0xff is what happens when
> >>> things go wrong during I2C transfers. Several problems can cause it,
> >>> including device misfunction. Every time someone comes with a patch
> >>> trying to ignore it, things go sideways for other devices (existing
> >>> or future ones).
> >>>
> >>> Ignoring errors is always a bad idea.  
> >> add module param say 'gonso_hack_vg0022a'
> >> if true, act on error by setting a flag
> >> if this flag is set don't load firmware  
> > Adding a module param should be the last resort, only when there's
> > no way for the driver to autodetect.  
> Remember the guy reported the hw fix? Could be that
> only some receiver units are affected. Therefore the
> module param.
> 
> The hw fix was original 4k7 and 10k added. That looks
> like 3k3 total and all 3 chips on the bus work. 10k per
> chip. Now Gon reported that said bus works with 2 chips
> active on a faulty device with 4k7 resistor, which is 2
> times 10k. It looks same hw error to me.

I'm not so sure. From the reports from Gonsolo, in the case of 
this specific issue with VG0022A, the device is not unstable. It is 
just that it works fine with the vendor-provided firmware, while it 
breaks with the new one.

We don't know that the same thing would happen with the original
reported bug. The only way to be sure would be to obtain the same
hardware from the original post and test it to check if the device
has issues without replacing the resistor.

Even the original reporter can't help, as his device was modified,
and the issue won't be there anymore.

Btw, if we end by noticing this bug happening on other it931x
device models, we could simply disable firmware load for all of them,
but we need more tests and reports before changing the behavior for
other models, as older firmwares may have other problems.

> > Making af9035 to detect vg0022a is quite simple.
> >
> > Considering this device's entry:
> >
> > 	{ DVB_USB_DEVICE(USB_VID_DEXATEK, 0x0100,
> > 		&it930x_props, "Logilink VG0022A", NULL) },
> >
> > the check, at af9035 would be:
> >
> > 	if (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK &&
> > 	    le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100)
> > 		/* do something to disable firmware load */
> >
> > So, no need to add any load time parameter.
> >
> > It should be noticed that a change just at af9035 won't work, as the
> > firmware is updated by si2168 driver. So, the caller code needs to
> > pass a config parameter to si2168 driver.  

> If it is a failing pull-up resistor on only some individual receiver
> units, this seems overkill to me. In my proposal I did not realized
> this change in the demod driver was needed.

Agreed, but we have no means to know that until someone buys other
units of the VG0022A and do tests with and without the patch.

Thanks,
Mauro