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 29-10-2019 17:57, Andrey Zhizhikin wrote:
> Hi Hans!
> 
> On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> 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.
> 
> Thanks a lot for clarifying this point, I was also not sure whether I
> would need combine the platform-specific functionality with LPSS
> implementation.
> 
>>
>> 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 :)

Regards,

Hans