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

[Hans de Goede <hdegoede@redhat.com>]

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.

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.

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

Regards,

Hans