lkml.org 
[lkml]   [2022]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] regulator: Add MAX20086-MAX20089 driver
Hi Mark,

On Tue, Jan 04, 2022 at 04:33:54PM +0200, Laurent Pinchart wrote:
> On Tue, Jan 04, 2022 at 02:16:33PM +0000, Mark Brown wrote:
> > On Sun, Jan 02, 2022 at 11:11:24PM +0200, Laurent Pinchart wrote:
> >
> > > ---
> > > Changes since v0:
> > >
> > > - Remove unused regulator_config members
> > > - Drop unused header
> >
> > This is a *very* long list relative to something that was never posted
> > :/
>
> I've included it for reference for Watson. It's not meant for upstream,
> I'll drop it in v2.
>
> > > @@ -1415,4 +1424,3 @@ config REGULATOR_QCOM_LABIBB
> > > for LCD display panel.
> > >
> > > endif
> > > -
> >
> > Unrelated whitespace change.
>
> Oops.
>
> > > --- /dev/null
> > > +++ b/drivers/regulator/max20086-regulator.c
> > > @@ -0,0 +1,333 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * max20086-regulator.c - MAX20086-MAX20089 camera power protector driver
> > > + *
> >
> > Please keep the entire comment a C++ one so things look more
> > intentional.
>
> OK.
>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/driver.h>
> > > +#include <linux/regulator/machine.h>
> >
> > It is worrying that a regulator driver should need the interfaces for
> > machines... the driver doesn't look like it actually does though.
>
> I'll try to remove it.

It compiles fine, but I won't be able to check the init data to figure
out the initial enable GPIO state if I don't include machine.h, as
that's where regulator_init_data is defined. Am I missing something ?

> > > +static int max20086_parse_regulators_dt(struct max20086 *chip)
> > > +{
> > > + struct of_regulator_match matches[MAX20086_MAX_REGULATORS] = { };
> > > + struct device_node *node;
> > > + unsigned int i;
> > > + unsigned int n;
> > > + int num;
> >
> > You should be able to remove the stuff about looking for the regulators
> > node and just set of_match and regulators_node in the descs.
>
> I'll give it a try. I'm not very experienced with the regulator
> framework, sorry for the rookie mistakes.
>
> > > + num = of_regulator_match(chip->dev, node, matches,
> > > + chip->info->num_outputs);
> > > + of_node_put(node);
> > > + if (num <= 0) {
> > > + dev_err(chip->dev, "Failed to match regulators\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + chip->num_outputs = num;
> >
> > The number of regulators the device supports should be known from the
> > compatible, I'd expect a data table for this. It should be possible to
> > read the state of regulators not described in the DT.
>
> Does this mean that the driver should register all regulators, even the
> ones not described in DT ? Who would read the state ?
>
> > > +static const struct regmap_config max20086_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .writeable_reg = max20086_gen_is_writeable_reg,
> > > + .max_register = 0x9,
> > > + .cache_type = REGCACHE_NONE,
> > > +};
> >
> > No readback support?
>
> I'll fix that.

Actually I'm not sure what you mean here. All registers are readable,
what's wrong with the above regmap_config ?

> > > + /* Turn off all outputs. */
> > > + ret = regmap_update_bits(chip->regmap, MAX20086_REG_CONFIG,
> > > + MAX20086_EN_MASK, 0);
> > > + if (ret < 0) {
> > > + dev_err(chip->dev, "Failed to disable outputs: %d\n", ret);
> > > + return ret;
> > > + }
> >
> > The driver should not do not do this - the driver should only configure
> > the hardware if told to by the core which in turn will only do this if
> > there's explicit permission to do so in the machine constraints. We
> > don't know what some system integrator might have thought to do with
> > the device.
>
> I'll fix that too (I actually suspected the topic could get raised
> during review :-)).
>
> > > + /* Get the chip out of low-power shutdown state. */
> > > + chip->gpio_en = devm_gpiod_get(chip->dev, "enable", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(chip->gpio_en)) {
> > > + ret = PTR_ERR(chip->gpio_en);
> > > + dev_err(chip->dev, "Failed to get enable GPIO: %d\n", ret);
> > > + return ret;
> > > + }
> >
> > This one is more OK - it's changing the state of the outputs that's an
> > issue. I guess this might cause the outputs to come on though if the
> > GPIO was left off by the bootloader which is awkward. If there's
> > nothing other than the outputs going on with the chip I would be tempted
> > to map this onto the per regulator enable GPIO that the core supports,
> > the core will then be able to manage the low power state at runtime.
> > That's *probably* the least bad option we have with current interfaces.
>
> While fishing for code I can copy in the always unfashionable cargocult
> style, I came across max8973-regulator.c that handles the enable GPIO in
> the following way:
>
> if (ridata && (ridata->constraints.always_on ||
> ridata->constraints.boot_on))
> gflags = GPIOD_OUT_HIGH;
> else
> gflags = GPIOD_OUT_LOW;
> gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
> gpiod = devm_gpiod_get_optional(&client->dev,
> "maxim,enable",
> gflags);
>
> Should I try to replicate that ? It gets more difficult with multiple
> regulators that share the same GPIO. That's why I left it as-is.
>
> > It's a real shame we can't easily get the GPIO state at startup for
> > bootstrapping :/

--
Regards,

Laurent Pinchart

\
 
 \ /
  Last update: 2022-01-06 00:23    [W:0.141 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site