Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and external charger tweaks

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

Hi,

On 14-02-19 15:15, Yauhen Kharuzhy wrote:
> 
> 
> чц, 14 лют 2019, 15.47: карыстальнік Andy Shevchenko <andriy.shevchenko@linux.intel.com <mailto:andriy.shevchenko@linux.intel.com>> напісаў:
> 
>     On Thu, Feb 14, 2019 at 12:00:44AM +0100, Hans de Goede wrote:
>      > On 10-02-19 21:36, Yauhen Kharuzhy wrote:
> 
>      > A kind request to the platform-x86 driver maintainers (hi Andy): Please
>      > do not apply these patches until I've been able to test they don't cause
>      > issues elsewhere.
> 
>     Yes, that's my plan from the day one.
> 
>     I also asked Yauhen to keep you in Cc list for all patches regarding the
>     platform he is enabling. On his GH page you may find, btw, a pile of patches.
>     I hope we will not get a patch bomb at once.
> 
> 
> 
> Yes, I plan to propose remained patches only after discussing and accepting already sent series and some reworking.
> 
> 
> The charger-related part will be very discussable.

Yes I just took a look at the patches from your kernel-tree at github,
it seems this is another quite "interesting" Cherry Trail device.

Oh if only the engineers who designed these had just use ACPI as intended
instead of doing a bunch of spaghetti code and duck-taping it all together
with proprietary / vendor-specific ACPI opregions. Ah well.

Note I see that your DSDT does not have any *valid* ACPI battery device
(PNP0C0A dev), so we need to directly talk to the fuel-gauge. I also see
that you already have some WIP code for this, good.

Taking a quick peek I also noticed the changes you did for the
drivers/i2c/busses/i2c-cht-wc.c code instantiating the charger device.

I think it would be better to instead of using DMI matching, to
actually probe which device is there, you can create a dummy
client on the adapter after the i2c_add_adapter call using:
i2c_new_dummy() and then you can do an smbus byte read from
register 0x14, on the bq24292i which the other devices with a wcove
pmic have you will get 0xff then since the addresses on the
bq24292i only go up to 0x0a and on the bq2589x your device has
you should then actually be able to check the device id you expect.

If you do this, please also read and check the 0x0a device-id register
of the bq24292i if the 0x14 check fails, I can test this. For the
bq24292i the expectation is for bits 3-5 to encode the value 3 (011).

If you go this route, I would also advice to change the:

if (acpi_dev_present("INT33FE", NULL, -1)) {
	....
}

To:

if (!acpi_dev_present("INT33FE", NULL, -1))
	goto done;

So that you don't get a too deep indentation level, making
the end result look something like this:


if (!acpi_dev_present("INT33FE", NULL, -1))
	goto done;

test_client = i2c_new_dummy(&adap->adapter, 0x14);
// test for bq25892 or bq24292i
i2c_unregister_device(test_client);
// register correct device, this must be done after
// unregister-ing the dummy to avoid an EBUSY error on the address

done:
platform_set_drvdata(pdev, adap);
return 0;

###

I would do something similar with the fuel-gauge in
drivers/platform/x86/intel_cht_int33fe.c, one option would
be to simply count the number of resources in the ACPI
resource table for the INT33FE device, versions with
the Type-C port have 7 resources, where as your INT33FE
device has only 3.

I'm even thinking that it might be best to rename
intel_cht_int33fe.c to intel_cht_int33fe_typec.c and add
a check for the resource table having 7 entries there, then
you can make a intel_cht_int33fe_micro_usb.c copy and strip
that mostly empty. Both would bind to the same "INT33FE"
id and they would both silently bail with -ENODEV if the
resource-count (or the PTYP value) don't match.

The reason I'm thinking of having 2 drivers is because
the current intel_cht_int33fe.c is quite special / ugly and
already has enough ifs.

If you do a stand-alone intel_cht_int33fe_micro_usb.c that can
hopefully be much simpler.

Andy what is your take on having separate intel_cht_int33fe_typec.c
and intel_cht_int33fe_micro_usb.c drivers, both binding to
the "INT33FE" ACPI-ID (with its totally !@#%$#-ed up "API") ?

Having 2 drivers bind to the same id and exit silently with -ENODEV
is somewhat normal for USB ids where we also sometimes have these
kinda ID clashes with different devices hiding behind the same id.

Regards,

Hans