[PATCH v3 0/4] regulator: Add support for Allwinner D1 LDOs

[PATCH v3 0/4] regulator: Add support for Allwinner D1 LDOs

[Samuel Holland]

This series adds bindings and a driver for the two pairs of LDOs
inside the Allwinner D1 SoC.

A binding and driver change is required for the SRAM controller, to
accept the regulators device as its child node. The new example in the
SRAM controller binding uses the compatible string added in this series:
https://lore.kernel.org/lkml/20220815041248.53268-1-samuel@sholland.org/

Changes in v3:
 - Add "reg" property to bindings
 - Add "unevaluatedProperties: true" to regulator nodes
 - Minor changes to regulator node name patterns
 - Remove system-ldos example (now added in patch 3)
 - Adjust control flow in sun20i_regulator_get_regmap() for clarity
 - Require the regulators node to have a unit address
 - Reference the regulator schema from the SRAM controller schema
 - Move the system LDOs example to the SRAM controller schema
 - Reorder the patches so the example passes validation

Changes in v2:
 - Remove syscon property from bindings
 - Update binding examples to fix warnings and provide context
 - Use decimal numbers for .n_voltages instead of field widths
 - Get the regmap from the parent device instead of a property/phandle

Samuel Holland (4):
  regulator: dt-bindings: Add Allwinner D1 LDOs
  regulator: sun20i: Add support for Allwinner D1 LDOs
  dt-bindings: sram: sunxi-sram: Add optional regulators child
  soc: sunxi: sram: Only iterate over SRAM children

 .../allwinner,sun20i-d1-analog-ldos.yaml      |  74 ++++++
 .../allwinner,sun20i-d1-system-ldos.yaml      |  37 +++
 .../allwinner,sun4i-a10-system-control.yaml   |  29 +++
 drivers/regulator/Kconfig                     |   8 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/sun20i-regulator.c          | 232 ++++++++++++++++++
 drivers/soc/sunxi/sunxi_sram.c                |   3 +
 7 files changed, 384 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
 create mode 100644 drivers/regulator/sun20i-regulator.c

-- 
2.35.1

[PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Samuel Holland]

The Allwinner D1 SoC contains two pairs of in-package LDOs. One pair is
for general purpose use. LDOA generally powers the board's 1.8 V rail.
LDOB generally powers the in-package DRAM, where applicable.

The other pair of LDOs powers the analog power domains inside the SoC,
including the audio codec, thermal sensor, and ADCs. These LDOs require
a 0.9 V bandgap voltage reference. The calibration value for the voltage
reference is stored in an eFuse, accessed via an NVMEM cell.

Neither LDO control register is in its own MMIO range; instead, each
regulator device relies on a regmap/syscon exported by its parent.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Add "reg" property to bindings
 - Add "unevaluatedProperties: true" to regulator nodes
 - Minor changes to regulator node name patterns
 - Remove system-ldos example (now added in patch 3)

Changes in v2:
 - Remove syscon property from bindings
 - Update binding examples to fix warnings and provide context

 .../allwinner,sun20i-d1-analog-ldos.yaml      | 74 +++++++++++++++++++
 .../allwinner,sun20i-d1-system-ldos.yaml      | 37 ++++++++++
 2 files changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
 create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml

diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
new file mode 100644
index 000000000000..d6964b44ef21
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-analog-ldos.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1 Analog LDOs
+
+description:
+  Allwinner D1 contains a set of LDOs which are designed to supply analog power
+  inside and outside the SoC. They are controlled by a register within the audio
+  codec MMIO space, but which is not part of the audio codec clock/reset domain.
+
+maintainers:
+  - Samuel Holland <samuel@sholland.org>
+
+properties:
+  compatible:
+    enum:
+      - allwinner,sun20i-d1-analog-ldos
+
+  reg:
+    maxItems: 1
+
+  nvmem-cells:
+    items:
+      - description: NVMEM cell for the calibrated bandgap reference trim value
+
+  nvmem-cell-names:
+    items:
+      - const: bg_trim
+
+patternProperties:
+  "^(a|hp)ldo$":
+    type: object
+    $ref: regulator.yaml#
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - nvmem-cells
+  - nvmem-cell-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    audio-codec@2030000 {
+        compatible = "simple-mfd", "syscon";
+        reg = <0x2030000 0x1000>;
+        ranges;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        regulators@2030348 {
+            compatible = "allwinner,sun20i-d1-analog-ldos";
+            reg = <0x2030348 0x4>;
+            nvmem-cells = <&bg_trim>;
+            nvmem-cell-names = "bg_trim";
+
+            reg_aldo: aldo {
+                regulator-min-microvolt = <1800000>;
+                regulator-max-microvolt = <1800000>;
+            };
+
+            reg_hpldo: hpldo {
+                regulator-min-microvolt = <1800000>;
+                regulator-max-microvolt = <1800000>;
+            };
+        };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
new file mode 100644
index 000000000000..e3e2810fb3d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Allwinner D1 System LDOs
+
+description:
+  Allwinner D1 contains a pair of general-purpose LDOs which are designed to
+  supply power inside and outside the SoC. They are controlled by a register
+  within the system control MMIO space.
+
+maintainers:
+  - Samuel Holland <samuel@sholland.org>
+
+properties:
+  compatible:
+    enum:
+      - allwinner,sun20i-d1-system-ldos
+
+  reg:
+    maxItems: 1
+
+patternProperties:
+  "^ldo[ab]$":
+    type: object
+    $ref: regulator.yaml#
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+...
-- 
2.35.1

[PATCH v3 2/4] regulator: sun20i: Add support for Allwinner D1 LDOs

[Samuel Holland]

D1 contains two pairs of LDOs. Since they have similar bindings, and
they always exist together, put them in a single driver.

The analog LDOs are relatively boring, with a single linear range. Their
one quirk is that a bandgap reference must be calibrated for them to
produce the correct voltage.

The system LDOs have the complication that their voltage step is not an
integer, so a custom .list_voltage is needed to get the rounding right.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Adjust control flow in sun20i_regulator_get_regmap() for clarity

Changes in v2:
 - Use decimal numbers for .n_voltages instead of field widths
 - Get the regmap from the parent device instead of a property/phandle

 drivers/regulator/Kconfig            |   8 +
 drivers/regulator/Makefile           |   1 +
 drivers/regulator/sun20i-regulator.c | 232 +++++++++++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100644 drivers/regulator/sun20i-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 23e3e4a35cc9..0c5727173fa0 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1262,6 +1262,14 @@ config REGULATOR_STW481X_VMMC
 	  This driver supports the internal VMMC regulator in the STw481x
 	  PMIC chips.
 
+config REGULATOR_SUN20I
+	tristate "Allwinner D1 internal LDOs"
+	depends on ARCH_SUNXI || COMPILE_TEST
+	depends on MFD_SYSCON && NVMEM
+	default ARCH_SUNXI
+	help
+	  This driver supports the internal LDOs in the Allwinner D1 SoC.
+
 config REGULATOR_SY7636A
 	tristate "Silergy SY7636A voltage regulator"
 	help
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index fa49bb6cc544..5dff112eb015 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -148,6 +148,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
 obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
 obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SUN20I) += sun20i-regulator.o
 obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
 obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
