Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator

[Andy Shevchenko]

On Fri, Oct 25, 2019 at 05:26:56PM +0200, Andrey Zhizhikin wrote:
> On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote:
> > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:
> >
> > Please don't take things off-list unless there is a really strong reason
> > to do so.  Sending things to the list ensures that everyone gets a
> > chance to read and comment on things.  (From some of the things
> > in your mail I think this might've been unintentional.)

> Sorry for mess, sometimes it happens but I try not to create it...

Script nodded... :)

> On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
> > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote:

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver
> > > + *
> >
> > Please use C++ style for the entire comment so things look more
> > consistent.
> 
> This is what I'm puzzled about - which style to use for the file
> header since the introduction of SPDX and a rule that it should be
> "C++ style" commented for source files and "C style" for header files.
> After this introduction, should the more-or-less standard header be
> also done in C++ style? I saw different source files are doing
> different things... But all-in-all I would follow you advise here with
> converting entire block to C++.
> 
> [Mark]: The only thing SPDX cares about is the first line, the making
> the rest of the block a C++ one is mostly a preference I have.
> 
> Got it, would be done!

Also remove the file name(s) from file(s). If we would ever rename the file,
its name will be additional churn inside file (or often being forgotten).

> > Is this really a limitation of the *regulator* or is it a
> > limitation of the consumer?  The combination of the way this is
> > written and the register layout makes it look like it's a
> > consumer limitation in which case leave it up to the consumer to
> > figure out what constraints it has.
> 
> This is a tricky point. Since there is no datasheet available from

Key word "publicly".

I may look for it some like in November and perhaps be able to answer to
(some) questions.

> Intel on this IP - I went with a safe option of taking this part from
> original Intel patch, which they did for Aero board as the range was
> described there in exactly this way.

> > This *definitely* appears to be board specific configuration and
> > should be defined for the board.
> 
> Above those two points above: I totally agree this is not the
> regulator configuration but rather a specific board one. The only
> thing I was not able to locate is a correct board file to put this
> into.

See below.

> Maybe you or Hans can guide me here on where to have this code as best?
> 
> [Mark]: I *think* drivers/platform/x86 might be what you're looking
> for but I'm not super familiar with x86.  There's also
> arch/x86/platform but I think they're also trying to push things out
> of arch/.

It's more likely drivers/acpi/acpi_lpss.c.

-- 
With Best Regards,
Andy Shevchenko