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

From: Hans de Goede
Date: Fri Feb 15 2019 - 04:26:38 EST


On 15-02-19 08:01, Yauhen Kharuzhy wrote:
On Thu, Feb 14, 2019 at 04:05:26PM +0100, Hans de Goede wrote:


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.

Hmm... And we need to handle case when all three INT33FE devices are
enabled in the DSDT...

I would not worry about that, I've never seen that and even then
counting the resources + the PTYP check should catch this.

Instead of separating of driver to two (and more when we will find new
CHT device...) I think about some kind of configuration variants table
with selection by PTYP/resource count/etc. But typeC devices will
require to define interconnections for example and we will get yet
another hardly readable code with quirks, autodetection magic and big
constant tables...

Right, the issue is the code for the variant with the Type-C connector
is just quite ugly, also with the cht_int33fe_check_for_max17047 firmware
bug workaround, I would prefer to keep that isolated to a single
file for just the devices which need all these kludges.

The version for your device should be much cleaner and if we need
another variant we can maybe make your version be a generic version
for this with some config table, but adding more special casing to
the current code is not a good idea IMHO.

OK, I plan to start to make some experiments with this at weekend.

Sounds good.