diff --git a/drivers/regulator/sun20i-regulator.c b/drivers/regulator/sun20i-regulator.c
new file mode 100644
index 000000000000..46f3927d7d10
--- /dev/null
+++ b/drivers/regulator/sun20i-regulator.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
+//
+
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+
+#define SUN20I_POWER_REG		0x348
+
+#define SUN20I_SYS_LDO_CTRL_REG		0x150
+
+struct sun20i_regulator_data {
+	int				(*init)(struct device *dev,
+						struct regmap *regmap);
+	const struct regulator_desc	*descs;
+	unsigned int			ndescs;
+};
+
+static int sun20i_d1_analog_ldos_init(struct device *dev, struct regmap *regmap)
+{
+	u8 bg_trim;
+	int ret;
+
+	ret = nvmem_cell_read_u8(dev, "bg_trim", &bg_trim);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get bg_trim value\n");
+
+	/* The default value corresponds to 900 mV. */
+	if (!bg_trim)
+		bg_trim = 0x19;
+
+	return regmap_update_bits(regmap, SUN20I_POWER_REG,
+				  GENMASK(7, 0), bg_trim);
+}
+
+static const struct regulator_ops sun20i_d1_analog_ldo_ops = {
+	.list_voltage		= regulator_list_voltage_linear,
+	.map_voltage		= regulator_map_voltage_linear,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+static const struct regulator_desc sun20i_d1_analog_ldo_descs[] = {
+	{
+		.name		= "aldo",
+		.supply_name	= "vdd33",
+		.of_match	= "aldo",
+		.ops		= &sun20i_d1_analog_ldo_ops,
+		.type		= REGULATOR_VOLTAGE,
+		.owner		= THIS_MODULE,
+		.n_voltages	= 8,
+		.min_uV		= 1650000,
+		.uV_step	= 50000,
+		.vsel_reg	= SUN20I_POWER_REG,
+		.vsel_mask	= GENMASK(14, 12),
+		.enable_reg	= SUN20I_POWER_REG,
+		.enable_mask	= BIT(31),
+	},
+	{
+		.name		= "hpldo",
+		.supply_name	= "hpldoin",
+		.of_match	= "hpldo",
+		.ops		= &sun20i_d1_analog_ldo_ops,
+		.type		= REGULATOR_VOLTAGE,
+		.owner		= THIS_MODULE,
+		.n_voltages	= 8,
+		.min_uV		= 1650000,
+		.uV_step	= 50000,
+		.vsel_reg	= SUN20I_POWER_REG,
+		.vsel_mask	= GENMASK(10, 8),
+		.enable_reg	= SUN20I_POWER_REG,
+		.enable_mask	= BIT(30),
+	},
+};
+
+static const struct sun20i_regulator_data sun20i_d1_analog_ldos = {
+	.init	= sun20i_d1_analog_ldos_init,
+	.descs	= sun20i_d1_analog_ldo_descs,
+	.ndescs	= ARRAY_SIZE(sun20i_d1_analog_ldo_descs),
+};
+
+/* regulator_list_voltage_linear() modified for the non-integral uV_step. */
+static int sun20i_d1_system_ldo_list_voltage(struct regulator_dev *rdev,
+					     unsigned int selector)
+{
+	const struct regulator_desc *desc = rdev->desc;
+	unsigned int uV;
+
+	if (selector >= desc->n_voltages)
+		return -EINVAL;
+
+	uV = desc->min_uV + (desc->uV_step * selector);
+
+	/* Produce correctly-rounded absolute voltages. */
+	return uV + ((selector + 1 + (desc->min_uV % 4)) / 3);
+}
+
+static const struct regulator_ops sun20i_d1_system_ldo_ops = {
+	.list_voltage		= sun20i_d1_system_ldo_list_voltage,
+	.map_voltage		= regulator_map_voltage_ascend,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+};
+
+static const struct regulator_desc sun20i_d1_system_ldo_descs[] = {
+	{
+		.name		= "ldoa",
+		.supply_name	= "ldo-in",
+		.of_match	= "ldoa",
+		.ops		= &sun20i_d1_system_ldo_ops,
+		.type		= REGULATOR_VOLTAGE,
+		.owner		= THIS_MODULE,
+		.n_voltages	= 32,
+		.min_uV		= 1600000,
+		.uV_step	= 13333, /* repeating */
+		.vsel_reg	= SUN20I_SYS_LDO_CTRL_REG,
+		.vsel_mask	= GENMASK(7, 0),
+	},
+	{
+		.name		= "ldob",
+		.supply_name	= "ldo-in",
+		.of_match	= "ldob",
+		.ops		= &sun20i_d1_system_ldo_ops,
+		.type		= REGULATOR_VOLTAGE,
+		.owner		= THIS_MODULE,
+		.n_voltages	= 64,
+		.min_uV		= 1166666,
+		.uV_step	= 13333, /* repeating */
+		.vsel_reg	= SUN20I_SYS_LDO_CTRL_REG,
+		.vsel_mask	= GENMASK(15, 8),
+	},
+};
+
+static const struct sun20i_regulator_data sun20i_d1_system_ldos = {
+	.descs	= sun20i_d1_system_ldo_descs,
+	.ndescs	= ARRAY_SIZE(sun20i_d1_system_ldo_descs),
+};
+
+static const struct of_device_id sun20i_regulator_of_match[] = {
+	{
+		.compatible = "allwinner,sun20i-d1-analog-ldos",
+		.data = &sun20i_d1_analog_ldos,
+	},
+	{
+		.compatible = "allwinner,sun20i-d1-system-ldos",
+		.data = &sun20i_d1_system_ldos,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sun20i_regulator_of_match);
+
+static struct regmap *sun20i_regulator_get_regmap(struct device *dev)
+{
+	struct regmap *regmap;
+
+	/*
+	 * First try the syscon interface. The system control device is not
+	 * compatible with "syscon", so fall back to getting the regmap from
+	 * its platform device. This is ugly, but required for devicetree
+	 * backward compatibility.
+	 */
+	regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (!IS_ERR(regmap))
+		return regmap;
+
+	regmap = dev_get_regmap(dev->parent, NULL);
+	if (regmap)
+		return regmap;
+
+	return ERR_PTR(-EPROBE_DEFER);
+}
+
+static int sun20i_regulator_probe(struct platform_device *pdev)
+{
+	const struct sun20i_regulator_data *data;
+	struct device *dev = &pdev->dev;
+	struct regulator_config config;
+	struct regmap *regmap;
+	int ret;
+
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
+	regmap = sun20i_regulator_get_regmap(dev);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to get regmap\n");
+
+	if (data->init) {
+		ret = data->init(dev, regmap);
+		if (ret)
+			return ret;
+	}
+
+	config = (struct regulator_config) {
+		.dev	= dev,
+		.regmap	= regmap,
+	};
+
+	for (unsigned int i = 0; i < data->ndescs; ++i) {
+		const struct regulator_desc *desc = &data->descs[i];
+		struct regulator_dev *rdev;
+
+		rdev = devm_regulator_register(dev, desc, &config);
+		if (IS_ERR(rdev))
+			return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+static struct platform_driver sun20i_regulator_driver = {
+	.probe	= sun20i_regulator_probe,
+	.driver	= {
+		.name		= "sun20i-regulator",
+		.of_match_table	= sun20i_regulator_of_match,
+	},
+};
+module_platform_driver(sun20i_regulator_driver);
+
+MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
+MODULE_DESCRIPTION("Allwinner D1 internal LDO driver");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Samuel Holland]

Some sunxi SoCs have in-package regulators controlled by a register in
the system control MMIO block. Allow a child node for these regulators
in addition to SRAM child nodes.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Require the regulators node to have a unit address
 - Reference the regulator schema from the SRAM controller schema
 - Move the system LDOs example to the SRAM controller schema
 - Reorder the patches so the example passes validation

Changes in v2:
 - New patch for v2

 .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
index d64c1b28fb61..915ca85c3f10 100644
--- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
+++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
@@ -56,6 +56,10 @@ properties:
   ranges: true
 
 patternProperties:
+  "^regulators@[0-9a-f]+$":
+    $ref: /schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
+    unevaluatedProperties: false
+
   "^sram@[a-z0-9]+":
     type: object
 
@@ -130,3 +134,28 @@ examples:
         };
       };
     };
