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

[Andrey Zhizhikin]

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?

Thanks a lot!

>
> Regards,
>
> Hans
>


--
Regards,
Andrey.