[PATCH 1/2] dt-bindings: regulator: Add Spreadtrum SC27xx regulator documentation

[PATCH 1/2] dt-bindings: regulator: Add Spreadtrum SC27xx regulator documentation

[Erick Chen]

This patch adds support for the Spreadtrum SC2731
voltage regulator device.

Signed-off-by: Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
---
 .../bindings/regulator/sprd,sc2731-regulator.txt   |   45 ++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt b/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt
new file mode 100644
index 0000000..619385e
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/sprd,sc2731-regulator.txt
@@ -0,0 +1,45 @@
+Spreadtrum SC2731 Voltage regulators
+
+The SC2731 integrates low-voltage and low quiescent current DCDC/LDO.
+14 LDO and 3 DCDCs are designed for external use. All DCDCs/LDOs have
+their own bypass (power-down) control signals. External tantalum or MLCC
+ceramic capacitors are recommended to use with these LDOs.
+
+Required properties:
+ - compatible: should be "sprd,sc2731-regulator".
+ - reg: address offset of regulator registers.
+ - regulators: List of regulators provided by this controller. It is named
+ according to its regulator type, BUCK_<name> and LDO_<name>. The definition
+ for each of these nodes is defined using the standard binding for regulators
+ at Documentation/devicetree/bindings/regulator/regulator.txt.
+
+The valid names for regulators are::
+BUCK:
+	BUCK_CPU0, BUCK_CPU1, BUCK_RF
+LDO:
+	LDO_CAMA0, LDO_CAMA1, LDO_CAMMOT, LDO_VLDO, LDO_EMMCCORE, LDO_SDCORE, LDO_SDIO,
+	LDO_WIFIPA, LDO_USB33, LDO_CAMD0, LDO_CAMD1, LDO_CON, LDO_CAMIO, LDO_SRAM
+
+Example:
+	power-controller@810 {
+		compatible = "sprd,sc2731-regulator";
+		reg = <0x810>;
+
+		regulators {
+			vddarm0: BUCK_CPU0 {
+				regulator-name = "vddarm0";
+				regulator-min-microvolt = <400000>;
+				regulator-max-microvolt = <1996875>;
+				regulator-ramp-delay = <25000>;
+				regulator-always-on;
+			};
+
+			vddcama0: LDO_CAMA0 {
+				regulator-name = "vddcama0";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <3750000>;
+				regulator-enable-ramp-delay = <100>;
+			};
+			...
+		};
+	};
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Erick Chen]

Add regulator driver for Spreadtrum SC2731 device.
It has 17 general purpose LDOs, BUCKs generator and
digital output to control regulators.

Signed-off-by: Erick Chen <erick.chen@spreadtrum.com>
Reviewed-by: Baolin Wang <baolin.wang@spreadtrum.com>
---
 drivers/regulator/Kconfig            |    7 +
 drivers/regulator/Makefile           |    1 +
 drivers/regulator/sc2731-regulator.c |  276 ++++++++++++++++++++++++++++++++++
 3 files changed, 284 insertions(+)
 create mode 100644 drivers/regulator/sc2731-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 96cd55f..b27417c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -744,6 +744,13 @@ config REGULATOR_S5M8767
 	 via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
 	 supports DVS mode with 8bits of output voltage control.
 
+config REGULATOR_SC2731
+	tristate "Spreadtrum SC2731 power regulator driver"
+	depends on MFD_SC27XX_PMIC || COMPILE_TEST
+	help
+	  This driver provides support for the voltage regulators on the
+	  SC2731 PMIC.
+
 config REGULATOR_SKY81452
 	tristate "Skyworks Solutions SKY81452 voltage regulator"
 	depends on MFD_SKY81452
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 80ffc57..19fea09 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -95,6 +95,7 @@ obj-$(CONFIG_REGULATOR_RT5033)	+= rt5033-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
 obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
+obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
 obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
 obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
 obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
