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

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

Hi,

On 28-10-2019 16:01, Andrey Zhizhikin wrote:
> Hi Hans,
> 
> On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> 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:
>> drivers/mfd/intel_soc_pmic_chtwc.c
>>
>> 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.

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

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.

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.

Regards,

Hans