Re: [PATCH 2/2] HID: i2c-hid: Do not bind to CHPN0001 touchscreen

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Hans,

On Mon, Aug 28, 2017 at 03:04:04PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 18-08-17 00:15, Dmitry Torokhov wrote:
> >On Thu, Aug 17, 2017 at 12:39 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> >>Hey guys,
> >>
> >>Sorry, I don't understand some of the stuff here. But I'd like to
> >>understand it before I add something to the I2C core. Especially as it
> >>feels a bit a the edge of the driver model to me.
> >>
> >>On Sat, Jul 22, 2017 at 08:55:37PM +0200, Hans de Goede wrote:
> >>>The CHPN0001 ACPI device has a _CID of PNP0C50 but is not HID compatible,
> >>>it uses its own protocol which is handled by the chipone_icn8318 driver.
> >>>
> >>>If the i2c_hid_driver's probe functon gets called it will fail with a
> >>>"hid_descr_cmd failed" error.
> >>
> >>That sounds like it fails pretty late. I'd assume we could check the
> >>blacklist right at the beginning of probe and bail out immediately?
> >>
> >>>Worse, after the probe failure the i2c / ACPI core code will put the ACPI
> >>>device in D3 state
> >
> >So I guess this will also happen if I unbind/rebind chipone_icn8318 driver?
> 
> No because on probe the chipone_icn8318 driver does (with the patches
> I've pending) :
> 
>     struct acpi_device *adev;
> 
>     adev = ACPI_COMPANION(dev);
> 
>     /*
>      * Disable ACPI power management the _PS3 method is empty, so
>      * there is no powersaving when using ACPI power management.
>      * The _PS0 method resets the controller causing it to loose its
>      * firmware, which has been loaded by the BIOS and we do not
>      * know how to restore the firmware.
>      */
>     adev->flags.power_manageable = 0;
> 
> So it disables ACPI pm for the device, keeping the controller in D0 so that
> it will never loose its firmware.
> 
> 
> >>Where does that happen? Sorry, I can't find it. Would it be an idea to
> >>add a flag somewhere telling the device should not be put into D3? That
> >>would be way more generic in case this happens outside I2C world, or?
> >>Disclaimer: I am brainstorming here, don't know super much about ACPI.
> >>
> >>>This commit adds a match callback and returns -ENODEV for i2c_client-s
> >>>with a CHPN0001 ACPI device id, so that the probe function never gets
> >>>called, fixing the controller losing its firmware.
> >>
> >>Do you know if something like a match-callback exists somewhere else in
> >>the kernel?
> >
> >Having blacklist means that we are failing at design somewhere...
> >
> >In this case what we have is wrong ACPI table. Instead of adding hooks
> >to i2c core can we fix it (DSDT) up? I know people do not like it, but
> >I'd say this is task for yet another platform driver to adjust ACPI
> >properties (kill the wrong _CID, disable PS0) before instantiating the
> >device.
> 
> I don't think that is going to fly. Certainly DSDT patching is out of the
> question, we could modify the acpi_dev after enumeration, but that is not
> going to be pretty. This is going to be drivers/platform/x86/silead_dmi.c
> all over again, but where that made sense as to decouple the board-info
> from the driver this case is different as we are not talking board
> specific behavior here, but device specific so this really belongs in the
> drivers IMHO, also the reception of silead_dmi.c has not been 100% positive.

No, this is definitely board-specific as well. This is a brain dead
firmware on a particular board that declares PS0 and wrong CID. The
device itself can be made working perfectly well in another box.

I think I also mentioned before that encoding particular platform
behavior in the device driver is not the best option.

I understand that nobody likes silead_dmi as it is nasty (as it shoudl
be, it fixes missing/wrong data suplied by the platform), but I think
the main objection is that we had to make it built-in. I wonder if we
could work on making it usable as a module, by having driver core try
loading "<device-alias>-quirk" module before doing probes so that quirk
is guaranteed to be available?

> 
> I don't think the blacklist entry in i2c-hid is a problem per se, we've
> similar issues with USB devices where they claim a generic class but need
> a specific driver and we typically solve that with a blacklist for the
> specific vid:pid in the class driver.

Yes, you do that for a particular _device_. You move this device to a
brand new box and you still need the same quirk. Here it is the box that
is deficient.

> 
> The nastiness here is not the blacklist, but the pm issues when the wrong
> driver gets its probe() method called first.

BTW, how do we order probing? Should we try matching on _HID before
trying _CID?

Thanks.

-- 
Dmitry