Messages in this thread | | | Date | Thu, 6 Jan 2022 01:23:07 +0200 | From | Laurent Pinchart <> | Subject | Re: [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
| |