diff --git a/drivers/regulator/sc2731-regulator.c b/drivers/regulator/sc2731-regulator.c
new file mode 100644
index 0000000..e56448a
--- /dev/null
+++ b/drivers/regulator/sc2731-regulator.c
@@ -0,0 +1,276 @@
+/*
+ * Copyright (C) 2017 Spreadtrum Communications Inc.
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+/*
+ * SC2731 regulator lock register
+ */
+#define SC2731_PWR_WR_PROT_VALUE	0xf0c
+#define SC2731_WR_UNLOCK		0x6e7f
+
+/*
+ * SC2731 enable register
+ */
+#define SC2731_POWER_PD_SW		0xc28
+#define SC2731_LDO_CAMA0_PD		0xcfc
+#define SC2731_LDO_CAMA1_PD		0xd04
+#define SC2731_LDO_CAMMOT_PD		0xd0c
+#define SC2731_LDO_VLDO_PD		0xd6c
+#define SC2731_LDO_EMMCCORE_PD		0xd2c
+#define SC2731_LDO_SDCORE_PD		0xd74
+#define SC2731_LDO_SDIO_PD		0xd70
+#define SC2731_LDO_WIFIPA_PD		0xd4c
+#define SC2731_LDO_USB33_PD		0xd5c
+#define SC2731_LDO_CAMD0_PD		0xd7c
+#define SC2731_LDO_CAMD1_PD		0xd84
+#define SC2731_LDO_CON_PD		0xd8c
+#define SC2731_LDO_CAMIO_PD		0xd94
+#define SC2731_LDO_SRAM_PD		0xd78
+
+/*
+ * SC2731 enable mask
+ */
+#define SC2731_DCDC_CPU0_PD_MASK	BIT(4)
+#define SC2731_DCDC_CPU1_PD_MASK	BIT(3)
+#define SC2731_DCDC_RF_PD_MASK		BIT(11)
+#define SC2731_LDO_CAMA0_PD_MASK	BIT(0)
+#define SC2731_LDO_CAMA1_PD_MASK	BIT(0)
+#define SC2731_LDO_CAMMOT_PD_MASK	BIT(0)
+#define SC2731_LDO_VLDO_PD_MASK		BIT(0)
+#define SC2731_LDO_EMMCCORE_PD_MASK	BIT(0)
+#define SC2731_LDO_SDCORE_PD_MASK	BIT(0)
+#define SC2731_LDO_SDIO_PD_MASK		BIT(0)
+#define SC2731_LDO_WIFIPA_PD_MASK	BIT(0)
+#define SC2731_LDO_USB33_PD_MASK	BIT(0)
+#define SC2731_LDO_CAMD0_PD_MASK	BIT(0)
+#define SC2731_LDO_CAMD1_PD_MASK	BIT(0)
+#define SC2731_LDO_CON_PD_MASK		BIT(0)
+#define SC2731_LDO_CAMIO_PD_MASK	BIT(0)
+#define SC2731_LDO_SRAM_PD_MASK		BIT(0)
+
+/*
+ * SC2731 vsel register
+ */
+#define SC2731_DCDC_CPU0_VOL		0xc54
+#define SC2731_DCDC_CPU1_VOL		0xc64
+#define SC2731_DCDC_RF_VOL		0xcb8
+#define SC2731_LDO_CAMA0_VOL		0xd00
+#define SC2731_LDO_CAMA1_VOL		0xd08
+#define SC2731_LDO_CAMMOT_VOL		0xd10
+#define SC2731_LDO_VLDO_VOL		0xd28
+#define SC2731_LDO_EMMCCORE_VOL		0xd30
+#define SC2731_LDO_SDCORE_VOL		0xd38
+#define SC2731_LDO_SDIO_VOL		0xd40
+#define SC2731_LDO_WIFIPA_VOL		0xd50
+#define SC2731_LDO_USB33_VOL		0xd60
+#define SC2731_LDO_CAMD0_VOL		0xd80
+#define SC2731_LDO_CAMD1_VOL		0xd88
+#define SC2731_LDO_CON_VOL		0xd90
+#define SC2731_LDO_CAMIO_VOL		0xd98
+#define SC2731_LDO_SRAM_VOL		0xdB0
+
+/*
+ * SC2731 vsel register mask
+ */
+#define SC2731_DCDC_CPU0_VOL_MASK	GENMASK(8, 0)
+#define SC2731_DCDC_CPU1_VOL_MASK	GENMASK(8, 0)
+#define SC2731_DCDC_RF_VOL_MASK		GENMASK(8, 0)
+#define SC2731_LDO_CAMA0_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_CAMA1_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_CAMMOT_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_VLDO_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_EMMCCORE_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_SDCORE_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_SDIO_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_WIFIPA_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_USB33_VOL_MASK	GENMASK(7, 0)
+#define SC2731_LDO_CAMD0_VOL_MASK	GENMASK(6, 0)
+#define SC2731_LDO_CAMD1_VOL_MASK	GENMASK(6, 0)
+#define SC2731_LDO_CON_VOL_MASK		GENMASK(6, 0)
+#define SC2731_LDO_CAMIO_VOL_MASK	GENMASK(6, 0)
+#define SC2731_LDO_SRAM_VOL_MASK	GENMASK(6, 0)
+
+enum sc2731_regulator_id {
+	SC2731_BUCK_CPU0,
+	SC2731_BUCK_CPU1,
+	SC2731_BUCK_RF,
+	SC2731_LDO_CAMA0,
+	SC2731_LDO_CAMA1,
+	SC2731_LDO_CAMMOT,
+	SC2731_LDO_VLDO,
+	SC2731_LDO_EMMCCORE,
+	SC2731_LDO_SDCORE,
+	SC2731_LDO_SDIO,
+	SC2731_LDO_WIFIPA,
+	SC2731_LDO_USB33,
+	SC2731_LDO_CAMD0,
+	SC2731_LDO_CAMD1,
+	SC2731_LDO_CON,
+	SC2731_LDO_CAMIO,
+	SC2731_LDO_SRAM,
+};
+
+static const struct regulator_ops sc2731_regu_linear_ops = {
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.list_voltage = regulator_list_voltage_linear,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+};
+
+#define SC2731_REGU_LINEAR(_id, en_reg, en_mask, vreg, vmask,	\
+			  vstep, vmin, vmax) {			\
+	.name			= #_id,				\
+	.of_match		= of_match_ptr(#_id),		\
+	.ops			= &sc2731_regu_linear_ops,	\
+	.regulators_node	= of_match_ptr("regulators"),	\
+	.type			= REGULATOR_VOLTAGE,		\
+	.id			= SC2731_##_id,			\
+	.owner			= THIS_MODULE,			\
+	.min_uV			= vmin,				\
+	.n_voltages		= ((vmax) - (vmin)) / (vstep) + 1,	\
+	.uV_step		= vstep,			\
+	.enable_is_inverted	= true,				\
+	.enable_val		= 0,				\
+	.enable_reg		= en_reg,			\
+	.enable_mask		= en_mask,			\
+	.vsel_reg		= vreg,				\
+	.vsel_mask		= vmask,			\
+}
+
+static struct regulator_desc regulators[] = {
+	SC2731_REGU_LINEAR(BUCK_CPU0, SC2731_POWER_PD_SW,
+			   SC2731_DCDC_CPU0_PD_MASK, SC2731_DCDC_CPU0_VOL,
+			   SC2731_DCDC_CPU0_VOL_MASK, 3125, 400000, 1996875),
+	SC2731_REGU_LINEAR(BUCK_CPU1, SC2731_POWER_PD_SW,
+			   SC2731_DCDC_CPU1_PD_MASK, SC2731_DCDC_CPU1_VOL,
+			   SC2731_DCDC_CPU1_VOL_MASK, 3125, 400000, 1996875),
+	SC2731_REGU_LINEAR(BUCK_RF, SC2731_POWER_PD_SW, SC2731_DCDC_RF_PD_MASK,
+			   SC2731_DCDC_RF_VOL, SC2731_DCDC_RF_VOL_MASK,
+			   3125, 600000, 2196875),
+	SC2731_REGU_LINEAR(LDO_CAMA0, SC2731_LDO_CAMA0_PD,
+			   SC2731_LDO_CAMA0_PD_MASK, SC2731_LDO_CAMA0_VOL,
+			   SC2731_LDO_CAMA0_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_CAMA1, SC2731_LDO_CAMA1_PD,
+			   SC2731_LDO_CAMA1_PD_MASK, SC2731_LDO_CAMA1_VOL,
+			   SC2731_LDO_CAMA1_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_CAMMOT, SC2731_LDO_CAMMOT_PD,
+			   SC2731_LDO_CAMMOT_PD_MASK, SC2731_LDO_CAMMOT_VOL,
+			   SC2731_LDO_CAMMOT_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_VLDO, SC2731_LDO_VLDO_PD,
+			   SC2731_LDO_VLDO_PD_MASK, SC2731_LDO_VLDO_VOL,
+			   SC2731_LDO_VLDO_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_EMMCCORE, SC2731_LDO_EMMCCORE_PD,
+			   SC2731_LDO_EMMCCORE_PD_MASK, SC2731_LDO_EMMCCORE_VOL,
+			   SC2731_LDO_EMMCCORE_VOL_MASK, 10000, 1200000,
+			   3750000),
+	SC2731_REGU_LINEAR(LDO_SDCORE, SC2731_LDO_SDCORE_PD,
+			   SC2731_LDO_SDCORE_PD_MASK, SC2731_LDO_SDCORE_VOL,
+			   SC2731_LDO_SDCORE_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_SDIO, SC2731_LDO_SDIO_PD,
+			   SC2731_LDO_SDIO_PD_MASK, SC2731_LDO_SDIO_VOL,
+			   SC2731_LDO_SDIO_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_WIFIPA, SC2731_LDO_WIFIPA_PD,
+			   SC2731_LDO_WIFIPA_PD_MASK, SC2731_LDO_WIFIPA_VOL,
+			   SC2731_LDO_WIFIPA_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_USB33, SC2731_LDO_USB33_PD,
+			   SC2731_LDO_USB33_PD_MASK, SC2731_LDO_USB33_VOL,
+			   SC2731_LDO_USB33_VOL_MASK, 10000, 1200000, 3750000),
+	SC2731_REGU_LINEAR(LDO_CAMD0, SC2731_LDO_CAMD0_PD,
+			   SC2731_LDO_CAMD0_PD_MASK, SC2731_LDO_CAMD0_VOL,
+			   SC2731_LDO_CAMD0_VOL_MASK, 6250, 1000000, 1793750),
+	SC2731_REGU_LINEAR(LDO_CAMD1, SC2731_LDO_CAMD1_PD,
+			   SC2731_LDO_CAMD1_PD_MASK, SC2731_LDO_CAMD1_VOL,
+			   SC2731_LDO_CAMD1_VOL_MASK, 6250, 1000000, 1793750),
+	SC2731_REGU_LINEAR(LDO_CON, SC2731_LDO_CON_PD,
+			   SC2731_LDO_CON_PD_MASK, SC2731_LDO_CON_VOL,
+			   SC2731_LDO_CON_VOL_MASK, 6250, 1000000, 1793750),
+	SC2731_REGU_LINEAR(LDO_CAMIO, SC2731_LDO_CAMIO_PD,
+			   SC2731_LDO_CAMIO_PD_MASK, SC2731_LDO_CAMIO_VOL,
+			   SC2731_LDO_CAMIO_VOL_MASK, 6250, 1000000, 1793750),
+	SC2731_REGU_LINEAR(LDO_SRAM, SC2731_LDO_SRAM_PD,
+			   SC2731_LDO_SRAM_PD_MASK, SC2731_LDO_SRAM_VOL,
+			   SC2731_LDO_SRAM_VOL_MASK, 6250, 1000000, 1793750),
+};
+
+static int sc2731_regulator_unlock(struct regmap *regmap)
+{
+	return regmap_write(regmap, SC2731_PWR_WR_PROT_VALUE,
+			    SC2731_WR_UNLOCK);
+}
+
+static int sc2731_regulator_probe(struct platform_device *pdev)
+{
+	int i, ret;
+	struct regmap *regmap;
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap) {
+		dev_err(&pdev->dev, "failed to get regmap.\n");
+		return -ENODEV;
+	}
+
+	ret = sc2731_regulator_unlock(regmap);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to release regulator lock\n");
+		return ret;
+	}
+
+	config.dev = &pdev->dev;
+	config.regmap = regmap;
+
+	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
+		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "failed to register regulator %s\n",
+				regulators[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id sc2731_regulator_of_match[] = {
+	{.compatible = "sprd,sc2731-regulator",},
+	{}
+};
+MODULE_DEVICE_TABLE(of, sc2731_regulator_of_match);
+
+static struct platform_driver sc2731_regulator_driver = {
+	.driver = {
+		.name = "sc2731-regulator",
+		.of_match_table = sc2731_regulator_of_match,
+	},
+	.probe = sc2731_regulator_probe,
+};
+
+static int __init sc2731_regulator_init(void)
+{
+	return platform_driver_register(&sc2731_regulator_driver);
+}
+
+static void __exit sc2731_regulator_exit(void)
+{
+	platform_driver_unregister(&sc2731_regulator_driver);
+}
+
+subsys_initcall(sc2731_regulator_init);
+module_exit(sc2731_regulator_exit);
+
+MODULE_AUTHOR("Chen Junhui <erick.chen@spreadtrum.com>");
+MODULE_DESCRIPTION("Spreadtrum SC2731 regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Philippe Ombredanne]

Erik,

On Fri, Dec 1, 2017 at 9:58 AM, Erick Chen <erick.chen@spreadtrum.com> wrote:
> Add regulator driver for Spreadtrum SC2731 device.
> It has 17 general purpose LDOs, BUCKs generator and
> digital output to control regulators.
>
> Signed-off-by: Erick Chen <erick.chen@spreadtrum.com>
> Reviewed-by: Baolin Wang <baolin.wang@spreadtrum.com>
> ---
>  drivers/regulator/Kconfig            |    7 +
>  drivers/regulator/Makefile           |    1 +
>  drivers/regulator/sc2731-regulator.c |  276 ++++++++++++++++++++++++++++++++++
>  3 files changed, 284 insertions(+)
>  create mode 100644 drivers/regulator/sc2731-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 96cd55f..b27417c 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -744,6 +744,13 @@ config REGULATOR_S5M8767
>          via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
>          supports DVS mode with 8bits of output voltage control.
>
> +config REGULATOR_SC2731
> +       tristate "Spreadtrum SC2731 power regulator driver"
> +       depends on MFD_SC27XX_PMIC || COMPILE_TEST
> +       help
> +         This driver provides support for the voltage regulators on the
> +         SC2731 PMIC.
> +
>  config REGULATOR_SKY81452
>         tristate "Skyworks Solutions SKY81452 voltage regulator"
>         depends on MFD_SKY81452
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 80ffc57..19fea09 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -95,6 +95,7 @@ obj-$(CONFIG_REGULATOR_RT5033)        += rt5033-regulator.o
>  obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
>  obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
>  obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
> +obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
>  obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
>  obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
>  obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
> diff --git a/drivers/regulator/sc2731-regulator.c b/drivers/regulator/sc2731-regulator.c
> new file mode 100644
> index 0000000..e56448a
> --- /dev/null
> +++ b/drivers/regulator/sc2731-regulator.c
> @@ -0,0 +1,276 @@
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */

I think that per Linus, and Thomas doc patches for SPDX ids this
should be instead either:

> +// SPDX-License-Identifier: GPL-2.0
> + // Copyright (c) 2017 Spreadtrum Communications Inc.

or at least this with the id on the first and the // comment style

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017 Spreadtrum Communications Inc.
> + *
> + */


-- 
Cordially
Philippe Ombredanne

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Mark Brown]

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

On Fri, Dec 01, 2017 at 10:13:27AM +0100, Philippe Ombredanne wrote:
> On Fri, Dec 1, 2017 at 9:58 AM, Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org> wrote:

> I think that per Linus, and Thomas doc patches for SPDX ids this
> should be instead either:

> > +// SPDX-License-Identifier: GPL-2.0
> > + // Copyright (c) 2017 Spreadtrum Communications Inc.

> or at least this with the id on the first and the // comment style

> > +// SPDX-License-Identifier: GPL-2.0

Are you saying SPDX requires C++ style comments?  That seems totally
broken.

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

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Philippe Ombredanne]

