Date:   Tue, 4 Jan 2022 16:33:52 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     linux-kernel@vger.kernel.org, Watson Chow <watson.chow@avnet.com>,
        Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH 2/2] regulator: Add MAX20086-MAX20089 driver
Message-ID: <YdRa0GoSoX8CP694@pendragon.ideasonboard.com>
References: <20220102211124.18435-1-laurent.pinchart+renesas@ideasonboard.com>
 <20220102211124.18435-3-laurent.pinchart+renesas@ideasonboard.com>
 <YdRWwWmoQGQuUyLz@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YdRWwWmoQGQuUyLz@sirena.org.uk>
Precedence: bulk

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