+
+  - |
+    syscon@3000000 {
+      compatible = "allwinner,sun20i-d1-system-control";
+      reg = <0x3000000 0x1000>;
+      ranges;
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      regulators@3000150 {
+        compatible = "allwinner,sun20i-d1-system-ldos";
+        reg = <0x3000150 0x4>;
+
+        reg_ldoa: ldoa {
+          regulator-min-microvolt = <1800000>;
+          regulator-max-microvolt = <1800000>;
+        };
+
+        reg_ldob: ldob {
+          regulator-name = "vcc-dram";
+          regulator-min-microvolt = <1500000>;
+          regulator-max-microvolt = <1500000>;
+        };
+      };
+    };
-- 
2.35.1

[PATCH v3 4/4] soc: sunxi: sram: Only iterate over SRAM children

[Samuel Holland]

Now that a "regulators" child is accepted by the controller binding, the
debugfs show routine must be explicitly limited to "sram" children.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

(no changes since v2)

Changes in v2:
 - New patch for v2

 drivers/soc/sunxi/sunxi_sram.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
index 92f9186c1c42..6acaaeb65652 100644
--- a/drivers/soc/sunxi/sunxi_sram.c
+++ b/drivers/soc/sunxi/sunxi_sram.c
@@ -120,6 +120,9 @@ static int sunxi_sram_show(struct seq_file *s, void *data)
 	seq_puts(s, "--------------------\n\n");
 
 	for_each_child_of_node(sram_dev->of_node, sram_node) {
+		if (!of_node_name_eq(sram_node, "sram"))
+			continue;
+
 		sram_addr_p = of_get_address(sram_node, 0, NULL, NULL);
 
 		seq_printf(s, "sram@%08x\n",
-- 
2.35.1

Re: [PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Rob Herring]

On Sun, 14 Aug 2022 23:34:34 -0500, Samuel Holland wrote:
> Some sunxi SoCs have in-package regulators controlled by a register in
> the system control MMIO block. Allow a child node for these regulators
> in addition to SRAM child nodes.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Require the regulators node to have a unit address
>  - Reference the regulator schema from the SRAM controller schema
>  - Move the system LDOs example to the SRAM controller schema
>  - Reorder the patches so the example passes validation
> 
> Changes in v2:
>  - New patch for v2
> 
>  .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.example.dtb:0:0: /example-1/syscon@3000000: failed to match any schema with compatible: ['allwinner,sun20i-d1-system-control']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Heiko Stübner]

Am Montag, 15. August 2022, 06:34:32 CEST schrieb Samuel Holland:
> The Allwinner D1 SoC contains two pairs of in-package LDOs. One pair is
> for general purpose use. LDOA generally powers the board's 1.8 V rail.
> LDOB generally powers the in-package DRAM, where applicable.
> 
> The other pair of LDOs powers the analog power domains inside the SoC,
> including the audio codec, thermal sensor, and ADCs. These LDOs require
> a 0.9 V bandgap voltage reference. The calibration value for the voltage
> reference is stored in an eFuse, accessed via an NVMEM cell.
> 
> Neither LDO control register is in its own MMIO range; instead, each
> regulator device relies on a regmap/syscon exported by its parent.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Add "reg" property to bindings
>  - Add "unevaluatedProperties: true" to regulator nodes
>  - Minor changes to regulator node name patterns
>  - Remove system-ldos example (now added in patch 3)
> 
> Changes in v2:
>  - Remove syscon property from bindings
>  - Update binding examples to fix warnings and provide context
> 
>  .../allwinner,sun20i-d1-analog-ldos.yaml      | 74 +++++++++++++++++++
>  .../allwinner,sun20i-d1-system-ldos.yaml      | 37 ++++++++++
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
> new file mode 100644
> index 000000000000..d6964b44ef21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-analog-ldos.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner D1 Analog LDOs
> +
> +description:
> +  Allwinner D1 contains a set of LDOs which are designed to supply analog power
> +  inside and outside the SoC. They are controlled by a register within the audio
> +  codec MMIO space, but which is not part of the audio codec clock/reset domain.
> +
> +maintainers:
> +  - Samuel Holland <samuel@sholland.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - allwinner,sun20i-d1-analog-ldos
> +
> +  reg:
> +    maxItems: 1
> +
> +  nvmem-cells:
> +    items:
> +      - description: NVMEM cell for the calibrated bandgap reference trim value
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: bg_trim

aren't dashes "-" preferred over underscores "_" in
string names?

Maybe even make this "bandgap-trim" for a bit more
explanatory naming?


Heiko

Re: [PATCH v3 2/4] regulator: sun20i: Add support for Allwinner D1 LDOs

[Heiko Stübner]

Am Montag, 15. August 2022, 06:34:33 CEST schrieb Samuel Holland:
> D1 contains two pairs of LDOs. Since they have similar bindings, and
> they always exist together, put them in a single driver.
> 
> The analog LDOs are relatively boring, with a single linear range. Their
> one quirk is that a bandgap reference must be calibrated for them to
> produce the correct voltage.
> 
> The system LDOs have the complication that their voltage step is not an
> integer, so a custom .list_voltage is needed to get the rounding right.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Adjust control flow in sun20i_regulator_get_regmap() for clarity
> 
> Changes in v2:
>  - Use decimal numbers for .n_voltages instead of field widths
>  - Get the regmap from the parent device instead of a property/phandle
> 
>  drivers/regulator/Kconfig            |   8 +
>  drivers/regulator/Makefile           |   1 +
>  drivers/regulator/sun20i-regulator.c | 232 +++++++++++++++++++++++++++
>  3 files changed, 241 insertions(+)
>  create mode 100644 drivers/regulator/sun20i-regulator.c
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 23e3e4a35cc9..0c5727173fa0 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1262,6 +1262,14 @@ config REGULATOR_STW481X_VMMC
>  	  This driver supports the internal VMMC regulator in the STw481x
>  	  PMIC chips.
>  
> +config REGULATOR_SUN20I
> +	tristate "Allwinner D1 internal LDOs"
> +	depends on ARCH_SUNXI || COMPILE_TEST
> +	depends on MFD_SYSCON && NVMEM
> +	default ARCH_SUNXI
> +	help
> +	  This driver supports the internal LDOs in the Allwinner D1 SoC.
> +
>  config REGULATOR_SY7636A
>  	tristate "Silergy SY7636A voltage regulator"
>  	help
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index fa49bb6cc544..5dff112eb015 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -148,6 +148,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
>  obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
>  obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
>  obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
> +obj-$(CONFIG_REGULATOR_SUN20I) += sun20i-regulator.o
>  obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
>  obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>  obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
> diff --git a/drivers/regulator/sun20i-regulator.c b/drivers/regulator/sun20i-regulator.c
> new file mode 100644
> index 000000000000..46f3927d7d10
> --- /dev/null
> +++ b/drivers/regulator/sun20i-regulator.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
> +//

nit: shouldn't the comment look like
/*
 * Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
 */


otherwise looks great
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Heiko Stübner]