Mark,

On Fri, Dec 1, 2017 at 1:49 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Dec 01, 2017 at 10:13:27AM +0100, Philippe Ombredanne wrote:
>> On Fri, Dec 1, 2017 at 9:58 AM, Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org> wrote:
>
>> I think that per Linus, and Thomas doc patches for SPDX ids this
>> should be instead either:
>
>> > +// SPDX-License-Identifier: GPL-2.0
>> > + // Copyright (c) 2017 Spreadtrum Communications Inc.
>
>> or at least this with the id on the first and the // comment style
>
>> > +// SPDX-License-Identifier: GPL-2.0
>
> Are you saying SPDX requires C++ style comments?  That seems totally
> broken.

In can understand your point, but for reference please check Linus
[1][2][3], Thomas[4] and Greg[5] comments on the topic.

I am just a lowly messenger and even though I personally agree with
Linus points and taste in this area, my weightless voice does not
matter.

CC: Linus, Greg and Thomas

[1] https://lkml.org/lkml/2017/11/2/715
[2] https://lkml.org/lkml/2017/11/25/125
[3] https://lkml.org/lkml/2017/11/25/133
[4] https://lkml.org/lkml/2017/11/2/805
[5] https://lkml.org/lkml/2017/10/19/165
-- 
Cordially
Philippe Ombredanne
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Mark Brown]

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

