Messages in this thread | | | From | Andrey Zhizhikin <> | Date | Tue, 29 Oct 2019 17:57:45 +0100 | Subject | Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC |
| |
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.
> > Regards, > > Hans >
-- Regards, Andrey.
| |