Am Montag, 15. August 2022, 16:01:47 CEST schrieb Rob Herring:
> On Sun, 14 Aug 2022 23:34:34 -0500, Samuel Holland wrote:
> > Some sunxi SoCs have in-package regulators controlled by a register in
> > the system control MMIO block. Allow a child node for these regulators
> > in addition to SRAM child nodes.
> > 
> > Signed-off-by: Samuel Holland <samuel@sholland.org>
> > ---
> > 
> > Changes in v3:
> >  - Require the regulators node to have a unit address
> >  - Reference the regulator schema from the SRAM controller schema
> >  - Move the system LDOs example to the SRAM controller schema
> >  - Reorder the patches so the example passes validation
> > 
> > Changes in v2:
> >  - New patch for v2
> > 
> >  .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.example.dtb:0:0: /example-1/syscon@3000000: failed to match any schema with compatible: ['allwinner,sun20i-d1-system-control']

This got added in
	"dt-bindings: sram: sunxi-sram: Add D1 compatible string"

( https://lore.kernel.org/r/20220815041248.53268-3-samuel@sholland.org )

> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/patch/
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH v3 4/4] soc: sunxi: sram: Only iterate over SRAM children

[Heiko Stübner]

Am Montag, 15. August 2022, 06:34:35 CEST schrieb Samuel Holland:
> Now that a "regulators" child is accepted by the controller binding, the
> debugfs show routine must be explicitly limited to "sram" children.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 15/08/2022 07:34, Samuel Holland wrote:
> The Allwinner D1 SoC contains two pairs of in-package LDOs. One pair is
> for general purpose use. LDOA generally powers the board's 1.8 V rail.
> LDOB generally powers the in-package DRAM, where applicable.
> 
> The other pair of LDOs powers the analog power domains inside the SoC,
> including the audio codec, thermal sensor, and ADCs. These LDOs require
> a 0.9 V bandgap voltage reference. The calibration value for the voltage
> reference is stored in an eFuse, accessed via an NVMEM cell.
> 
> Neither LDO control register is in its own MMIO range; instead, each
> regulator device relies on a regmap/syscon exported by its parent.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Add "reg" property to bindings
>  - Add "unevaluatedProperties: true" to regulator nodes
>  - Minor changes to regulator node name patterns
>  - Remove system-ldos example (now added in patch 3)
> 
> Changes in v2:
>  - Remove syscon property from bindings
>  - Update binding examples to fix warnings and provide context
> 
>  .../allwinner,sun20i-d1-analog-ldos.yaml      | 74 +++++++++++++++++++
>  .../allwinner,sun20i-d1-system-ldos.yaml      | 37 ++++++++++
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
> 
> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
> new file mode 100644
> index 000000000000..d6964b44ef21
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-analog-ldos.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner D1 Analog LDOs
> +
> +description:
> +  Allwinner D1 contains a set of LDOs which are designed to supply analog power
> +  inside and outside the SoC. They are controlled by a register within the audio
> +  codec MMIO space, but which is not part of the audio codec clock/reset domain.
> +
> +maintainers:
> +  - Samuel Holland <samuel@sholland.org>

Please follow the example schema. Order is: title, maintainers, description.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - allwinner,sun20i-d1-analog-ldos
> +
> +  reg:
> +    maxItems: 1
> +
> +  nvmem-cells:
> +    items:
> +      - description: NVMEM cell for the calibrated bandgap reference trim value
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: bg_trim
> +
> +patternProperties:
> +  "^(a|hp)ldo$":
> +    type: object
> +    $ref: regulator.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - nvmem-cells
> +  - nvmem-cell-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    audio-codec@2030000 {
> +        compatible = "simple-mfd", "syscon";

This cannot be on its own. Both require device specific compatible.

> +        reg = <0x2030000 0x1000>;
> +        ranges;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        regulators@2030348 {
> +            compatible = "allwinner,sun20i-d1-analog-ldos";
> +            reg = <0x2030348 0x4>;
> +            nvmem-cells = <&bg_trim>;
> +            nvmem-cell-names = "bg_trim";
> +
> +            reg_aldo: aldo {
> +                regulator-min-microvolt = <1800000>;
> +                regulator-max-microvolt = <1800000>;
> +            };
> +
> +            reg_hpldo: hpldo {
> +                regulator-min-microvolt = <1800000>;
> +                regulator-max-microvolt = <1800000>;
> +            };
> +        };
> +    };
> +
> +...
> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
> new file mode 100644
> index 000000000000..e3e2810fb3d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allwinner D1 System LDOs
> +
> +description:
> +  Allwinner D1 contains a pair of general-purpose LDOs which are designed to
> +  supply power inside and outside the SoC. They are controlled by a register
> +  within the system control MMIO space.

Fix order.


> +
> +maintainers:
> +  - Samuel Holland <samuel@sholland.org>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - allwinner,sun20i-d1-system-ldos
> +
> +  reg:
> +    maxItems: 1
> +
> +patternProperties:
> +  "^ldo[ab]$":
> +    type: object
> +    $ref: regulator.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false


Example please.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 15/08/2022 07:34, Samuel Holland wrote:
> +  reg:
> +    maxItems: 1
> +
> +  nvmem-cells:
> +    items:
> +      - description: NVMEM cell for the calibrated bandgap reference trim value
> +
> +  nvmem-cell-names:
> +    items:
> +      - const: bg_trim
> +
> +patternProperties:
> +  "^(a|hp)ldo$":
> +    type: object
> +    $ref: regulator.yaml#
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - nvmem-cells
> +  - nvmem-cell-names
> +
> +unevaluatedProperties: false

one more: this must be additionalProperties:false, because you do not
reference any other schema in top level. Same in second file.

Best regards,
Krzysztof

Re: [PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Krzysztof Kozlowski]

On 15/08/2022 07:34, Samuel Holland wrote:
> Some sunxi SoCs have in-package regulators controlled by a register in
> the system control MMIO block. Allow a child node for these regulators
> in addition to SRAM child nodes.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Require the regulators node to have a unit address
>  - Reference the regulator schema from the SRAM controller schema
>  - Move the system LDOs example to the SRAM controller schema
>  - Reorder the patches so the example passes validation
> 
> Changes in v2:
>  - New patch for v2
> 
>  .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> index d64c1b28fb61..915ca85c3f10 100644
> --- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> +++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> @@ -56,6 +56,10 @@ properties:
>    ranges: true
>  
>  patternProperties:
> +  "^regulators@[0-9a-f]+$":
> +    $ref: /schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
> +    unevaluatedProperties: false

unevaluatedProperties is not needed. Your other schema does not allow
anything else here.

