Re: [PATCH 2/2] regulator: Add MAX20086-MAX20089 driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

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
:/

> @@ -1415,4 +1424,3 @@ config REGULATOR_QCOM_LABIBB
>  	  for LCD display panel.
>  
>  endif
> -

Unrelated whitespace change.

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

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

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

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

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

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

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

It's a real shame we can't easily get the GPIO state at startup for
bootstrapping :/  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]