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

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 11:38:52AM +0200, Hans de Goede wrote:
> On 25-10-2019 09:53, Andy Shevchenko wrote:
> > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote:
> > > This patchset introduces additional regulator driver for Intel Cherry
> > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this
> > > PMIC, which is used to instantiate this regulator.
> > > 
> > > Regulator support for this PMIC was present in kernel release from Intel
> > > targeted Aero platform, but was not entirely ported upstream and has
> > > been omitted in mainline kernel releases. Consecutively, absence of
> > > regulator caused the SD Card interface not to be provided with Vqcc
> > > voltage source needed to operate with UHS-I cards.
> > > 
> > > Following patches are addessing this issue and making sd card interface
> > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id
> > > of the SD Card interface in consumers and exposes optional "vqmmc"
> > > voltage source, which mmc driver uses to switch signalling voltages
> > > between 1.8V and 3.3V.
> > > 
> > > This set contains of 2 patches: one is implementing the regulator driver
> > > (based on a non upstreamed version from Intel Aero), and another patch
> > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver.
> > 
> > Thank you.
> > Hans, Cc'ed, has quite interested in these kind of patches.
> > Am I right, Hans?
> 
> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm
> always interested in CHT specific patches.
> 
> Overall this series looks good (from a high level view I did not
> do a detailed review) but I wonder if we really want to export all the
> regulators when we just need the vsdio one?
> 
> Most regulators are controlled by either the P-Unit inside the CHT SoC
> or through ACPI OpRegion accesses.  Luckily the regulator subsys does not
> expose a sysfs interface for users to directly poke regulators, but this will
> still make it somewhat easier for users to poke regulators which they should
> leave alone.
> 
> Note I'm not saying this is wrong, having support for all regulators in place
> in case we need it in the future is also kinda nice. OTOH if we just need the
> one now, maybe we should just support the one for now ?
> 
> Andy do you have an opinion on this?

Usually on such patches I have two (a bit contradicted) wishes:
- be as little as needed
- for sake of documentation purposes keep as many definitions as we can

But you know the dangling code / definitions not accepted by all maintainers.

So, that said, I have no strong opinion here, basically rely on maintainer's
point of view.

Perhaps also good idea is to put a reference as URL to the published code from
which this has been derived.

> 
> Andrey, may I ask which device you are testing this on?
> 
> Anyways overall good work, thank you for doing this!

+1 to the cheering up!

-- 
With Best Regards,
Andy Shevchenko