> +
>    "^sram@[a-z0-9]+":
>      type: object
>  
> @@ -130,3 +134,28 @@ examples:
>          };
>        };
>      };
> +
> +  - |
> +    syscon@3000000 {
> +      compatible = "allwinner,sun20i-d1-system-control";

Your other example uses simple-mfd, syscon... A bit confusing.

> +      reg = <0x3000000 0x1000>;
> +      ranges;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      regulators@3000150 {
> +        compatible = "allwinner,sun20i-d1-system-ldos";
> +        reg = <0x3000150 0x4>;
> +
> +        reg_ldoa: ldoa {
> +          regulator-min-microvolt = <1800000>;
> +          regulator-max-microvolt = <1800000>;
> +        };
> +
> +        reg_ldob: ldob {
> +          regulator-name = "vcc-dram";
> +          regulator-min-microvolt = <1500000>;
> +          regulator-max-microvolt = <1500000>;
> +        };
> +      };
> +    };


Best regards,
Krzysztof

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 15/08/2022 18:32, Heiko Stübner wrote:
> Am Montag, 15. August 2022, 06:34:32 CEST schrieb Samuel Holland:
>> The Allwinner D1 SoC contains two pairs of in-package LDOs. One pair is
>> for general purpose use. LDOA generally powers the board's 1.8 V rail.
>> LDOB generally powers the in-package DRAM, where applicable.
>>
>> The other pair of LDOs powers the analog power domains inside the SoC,
>> including the audio codec, thermal sensor, and ADCs. These LDOs require
>> a 0.9 V bandgap voltage reference. The calibration value for the voltage
>> reference is stored in an eFuse, accessed via an NVMEM cell.
>>
>> Neither LDO control register is in its own MMIO range; instead, each
>> regulator device relies on a regmap/syscon exported by its parent.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Add "reg" property to bindings
>>  - Add "unevaluatedProperties: true" to regulator nodes
>>  - Minor changes to regulator node name patterns
>>  - Remove system-ldos example (now added in patch 3)
>>
>> Changes in v2:
>>  - Remove syscon property from bindings
>>  - Update binding examples to fix warnings and provide context
>>
>>  .../allwinner,sun20i-d1-analog-ldos.yaml      | 74 +++++++++++++++++++
>>  .../allwinner,sun20i-d1-system-ldos.yaml      | 37 ++++++++++
>>  2 files changed, 111 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>> new file mode 100644
>> index 000000000000..d6964b44ef21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-analog-ldos.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Allwinner D1 Analog LDOs
>> +
>> +description:
>> +  Allwinner D1 contains a set of LDOs which are designed to supply analog power
>> +  inside and outside the SoC. They are controlled by a register within the audio
>> +  codec MMIO space, but which is not part of the audio codec clock/reset domain.
>> +
>> +maintainers:
>> +  - Samuel Holland <samuel@sholland.org>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - allwinner,sun20i-d1-analog-ldos
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  nvmem-cells:
>> +    items:
>> +      - description: NVMEM cell for the calibrated bandgap reference trim value
>> +
>> +  nvmem-cell-names:
>> +    items:
>> +      - const: bg_trim
> 
> aren't dashes "-" preferred over underscores "_" in
> string names?
> 
> Maybe even make this "bandgap-trim" for a bit more
> explanatory naming?

In node names yes. In strings, I think there is no preference. At least
I am not aware of it.

Best regards,
Krzysztof

Re: [PATCH v3 4/4] soc: sunxi: sram: Only iterate over SRAM children

[Krzysztof Kozlowski]

On 15/08/2022 07:34, Samuel Holland wrote:
> Now that a "regulators" child is accepted by the controller binding, the
> debugfs show routine must be explicitly limited to "sram" children.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> (no changes since v2)
> 
> Changes in v2:
>  - New patch for v2
> 
>  drivers/soc/sunxi/sunxi_sram.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
> index 92f9186c1c42..6acaaeb65652 100644
> --- a/drivers/soc/sunxi/sunxi_sram.c
> +++ b/drivers/soc/sunxi/sunxi_sram.c
> @@ -120,6 +120,9 @@ static int sunxi_sram_show(struct seq_file *s, void *data)
>  	seq_puts(s, "--------------------\n\n");
>  
>  	for_each_child_of_node(sram_dev->of_node, sram_node) {
> +		if (!of_node_name_eq(sram_node, "sram"))

You should not rely on node names. They can change in DTS. Why do you
need to test for the name?

Best regards,
Krzysztof

Re: [PATCH v3 4/4] soc: sunxi: sram: Only iterate over SRAM children

[Krzysztof Kozlowski]

On 16/08/2022 13:01, Krzysztof Kozlowski wrote:
> On 15/08/2022 07:34, Samuel Holland wrote:
>> Now that a "regulators" child is accepted by the controller binding, the
>> debugfs show routine must be explicitly limited to "sram" children.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> (no changes since v2)
>>
>> Changes in v2:
>>  - New patch for v2
>>
>>  drivers/soc/sunxi/sunxi_sram.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
>> index 92f9186c1c42..6acaaeb65652 100644
>> --- a/drivers/soc/sunxi/sunxi_sram.c
>> +++ b/drivers/soc/sunxi/sunxi_sram.c
>> @@ -120,6 +120,9 @@ static int sunxi_sram_show(struct seq_file *s, void *data)
>>  	seq_puts(s, "--------------------\n\n");
>>  
>>  	for_each_child_of_node(sram_dev->of_node, sram_node) {
>> +		if (!of_node_name_eq(sram_node, "sram"))
> 
> You should not rely on node names. They can change in DTS. Why do you
> need to test for the name?
> 

Ah, it is not a device node but a child property, right? In such case,
it's of course fine.

The device node names could change and should not be considered ABI (at
least I hope should not...).

Best regards,
Krzysztof

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Samuel Holland]

On 8/16/22 4:55 AM, Krzysztof Kozlowski wrote:
> On 15/08/2022 07:34, Samuel Holland wrote:
>> The Allwinner D1 SoC contains two pairs of in-package LDOs. One pair is
>> for general purpose use. LDOA generally powers the board's 1.8 V rail.
>> LDOB generally powers the in-package DRAM, where applicable.
>>
>> The other pair of LDOs powers the analog power domains inside the SoC,
>> including the audio codec, thermal sensor, and ADCs. These LDOs require
>> a 0.9 V bandgap voltage reference. The calibration value for the voltage
>> reference is stored in an eFuse, accessed via an NVMEM cell.
>>
>> Neither LDO control register is in its own MMIO range; instead, each
>> regulator device relies on a regmap/syscon exported by its parent.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Add "reg" property to bindings
>>  - Add "unevaluatedProperties: true" to regulator nodes
>>  - Minor changes to regulator node name patterns
>>  - Remove system-ldos example (now added in patch 3)
>>
>> Changes in v2:
>>  - Remove syscon property from bindings
>>  - Update binding examples to fix warnings and provide context
>>
>>  .../allwinner,sun20i-d1-analog-ldos.yaml      | 74 +++++++++++++++++++
>>  .../allwinner,sun20i-d1-system-ldos.yaml      | 37 ++++++++++
>>  2 files changed, 111 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>>  create mode 100644 Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>> new file mode 100644
>> index 000000000000..d6964b44ef21
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-analog-ldos.yaml
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-analog-ldos.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Allwinner D1 Analog LDOs
>> +
>> +description:
>> +  Allwinner D1 contains a set of LDOs which are designed to supply analog power
>> +  inside and outside the SoC. They are controlled by a register within the audio
>> +  codec MMIO space, but which is not part of the audio codec clock/reset domain.
>> +
>> +maintainers:
>> +  - Samuel Holland <samuel@sholland.org>
> 
> Please follow the example schema. Order is: title, maintainers, description.
> 
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - allwinner,sun20i-d1-analog-ldos
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  nvmem-cells:
>> +    items:
>> +      - description: NVMEM cell for the calibrated bandgap reference trim value
>> +
>> +  nvmem-cell-names:
>> +    items:
>> +      - const: bg_trim
>> +
>> +patternProperties:
>> +  "^(a|hp)ldo$":
>> +    type: object
>> +    $ref: regulator.yaml#
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - nvmem-cells
>> +  - nvmem-cell-names
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    audio-codec@2030000 {
>> +        compatible = "simple-mfd", "syscon";
> 
> This cannot be on its own. Both require device specific compatible.

Again, the device-specific compatible does not exist, because the binding for
the audio codec has not been written (and it will be quite nontrivial).

So I can:
  1) Leave the example as-is until the audio codec binding gets written,
     and fill in the specific compatible at that time.
  2) Remove the example, with the reasoning that the example really
     belongs with the MFD parent (like for the other regulator). Then
     there will be no example until the audio codec binding is written.
  3) Drop the analog LDOs from this series entirely, and some parts
     of the SoC (like thermal monitoring) cannot be added to the DTSI
     until the audio codec binding is written.

