[PATCH v2 0/3] regulator: Add Maxim MAX20411 support

[PATCH v2 0/3] regulator: Add Maxim MAX20411 support

[Bjorn Andersson]

Introduce binding and driver for the Maxim MAX20411, and wire these up
on the Qualcomm SA8295P ADP.

Bjorn Andersson (3):
  regulator: dt-bindings: Describe Maxim MAX20411
  regulator: Introduce Maxim MAX20411 Step-Down converter
  arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

 .../bindings/regulator/maxim,max20411.yaml    |  58 +++++++
 arch/arm64/boot/dts/qcom/sa8295p-adp.dts      |  41 +++++
 drivers/regulator/Kconfig                     |   8 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/max20411-regulator.c        | 163 ++++++++++++++++++
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max20411.yaml
 create mode 100644 drivers/regulator/max20411-regulator.c

-- 
2.25.1

[PATCH v2 1/3] regulator: dt-bindings: Describe Maxim MAX20411

[Bjorn Andersson]

Add binding for the Maxim MAX20411 step-down DC-DC converter.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

Changes since v1:
- Swapped "dt-bindings" and "regulator" in $subject
- Dropped maxItems from enable-gpios

 .../bindings/regulator/maxim,max20411.yaml    | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max20411.yaml

diff --git a/Documentation/devicetree/bindings/regulator/maxim,max20411.yaml b/Documentation/devicetree/bindings/regulator/maxim,max20411.yaml
new file mode 100644
index 000000000000..5b3a42d24e51
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/maxim,max20411.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/maxim,max20411.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim Integrated MAX20411 Step-Down DC-DC Converter
+
+maintainers:
+  - Bjorn Andersson <andersson@kernel.org>
+
+description:
+  The MAX20411 is a high-efficiency, DC-DC step-down converter. It provides
+  configurable output voltage in the range of 0.5V to 1.275V, configurable over
+  I2C.
+
+allOf:
+  - $ref: regulator.yaml#
+
+properties:
+  compatible:
+    const: maxim,max20411
+
+  reg:
+    maxItems: 1
+
+  enable-gpios:
+    description: GPIO connected to the EN pin, active high
+
+  vdd-supply:
+    description: Input supply for the device (VDD pin, 3.0V to 5.5V)
+
+required:
+  - compatible
+  - reg
+  - enable-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        regulator@39 {
+            compatible = "maxim,max20411";
+            reg = <0x39>;
+
+            enable-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+
+            regulator-min-microvolt = <800000>;
+            regulator-max-microvolt = <1000000>;
+        };
+    };
+...
-- 
2.25.1

[PATCH v2 2/3] regulator: Introduce Maxim MAX20411 Step-Down converter

[Bjorn Andersson]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

Introduce a driver to control the Maxim MAX20411 family of
high-efficiency, syncrhonous step-down converters.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

Changes since v1:
- Extracted regulator_desc initialization from max20411_probe()

 drivers/regulator/Kconfig              |   8 ++
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/max20411-regulator.c | 163 +++++++++++++++++++++++++
 3 files changed, 172 insertions(+)
 create mode 100644 drivers/regulator/max20411-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 820c9a0788e5..aae28d0a489c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -655,6 +655,14 @@ config REGULATOR_MAX20086
 	  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_MAX20411
+	tristate "Maxim MAX20411 High-Efficiency Single Step-Down Converter"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  This driver controls the Maxim MAX20411 family of high-efficiency,
+	  syncrhonous step-down converters.
+
 config REGULATOR_MAX77686
 	tristate "Maxim 77686 regulator"
 	depends on MFD_MAX77686 || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index b9f5eb35bf5f..ee383d8fc835 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -80,6 +80,7 @@ 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_MAX20411) += max20411-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/max20411-regulator.c b/drivers/regulator/max20411-regulator.c
