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

Thank you for the review.

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.

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

> > + /* 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-04 15:34    [W:0.790 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site