What do you think?

The same question applies for the D1 SoC DTSI, where I use this same construct.

(And technically this does validate with the current schema.)

>> +        reg = <0x2030000 0x1000>;
>> +        ranges;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        regulators@2030348 {
>> +            compatible = "allwinner,sun20i-d1-analog-ldos";
>> +            reg = <0x2030348 0x4>;
>> +            nvmem-cells = <&bg_trim>;
>> +            nvmem-cell-names = "bg_trim";
>> +
>> +            reg_aldo: aldo {
>> +                regulator-min-microvolt = <1800000>;
>> +                regulator-max-microvolt = <1800000>;
>> +            };
>> +
>> +            reg_hpldo: hpldo {
>> +                regulator-min-microvolt = <1800000>;
>> +                regulator-max-microvolt = <1800000>;
>> +            };
>> +        };
>> +    };
>> +
>> +...
>> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>> new file mode 100644
>> index 000000000000..e3e2810fb3d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>> @@ -0,0 +1,37 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Allwinner D1 System LDOs
>> +
>> +description:
>> +  Allwinner D1 contains a pair of general-purpose LDOs which are designed to
>> +  supply power inside and outside the SoC. They are controlled by a register
>> +  within the system control MMIO space.
> 
> Fix order.
> 
> 
>> +
>> +maintainers:
>> +  - Samuel Holland <samuel@sholland.org>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - allwinner,sun20i-d1-system-ldos
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +patternProperties:
>> +  "^ldo[ab]$":
>> +    type: object
>> +    $ref: regulator.yaml#
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
> 
> 
> Example please.

Rob asked me to move the example to the parent binding, so I did. The example is
added in patch 3.

Regards,
Samuel

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 17/08/2022 11:15, Samuel Holland wrote:

>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - allwinner,sun20i-d1-analog-ldos
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  nvmem-cells:
>>> +    items:
>>> +      - description: NVMEM cell for the calibrated bandgap reference trim value
>>> +
>>> +  nvmem-cell-names:
>>> +    items:
>>> +      - const: bg_trim
>>> +
>>> +patternProperties:
>>> +  "^(a|hp)ldo$":
>>> +    type: object
>>> +    $ref: regulator.yaml#
>>> +    unevaluatedProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - nvmem-cells
>>> +  - nvmem-cell-names
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    audio-codec@2030000 {
>>> +        compatible = "simple-mfd", "syscon";
>>
>> This cannot be on its own. Both require device specific compatible.
> 
> Again, the device-specific compatible does not exist, because the binding for
> the audio codec has not been written (and it will be quite nontrivial).
> 
> So I can:
>   1) Leave the example as-is until the audio codec binding gets written,
>      and fill in the specific compatible at that time.
>   2) Remove the example, with the reasoning that the example really
>      belongs with the MFD parent (like for the other regulator). Then
>      there will be no example until the audio codec binding is written.
>   3) Drop the analog LDOs from this series entirely, and some parts
>      of the SoC (like thermal monitoring) cannot be added to the DTSI
>      until the audio codec binding is written.
> 
> What do you think?

How about just removing the audio-codec node? The schema is about
regulators, not audio-codec.

OTOH, if you have parent device schema, you could put the example only
there. But as I understand, you don't have, right?

> 
> The same question applies for the D1 SoC DTSI, where I use this same construct.

This is not correct and should be fixed. Either you add the schema with
compatible or please drop the device node from the DTSI.

> 
> (And technically this does validate with the current schema.)
> 
>>> +        reg = <0x2030000 0x1000>;
>>> +        ranges;
>>> +        #address-cells = <1>;
>>> +        #size-cells = <1>;
>>> +
>>> +        regulators@2030348 {
>>> +            compatible = "allwinner,sun20i-d1-analog-ldos";
>>> +            reg = <0x2030348 0x4>;
>>> +            nvmem-cells = <&bg_trim>;
>>> +            nvmem-cell-names = "bg_trim";
>>> +
>>> +            reg_aldo: aldo {
>>> +                regulator-min-microvolt = <1800000>;
>>> +                regulator-max-microvolt = <1800000>;
>>> +            };
>>> +
>>> +            reg_hpldo: hpldo {
>>> +                regulator-min-microvolt = <1800000>;
>>> +                regulator-max-microvolt = <1800000>;
>>> +            };
>>> +        };
>>> +    };
>>> +
>>> +...
>>> diff --git a/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>>> new file mode 100644
>>> index 000000000000..e3e2810fb3d7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/regulator/allwinner,sun20i-d1-system-ldos.yaml
>>> @@ -0,0 +1,37 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Allwinner D1 System LDOs
>>> +
>>> +description:
>>> +  Allwinner D1 contains a pair of general-purpose LDOs which are designed to
>>> +  supply power inside and outside the SoC. They are controlled by a register
>>> +  within the system control MMIO space.
>>
>> Fix order.
>>
>>
>>> +
>>> +maintainers:
>>> +  - Samuel Holland <samuel@sholland.org>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - allwinner,sun20i-d1-system-ldos
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +patternProperties:
>>> +  "^ldo[ab]$":
>>> +    type: object
>>> +    $ref: regulator.yaml#
>>> +    unevaluatedProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +unevaluatedProperties: false
>>
>>
>> Example please.
> 
> Rob asked me to move the example to the parent binding, so I did. The example is
> added in patch 3.

Yeah, I noticed it later. It's fine.

Best regards,
Krzysztof

Re: [PATCH v3 2/4] regulator: sun20i: Add support for Allwinner D1 LDOs

[Samuel Holland]

