Messages in this thread Patch in this message | | | Date | Tue, 4 Jan 2022 16:33:52 +0200 | From | Laurent Pinchart <> | Subject | Re: [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
| |