[PATCH 0/2] regulator: Add driver for Maxim MAX20086-MAX20089

[PATCH 0/2] regulator: Add driver for Maxim MAX20086-MAX20089

[Laurent Pinchart]

Hello,

This small series adds a new driver (along with DT bindings) for the
Maxim MAX20086-MAX20089 camera power protectors.

The code originates from initial work done by Avnet, which I have
reworked extensively. Watson, I've kept your authorship on patch 2/2.
Would you like to be listed as a maintainer for the driver and the DT
bindings ?

Laurent Pinchart (1):
  dt-bindings: regulators: Add bindings for Maxim MAX20086-MAX20089

Watson Chow (1):
  regulator: Add MAX20086-MAX20089 driver

 .../bindings/regulator/maxim,max20086.yaml    | 116 ++++++
 MAINTAINERS                                   |   7 +
 drivers/regulator/Kconfig                     |  10 +-
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/max20086-regulator.c        | 333 ++++++++++++++++++
 5 files changed, 466 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
 create mode 100644 drivers/regulator/max20086-regulator.c

-- 
Regards,

Laurent Pinchart

[PATCH 1/2] dt-bindings: regulators: Add bindings for Maxim MAX20086-MAX20089

[Laurent Pinchart]

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

The MAX20086-MAX20089 are dual/quad power protectors for cameras. Add
corresponding DT bindings.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../bindings/regulator/maxim,max20086.yaml    | 116 ++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max20086.yaml

diff --git a/Documentation/devicetree/bindings/regulator/maxim,max20086.yaml b/Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
new file mode 100644
index 000000000000..4663716e47a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/maxim,max20086.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim Integrated MAX20086-MAX20089 Camera Power Protector
+
+maintainers:
+  - Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+description: |
+  The MAX20086-MAX20089 are dual/quad camera power protectors, designed to
+  deliver power over coax for radar and camera modules. They support
+  software-configurable output switching and monitoring. The output voltage and
+  current limit are fixed by the hardware design.
+
+properties:
+  compatible:
+    enum:
+      - maxim,max20086
+      - maxim,max20087
+      - maxim,max20088
+      - maxim,max20089
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    maxItems: 1
+    description: GPIO connected to the EN pin, active high
+
+  in-supply:
+    description: Input supply for the camera outputs (IN pin, 3.0V to 15.0V)
+
+  vdd-supply:
+    description: Input supply for the device (VDD pin, 3.0V to 5.5V)
+
+  regulators:
+    type: object
+
+    patternProperties:
+      "^OUT[1-4]$":
+        type: object
+        $ref: regulator.yaml#
+
+    required:
+      - OUT1
+      - OUT2
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - in-supply
+  - vdd-supply
+  - regulators
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max20086
+              - maxim,max20087
+    then:
+      properties:
+        regulators:
+          required:
+            - OUT3
+            - OUT4
+    else:
+      properties:
+        regulators:
+          properties:
+            OUT3: false
+            OUT4: false
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        regulator@28 {
+            compatible = "maxim,max20087";
+            reg = <0x28>;
+
+            in-supply = <&reg_12v0>;
+            vdd-supply = <&reg_3v3>;
+
+            enable-gpios = <&gpio 108 GPIO_ACTIVE_HIGH>;
+
+            regulators {
+                OUT1 {
+                    regulator-name = "VOUT1";
+                };
+                OUT2 {
+                    regulator-name = "VOUT2";
+                };
+                OUT3 {
+                    regulator-name = "VOUT3";
+                };
+                OUT4 {
+                    regulator-name = "VOUT4";
+                };
+            };
+        };
+    };
+...
-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

From: Watson Chow <watson.chow@avnet.com>

The MAX20086-MAX20089 are dual/quad power protectors for cameras. Add a
driver that supports controlling the outputs individually. Additional
features, such as overcurrent detection, may be added later if needed.

Signed-off-by: Watson Chow <watson.chow@avnet.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v0:

- Remove unused regulator_config members
- Drop unused header
- Use SPDX header
- Sort headers alphabetically
- Fix macros indendation
- Removed unused macros
- Remove debugging printks
- Drop IRQ support
- Fix indentation and formatting
- Drop unsupported operations
- Allocate of_regulator_match on the stack
- Make regulator desc macro parameterized
- Reinit regulator_config for each regulator
- Disable interrupts before registering regulators
- Drop error variable in probe()
- Use sizeof(variable) instead of sizeof(type)
- Register regulators last
- Move configuration out of detection
- Move detection to separate function
- Move gpiod_get() to probe()
- Reorganize struct max20087
- Specify device ID in match data
- Specify number of outputs in info structure
- Drop min/max voltages
- Reorder functions
- Don't read CONFIG at probe time
- Fail probe() if disabling outputs fails
- Clean up messages
- Use .probe_new()
- Include vendor prefix in compatible string
- Support MAX20086, 88 and 89
- Reorder Kconfig and Makefile entries
- Reorganize regulator data
- Drop unneeded headers
- Fail if no output is found in DT
- Set enable GPIO after disabling outputs
- Add MAINTAINERS entry
- Depend on GPIOLIB and OF
- Add support for IN supply
---
 MAINTAINERS                            |   7 +
 drivers/regulator/Kconfig              |  10 +-
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/max20086-regulator.c | 333 +++++++++++++++++++++++++
 4 files changed, 350 insertions(+), 1 deletion(-)
 create mode 100644 drivers/regulator/max20086-regulator.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 79fd8a012893..2891e71c0b9e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11577,6 +11577,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/power/supply/maxim,max17042.yaml
 F:	drivers/power/supply/max17042_battery.c
 
+MAXIM MAX20086 CAMERA POWER PROTECTOR DRIVER
+M:	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/regulator/maxim,max20086.yaml
+F:	drivers/regulator/max20086-regulator.c
+
 MAXIM MAX77650 PMIC MFD DRIVER
 M:	Bartosz Golaszewski <brgl@bgdev.pl>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 6be9b1c8a615..dd5c70fe7438 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -636,6 +636,15 @@ config REGULATOR_MAX8998
 	  via I2C bus. The provided regulator is suitable for S3C6410
 	  and S5PC1XX chips to control VCC_CORE and VCC_USIM voltages.
 
+config REGULATOR_MAX20086
+	tristate "Maxim MAX20086-MAX20089 Camera Power Protectors"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This driver controls a Maxim MAX20086-MAX20089 camera power
+	  protectorvia I2C bus. The regulator has 2 or 4 outputs depending on
+	  the device model. This driver is only capable to turn on/off them.
+
 config REGULATOR_MAX77686
 	tristate "Maxim 77686 regulator"
 	depends on MFD_MAX77686 || COMPILE_TEST
@@ -1415,4 +1424,3 @@ config REGULATOR_QCOM_LABIBB
 	  for LCD display panel.
 
 endif
-
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b07d2a22df0b..a9e986231eb8 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_REGULATOR_MAX8952) += max8952.o
 obj-$(CONFIG_REGULATOR_MAX8973) += max8973-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8997) += max8997-regulator.o
 obj-$(CONFIG_REGULATOR_MAX8998) += max8998.o
+obj-$(CONFIG_REGULATOR_MAX20086) += max20086-regulator.o
 obj-$(CONFIG_REGULATOR_MAX77686) += max77686-regulator.o
 obj-$(CONFIG_REGULATOR_MAX77693) += max77693-regulator.o
 obj-$(CONFIG_REGULATOR_MAX77802) += max77802-regulator.o