On 8/15/22 12:00 PM, Heiko Stübner wrote:
> Am Montag, 15. August 2022, 06:34:33 CEST schrieb Samuel Holland:
>> D1 contains two pairs of LDOs. Since they have similar bindings, and
>> they always exist together, put them in a single driver.
>>
>> The analog LDOs are relatively boring, with a single linear range. Their
>> one quirk is that a bandgap reference must be calibrated for them to
>> produce the correct voltage.
>>
>> The system LDOs have the complication that their voltage step is not an
>> integer, so a custom .list_voltage is needed to get the rounding right.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Adjust control flow in sun20i_regulator_get_regmap() for clarity
>>
>> Changes in v2:
>>  - Use decimal numbers for .n_voltages instead of field widths
>>  - Get the regmap from the parent device instead of a property/phandle
>>
>>  drivers/regulator/Kconfig            |   8 +
>>  drivers/regulator/Makefile           |   1 +
>>  drivers/regulator/sun20i-regulator.c | 232 +++++++++++++++++++++++++++
>>  3 files changed, 241 insertions(+)
>>  create mode 100644 drivers/regulator/sun20i-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 23e3e4a35cc9..0c5727173fa0 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -1262,6 +1262,14 @@ config REGULATOR_STW481X_VMMC
>>  	  This driver supports the internal VMMC regulator in the STw481x
>>  	  PMIC chips.
>>  
>> +config REGULATOR_SUN20I
>> +	tristate "Allwinner D1 internal LDOs"
>> +	depends on ARCH_SUNXI || COMPILE_TEST
>> +	depends on MFD_SYSCON && NVMEM
>> +	default ARCH_SUNXI
>> +	help
>> +	  This driver supports the internal LDOs in the Allwinner D1 SoC.
>> +
>>  config REGULATOR_SY7636A
>>  	tristate "Silergy SY7636A voltage regulator"
>>  	help
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index fa49bb6cc544..5dff112eb015 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -148,6 +148,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
>>  obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
>>  obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
>>  obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
>> +obj-$(CONFIG_REGULATOR_SUN20I) += sun20i-regulator.o
>>  obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
>>  obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>>  obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
>> diff --git a/drivers/regulator/sun20i-regulator.c b/drivers/regulator/sun20i-regulator.c
>> new file mode 100644
>> index 000000000000..46f3927d7d10
>> --- /dev/null
>> +++ b/drivers/regulator/sun20i-regulator.c
>> @@ -0,0 +1,232 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +//
>> +// Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
>> +//
> 
> nit: shouldn't the comment look like
> /*
>  * Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
>  */

I have had multiple requests from maintainers to use the former style because it
is more visually consistent. `git grep '^// Copy' drivers sound` returns over
1500 hits. But it doesn't really matter to me.

Regards,
Samuel

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Samuel Holland]

On 8/17/22 3:27 AM, Krzysztof Kozlowski wrote:
> On 17/08/2022 11:15, Samuel Holland wrote:
>>>> +examples:
>>>> +  - |
>>>> +    audio-codec@2030000 {
>>>> +        compatible = "simple-mfd", "syscon";
>>>
>>> This cannot be on its own. Both require device specific compatible.
>>
>> Again, the device-specific compatible does not exist, because the binding for
>> the audio codec has not been written (and it will be quite nontrivial).
>>
>> So I can:
>>   1) Leave the example as-is until the audio codec binding gets written,
>>      and fill in the specific compatible at that time.
>>   2) Remove the example, with the reasoning that the example really
>>      belongs with the MFD parent (like for the other regulator). Then
>>      there will be no example until the audio codec binding is written.
>>   3) Drop the analog LDOs from this series entirely, and some parts
>>      of the SoC (like thermal monitoring) cannot be added to the DTSI
>>      until the audio codec binding is written.
>>
>> What do you think?
> 
> How about just removing the audio-codec node? The schema is about
> regulators, not audio-codec.

That works for me. I put the extra node there to signify that this is a MFD
child and requires some parent node to work, but I suppose it is not that
helpful to have.

> OTOH, if you have parent device schema, you could put the example only
> there. But as I understand, you don't have, right?

Right.

>> The same question applies for the D1 SoC DTSI, where I use this same construct.
> 
> This is not correct and should be fixed. Either you add the schema with
> compatible or please drop the device node from the DTSI.

That's what I was afraid of.

Regards,
Samuel

>> (And technically this does validate with the current schema.)

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 17/08/2022 11:39, Samuel Holland wrote:
>>> The same question applies for the D1 SoC DTSI, where I use this same construct.
>>
>> This is not correct and should be fixed. Either you add the schema with
>> compatible or please drop the device node from the DTSI.
> 
> That's what I was afraid of.

The bindings can grow, so you can add a bindings stub documenting
compatibles (device + simple-mfd + syscon) and IO space, then later
extend it. Of course it is preferred to add bindings as complete as
possible, but incremental growing also works.

Best regards,
Krzysztof

Re: [PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Samuel Holland]

On 8/16/22 4:59 AM, Krzysztof Kozlowski wrote:
> On 15/08/2022 07:34, Samuel Holland wrote:
>> Some sunxi SoCs have in-package regulators controlled by a register in
>> the system control MMIO block. Allow a child node for these regulators
>> in addition to SRAM child nodes.
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>> Changes in v3:
>>  - Require the regulators node to have a unit address
>>  - Reference the regulator schema from the SRAM controller schema
>>  - Move the system LDOs example to the SRAM controller schema
>>  - Reorder the patches so the example passes validation
>>
>> Changes in v2:
>>  - New patch for v2
>>
>>  .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>> index d64c1b28fb61..915ca85c3f10 100644
>> --- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>> +++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>> @@ -56,6 +56,10 @@ properties:
>>    ranges: true
>>  
>>  patternProperties:
>> +  "^regulators@[0-9a-f]+$":
>> +    $ref: /schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
>> +    unevaluatedProperties: false
> 
> unevaluatedProperties is not needed. Your other schema does not allow
> anything else here.

I can remove it. I added it because it looks like the dt-schema tools use it as
an indicator that the matched properties are child nodes[1]. Maybe that is not
relevant here?

Regards,
Samuel

[1]: https://github.com/devicetree-org/dt-schema/commit/b12b3737cabc

Re: [PATCH v3 4/4] soc: sunxi: sram: Only iterate over SRAM children

[Samuel Holland]

