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

[Hans de Goede]

Hi,

On 15-02-19 10:29, Andy Shevchenko wrote:

On Fri, Feb 15, 2019 at 10:31 AM Hans de Goede  wrote:

On 14-02-19 15:15, Yauhen Kharuzhy 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") ?


Depends on how code would look better,


Well the existing drivers/platform/x86/intel_cht_int33fe.c file,
which already is full of kludges would not get even more code-paths
added; and the new file which Yauhen will wrote should be nice and
clean with only 1 straight code-path pretty much.


though I care about users that
they will not get additional Kconfig option and broken their
configurations when new piece of code landed up. So, from mine, as
user, prospective, we may split driver as we wish, but we should get
it working as previously for the existing cases.


That is a valid point, I'm not a fan of having even more Kconfig
options either, so we can simply enable/disable both modules through
the same Kconfig option.

Regards,

Hans

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

[Andy Shevchenko]

On Fri, Feb 15, 2019 at 10:31 AM Hans de Goede  wrote:
> On 14-02-19 15:15, Yauhen Kharuzhy 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") ?

Depends on how code would look better, though I care about users that
they will not get additional Kconfig option and broken their
configurations when new piece of code landed up. So, from mine, as
user, prospective, we may split driver as we wish, but we should get
it working as previously for the existing cases.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

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.

Regards,

Hans

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

[Chanwoo Choi]

Hi Yauhen,

You are missing me on cc list. I didn't know this patchset.
On next, please use the get_maintainer script in order to
send the correct list.

On 19. 2. 11. 오전 5:36, Yauhen Kharuzhy wrote:
> At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
> based with Whiskey Cove PMIC), two pitfalls were found:
> 
> - for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
>   should be set in 0 (and set to 1 for Host mode). Pick up its definition
>   and logic from from Intel code drop[1];
> 
> - "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
>   connected to one of PMIC outputs controlled by CHGDISCTRL register.
>   Enable charging at driver initialization. Pick up this from Lenovo's code
>   drop[2,3].
> 
> Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
> are based on some kind of reverse engineering and suppositions, correct me if
> this semantic is wrong for common case.
> 
> [1]. 
> https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
> [2]. 
> https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
> [3]. 
> https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch
> 
> Yauhen Kharuzhy (2):
>   extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode
>   extcon intel-cht-wc: Enable external charger
> 
>  drivers/extcon/extcon-intel-cht-wc.c | 71 +++-
>  1 file changed, 70 insertions(+), 1 deletion(-)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

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

[Yauhen Kharuzhy]

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 
> >  > > напісаў:
> > 
> > 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(>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 

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

[Hans de Goede]

Hi,

On 14-02-19 15:15, Yauhen Kharuzhy wrote:



чц, 14 лют 2019, 15.47: карыстальнік Andy Shevchenko mailto:andriy.shevche...@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(>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

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 14-02-19 00:00, Hans de Goede wrote:

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:

At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
based with Whiskey Cove PMIC), two pitfalls were found:

- for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
   should be set in 0 (and set to 1 for Host mode). Pick up its definition
   and logic from from Intel code drop[1];

- "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
   connected to one of PMIC outputs controlled by CHGDISCTRL register.
   Enable charging at driver initialization. Pick up this from Lenovo's code
   drop[2,3].

Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
are based on some kind of reverse engineering and suppositions, correct me if
this semantic is wrong for common case.

[1]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
[2]. 
https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
[3]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch


Thank you for these patches, besides your Lenovo Yoga Book I'm aware of
only 2 other device models using the CHT Whiskey Cove PMIC, the GPD win
and GPD pocket devices. These both work fine without the changes your
patches introduce.

I need to check if your changes do not cause regressions on these 2
devices, which are used with Linux by quite a few people. I will try
to make some time for testing this sometime next week.

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.


Erm, I just realized these are note platform-x86 driver patches at all.

Anyways same request to the extcon maintainers, please do not apply this
until I've had a chance to test this (which I'm doing right now).

Regards,

Hans

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

[Hans de Goede]

Hi,

On 10-02-19 21:36, Yauhen Kharuzhy wrote:

At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
based with Whiskey Cove PMIC), two pitfalls were found:

- for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
   should be set in 0 (and set to 1 for Host mode). Pick up its definition
   and logic from from Intel code drop[1];

- "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
   connected to one of PMIC outputs controlled by CHGDISCTRL register.
   Enable charging at driver initialization. Pick up this from Lenovo's code
   drop[2,3].

Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
are based on some kind of reverse engineering and suppositions, correct me if
this semantic is wrong for common case.

[1]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
[2]. 
https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
[3]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch


Thank you for these patches, besides your Lenovo Yoga Book I'm aware of
only 2 other device models using the CHT Whiskey Cove PMIC, the GPD win
and GPD pocket devices. These both work fine without the changes your
patches introduce.

I need to check if your changes do not cause regressions on these 2
devices, which are used with Linux by quite a few people. I will try
to make some time for testing this sometime next week.

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.

Regards,

Hans

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

[Yauhen Kharuzhy]

At implementation of charging support for Lenovo Yoga Book (Intel Cherry Trail
based with Whiskey Cove PMIC), two pitfalls were found:

- for detection of charger type by PMIC, bit 6 in the CHGRCTRL1 register
  should be set in 0 (and set to 1 for Host mode). Pick up its definition
  and logic from from Intel code drop[1];

- "#CHARGE ENABLE" signal of external charger (bq25892) in Yoga Book is
  connected to one of PMIC outputs controlled by CHGDISCTRL register.
  Enable charging at driver initialization. Pick up this from Lenovo's code
  drop[2,3].

Please keep in mind that I have no docs for Whiskey Cove PMIC, so this patches
are based on some kind of reverse engineering and suppositions, correct me if
this semantic is wrong for common case.

[1]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/0001-power_supply-intel-pmic-ccsm-driver.patch
[2]. 
https://github.com/jekhor/yogabook-linux-android-kernel/blob/b7aa015ab794b516da7b6cb76e5e2d427e3b8b0c/drivers/power/bq2589x_charger.c#L2257
[3]. 
https://github.com/01org/ProductionKernelQuilts/uefi/cht-m1stable/patches/EM-Charger-Disable-battery-charging-in-S3-and-enable.patch

Yauhen Kharuzhy (2):
  extcon-intel-cht-wc: Make charger detection co-existed with OTG host mode
  extcon intel-cht-wc: Enable external charger

 drivers/extcon/extcon-intel-cht-wc.c | 71 +++-
 1 file changed, 70 insertions(+), 1 deletion(-)

-- 
2.20.1