Date:   Fri, 15 Feb 2019 10:01:03 +0300
From:   Yauhen Kharuzhy <jekhor@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-kernel@...r.kernel.org,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Subject: Re: [PATCH 0/2] extcon: Intel Cherry Trail Whiskey Cove PMIC and
 external charger tweaks

On Thu, Feb 14, 2019 at 04:05:26PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 14-02-19 15:15, Yauhen Kharuzhy wrote:
> > 
> > 
> > чц, 14 лют 2019, 15.47: карыстальнік Andy Shevchenko <andriy.shevchenko@...ux.intel.com <mailto:andriy.shevchenko@...ux.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).

Sounds reasonable. I don't like approach when we store information about
I2C devices inside of I2C bus driver but this kind of autodetection
seems to be lesser evil than a DMI matching.

> 
> 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.

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

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...

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

-- 
Yauhen Kharuzhy

Powered by blists - more mailing lists