On Fri, Dec 01, 2017 at 02:39:42PM +0100, Philippe Ombredanne wrote:
> On Fri, Dec 1, 2017 at 1:49 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> >> or at least this with the id on the first and the // comment style

> >> > +// SPDX-License-Identifier: GPL-2.0

I snipped a bit of context here, the full example was actually:

| > +// SPDX-License-Identifier: GPL-2.0
| > +/*
| > + * Copyright (C) 2017 Spreadtrum Communications Inc.
| > + *

> > Are you saying SPDX requires C++ style comments?  That seems totally
> > broken.

> In can understand your point, but for reference please check Linus
> [1][2][3], Thomas[4] and Greg[5] comments on the topic.

> I am just a lowly messenger and even though I personally agree with
> Linus points and taste in this area, my weightless voice does not
> matter.

OK, so that's not quite what you were saying then.  The desire is to
have the SPDX block be the first line of the file (which you didn't
mention) and in general to encourage the use of C++ comments.  The
second example you gave there with the C++ comment followed immediately
by a C comment is definitely bogus.  If this is going to be the standard
we probably need tooling to enforce it, it's an extra requirement
compared to both cross project SPDX needs and existing Linux practice so
it's going to be really easy to end up with different things.

I have to say I'd have thought that if we were going to have a fixed
format for ease of parsing we'd be putting it at the end of files where
we already often have a block of very regularly formatted MODULE_FOO()
lines (not that that's 100% regular but it's more common than not) but
it doesn't make much difference and we've already got a large number of
these SPDX things done so it's a bit of a done deal.

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

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Erick Chen]

Hi Philippe,

On Fri, Dec 01, 2017 at 10:13:27AM +0100, Philippe Ombredanne wrote:
> Erik,
> 
> On Fri, Dec 1, 2017 at 9:58 AM, Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org> wrote:
> > Add regulator driver for Spreadtrum SC2731 device.
> > It has 17 general purpose LDOs, BUCKs generator and
> > digital output to control regulators.
> >
> > Signed-off-by: Erick Chen <erick.chen-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> > Reviewed-by: Baolin Wang <baolin.wang-lxIno14LUO0EEoCn2XhGlw@public.gmane.org>
> > ---
> >  drivers/regulator/Kconfig            |    7 +
> >  drivers/regulator/Makefile           |    1 +
> >  drivers/regulator/sc2731-regulator.c |  276 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 284 insertions(+)
> >  create mode 100644 drivers/regulator/sc2731-regulator.c
> >
> > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> > index 96cd55f..b27417c 100644
> > --- a/drivers/regulator/Kconfig
> > +++ b/drivers/regulator/Kconfig
> > @@ -744,6 +744,13 @@ config REGULATOR_S5M8767
> >          via I2C bus. S5M8767A have 9 Bucks and 28 LDOs output and
> >          supports DVS mode with 8bits of output voltage control.
> >
> > +config REGULATOR_SC2731
> > +       tristate "Spreadtrum SC2731 power regulator driver"
> > +       depends on MFD_SC27XX_PMIC || COMPILE_TEST
> > +       help
> > +         This driver provides support for the voltage regulators on the
> > +         SC2731 PMIC.
> > +
> >  config REGULATOR_SKY81452
> >         tristate "Skyworks Solutions SKY81452 voltage regulator"
> >         depends on MFD_SKY81452
> > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> > index 80ffc57..19fea09 100644
> > --- a/drivers/regulator/Makefile
> > +++ b/drivers/regulator/Makefile
> > @@ -95,6 +95,7 @@ obj-$(CONFIG_REGULATOR_RT5033)        += rt5033-regulator.o
> >  obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
> >  obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
> >  obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
> > +obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
> >  obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
> >  obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
> >  obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
> > diff --git a/drivers/regulator/sc2731-regulator.c b/drivers/regulator/sc2731-regulator.c
> > new file mode 100644
> > index 0000000..e56448a
> > --- /dev/null
> > +++ b/drivers/regulator/sc2731-regulator.c
> > @@ -0,0 +1,276 @@
> > +/*
> > + * Copyright (C) 2017 Spreadtrum Communications Inc.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> 
> I think that per Linus, and Thomas doc patches for SPDX ids this
> should be instead either:
> 
> > +// SPDX-License-Identifier: GPL-2.0
> > + // Copyright (c) 2017 Spreadtrum Communications Inc.
> 
> or at least this with the id on the first and the // comment style
>

Thanks for pointing out this, and I will modify them in next version.
 
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2017 Spreadtrum Communications Inc.
> > + *
> > + */
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Mark Brown]

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

