Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC

From: Hans de Goede
Date: Tue Oct 29 2019 - 13:14:22 EST


On 29-10-2019 17:57, Andrey Zhizhikin wrote:
Hi Hans!

On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:


On 28-10-2019 16:01, Andrey Zhizhikin wrote:
Hi Hans,

On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:


On 28-10-2019 13:45, Mark Brown wrote:
On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote:
On 25/10/19 10:55 AM, Andy Shevchenko wrote:

Since it's about UHS/SD, Cc to Adrian as well.

My only concern is that the driver might conflict with ACPI methods trying
to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT
with code like this:

Oh, right that is a very good point.

That's certainly what's idiomatic for ACPI (though machine specific
quirks are too!). The safe thing to do would be to only register the
supply on systems where we know there's no ACPI method.

Right, so as I mentioned before Andrey told me about the evaluation
board he is using I was aware of only 3 Cherry Trail devices using
the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo
Yoga book. I've checked the DSDT of all 3 and all 3 of them offer
voltage control through the Intel _DSM method for voltage control.

I've also actually tested this on the GPD win and 1.8V signalling
works fine there without needing Andrey's patch.

Thanks a lot for checking this one out! At least this proves now that
the only platform affected is in fact Intel Aero board, and the patch
as it is might not be necessary to accommodate for all CHT-based
products with Whiskey Cove.

So it seems that Andrey's patch should only be active on his
dev-board, as actual production hardware ships with the _DSM method.

I believe that the best solution is for the Whiskey Cove MFD driver:

To only register the new cell on Andrey's evaluation board model
(based in a DMI match I guess). Another option would be to do
the DMI check in the regulator driver, but that would mean
udev will needlessly modprobe the regulator driver on production
hardware, so doing it in the MFD driver and not registering the cell
seems best,

I tend to lean to a solution to perform a DMI check in MFD rather than
in the regulator driver, since this would keep the regulator
more-or-less agnostic to the where it is running on.

Or maybe it would even make more sense to create a board-specific hook
(like it was suggested for vqmmc voltage and sdmmc ACPI d of
consumer), and then only register a cell for Aero match? This would
actually keep the regulator consumer and mfd cell together and would
not allow the device-specific code to leak into generic driver
implementation. In this case I'd go with mfd_add_cell() if I get a DMI
match and register a vqmmc consumer in there.

In that case, can you please tell me what you think about it and if
the drivers/acpi/acpi_lpss.c would still be an appropriate location to
put this code to?

I do not think that drivers/acpi/acpi_lpss.c is a good place.

Thanks a lot for clarifying this point, I was also not sure whether I
would need combine the platform-specific functionality with LPSS

Thinking a bit more about this, my preferred solutions would be:

1. A BIOS update fixing the DSDT, as Andy suggested. Note we can
lso use an overlay DSDT in the initrd, but that will only help users
which take manual steps to add this to their initrd...

This I believe would not be an option since the Aero platform has been
phased-out from Intel, and Insyde most probably would not do an update
on the BIOS. I can go with DSDT overlay, but I was not sure whether
this is a good way to solve this.

2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo
board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing,
unlike that one you do not need to register a platform_device from
the module_init() function, you can just add the mfd-cell and the
regulator constraints from the module_init() function.

This would be my preferred solution, since in this case I can contain
all the Aero-specific modifications to it's own board file. If there
would be further modifications needed for it - they would be nicely
contained within that board file.

Assuming 1. is not an option (I do not know if Intel still
supports the Aero), then 2 will nicely isolate all the Aero
specific code into a driver which will only auto-load on
the Aero.

Just as I indicated above, chances that BIOS would receive an update
are between slim and nil. If no one have any objection, I'd prefer to
go with the approach [2] from above.

I agree that 2. probably is the best, so I say go for it :)