new file mode 100644
index 000000000000..69f04cbe69f1
--- /dev/null
+++ b/drivers/regulator/max20411-regulator.c
@@ -0,0 +1,163 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2022, Linaro Ltd.
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
+#define MAX20411_UV_STEP		6250
+#define MAX20411_BASE_UV		243750
+#define MAX20411_MIN_SEL		41 /* 0.5V */
+#define MAX20411_MAX_SEL		165 /* 1.275V */
+#define MAX20411_VID_OFFSET		0x7
+#define MAX20411_VID_MASK		0xff
+#define MAX20411_SLEW_OFFSET		0x6
+#define MAX20411_SLEW_DVS_MASK		0xc
+#define MAX20411_SLEW_SR_MASK		0x3
+
+struct max20411 {
+	struct device *dev;
+	struct device_node *of_node;
+	struct regulator_desc desc;
+	struct regulator_dev *rdev;
+	struct regmap *regmap;
+};
+
+static const unsigned int max20411_slew_rates[] = { 13100, 6600, 3300, 1600 };
+
+static int max20411_enable_time(struct regulator_dev *rdev)
+{
+	int voltage, rate, ret;
+	unsigned int val;
+
+	/* get voltage */
+	ret = regmap_read(rdev->regmap, rdev->desc->vsel_reg, &val);
+	if (ret)
+		return ret;
+
+	val &= rdev->desc->vsel_mask;
+	voltage = regulator_list_voltage_linear(rdev, val);
+
+	/* get rate */
+	ret = regmap_read(rdev->regmap, MAX20411_SLEW_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(MAX20411_SLEW_SR_MASK, val);
+	rate = max20411_slew_rates[val];
+
+	return DIV_ROUND_UP(voltage, rate);
+}
+
+static const struct regmap_config max20411_regmap_config = {
+	.reg_bits		= 8,
+	.val_bits		= 8,
+	.max_register		= 0xe,
+};
+
+static const struct regulator_ops max20411_ops = {
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear,
+	.enable_time		= max20411_enable_time,
+};
+
+static const struct regulator_desc max20411_desc = {
+	.ops = &max20411_ops,
+	.owner = THIS_MODULE,
+	.type = REGULATOR_VOLTAGE,
+	.supply_name = "vin",
+	.name = "max20411",
+
+	/*
+	 * voltage = 0.24375V + selector * 6.25mV
+	 * with valid selector between 41 to 165 (0.5V to 1.275V)
+	 */
+	.min_uV = MAX20411_BASE_UV,
+	.uV_step = MAX20411_UV_STEP,
+	.linear_min_sel = MAX20411_MIN_SEL,
+	.n_voltages = MAX20411_MAX_SEL,
+
+	.vsel_reg = MAX20411_VID_OFFSET,
+	.vsel_mask = MAX20411_VID_MASK,
+
+	.ramp_reg = MAX20411_SLEW_OFFSET,
+	.ramp_mask = MAX20411_SLEW_DVS_MASK,
+	.ramp_delay_table = max20411_slew_rates,
+	.n_ramp_values = ARRAY_SIZE(max20411_slew_rates),
+};
+
+static int max20411_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct regulator_init_data *init_data;
+	struct device *dev = &client->dev;
+	struct regulator_config cfg = {};
+	struct max20411 *max20411;
+
+	max20411 = devm_kzalloc(dev, sizeof(*max20411), GFP_KERNEL);
+	if (!max20411)
+		return -ENOMEM;
+
+	max20411->regmap = devm_regmap_init_i2c(client, &max20411_regmap_config);
+	if (IS_ERR(max20411->regmap)) {
+		dev_err(dev, "Failed to allocate regmap!\n");
+		return PTR_ERR(max20411->regmap);
+	}
+
+	max20411->dev = dev;
+	max20411->of_node = dev->of_node;
+
+	max20411->desc = max20411_desc;
+	init_data = of_get_regulator_init_data(max20411->dev, max20411->of_node, &max20411->desc);
+	if (!init_data)
+		return -ENODATA;
+
+	cfg.dev = max20411->dev;
+	cfg.init_data = init_data;
+	cfg.of_node = max20411->of_node;
+	cfg.driver_data = max20411;
+
+	cfg.ena_gpiod = gpiod_get(max20411->dev, "enable", GPIOD_ASIS);
+	if (IS_ERR(cfg.ena_gpiod))
+		return dev_err_probe(dev, PTR_ERR(cfg.ena_gpiod),
+				     "unable to acquire enable gpio\n");
+
+	max20411->rdev = devm_regulator_register(max20411->dev, &max20411->desc, &cfg);
+	if (IS_ERR(max20411->rdev))
+		dev_err(max20411->dev, "Failed to register regulator\n");
+
+	return PTR_ERR_OR_ZERO(max20411->rdev);
+}
+
+static const struct of_device_id of_max20411_match_tbl[] = {
+	{ .compatible = "maxim,max20411", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_max20411_match_tbl);
+
+static const struct i2c_device_id max20411_id[] = {
+	{ "max20411", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, max20411_id);
+
+static struct i2c_driver max20411_i2c_driver = {
+	.driver	= {
+		.name = "max20411",
+		.of_match_table	= of_max20411_match_tbl,
+	},
+	.probe = max20411_probe,
+	.id_table = max20411_id,
+};
+module_i2c_driver(max20411_i2c_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Bjorn Andersson]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---

Changes since v1:
- i2c node had changed name

 arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index bb4270e8f551..642000d95812 100644
--- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
@@ -266,6 +266,27 @@ &dispcc1 {
 	status = "okay";
 };
 
+&i2c12 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c12_state>;
+
+	status = "okay";
+
+	vdd_gfx: regulator@39 {
+		compatible = "maxim,max20411";
+		reg = <0x39>;
+
+		regulator-name = "vdd_gfx";
+		regulator-min-microvolt = <800000>;
+		regulator-max-microvolt = <968750>;
+
+		enable-gpios = <&pmm8540a_gpios 2 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&vdd_gfx_enable_state>;
+	};
+};
+
 &mdss0 {
 	status = "okay";
 };
@@ -476,6 +497,10 @@ &pcie4_phy {
 	status = "okay";
 };
 
+&qup1 {
+	status = "okay";
+};
+
 &qup2 {
 	status = "okay";
 };
@@ -636,7 +661,23 @@ &xo_board_clk {
 
 /* PINCTRL */
 
+&pmm8540a_gpios {
+	vdd_gfx_enable_state: vdd-gfx-enable-state {
+		pins = "gpio2";
+		function = "normal";
+		output-enable;
+	};
+};
+
 &tlmm {
+	i2c12_state: i2c12-state {
+		pins = "gpio0", "gpio1";
+		function = "qup12";
+
+		drive-strength = <2>;
+		bias-pull-up;
+	};
+
 	pcie2a_default: pcie2a-default-state {
 		clkreq-n-pins {
 			pins = "gpio142";
-- 
2.25.1

Re: [PATCH v2 0/3] regulator: Add Maxim MAX20411 support

[Mark Brown]

On Tue, 24 Jan 2023 10:44:37 -0800, Bjorn Andersson wrote:
> Introduce binding and driver for the Maxim MAX20411, and wire these up
> on the Qualcomm SA8295P ADP.
> 
> Bjorn Andersson (3):
>   regulator: dt-bindings: Describe Maxim MAX20411
>   regulator: Introduce Maxim MAX20411 Step-Down converter
>   arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12
> 
> [...]

Applied to

   broonie/regulator.git for-next

Thanks!

[1/3] regulator: dt-bindings: Describe Maxim MAX20411
      commit: c1bf8de25d0aa6e399399d6410b1140d4402c2e0
[2/3] regulator: Introduce Maxim MAX20411 Step-Down converter
      commit: 047ebaffd8171a47eb5462aec0f6006416fbe62e
[3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Andrew Halaney]

On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> 
> Changes since v1:
> - i2c node had changed name
> 
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)

I realized today this has to do with the comment over at:

    https://lore.kernel.org/all/30166208-ba9d-e6e6-1cd2-807a80536052@quicinc.com/

and I just didn't realize that the schematic I've started looking at
black boxes the SOM/SIP which holds this... darn I thought I could see
more than I could :(

I took a similiar patch for a spin on sa8540p-ride (which I'll later
submit), and things worked fine (I'm not really consuming the output of
the regulator mind you).

Downstream devicetree indicates all of this looks ok except for possibly
the below comment:

> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index bb4270e8f551..642000d95812 100644
> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> @@ -266,6 +266,27 @@ &dispcc1 {
>  	status = "okay";
>  };
>  
> +&i2c12 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c12_state>;
> +
> +	status = "okay";
> +
> +	vdd_gfx: regulator@39 {
> +		compatible = "maxim,max20411";
> +		reg = <0x39>;
> +
> +		regulator-name = "vdd_gfx";
> +		regulator-min-microvolt = <800000>;

Is there a reason you chose this instead of the 500000 I see downstream?

> +		regulator-max-microvolt = <968750>;

Likewise, I see in this brief description of the regulator
that the upper bound is higher than this (1.275 V). I am not sure if
the values in the devicetree are supposed to describe the
min/max of the regulator itself, or of what your board can really
handle/needs (the latter I guess makes more sense since you wouldn't want to
accidentally request a current draw that could melt something.. that can
be fun). I do see you've got that min/max in the driver itself (now that
I peaked at that patch).

https://www.analog.com/en/products/MAX20411.html#product-overview

For what it is worth, I also see a SIP document that states vdd_gfx min/max
is 0.56/1.03 V, which is ultimately what you'd feed this into. The
downstream devicetree uses the max value you provide though.

No idea how much faith I should put into the SIP document's bounds, or
downstream, but I thought I should at least highlight them.

Thanks,
Andrew

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Konrad Dybcio]

On 26.01.2023 23:54, Andrew Halaney wrote:
> On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
>> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>>
>> The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
>>
>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>> ---
>>
>> Changes since v1:
>> - i2c node had changed name
>>
>>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
> 
> I realized today this has to do with the comment over at:
> 
>     https://lore.kernel.org/all/30166208-ba9d-e6e6-1cd2-807a80536052@quicinc.com/
> 
> and I just didn't realize that the schematic I've started looking at
> black boxes the SOM/SIP which holds this... darn I thought I could see
> more than I could :(
> 
> I took a similiar patch for a spin on sa8540p-ride (which I'll later
> submit), and things worked fine (I'm not really consuming the output of
> the regulator mind you).
> 
> Downstream devicetree indicates all of this looks ok except for possibly
> the below comment:
> 
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> index bb4270e8f551..642000d95812 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> @@ -266,6 +266,27 @@ &dispcc1 {
>>  	status = "okay";
>>  };
>>  
>> +&i2c12 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&i2c12_state>;
>> +
>> +	status = "okay";
>> +
>> +	vdd_gfx: regulator@39 {
>> +		compatible = "maxim,max20411";
>> +		reg = <0x39>;
>> +
>> +		regulator-name = "vdd_gfx";
>> +		regulator-min-microvolt = <800000>;
> 
> Is there a reason you chose this instead of the 500000 I see downstream?
> 
>> +		regulator-max-microvolt = <968750>;
> 
> Likewise, I see in this brief description of the regulator
> that the upper bound is higher than this (1.275 V). I am not sure if
> the values in the devicetree are supposed to describe the
> min/max of the regulator itself, or of what your board can really
> handle/needs (the latter I guess makes more sense since you wouldn't want to
> accidentally request a current draw that could melt something.. that can
> be fun). I do see you've got that min/max in the driver itself (now that
> I peaked at that patch).
Yes, your suspicions are correct and the DT sets the actual ranges
for the voltage regulators on this specific board while the
hardware reachable ranges are defined in the .c driver.

Konrad
> 
> https://www.analog.com/en/products/MAX20411.html#product-overview
> 
> For what it is worth, I also see a SIP document that states vdd_gfx min/max
> is 0.56/1.03 V, which is ultimately what you'd feed this into. The
> downstream devicetree uses the max value you provide though.
> 
> No idea how much faith I should put into the SIP document's bounds, or
> downstream, but I thought I should at least highlight them.
> 
> Thanks,
> Andrew
> 

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Andrew Halaney]

On Fri, Jan 27, 2023 at 12:35:37AM +0100, Konrad Dybcio wrote:
> 
> 
> On 26.01.2023 23:54, Andrew Halaney wrote:
> > On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> >> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>
> >> The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> >>
> >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> >> ---
> >>
> >> Changes since v1:
> >> - i2c node had changed name
> >>
> >>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
> >>  1 file changed, 41 insertions(+)
> > 
> > I realized today this has to do with the comment over at:
> > 
> >     https://lore.kernel.org/all/30166208-ba9d-e6e6-1cd2-807a80536052@quicinc.com/
> > 
> > and I just didn't realize that the schematic I've started looking at
> > black boxes the SOM/SIP which holds this... darn I thought I could see
> > more than I could :(
> > 
> > I took a similiar patch for a spin on sa8540p-ride (which I'll later
> > submit), and things worked fine (I'm not really consuming the output of
> > the regulator mind you).
> > 
> > Downstream devicetree indicates all of this looks ok except for possibly
> > the below comment:
> > 
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> index bb4270e8f551..642000d95812 100644
> >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> >> @@ -266,6 +266,27 @@ &dispcc1 {
> >>  	status = "okay";
> >>  };
> >>  
> >> +&i2c12 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&i2c12_state>;
> >> +
> >> +	status = "okay";
> >> +
> >> +	vdd_gfx: regulator@39 {
> >> +		compatible = "maxim,max20411";
> >> +		reg = <0x39>;
> >> +
> >> +		regulator-name = "vdd_gfx";
> >> +		regulator-min-microvolt = <800000>;
> > 
> > Is there a reason you chose this instead of the 500000 I see downstream?
> > 
> >> +		regulator-max-microvolt = <968750>;
> > 
> > Likewise, I see in this brief description of the regulator
> > that the upper bound is higher than this (1.275 V). I am not sure if
> > the values in the devicetree are supposed to describe the
> > min/max of the regulator itself, or of what your board can really
> > handle/needs (the latter I guess makes more sense since you wouldn't want to
> > accidentally request a current draw that could melt something.. that can
> > be fun). I do see you've got that min/max in the driver itself (now that
> > I peaked at that patch).
> Yes, your suspicions are correct and the DT sets the actual ranges
> for the voltage regulators on this specific board while the
> hardware reachable ranges are defined in the .c driver.
> 
> Konrad

Thanks Konrad, then I think:

Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
Tested-by: Andrew Halaney <ahalaney@redhat.com>

is appropriate since things are within range on all accounts. I would
appreciate an explanation on the current min/max values though if possible!

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Johan Hovold]

On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
> 
> Changes since v1:
> - i2c node had changed name
> 
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index bb4270e8f551..642000d95812 100644
> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> @@ -266,6 +266,27 @@ &dispcc1 {
>  	status = "okay";
>  };
>  
> +&i2c12 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c12_state>;
> +
> +	status = "okay";
> +
> +	vdd_gfx: regulator@39 {

Nit: Should the label be named 'vreg_gfx' (or 'vreg_vdd_gfx)') for
consistency with rest of the file?

> +		compatible = "maxim,max20411";
> +		reg = <0x39>;
> +
> +		regulator-name = "vdd_gfx";
> +		regulator-min-microvolt = <800000>;
> +		regulator-max-microvolt = <968750>;
> +
> +		enable-gpios = <&pmm8540a_gpios 2 GPIO_ACTIVE_HIGH>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vdd_gfx_enable_state>;
> +	};
> +};
> +
>  &mdss0 {
>  	status = "okay";
>  };
> @@ -476,6 +497,10 @@ &pcie4_phy {
>  	status = "okay";
>  };
>  
> +&qup1 {
> +	status = "okay";
> +};
> +
>  &qup2 {
>  	status = "okay";
>  };
> @@ -636,7 +661,23 @@ &xo_board_clk {
>  
>  /* PINCTRL */
>  
> +&pmm8540a_gpios {
> +	vdd_gfx_enable_state: vdd-gfx-enable-state {

For consistency with the rest of sc8280xp, can you rename this

	vdd_gfx_en: vdd-gfx-en-state {

(i.e. drop the 'state' from the label and shorten 'enable')?

> +		pins = "gpio2";
> +		function = "normal";
> +		output-enable;
> +	};
> +};
> +
>  &tlmm {
> +	i2c12_state: i2c12-state {

Similar here, this should be

	i2c12_default: i2c12-default-state {

> +		pins = "gpio0", "gpio1";
> +		function = "qup12";
> +

And this newline can be removed.

> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
>  	pcie2a_default: pcie2a-default-state {
>  		clkreq-n-pins {
>  			pins = "gpio142";

Johan

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Bjorn Andersson]

On Fri, Jan 27, 2023 at 10:55:21AM +0100, Johan Hovold wrote:
> On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> > 
> > Changes since v1:
> > - i2c node had changed name
> > 
> >  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > index bb4270e8f551..642000d95812 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > @@ -266,6 +266,27 @@ &dispcc1 {
> >  	status = "okay";
> >  };
> >  
> > +&i2c12 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c12_state>;
> > +
> > +	status = "okay";
> > +
> > +	vdd_gfx: regulator@39 {
> 
> Nit: Should the label be named 'vreg_gfx' (or 'vreg_vdd_gfx)') for
> consistency with rest of the file?
> 

I will investigate what the appropriate name is, consistency would be
nice though.

> > +		compatible = "maxim,max20411";
> > +		reg = <0x39>;
> > +
> > +		regulator-name = "vdd_gfx";
> > +		regulator-min-microvolt = <800000>;
> > +		regulator-max-microvolt = <968750>;
> > +
> > +		enable-gpios = <&pmm8540a_gpios 2 GPIO_ACTIVE_HIGH>;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&vdd_gfx_enable_state>;
> > +	};
> > +};
> > +
> >  &mdss0 {
> >  	status = "okay";
> >  };
> > @@ -476,6 +497,10 @@ &pcie4_phy {
> >  	status = "okay";
> >  };
> >  
> > +&qup1 {
> > +	status = "okay";
> > +};
> > +
> >  &qup2 {
> >  	status = "okay";
> >  };
> > @@ -636,7 +661,23 @@ &xo_board_clk {
> >  
> >  /* PINCTRL */
> >  
> > +&pmm8540a_gpios {
> > +	vdd_gfx_enable_state: vdd-gfx-enable-state {
> 
> For consistency with the rest of sc8280xp, can you rename this
> 
> 	vdd_gfx_en: vdd-gfx-en-state {
> 
> (i.e. drop the 'state' from the label and shorten 'enable')?
> 

Will do.

> > +		pins = "gpio2";
> > +		function = "normal";
> > +		output-enable;
> > +	};
> > +};
> > +
> >  &tlmm {
> > +	i2c12_state: i2c12-state {
> 
> Similar here, this should be
> 
> 	i2c12_default: i2c12-default-state {
> 

Sounds reasonable.

> > +		pins = "gpio0", "gpio1";
> > +		function = "qup12";
> > +
> 
> And this newline can be removed.
> 

Sure

> > +		drive-strength = <2>;
> > +		bias-pull-up;
> > +	};
> > +
> >  	pcie2a_default: pcie2a-default-state {
> >  		clkreq-n-pins {
> >  			pins = "gpio142";
> 
> Johan

Thanks Johan,
Bjorn

Re: [PATCH v2 3/3] arm64: dts: qcom: sa8295p-adp: Add max20411 on i2c12

[Bjorn Andersson]

On Thu, Jan 26, 2023 at 05:43:54PM -0600, Andrew Halaney wrote:
> On Fri, Jan 27, 2023 at 12:35:37AM +0100, Konrad Dybcio wrote:
> > 
> > 
> > On 26.01.2023 23:54, Andrew Halaney wrote:
> > > On Tue, Jan 24, 2023 at 10:44:40AM -0800, Bjorn Andersson wrote:
> > >> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >>
> > >> The SA8295P ADP has a Maxim max20411 step-down converter on i2c12.
> > >>
> > >> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > >> ---
> > >>
> > >> Changes since v1:
> > >> - i2c node had changed name
> > >>
> > >>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 41 ++++++++++++++++++++++++
> > >>  1 file changed, 41 insertions(+)
> > > 
> > > I realized today this has to do with the comment over at:
> > > 
> > >     https://lore.kernel.org/all/30166208-ba9d-e6e6-1cd2-807a80536052@quicinc.com/
> > > 
> > > and I just didn't realize that the schematic I've started looking at
> > > black boxes the SOM/SIP which holds this... darn I thought I could see
> > > more than I could :(
> > > 
> > > I took a similiar patch for a spin on sa8540p-ride (which I'll later
> > > submit), and things worked fine (I'm not really consuming the output of
> > > the regulator mind you).
> > > 
> > > Downstream devicetree indicates all of this looks ok except for possibly
> > > the below comment:
> > > 
> > >>
> > >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > >> index bb4270e8f551..642000d95812 100644
> > >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> > >> @@ -266,6 +266,27 @@ &dispcc1 {
> > >>  	status = "okay";
> > >>  };
> > >>  
> > >> +&i2c12 {
> > >> +	pinctrl-names = "default";
> > >> +	pinctrl-0 = <&i2c12_state>;
> > >> +
> > >> +	status = "okay";
> > >> +
> > >> +	vdd_gfx: regulator@39 {
> > >> +		compatible = "maxim,max20411";
> > >> +		reg = <0x39>;
> > >> +
> > >> +		regulator-name = "vdd_gfx";
> > >> +		regulator-min-microvolt = <800000>;
> > > 
> > > Is there a reason you chose this instead of the 500000 I see downstream?
> > > 
> > >> +		regulator-max-microvolt = <968750>;
> > > 
> > > Likewise, I see in this brief description of the regulator
> > > that the upper bound is higher than this (1.275 V). I am not sure if
> > > the values in the devicetree are supposed to describe the
> > > min/max of the regulator itself, or of what your board can really
> > > handle/needs (the latter I guess makes more sense since you wouldn't want to
> > > accidentally request a current draw that could melt something.. that can
> > > be fun). I do see you've got that min/max in the driver itself (now that
> > > I peaked at that patch).
> > Yes, your suspicions are correct and the DT sets the actual ranges
> > for the voltage regulators on this specific board while the
> > hardware reachable ranges are defined in the .c driver.
> > 
> > Konrad
> 
> Thanks Konrad, then I think:
> 
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> Tested-by: Andrew Halaney <ahalaney@redhat.com>
> 
> is appropriate since things are within range on all accounts. I would
> appreciate an explanation on the current min/max values though if possible!
> 

I will add a line about the range as I resubmit the patch.

Thanks,
Bjorn