diff --git a/drivers/regulator/max20086-regulator.c b/drivers/regulator/max20086-regulator.c
new file mode 100644
index 000000000000..8edefb0090c4
--- /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
+ *
+ * Copyright (C) 2022 Laurent Pinchart <laurent.pinchart@idesonboard.com>
+ * Copyright (C) 2018 Avnet, Inc.
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+/* Register Offset */
+#define MAX20086_REG_MASK		0x00
+#define MAX20086_REG_CONFIG		0x01
+#define	MAX20086_REG_ID			0x02
+#define	MAX20086_REG_STAT1		0x03
+#define	MAX20086_REG_STAT2_L		0x04
+#define	MAX20086_REG_STAT2_H		0x05
+#define	MAX20086_REG_ADC1		0x06
+#define	MAX20086_REG_ADC2		0x07
+#define	MAX20086_REG_ADC3		0x08
+#define	MAX20086_REG_ADC4		0x09
+
+/* DEVICE IDs */
+#define MAX20086_DEVICE_ID_MAX20086	0x40
+#define MAX20086_DEVICE_ID_MAX20087	0x20
+#define MAX20086_DEVICE_ID_MAX20088	0x10
+#define MAX20086_DEVICE_ID_MAX20089	0x00
+#define DEVICE_ID_MASK			0xf0
+
+/* Register bits */
+#define MAX20086_EN_MASK		0x0f
+#define MAX20086_EN_OUT1		0x01
+#define MAX20086_EN_OUT2		0x02
+#define MAX20086_EN_OUT3		0x04
+#define MAX20086_EN_OUT4		0x08
+#define MAX20086_INT_DISABLE_ALL	0x3f
+
+#define MAX20086_MAX_REGULATORS		4
+
+struct max20086_chip_info {
+	u8 id;
+	unsigned int num_outputs;
+};
+
+struct max20086_regulator {
+	struct device_node *of_node;
+	struct regulator_init_data *init_data;
+	const struct regulator_desc *desc;
+	struct regulator_dev *rdev;
+};
+
+struct max20086 {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *gpio_en;
+
+	const struct max20086_chip_info *info;
+	unsigned int num_outputs;
+
+	struct max20086_regulator regulators[MAX20086_MAX_REGULATORS];
+};
+
+static const struct regulator_ops max20086_buck_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+};
+
+#define MAX20086_REGULATOR_DESC(n)		\
+{						\
+	.name = max20086_output_names[n],	\
+	.supply_name = "in",			\
+	.id = (n),				\
+	.ops = &max20086_buck_ops,		\
+	.type = REGULATOR_VOLTAGE,		\
+	.owner = THIS_MODULE,			\
+	.enable_reg = MAX20086_REG_CONFIG,	\
+	.enable_mask = (1 << (n)),		\
+	.enable_val = (1 << (n)),		\
+	.disable_val = 0,			\
+}
+
+static const char * const max20086_output_names[] = {
+	"OUT1",
+	"OUT2",
+	"OUT3",
+	"OUT4",
+};
+
+static const struct regulator_desc max20086_regulators[] = {
+	MAX20086_REGULATOR_DESC(0),
+	MAX20086_REGULATOR_DESC(1),
+	MAX20086_REGULATOR_DESC(2),
+	MAX20086_REGULATOR_DESC(3),
+};
+
+static int max20086_regulators_register(struct max20086 *chip)
+{
+	unsigned int i;
+
+	for (i = 0; i < chip->num_outputs; i++) {
+		struct max20086_regulator *reg = &chip->regulators[i];
+		struct regulator_config config = { };
+		struct regulator_dev *rdev;
+
+		config.init_data = reg->init_data;
+		config.dev = chip->dev;
+		config.driver_data = chip;
+		config.regmap = chip->regmap;
+		config.of_node = reg->of_node;
+
+		rdev = devm_regulator_register(chip->dev, reg->desc, &config);
+		if (IS_ERR(rdev)) {
+			dev_err(chip->dev,
+				"Failed to register regulator output %s\n",
+				reg->desc->name);
+			return PTR_ERR(rdev);
+		}
+
+		reg->rdev = rdev;
+	}
+
+	return 0;
+}
+
+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;
+
+	node = of_get_child_by_name(chip->dev->of_node, "regulators");
+	if (!node) {
+		dev_err(chip->dev, "regulators node not found\n");
+		return PTR_ERR(node);
+	}
+
+	for (i = 0; i < chip->info->num_outputs; ++i)
+		matches[i].name = max20086_output_names[i];
+
+	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;
+
+	for (i = 0, n = 0; i < chip->num_outputs; i++) {
+		struct max20086_regulator *reg = &chip->regulators[n];
+
+		if (!matches[i].init_data)
+			continue;
+
+		reg->init_data = matches[i].init_data;
+		reg->of_node = matches[i].of_node;
+		reg->desc = &max20086_regulators[i];
+
+		dev_dbg(chip->dev, "Found output %s\n", reg->desc->name);
+		n++;
+	}
+
+	return 0;
+}
+
+static int max20086_detect(struct max20086 *chip)
+{
+	unsigned int data;
+	int ret;
+
+	ret = regmap_read(chip->regmap, MAX20086_REG_ID, &data);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read DEVICE_ID reg: %d\n", ret);
+		return ret;
+	}
+
+	if ((data & DEVICE_ID_MASK) != chip->info->id) {
+		dev_err(chip->dev, "Invalid device ID 0x%02x\n", data);
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
+static bool max20086_gen_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MAX20086_REG_MASK:
+	case MAX20086_REG_CONFIG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+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,
+};
+
+static int max20086_i2c_probe(struct i2c_client *i2c)
+{
+	struct max20086 *chip;
+	int ret;
+
+	chip = devm_kzalloc(&i2c->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->dev = &i2c->dev;
+	chip->info = device_get_match_data(chip->dev);
+
+	i2c_set_clientdata(i2c, chip);
+
+	chip->regmap = devm_regmap_init_i2c(i2c, &max20086_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		ret = PTR_ERR(chip->regmap);
+		dev_err(chip->dev, "Failed to allocate register map: %d\n", ret);
+		return ret;
+	}
+
+	ret = max20086_parse_regulators_dt(chip);
+	if (ret < 0)
+		return ret;
+
+	ret = max20086_detect(chip);
+	if (ret < 0)
+		return ret;
+
+	/* 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;
+	}
+
+	/* Until IRQ support is added, just disable all interrupts. */
+	ret = regmap_update_bits(chip->regmap, MAX20086_REG_MASK,
+				 MAX20086_INT_DISABLE_ALL,
+				 MAX20086_INT_DISABLE_ALL);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to disable interrupts: %d\n", ret);
+		return ret;
+	}
+
+	/* 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;
+	}
+
+	ret = max20086_regulators_register(chip);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to register regulators: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct i2c_device_id max20086_i2c_id[] = {
+	{ "max20086" },
+	{ "max20087" },
+	{ "max20088" },
+	{ "max20089" },
+	{ /* Sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(i2c, max20086_i2c_id);
+
+static const struct of_device_id max20086_dt_ids[] = {
+	{
+		.compatible = "maxim,max20086",
+		.data = &(const struct max20086_chip_info) {
+			.id = MAX20086_DEVICE_ID_MAX20086,
+			.num_outputs = 4,
+		}
+	}, {
+		.compatible = "maxim,max20087",
+		.data = &(const struct max20086_chip_info) {
+			.id = MAX20086_DEVICE_ID_MAX20087,
+			.num_outputs = 4,
+		}
+	}, {
+		.compatible = "maxim,max20088",
+		.data = &(const struct max20086_chip_info) {
+			.id = MAX20086_DEVICE_ID_MAX20088,
+			.num_outputs = 2,
+		}
+	}, {
+		.compatible = "maxim,max20089",
+		.data = &(const struct max20086_chip_info) {
+			.id = MAX20086_DEVICE_ID_MAX20089,
+			.num_outputs = 2,
+		}
+	},
+	{ /* Sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, max20086_dt_ids);
+
+static struct i2c_driver max20086_regulator_driver = {
+	.driver = {
+		.name = "max20086",
+		.of_match_table = of_match_ptr(max20086_dt_ids),
+	},
+	.probe_new = max20086_i2c_probe,
+	.id_table = max20086_i2c_id,
+};
+
+module_i2c_driver(max20086_regulator_driver);
+
+MODULE_AUTHOR("Watson Chow <watson.chow@avnet.com>");
+MODULE_DESCRIPTION("MAX20086-MAX20089 Camera Power Protector Driver");
+MODULE_LICENSE("GPL");
-- 
Regards,

Laurent Pinchart

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

[Mark Brown]

[-- 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 --]

Re: [PATCH 1/2] dt-bindings: regulators: Add bindings for Maxim MAX20086-MAX20089

[Mark Brown]

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

On Sun, Jan 02, 2022 at 11:11:23PM +0200, Laurent Pinchart wrote:

> +    patternProperties:
> +      "^OUT[1-4]$":
> +        type: object
> +        $ref: regulator.yaml#
> +
> +    required:
> +      - OUT1
> +      - OUT2

Why are we requiring that there be machine constraints for the
individual regulators?  There's already a problem with people just
using the maximum possible control a regulator has as the default for
devices without regard to what the specific system can support.

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

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

[Laurent Pinchart]

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

Re: [PATCH 1/2] dt-bindings: regulators: Add bindings for Maxim MAX20086-MAX20089

[Laurent Pinchart]

Hi Mark,

Thank you for the review.

On Tue, Jan 04, 2022 at 02:26:45PM +0000, Mark Brown wrote:
> On Sun, Jan 02, 2022 at 11:11:23PM +0200, Laurent Pinchart wrote:
> 
> > +    patternProperties:
> > +      "^OUT[1-4]$":
> > +        type: object
> > +        $ref: regulator.yaml#
> > +
> > +    required:
> > +      - OUT1
> > +      - OUT2
> 
> Why are we requiring that there be machine constraints for the
> individual regulators?  There's already a problem with people just
> using the maximum possible control a regulator has as the default for
> devices without regard to what the specific system can support.

Could you elaborate a bit, keeping in mind that I'm a newbie when it
comes to the regulator framework ? :-)

The MAX2008[6789] can't control the voltage or current. The outputs can
be enabled or disabled individually, but their voltage is fixed to the
input voltage. There's overcurrent protection, with a threshold set by a
resistor and not known to the driver. The voltage (and current I
believe) can be measured, and alarms can be raised through an interrupt.

How should I modify the DT bindings to match that correctly ?

-- 
Regards,

Laurent Pinchart

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

[Mark Brown]

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

On Tue, Jan 04, 2022 at 04:33:52PM +0200, Laurent Pinchart wrote:
> On Tue, Jan 04, 2022 at 02:16:33PM +0000, Mark Brown wrote:

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

Yes, just register everything.  Someone doing system debugging or
bringup might want to read the state, and this also goes along with the
ability to have the core pull the constraints out of the DT based on
data supplied by the driver - the general goal is to have routine
drivers simply register data tables with the core and need minimal code.

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

We should really factor that bit out to the core too, though at the
minute we pass in a gpio_desc so it's too late.  Doing the above is
probably best, though I wouldn't loose any sleep over it being missing.
you should definitely set the _NONEXCLUSIVE flag.  If someone specifies
an incompatible mix of settings in the machine constraints I wouldn't
worry about it too much, there's limits on what we can sort out.

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

Re: [PATCH 1/2] dt-bindings: regulators: Add bindings for Maxim MAX20086-MAX20089

[Mark Brown]

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

On Tue, Jan 04, 2022 at 04:43:12PM +0200, Laurent Pinchart wrote:
> On Tue, Jan 04, 2022 at 02:26:45PM +0000, Mark Brown wrote:
> > On Sun, Jan 02, 2022 at 11:11:23PM +0200, Laurent Pinchart wrote:

> > > +    required:
> > > +      - OUT1
> > > +      - OUT2

> > Why are we requiring that there be machine constraints for the
> > individual regulators?  There's already a problem with people just
> > using the maximum possible control a regulator has as the default for
> > devices without regard to what the specific system can support.

> Could you elaborate a bit, keeping in mind that I'm a newbie when it
> comes to the regulator framework ? :-)

Not really...  the question is why we are marking these as required
rather than just letting them be omitted as we normally do for
individual regulators on a device.  What purpose does it serve?

> How should I modify the DT bindings to match that correctly ?

Remove the required:.

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

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

[kernel test robot]

Hi Laurent,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on broonie-regulator/for-next]
[also build test ERROR on linux/master linus/master v5.16-rc8 next-20220104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Laurent-Pinchart/regulator-Add-driver-for-Maxim-MAX20086-MAX20089/20220103-051219
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-r014-20220105 (https://download.01.org/0day-ci/archive/20220105/202201051456.soqOHo2E-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d5b6e30ed3acad794dd0aec400e617daffc6cc3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b6d12d70aa18c4aea48971443a7467c78e719147
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Laurent-Pinchart/regulator-Add-driver-for-Maxim-MAX20086-MAX20089/20220103-051219
        git checkout b6d12d70aa18c4aea48971443a7467c78e719147
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/regulator/max20086-regulator.c:99:2: error: initializer element is not a compile-time constant
           MAX20086_REGULATOR_DESC(0),
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/max20086-regulator.c:79:10: note: expanded from macro 'MAX20086_REGULATOR_DESC'
           .name = max20086_output_names[n],       \
                   ^~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +99 drivers/regulator/max20086-regulator.c

    97	
    98	static const struct regulator_desc max20086_regulators[] = {
  > 99		MAX20086_REGULATOR_DESC(0),
   100		MAX20086_REGULATOR_DESC(1),
   101		MAX20086_REGULATOR_DESC(2),
   102		MAX20086_REGULATOR_DESC(3),
   103	};
   104	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

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

Hi Laurent,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on broonie-regulator/for-next]
[also build test ERROR on linux/master linus/master v5.16-rc8 next-20220104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Laurent-Pinchart/regulator-Add-driver-for-Maxim-MAX20086-MAX20089/20220103-051219
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
config: x86_64-randconfig-r014-20220105 (https://download.01.org/0day-ci/archive/20220105/202201051456.soqOHo2E-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d5b6e30ed3acad794dd0aec400e617daffc6cc3d)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/b6d12d70aa18c4aea48971443a7467c78e719147
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Laurent-Pinchart/regulator-Add-driver-for-Maxim-MAX20086-MAX20089/20220103-051219
        git checkout b6d12d70aa18c4aea48971443a7467c78e719147
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/regulator/max20086-regulator.c:99:2: error: initializer element is not a compile-time constant
           MAX20086_REGULATOR_DESC(0),
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/max20086-regulator.c:79:10: note: expanded from macro 'MAX20086_REGULATOR_DESC'
           .name = max20086_output_names[n],       \
                   ^~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +99 drivers/regulator/max20086-regulator.c

    97	
    98	static const struct regulator_desc max20086_regulators[] = {
  > 99		MAX20086_REGULATOR_DESC(0),
   100		MAX20086_REGULATOR_DESC(1),
   101		MAX20086_REGULATOR_DESC(2),
   102		MAX20086_REGULATOR_DESC(3),
   103	};
   104	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

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

[Laurent Pinchart]

Hi Mark,

On Tue, Jan 04, 2022 at 02:47:26PM +0000, Mark Brown wrote:
> On Tue, Jan 04, 2022 at 04:33:52PM +0200, Laurent Pinchart wrote:
> > On Tue, Jan 04, 2022 at 02:16:33PM +0000, Mark Brown wrote:
> 
> > > > +	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 ?
> 
> Yes, just register everything.  Someone doing system debugging or
> bringup might want to read the state, and this also goes along with the
> ability to have the core pull the constraints out of the DT based on
> data supplied by the driver - the general goal is to have routine
> drivers simply register data tables with the core and need minimal code.

OK, that should simplify the driver. I'll give it a try.

> > > > +	/* 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.
> 
> We should really factor that bit out to the core too, though at the
> minute we pass in a gpio_desc so it's too late.  Doing the above is
> probably best, though I wouldn't loose any sleep over it being missing.
> you should definitely set the _NONEXCLUSIVE flag.  If someone specifies
> an incompatible mix of settings in the machine constraints I wouldn't
> worry about it too much, there's limits on what we can sort out.

I may skip it in the next version then, to first focus on getting the
other bits right.

Note that the outputs can be controlled individually over I2C even when
the enable GPIO is high, so keeping it high unconditionally will only
incur a bit of extra power consumption, it won't have any adverse effect
on the ability to control the outputs.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

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

[Mark Brown]

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

On Thu, Jan 06, 2022 at 01:23:07AM +0200, Laurent Pinchart wrote:
> 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:

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

Right, forgot about that bit - it's more of an issue with the gpiod
conversion.  You could just request always enabled but it's probably
fine to leave as is with looking at but not modifying the constraints.

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

It's not *wrong* if that's the case, it just looks weird to only provide
writable but not readable.

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