On 8/16/22 5:03 AM, Krzysztof Kozlowski wrote:
> On 16/08/2022 13:01, Krzysztof Kozlowski wrote:
>> On 15/08/2022 07:34, Samuel Holland wrote:
>>> Now that a "regulators" child is accepted by the controller binding, the
>>> debugfs show routine must be explicitly limited to "sram" children.
>>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>> ---
>>>
>>> (no changes since v2)
>>>
>>> Changes in v2:
>>>  - New patch for v2
>>>
>>>  drivers/soc/sunxi/sunxi_sram.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/soc/sunxi/sunxi_sram.c b/drivers/soc/sunxi/sunxi_sram.c
>>> index 92f9186c1c42..6acaaeb65652 100644
>>> --- a/drivers/soc/sunxi/sunxi_sram.c
>>> +++ b/drivers/soc/sunxi/sunxi_sram.c
>>> @@ -120,6 +120,9 @@ static int sunxi_sram_show(struct seq_file *s, void *data)
>>>  	seq_puts(s, "--------------------\n\n");
>>>  
>>>  	for_each_child_of_node(sram_dev->of_node, sram_node) {
>>> +		if (!of_node_name_eq(sram_node, "sram"))
>>
>> You should not rely on node names. They can change in DTS. Why do you
>> need to test for the name?
>>
> 
> Ah, it is not a device node but a child property, right? In such case,
> it's of course fine.

It is a child node.

> The device node names could change and should not be considered ABI (at
> least I hope should not...).

The node names are limited by patternProperties in the controller binding. I can
check the child nodes for compatibility with "mmio-sram" if that is better.

Regards,
Samuel

Re: [PATCH v3 2/4] regulator: sun20i: Add support for Allwinner D1 LDOs

[Heiko Stübner]

Am Mittwoch, 17. August 2022, 10:28:40 CEST schrieb Samuel Holland:
> On 8/15/22 12:00 PM, Heiko Stübner wrote:
> > Am Montag, 15. August 2022, 06:34:33 CEST schrieb Samuel Holland:
> >> D1 contains two pairs of LDOs. Since they have similar bindings, and
> >> they always exist together, put them in a single driver.
> >>
> >> The analog LDOs are relatively boring, with a single linear range. Their
> >> one quirk is that a bandgap reference must be calibrated for them to
> >> produce the correct voltage.
> >>
> >> The system LDOs have the complication that their voltage step is not an
> >> integer, so a custom .list_voltage is needed to get the rounding right.
> >>
> >> Signed-off-by: Samuel Holland <samuel@sholland.org>
> >> ---
> >>
> >> Changes in v3:
> >>  - Adjust control flow in sun20i_regulator_get_regmap() for clarity
> >>
> >> Changes in v2:
> >>  - Use decimal numbers for .n_voltages instead of field widths
> >>  - Get the regmap from the parent device instead of a property/phandle
> >>
> >>  drivers/regulator/Kconfig            |   8 +
> >>  drivers/regulator/Makefile           |   1 +
> >>  drivers/regulator/sun20i-regulator.c | 232 +++++++++++++++++++++++++++
> >>  3 files changed, 241 insertions(+)
> >>  create mode 100644 drivers/regulator/sun20i-regulator.c
> >>
> >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> >> index 23e3e4a35cc9..0c5727173fa0 100644
> >> --- a/drivers/regulator/Kconfig
> >> +++ b/drivers/regulator/Kconfig
> >> @@ -1262,6 +1262,14 @@ config REGULATOR_STW481X_VMMC
> >>  	  This driver supports the internal VMMC regulator in the STw481x
> >>  	  PMIC chips.
> >>  
> >> +config REGULATOR_SUN20I
> >> +	tristate "Allwinner D1 internal LDOs"
> >> +	depends on ARCH_SUNXI || COMPILE_TEST
> >> +	depends on MFD_SYSCON && NVMEM
> >> +	default ARCH_SUNXI
> >> +	help
> >> +	  This driver supports the internal LDOs in the Allwinner D1 SoC.
> >> +
> >>  config REGULATOR_SY7636A
> >>  	tristate "Silergy SY7636A voltage regulator"
> >>  	help
> >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> >> index fa49bb6cc544..5dff112eb015 100644
> >> --- a/drivers/regulator/Makefile
> >> +++ b/drivers/regulator/Makefile
> >> @@ -148,6 +148,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
> >>  obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
> >>  obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
> >>  obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
> >> +obj-$(CONFIG_REGULATOR_SUN20I) += sun20i-regulator.o
> >>  obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
> >>  obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
> >>  obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
> >> diff --git a/drivers/regulator/sun20i-regulator.c b/drivers/regulator/sun20i-regulator.c
> >> new file mode 100644
> >> index 000000000000..46f3927d7d10
> >> --- /dev/null
> >> +++ b/drivers/regulator/sun20i-regulator.c
> >> @@ -0,0 +1,232 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +//
> >> +// Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
> >> +//
> > 
> > nit: shouldn't the comment look like
> > /*
> >  * Copyright (c) 2021-2022 Samuel Holland <samuel@sholland.org>
> >  */
> 
> I have had multiple requests from maintainers to use the former style because it
> is more visually consistent. `git grep '^// Copy' drivers sound` returns over
> 1500 hits. But it doesn't really matter to me.

ok ... I guess that is something that's changing then :-)

I just looked into some drivers and the coding-style rst document, but I
guess it's the maintainer's call how this should look.

Re: [PATCH v3 3/4] dt-bindings: sram: sunxi-sram: Add optional regulators child

[Krzysztof Kozlowski]

On 17/08/2022 11:47, Samuel Holland wrote:
> On 8/16/22 4:59 AM, Krzysztof Kozlowski wrote:
>> On 15/08/2022 07:34, Samuel Holland wrote:
>>> Some sunxi SoCs have in-package regulators controlled by a register in
>>> the system control MMIO block. Allow a child node for these regulators
>>> in addition to SRAM child nodes.
>>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>> ---
>>>
>>> Changes in v3:
>>>  - Require the regulators node to have a unit address
>>>  - Reference the regulator schema from the SRAM controller schema
>>>  - Move the system LDOs example to the SRAM controller schema
>>>  - Reorder the patches so the example passes validation
>>>
>>> Changes in v2:
>>>  - New patch for v2
>>>
>>>  .../allwinner,sun4i-a10-system-control.yaml   | 29 +++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>>> index d64c1b28fb61..915ca85c3f10 100644
>>> --- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>>> +++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
>>> @@ -56,6 +56,10 @@ properties:
>>>    ranges: true
>>>  
>>>  patternProperties:
>>> +  "^regulators@[0-9a-f]+$":
>>> +    $ref: /schemas/regulator/allwinner,sun20i-d1-system-ldos.yaml#
>>> +    unevaluatedProperties: false
>>
>> unevaluatedProperties is not needed. Your other schema does not allow
>> anything else here.
> 
> I can remove it. I added it because it looks like the dt-schema tools use it as
> an indicator that the matched properties are child nodes[1]. Maybe that is not
> relevant here?

It is not relevant here as the other schema does not allow anything else.

Best regards,
Krzysztof

Re: [PATCH v3 1/4] regulator: dt-bindings: Add Allwinner D1 LDOs

[Krzysztof Kozlowski]

On 17/08/2022 11:15, Samuel Holland wrote:
>>> +    audio-codec@2030000 {
>>> +        compatible = "simple-mfd", "syscon";
>>
>> This cannot be on its own. Both require device specific compatible.
> 
> Again, the device-specific compatible does not exist, because the binding for
> the audio codec has not been written (and it will be quite nontrivial).
> 
> So I can:
>   1) Leave the example as-is until the audio codec binding gets written,
>      and fill in the specific compatible at that time.
>   2) Remove the example, with the reasoning that the example really
>      belongs with the MFD parent (like for the other regulator). Then
>      there will be no example until the audio codec binding is written.
>   3) Drop the analog LDOs from this series entirely, and some parts
>      of the SoC (like thermal monitoring) cannot be added to the DTSI
>      until the audio codec binding is written.
> 
> What do you think?
> 
> The same question applies for the D1 SoC DTSI, where I use this same construct.
> 
> (And technically this does validate with the current schema.)

BTW, it validates only because of limitation in DT schema. Such
combination is not allowed and I wonder if we can make the schema
stricter...

Best regards,
Krzysztof