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

[Andrey Zhizhikin]

Hi Hans!

On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> 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.

>
> Regards,
>
> Hans
>


--
Regards,
Andrey.