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

[Hans de Goede]

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

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

Regards,

Hans