On Fri, Dec 01, 2017 at 04:58:16PM +0800, Erick Chen wrote:

> +static const struct of_device_id sc2731_regulator_of_match[] = {
> +	{.compatible = "sprd,sc2731-regulator",},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sc2731_regulator_of_match);

This looks like a subdriver for one specific device that can't be reused
with anything else as it's specific to an individual device so I'd not
expect it to appear as a separate thing in DT - the way Linux splits
things up might not map well onto other OSs and may even change in
future versions of Linux (look at all the audio drivers with clock
controllers in them for example).  I'd expect the MFD to just register
the subdevice without needing it to appear in the DT.

> +subsys_initcall(sc2731_regulator_init);
> +module_exit(sc2731_regulator_exit);

Why not module_platform_driver()?  Deferred probe should sort out probe
order.

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

Re: [PATCH 2/2] regulator: sc2731: Add regulator driver to support Spreadtrum SC2731 PMIC

[Erick Chen]

Hi Mark,

On Fri, Dec 01, 2017 at 01:01:37PM +0000, Mark Brown wrote:
> On Fri, Dec 01, 2017 at 04:58:16PM +0800, Erick Chen wrote:
> 
> > +static const struct of_device_id sc2731_regulator_of_match[] = {
> > +	{.compatible = "sprd,sc2731-regulator",},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, sc2731_regulator_of_match);
> 
> This looks like a subdriver for one specific device that can't be reused
> with anything else as it's specific to an individual device so I'd not
> expect it to appear as a separate thing in DT - the way Linux splits
> things up might not map well onto other OSs and may even change in
> future versions of Linux (look at all the audio drivers with clock
> controllers in them for example).  I'd expect the MFD to just register
> the subdevice without needing it to appear in the DT.
>

Make sense, i will remove the of_device_id table in next version.
 
> > +subsys_initcall(sc2731_regulator_init);
> > +module_exit(sc2731_regulator_exit);
> 
> Why not module_platform_driver()?  Deferred probe should sort out probe
> order.

On some Spreadtrum old platforms, we need to power on the CPU cores ASAP.
But now this issue had been fixed after some investigation, so we can change
subsys_initcall() to module_init() level as you suggested.

Very appreciated for your helpful comments.