[PATCH v4 0/6] LM36274 Introduction

[PATCH v4 0/6] LM36274 Introduction

[Dan Murphy]

Hello

This is patch set v4 for the LM36274.  There were no changes made
to this patch set except to rebase this on top of the latest TI LMU common code
patchset.

This patch set was rebased on the series at:
https://lore.kernel.org/patchwork/project/lkml/list/?series=393071

Dan

Dan Murphy (6):
  regulator: lm363x: Make the gpio register enable flexible
  dt-bindings: mfd: Add lm36274 bindings to ti-lmu
  mfd: ti-lmu: Add LM36274 support to the ti-lmu
  regulator: lm363x: Add support for LM36274
  dt-bindings: leds: Add LED bindings for the LM36274
  leds: lm36274: Introduce the TI LM36274 LED driver

 .../devicetree/bindings/leds/leds-lm36274.txt |  82 +++++++++
 .../devicetree/bindings/mfd/ti-lmu.txt        |  54 ++++++
 drivers/leds/Kconfig                          |   7 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lm36274.c                   | 174 ++++++++++++++++++
 drivers/mfd/Kconfig                           |   5 +-
 drivers/mfd/ti-lmu.c                          |  14 ++
 drivers/regulator/Kconfig                     |   2 +-
 drivers/regulator/lm363x-regulator.c          |  56 +++++-
 include/linux/mfd/ti-lmu-register.h           |  23 +++
 include/linux/mfd/ti-lmu.h                    |   4 +
 11 files changed, 416 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm36274.txt
 create mode 100644 drivers/leds/leds-lm36274.c

-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Dan Murphy]

The use of and enablement of the GPIO can be used across devices.
Use the enable_reg in the regulator descriptor for the register to
write.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/regulator/lm363x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index c876e161052a..382b1cecdd93 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
 
 	if (gpiod) {
 		cfg.ena_gpiod = gpiod;
-
-		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
+		ret = regmap_update_bits(regmap,
+					 lm363x_regulator_desc[id].enable_reg,
 					 LM3632_EXT_EN_MASK,
 					 LM3632_EXT_EN_MASK);
 		if (ret) {
-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 2/6] dt-bindings: mfd: Add lm36274 bindings to ti-lmu

[Dan Murphy]

Add the LM36274 backlight driver with regulator support.
This is a multi-function device for backlight applications.

Backlight properties will be documented in it's a supplemental
bindings document.

Regulator support is documented in the regulator/lm363x-regulator.txt

http://www.ti.com/lit/ds/symlink/lm36274.pdf

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/mfd/ti-lmu.txt        | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
index 782d3c9812ed..2296b8f24de4 100644
--- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -8,6 +8,7 @@ TI LMU driver supports lighting devices below.
   LM3632       Backlight and regulator
   LM3633       Backlight, LED and fault monitor
   LM3695       Backlight
+  LM36274      Backlight and regulator
 
 Required properties:
   - compatible: Should be one of:
@@ -15,11 +16,13 @@ Required properties:
                 "ti,lm3632"
                 "ti,lm3633"
                 "ti,lm3695"
+		"ti,lm36274"
   - reg: I2C slave address.
          0x11 for LM3632
          0x29 for LM3631
          0x36 for LM3633
          0x63 for LM3695
+         0x11 for LM36274
 
 Optional properties:
   - enable-gpios: A GPIO specifier for hardware enable pin.
@@ -50,12 +53,14 @@ Optional nodes:
       - compatible: Should be one of:
                     "ti,lm3633-fault-monitor"
   - leds: LED properties for LM3633. Please refer to [2].
+	  LED properties for LM36274. Please refer to [4].
   - regulators: Regulator properties for LM3631 and LM3632.
                 Please refer to [3].
 
 [1] ../leds/backlight/ti-lmu-backlight.txt
 [2] ../leds/leds-lm3633.txt
 [3] ../regulator/lm363x-regulator.txt
+[4] ../leds/leds-lm36274.txt
 
 lm3631@29 {
 	compatible = "ti,lm3631";
@@ -213,3 +218,52 @@ lm3695@63 {
 		};
 	};
 };
+
+lm36274@11 {
+	compatible = "ti,lm36274";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x11>;
+
+	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,lm363x-regulator";
+
+		enable-gpios = <&pioC 0 GPIO_ACTIVE_HIGH>,
+			       <&pioC 1 GPIO_ACTIVE_HIGH>;
+
+		vboost {
+			regulator-name = "lcd_boost";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <7150000>;
+			regulator-always-on;
+		};
+
+		vpos {
+			regulator-name = "lcd_vpos";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <6500000>;
+		};
+
+		vneg {
+			regulator-name = "lcd_vneg";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <6500000>;
+		};
+	};
+
+	backlight {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,lm36274-backlight";
+
+		led@0 {
+			reg = <0>;
+			led-sources = <0 2>;
+			label = "white:backlight_cluster";
+			linux,default-trigger = "backlight";
+		};
+	};
+};
-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 3/6] mfd: ti-lmu: Add LM36274 support to the ti-lmu

[Dan Murphy]

Add the LM36274 register support to the ti-lmu MFD driver.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/mfd/Kconfig                 |  5 ++---
 drivers/mfd/ti-lmu.c                | 14 ++++++++++++++
 include/linux/mfd/ti-lmu-register.h | 23 +++++++++++++++++++++++
 include/linux/mfd/ti-lmu.h          |  4 ++++
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fcae244229b3..5606411038bb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1311,9 +1311,8 @@ config MFD_TI_LMU
 	select REGMAP_I2C
 	help
 	  Say yes here to enable support for TI LMU chips.
-
-	  TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, and LM3695.
-	  It consists of backlight, LED and regulator driver.
+	  TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and
+	  LM36274.  It consists of backlight, LED and regulator driver.
 	  It provides consistent device controls for lighting functions.
 
 config MFD_OMAP_USB_HOST
diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
index 89b1c5b584af..691ab9dd6236 100644
--- a/drivers/mfd/ti-lmu.c
+++ b/drivers/mfd/ti-lmu.c
@@ -111,6 +111,17 @@ static const struct mfd_cell lm3695_devices[] = {
 	},
 };
 
+static const struct mfd_cell lm36274_devices[] = {
+	LM363X_REGULATOR(LM36274_BOOST),
+	LM363X_REGULATOR(LM36274_LDO_POS),
+	LM363X_REGULATOR(LM36274_LDO_NEG),
+	{
+		.name          = "lm36274-leds",
+		.id            = LM36274,
+		.of_compatible = "ti,lm36274-backlight",
+	},
+};
+
 #define TI_LMU_DATA(chip, max_reg)		\
 static const struct ti_lmu_data chip##_data =	\
 {						\
@@ -123,6 +134,7 @@ TI_LMU_DATA(lm3631, LM3631_MAX_REG);
 TI_LMU_DATA(lm3632, LM3632_MAX_REG);
 TI_LMU_DATA(lm3633, LM3633_MAX_REG);
 TI_LMU_DATA(lm3695, LM3695_MAX_REG);
+TI_LMU_DATA(lm36274, LM36274_MAX_REG);
 
 static int ti_lmu_probe(struct i2c_client *cl, const struct i2c_device_id *id)
 {
@@ -191,6 +203,7 @@ static const struct of_device_id ti_lmu_of_match[] = {
 	{ .compatible = "ti,lm3632", .data = &lm3632_data },
 	{ .compatible = "ti,lm3633", .data = &lm3633_data },
 	{ .compatible = "ti,lm3695", .data = &lm3695_data },
+	{ .compatible = "ti,lm36274", .data = &lm36274_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_lmu_of_match);
@@ -200,6 +213,7 @@ static const struct i2c_device_id ti_lmu_ids[] = {
 	{ "lm3632", LM3632 },
 	{ "lm3633", LM3633 },
 	{ "lm3695", LM3695 },
+	{ "lm36274", LM36274 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ti_lmu_ids);
diff --git a/include/linux/mfd/ti-lmu-register.h b/include/linux/mfd/ti-lmu-register.h
index 76998b01764b..076d8dea38fd 100644
--- a/include/linux/mfd/ti-lmu-register.h
+++ b/include/linux/mfd/ti-lmu-register.h
@@ -189,4 +189,27 @@
 #define LM3695_REG_BRT_MSB			0x14
 
 #define LM3695_MAX_REG				0x14
+
+/* LM36274 */
+#define LM36274_REG_REV				0x01
+#define LM36274_REG_BL_CFG_1			0x02
+#define LM36274_REG_BL_CFG_2			0x03
+#define LM36274_REG_BRT_LSB			0x04
+#define LM36274_REG_BRT_MSB			0x05
+#define LM36274_REG_BL_EN			0x08
+
+#define LM36274_REG_BIAS_CONFIG_1		0x09
+#define LM36274_EXT_EN_MASK			BIT(0)
+#define LM36274_EN_VNEG_MASK			BIT(1)
+#define LM36274_EN_VPOS_MASK			BIT(2)
+
+#define LM36274_REG_BIAS_CONFIG_2		0x0a
+#define LM36274_REG_BIAS_CONFIG_3		0x0b
+#define LM36274_REG_VOUT_BOOST			0x0c
+#define LM36274_REG_VOUT_POS			0x0d
+#define LM36274_REG_VOUT_NEG			0x0e
+#define LM36274_VOUT_MASK			0x3F
+
+#define LM36274_MAX_REG				0x13
+
 #endif
diff --git a/include/linux/mfd/ti-lmu.h b/include/linux/mfd/ti-lmu.h
index 54e9d272e81c..0957598c7d41 100644
--- a/include/linux/mfd/ti-lmu.h
+++ b/include/linux/mfd/ti-lmu.h
@@ -26,6 +26,7 @@ enum ti_lmu_id {
 	LM3632,
 	LM3633,
 	LM3695,
+	LM36274,
 	LMU_MAX_ID,
 };
 
@@ -67,6 +68,9 @@ enum lm363x_regulator_id {
 	LM3632_BOOST,		/* Boost output */
 	LM3632_LDO_POS,		/* Positive display bias output */
 	LM3632_LDO_NEG,		/* Negative display bias output */
+	LM36274_BOOST,		/* Boost output */
+	LM36274_LDO_POS,	/* Positive display bias output */
+	LM36274_LDO_NEG,	/* Negative display bias output */
 };
 
 /**
-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 4/6] regulator: lm363x: Add support for LM36274

[Dan Murphy]

Adding regulator support for the LM36274 backlight driver.
This device can leverage this existing code as the functionality
and registers are common enough between the LM36274 and the LM363x
series of devices.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/regulator/Kconfig            |  2 +-
 drivers/regulator/lm363x-regulator.c | 52 ++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index b7f249ee5e68..23252ae81fdf 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -362,7 +362,7 @@ config REGULATOR_LM363X
 	tristate "TI LM363X voltage regulators"
 	depends on MFD_TI_LMU
 	help
-	  This driver supports LM3631 and LM3632 voltage regulators for
+	  This driver supports LM3631, LM3632 and LM36274 voltage regulators for
 	  the LCD bias.
 	  One boost output voltage is configurable and always on.
 	  Other LDOs are used for the display module.
diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index 382b1cecdd93..1944677b1448 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -37,6 +37,11 @@
 #define LM3632_VBOOST_MIN		4500000
 #define LM3632_VLDO_MIN			4000000
 
+/* LM36274 */
+#define LM36274_BOOST_VSEL_MAX		0x3f
+#define LM36274_LDO_VSEL_MAX		0x34
+#define LM36274_VOLTAGE_MIN		4000000
+
 /* Common */
 #define LM363X_STEP_50mV		50000
 #define LM363X_STEP_500mV		500000
@@ -217,6 +222,51 @@ static const struct regulator_desc lm363x_regulator_desc[] = {
 		.enable_reg     = LM3632_REG_BIAS_CONFIG,
 		.enable_mask    = LM3632_EN_VNEG_MASK,
 	},
+
+	/* LM36274 */
+	{
+		.name           = "vboost",
+		.of_match	= "vboost",
+		.id             = LM36274_BOOST,
+		.ops            = &lm363x_boost_voltage_table_ops,
+		.n_voltages     = LM36274_BOOST_VSEL_MAX,
+		.min_uV         = LM36274_VOLTAGE_MIN,
+		.uV_step        = LM363X_STEP_50mV,
+		.type           = REGULATOR_VOLTAGE,
+		.owner          = THIS_MODULE,
+		.vsel_reg       = LM36274_REG_VOUT_BOOST,
+		.vsel_mask      = LM36274_VOUT_MASK,
+	},
+	{
+		.name           = "ldo_vpos",
+		.of_match	= "vpos",
+		.id             = LM36274_LDO_POS,
+		.ops            = &lm363x_regulator_voltage_table_ops,
+		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.min_uV         = LM36274_VOLTAGE_MIN,
+		.uV_step        = LM363X_STEP_50mV,
+		.type           = REGULATOR_VOLTAGE,
+		.owner          = THIS_MODULE,
+		.vsel_reg       = LM36274_REG_VOUT_POS,
+		.vsel_mask      = LM36274_VOUT_MASK,
+		.enable_reg     = LM36274_REG_BIAS_CONFIG_1,
+		.enable_mask    = LM36274_EN_VPOS_MASK,
+	},
+	{
+		.name           = "ldo_vneg",
+		.of_match	= "vneg",
+		.id             = LM36274_LDO_NEG,
+		.ops            = &lm363x_regulator_voltage_table_ops,
+		.n_voltages     = LM36274_LDO_VSEL_MAX,
+		.min_uV         = LM36274_VOLTAGE_MIN,
+		.uV_step        = LM363X_STEP_50mV,
+		.type           = REGULATOR_VOLTAGE,
+		.owner          = THIS_MODULE,
+		.vsel_reg       = LM36274_REG_VOUT_NEG,
+		.vsel_mask      = LM36274_VOUT_MASK,
+		.enable_reg     = LM36274_REG_BIAS_CONFIG_1,
+		.enable_mask    = LM36274_EN_VNEG_MASK,
+	},
 };
 
 static struct gpio_desc *lm363x_regulator_of_get_enable_gpio(struct device *dev, int id)
@@ -229,9 +279,11 @@ static struct gpio_desc *lm363x_regulator_of_get_enable_gpio(struct device *dev,
 	 */
 	switch (id) {
 	case LM3632_LDO_POS:
+	case LM36274_LDO_POS:
 		return gpiod_get_index_optional(dev, "enable", 0,
 				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	case LM3632_LDO_NEG:
+	case LM36274_LDO_NEG:
 		return gpiod_get_index_optional(dev, "enable", 1,
 				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	default:
-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 5/6] dt-bindings: leds: Add LED bindings for the LM36274

[Dan Murphy]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="", Size: 2860 bytes --]

Add the LM36274 LED specific bindings.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/leds/leds-lm36274.txt | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm36274.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm36274.txt b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
new file mode 100644
index 000000000000..329393700191
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
@@ -0,0 +1,82 @@
+* Texas Instruments LM36274 4-Channel LCD Backlight Driver w/Integrated Bias
+
+The LM36274 is an integrated four-channel WLED driver and LCD bias supply.
+The backlight boost provides the power to bias four parallel LED strings with
+up to 29V total output voltage. The 11-bit LED current is programmable via
+the I2C bus and/or controlled via a logic level PWM input from 60 μA to 30 mA.
+
+Parent device properties are documented in ../mfd/ti_lmu.txt
+Regulator properties are documented in ../regulator/lm363x-regulator.txt
+
+Required backlight properties:
+	- compatible:
+		"ti,lm36274-backlight"
+	- reg : 0
+	- #address-cells : 1
+	- #size-cells : 0
+	- led-sources : Indicates which LED strings will be enabled.
+			Values from 0-3, sources is 0 based so strings will be
+			source value + 1.
+
+Optional backlight properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger :
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+HVLED string 1 and 3 are controlled by control bank A and HVLED 2 string is
+controlled by control bank B.
+
+lm36274@11 {
+	compatible = "ti,lm36274";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x11>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,lm363x-regulator";
+
+		enable-gpios = <&pioC 0 GPIO_ACTIVE_HIGH>,
+			       <&pioC 1 GPIO_ACTIVE_HIGH>;
+
+		vboost {
+			regulator-name = "lcd_boost";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <7150000>;
+			regulator-always-on;
+		};
+
+		vpos {
+			regulator-name = "lcd_vpos";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <6500000>;
+		};
+
+		vneg {
+			regulator-name = "lcd_vneg";
+			regulator-min-microvolt = <4000000>;
+			regulator-max-microvolt = <6500000>;
+		};
+	};
+
+	backlight {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "ti,lm36274-backlight";
+
+		led@0 {
+			reg = <0>;
+			led-sources = <0 2>;
+			label = "white:backlight_cluster";
+			linux,default-trigger = "backlight";
+		};
+	};
+};
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lm36274.pdf
-- 
2.21.0.5.gaeb582a983

[RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Dan Murphy]

Introduce the LM36274 LED driver.  This driver uses the ti-lmu
MFD driver to probe this LED driver.  The driver configures only the
LED registers and enables the outputs according to the config file.

The driver utilizes the TI LMU (Lighting Management Unit) LED common
framework to set the brightness bits.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/Kconfig        |   7 ++
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-lm36274.c | 174 ++++++++++++++++++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 drivers/leds/leds-lm36274.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 255fdd5e8491..db83a3feca01 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -791,6 +791,13 @@ config LEDS_LM3697
 	  Say Y to enable the LM3697 LED driver for TI LMU devices.
 	  This supports the LED device LM3697.
 
+config LEDS_LM36274
+	tristate "LED driver for LM36274"
+	depends on LEDS_TI_LMU_COMMON
+	help
+	  Say Y to enable the LM36274 LED driver for TI LMU devices.
+	  This supports the LED device LM36274.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8ab825c8b5c3..c229432b7fe7 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= leds-ti-lmu-common.o
 obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
+obj-$(CONFIG_LEDS_LM36274)		+= leds-lm36274.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/leds-lm36274.c b/drivers/leds/leds-lm36274.c
new file mode 100644
index 000000000000..b47786d36d21
--- /dev/null
+++ b/drivers/leds/leds-lm36274.c
@@ -0,0 +1,174 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LM36274 LED chip family driver
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/leds.h>
+#include <linux/leds-ti-lmu-common.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/ti-lmu.h>
+#include <linux/mfd/ti-lmu-register.h>
+
+#include <uapi/linux/uleds.h>
+
+#define LM36274_MAX_STRINGS	4
+#define LM36274_BL_EN		BIT(4)
+
+/**
+ * struct lm36274
+ * @pdev: platform device
+ * @led_dev: led class device
+ * @lmu_data: Register and setting values for common code
+ * @regmap: Devices register map
+ * @dev: Pointer to the devices device struct
+ * @led_sources - The LED strings supported in this array
+ * @num_leds - Number of LED strings are supported in this array
+ */
+struct lm36274 {
+	struct platform_device *pdev;
+	struct led_classdev led_dev;
+	struct ti_lmu_bank lmu_data;
+	struct regmap *regmap;
+	struct device *dev;
+
+	u32 led_sources[LM36274_MAX_STRINGS];
+	int num_leds;
+};
+
+static int lm36274_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lm36274 *led = container_of(led_cdev, struct lm36274, led_dev);
+
+	return ti_lmu_common_set_brightness(&led->lmu_data, brt_val);
+}
+
+static int lm36274_init(struct lm36274 *lm36274_data)
+{
+	int enable_val = 0;
+	int i;
+
+	for (i = 0; i < lm36274_data->num_leds; i++)
+		enable_val |= (1 << lm36274_data->led_sources[i]);
+
+	if (!enable_val) {
+		dev_err(lm36274_data->dev, "No LEDs were enabled\n");
+		return -EINVAL;
+	}
+
+	enable_val |= LM36274_BL_EN;
+
+	return regmap_write(lm36274_data->regmap, LM36274_REG_BL_EN,
+			    enable_val);
+}
+
+static int lm36274_parse_dt(struct lm36274 *lm36274_data)
+{
+	struct fwnode_handle *child = NULL;
+	char label[LED_MAX_NAME_SIZE];
+	struct device *dev = &lm36274_data->pdev->dev;
+	const char *name;
+	int child_cnt;
+	int ret = -EINVAL;
+
+	/* There should only be 1 node */
+	child_cnt = device_get_child_node_count(dev);
+	if (child_cnt != 1)
+		return ret;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_string(child, "label", &name);
+		if (ret)
+			snprintf(label, sizeof(label),
+				"%s::", lm36274_data->pdev->name);
+		else
+			snprintf(label, sizeof(label),
+				 "%s:%s", lm36274_data->pdev->name, name);
+
+		lm36274_data->num_leds = fwnode_property_read_u32_array(child,
+							  "led-sources",
+							  NULL, 0);
+		if (lm36274_data->num_leds <= 0)
+			return -ENODEV;
+
+		ret = fwnode_property_read_u32_array(child, "led-sources",
+						     lm36274_data->led_sources,
+						     lm36274_data->num_leds);
+		if (ret) {
+			dev_err(dev, "led-sources property missing\n");
+			return -EINVAL;
+		}
+
+		fwnode_property_read_string(child, "linux,default-trigger",
+					&lm36274_data->led_dev.default_trigger);
+
+	}
+
+	lm36274_data->lmu_data.regmap = lm36274_data->regmap;
+	lm36274_data->lmu_data.max_brightness = MAX_BRIGHTNESS_11BIT;
+	lm36274_data->lmu_data.msb_brightness_reg = LM36274_REG_BRT_MSB;
+	lm36274_data->lmu_data.lsb_brightness_reg = LM36274_REG_BRT_LSB;
+
+	lm36274_data->led_dev.name = label;
+	lm36274_data->led_dev.max_brightness = MAX_BRIGHTNESS_11BIT;
+	lm36274_data->led_dev.brightness_set_blocking = lm36274_brightness_set;
+
+	return ret;
+}
+
+static int lm36274_probe(struct platform_device *pdev)
+{
+	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct lm36274 *lm36274_data;
+	int ret;
+
+	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
+				    GFP_KERNEL);
+	if (!lm36274_data) {
+		ret = -ENOMEM;
+		return ret;
+	}
+
+	lm36274_data->pdev = pdev;
+	lm36274_data->dev = lmu->dev;
+	lm36274_data->regmap = lmu->regmap;
+	dev_set_drvdata(&pdev->dev, lm36274_data);
+
+	ret = lm36274_parse_dt(lm36274_data);
+	if (ret) {
+		dev_err(lm36274_data->dev, "Failed to parse DT node\n");
+		return ret;
+	}
+
+	ret = lm36274_init(lm36274_data);
+	if (ret) {
+		dev_err(lm36274_data->dev, "Failed to init the device\n");
+		return ret;
+	}
+
+	return devm_led_classdev_register(lm36274_data->dev,
+					 &lm36274_data->led_dev);
+}
+
+static const struct of_device_id of_lm36274_leds_match[] = {
+	{ .compatible = "ti,lm36274-backlight", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lm36274_leds_match);
+
+static struct platform_driver lm36274_driver = {
+	.probe  = lm36274_probe,
+	.driver = {
+		.name = "lm36274-leds",
+	},
+};
+module_platform_driver(lm36274_driver)
+
+MODULE_DESCRIPTION("Texas Instruments LM36274 LED driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0.5.gaeb582a983

Re: [PATCH v4 0/6] LM36274 Introduction

[Jacek Anaszewski]

Hi Dan,

On 5/22/19 9:27 PM, Dan Murphy wrote:
> Hello
> 
> This is patch set v4 for the LM36274.  There were no changes made
> to this patch set except to rebase this on top of the latest TI LMU common code
> patchset.

Why the rebase was needed? leds-lm36274.c was already including
leds-ti-lmu-common.h.

> This patch set was rebased on the series at:
> https://lore.kernel.org/patchwork/project/lkml/list/?series=393071
> 
> Dan
> 
> Dan Murphy (6):
>    regulator: lm363x: Make the gpio register enable flexible
>    dt-bindings: mfd: Add lm36274 bindings to ti-lmu
>    mfd: ti-lmu: Add LM36274 support to the ti-lmu
>    regulator: lm363x: Add support for LM36274
>    dt-bindings: leds: Add LED bindings for the LM36274
>    leds: lm36274: Introduce the TI LM36274 LED driver
> 
>   .../devicetree/bindings/leds/leds-lm36274.txt |  82 +++++++++
>   .../devicetree/bindings/mfd/ti-lmu.txt        |  54 ++++++
>   drivers/leds/Kconfig                          |   7 +
>   drivers/leds/Makefile                         |   1 +
>   drivers/leds/leds-lm36274.c                   | 174 ++++++++++++++++++
>   drivers/mfd/Kconfig                           |   5 +-
>   drivers/mfd/ti-lmu.c                          |  14 ++
>   drivers/regulator/Kconfig                     |   2 +-
>   drivers/regulator/lm363x-regulator.c          |  56 +++++-
>   include/linux/mfd/ti-lmu-register.h           |  23 +++
>   include/linux/mfd/ti-lmu.h                    |   4 +
>   11 files changed, 416 insertions(+), 6 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-lm36274.txt
>   create mode 100644 drivers/leds/leds-lm36274.c
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 0/6] LM36274 Introduction

[Dan Murphy]

Jacek

On 5/22/19 2:37 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 5/22/19 9:27 PM, Dan Murphy wrote:
>> Hello
>>
>> This is patch set v4 for the LM36274.  There were no changes made
>> to this patch set except to rebase this on top of the latest TI LMU common code
>> patchset.
> 
> Why the rebase was needed? leds-lm36274.c was already including
> leds-ti-lmu-common.h.
> 

I just resent the patchset.

This was the cover letter in the series I sent originally.

Dan

<snip>

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Pavel Machek]

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

On Wed 2019-05-22 14:27:28, Dan Murphy wrote:
> The use of and enablement of the GPIO can be used across devices.
> Use the enable_reg in the regulator descriptor for the register to
> write.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

> ---
>  drivers/regulator/lm363x-regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
> index c876e161052a..382b1cecdd93 100644
> --- a/drivers/regulator/lm363x-regulator.c
> +++ b/drivers/regulator/lm363x-regulator.c
> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>  
>  	if (gpiod) {
>  		cfg.ena_gpiod = gpiod;
> -
> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
> +		ret = regmap_update_bits(regmap,
> +					 lm363x_regulator_desc[id].enable_reg,
>  					 LM3632_EXT_EN_MASK,
>  					 LM3632_EXT_EN_MASK);
>  		if (ret) {

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 2/6] dt-bindings: mfd: Add lm36274 bindings to ti-lmu

[Pavel Machek]

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

On Wed 2019-05-22 14:27:29, Dan Murphy wrote:
> Add the LM36274 backlight driver with regulator support.
> This is a multi-function device for backlight applications.
> 
> Backlight properties will be documented in it's a supplemental
> bindings document.
> 
> Regulator support is documented in the regulator/lm363x-regulator.txt
> 
> http://www.ti.com/lit/ds/symlink/lm36274.pdf
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 3/6] mfd: ti-lmu: Add LM36274 support to the ti-lmu

[Pavel Machek]

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

On Wed 2019-05-22 14:27:30, Dan Murphy wrote:
> Add the LM36274 register support to the ti-lmu MFD driver.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 4/6] regulator: lm363x: Add support for LM36274

[Pavel Machek]

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

On Wed 2019-05-22 14:27:31, Dan Murphy wrote:
> Adding regulator support for the LM36274 backlight driver.
> This device can leverage this existing code as the functionality
> and registers are common enough between the LM36274 and the LM363x
> series of devices.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 5/6] dt-bindings: leds: Add LED bindings for the LM36274

[Pavel Machek]

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

On Wed 2019-05-22 14:27:32, Dan Murphy wrote:
> Add the LM36274 LED specific bindings.
> 
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  .../devicetree/bindings/leds/leds-lm36274.txt | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm36274.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm36274.txt b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
> new file mode 100644
> index 000000000000..329393700191
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm36274.txt
> @@ -0,0 +1,82 @@
> +* Texas Instruments LM36274 4-Channel LCD Backlight Driver w/Integrated Bias
> +
> +The LM36274 is an integrated four-channel WLED driver and LCD bias supply.
> +The backlight boost provides the power to bias four parallel LED strings with
> +up to 29V total output voltage. The 11-bit LED current is programmable via
> +the I2C bus and/or controlled via a logic level PWM input from 60 ??A to 30 mA.

??A -> uA.

Otherwise:

Acked-by: Pavel Machek <pavel@ucw.cz>
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Pavel Machek]

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

Hi!

> +++ b/drivers/leds/leds-lm36274.c

> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
> +{
> +	struct fwnode_handle *child = NULL;
> +	char label[LED_MAX_NAME_SIZE];
> +	struct device *dev = &lm36274_data->pdev->dev;
> +	const char *name;
> +	int child_cnt;
> +	int ret = -EINVAL;
> +
> +	/* There should only be 1 node */
> +	child_cnt = device_get_child_node_count(dev);
> +	if (child_cnt != 1)
> +		return ret;

I'd do explicit "return -EINVAL" here.

> +static int lm36274_probe(struct platform_device *pdev)
> +{
> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
> +	struct lm36274 *lm36274_data;
> +	int ret;
> +
> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
> +				    GFP_KERNEL);
> +	if (!lm36274_data) {
> +		ret = -ENOMEM;
> +		return ret;
> +	}

And certainly do "return -ENOMEM" explicitly here.

Acked-by: Pavel Machek <pavel@ucw.cz>
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

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

On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
> The use of and enablement of the GPIO can be used across devices.
> Use the enable_reg in the regulator descriptor for the register to
> write.

> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>  
>  	if (gpiod) {
>  		cfg.ena_gpiod = gpiod;
> -
> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
> +		ret = regmap_update_bits(regmap,
> +					 lm363x_regulator_desc[id].enable_reg,
>  					 LM3632_EXT_EN_MASK,
>  					 LM3632_EXT_EN_MASK);
>  		if (ret) {

Is it guaranteed that the bitmask for enabling the use of the GPIO is
going to be the same for all regulators?  The bitmasks for the regulator
enable look to be different, and it also looks like this setting might
affect multiple regulators since it seems there are multiple enable bits
in the same register.  If this affects multiple regulators then how's
that working at the minute?

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

Re: [RESEND PATCH v4 4/6] regulator: lm363x: Add support for LM36274

[Mark Brown]

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

On Wed, May 22, 2019 at 02:27:31PM -0500, Dan Murphy wrote:
> Adding regulator support for the LM36274 backlight driver.
> This device can leverage this existing code as the functionality
> and registers are common enough between the LM36274 and the LM363x
> series of devices.

Acked-by: Mark Brown <broonie@kernel.org>

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

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Dan Murphy]

Mark

On 5/23/19 8:03 AM, Mark Brown wrote:
> On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
>> The use of and enablement of the GPIO can be used across devices.
>> Use the enable_reg in the regulator descriptor for the register to
>> write.
> 
>> @@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
>>  
>>  	if (gpiod) {
>>  		cfg.ena_gpiod = gpiod;
>> -
>> -		ret = regmap_update_bits(regmap, LM3632_REG_BIAS_CONFIG,
>> +		ret = regmap_update_bits(regmap,
>> +					 lm363x_regulator_desc[id].enable_reg,
>>  					 LM3632_EXT_EN_MASK,
>>  					 LM3632_EXT_EN_MASK);
>>  		if (ret) {
> 
> Is it guaranteed that the bitmask for enabling the use of the GPIO is
> going to be the same for all regulators?  The bitmasks for the regulator
> enable look to be different, and it also looks like this setting might
> affect multiple regulators since it seems there are multiple enable bits
> in the same register.  If this affects multiple regulators then how's
> that working at the minute?
> 

Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
If it is then the developer implemented the DT wrong.

LM3631 - No LCM GPIO control

LM36274 reg 0x09 bit 0 7.6.9 of the data sheet
LM3632 reg 0x0c bit 0 7.6.12 of the data sheet

Dan

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Dan Murphy]

Pavel

Thanks for the review

On 5/23/19 7:50 AM, Pavel Machek wrote:
> Hi!
> 
>> +++ b/drivers/leds/leds-lm36274.c
> 
>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>> +{
>> +	struct fwnode_handle *child = NULL;
>> +	char label[LED_MAX_NAME_SIZE];
>> +	struct device *dev = &lm36274_data->pdev->dev;
>> +	const char *name;
>> +	int child_cnt;
>> +	int ret = -EINVAL;
>> +
>> +	/* There should only be 1 node */
>> +	child_cnt = device_get_child_node_count(dev);
>> +	if (child_cnt != 1)
>> +		return ret;
> 
> I'd do explicit "return -EINVAL" here.
> 

ACK

>> +static int lm36274_probe(struct platform_device *pdev)
>> +{
>> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>> +	struct lm36274 *lm36274_data;
>> +	int ret;
>> +
>> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
>> +				    GFP_KERNEL);
>> +	if (!lm36274_data) {
>> +		ret = -ENOMEM;
>> +		return ret;
>> +	}
> 
> And certainly do "return -ENOMEM" explicitly here.
> 

ACK

> Acked-by: Pavel Machek <pavel@ucw.cz>
> 									Pavel
> 

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

Hi,

On 5/23/19 9:09 PM, Dan Murphy wrote:
> Pavel
> 
> Thanks for the review
> 
> On 5/23/19 7:50 AM, Pavel Machek wrote:
>> Hi!
>>
>>> +++ b/drivers/leds/leds-lm36274.c
>>
>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>> +{
>>> +	struct fwnode_handle *child = NULL;
>>> +	char label[LED_MAX_NAME_SIZE];
>>> +	struct device *dev = &lm36274_data->pdev->dev;
>>> +	const char *name;
>>> +	int child_cnt;
>>> +	int ret = -EINVAL;
>>> +
>>> +	/* There should only be 1 node */
>>> +	child_cnt = device_get_child_node_count(dev);
>>> +	if (child_cnt != 1)
>>> +		return ret;
>>
>> I'd do explicit "return -EINVAL" here.
>>
> 
> ACK
> 
>>> +static int lm36274_probe(struct platform_device *pdev)
>>> +{
>>> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>> +	struct lm36274 *lm36274_data;
>>> +	int ret;
>>> +
>>> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
>>> +				    GFP_KERNEL);
>>> +	if (!lm36274_data) {
>>> +		ret = -ENOMEM;
>>> +		return ret;
>>> +	}
>>
>> And certainly do "return -ENOMEM" explicitly here.
>>
> 
> ACK
> 
>> Acked-by: Pavel Machek <pavel@ucw.cz>

I've done all amendments requested by Pavel and updated branch
ib-leds-mfd-regulator on linux-leds.git, but in the same time
dropped the merge from the for-next.

We will proceed further once we clarify the issue of cross-merging
recently raised again by Linus.

-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

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

On Thu, May 23, 2019 at 08:50:20AM -0500, Dan Murphy wrote:
> On 5/23/19 8:03 AM, Mark Brown wrote:
> > On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:

> > Is it guaranteed that the bitmask for enabling the use of the GPIO is
> > going to be the same for all regulators?  The bitmasks for the regulator
> > enable look to be different, and it also looks like this setting might
> > affect multiple regulators since it seems there are multiple enable bits
> > in the same register.  If this affects multiple regulators then how's
> > that working at the minute?

> Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
> LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
> If it is then the developer implemented the DT wrong.

This feels fragile - it works for the current users but it's just
assuming that the placement of this bit will always be in the same
position in the same register as the enable and will silently fail if a
new chip variant does things differently.  Either storing the data
separately somewhere driver specific or just having explicit switch
statements would be more robust.

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

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Dan Murphy]

Mark

On 5/26/19 7:48 AM, Mark Brown wrote:
> On Thu, May 23, 2019 at 08:50:20AM -0500, Dan Murphy wrote:
>> On 5/23/19 8:03 AM, Mark Brown wrote:
>>> On Wed, May 22, 2019 at 02:27:28PM -0500, Dan Murphy wrote:
>>> Is it guaranteed that the bitmask for enabling the use of the GPIO is
>>> going to be the same for all regulators?  The bitmasks for the regulator
>>> enable look to be different, and it also looks like this setting might
>>> affect multiple regulators since it seems there are multiple enable bits
>>> in the same register.  If this affects multiple regulators then how's
>>> that working at the minute?
>> Yes for the 3632 and 36274 bit0 is the EXT_EN for LCM on these chips.
>> LM3631 does not have LCM GPIO control so there is no setting and this should not be called.
>> If it is then the developer implemented the DT wrong.
> This feels fragile - it works for the current users but it's just
> assuming that the placement of this bit will always be in the same
> position in the same register as the enable and will silently fail if a
> new chip variant does things differently.  Either storing the data
> separately somewhere driver specific or just having explicit switch
> statements would be more robust.


Although I don't disagree with you I don't see how the interface is 
fragile with only these 3 regulators defined.

Would it not be prudent to amend this driver if/when a new regulator is 
needed that has a different enable bit/register combination?   And if 
that was the case I would almost expect a different driver completely if 
the regmap did not line up correctly.  I only reused this driver because 
the registers and bits lined up and did not think it was necessary to 
create a whole new driver.

Dan

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Lee Jones]

On Fri, 24 May 2019, Jacek Anaszewski wrote:

> Hi,
> 
> On 5/23/19 9:09 PM, Dan Murphy wrote:
> > Pavel
> > 
> > Thanks for the review
> > 
> > On 5/23/19 7:50 AM, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > +++ b/drivers/leds/leds-lm36274.c
> > > 
> > > > +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
> > > > +{
> > > > +	struct fwnode_handle *child = NULL;
> > > > +	char label[LED_MAX_NAME_SIZE];
> > > > +	struct device *dev = &lm36274_data->pdev->dev;
> > > > +	const char *name;
> > > > +	int child_cnt;
> > > > +	int ret = -EINVAL;
> > > > +
> > > > +	/* There should only be 1 node */
> > > > +	child_cnt = device_get_child_node_count(dev);
> > > > +	if (child_cnt != 1)
> > > > +		return ret;
> > > 
> > > I'd do explicit "return -EINVAL" here.
> > > 
> > 
> > ACK
> > 
> > > > +static int lm36274_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
> > > > +	struct lm36274 *lm36274_data;
> > > > +	int ret;
> > > > +
> > > > +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
> > > > +				    GFP_KERNEL);
> > > > +	if (!lm36274_data) {
> > > > +		ret = -ENOMEM;
> > > > +		return ret;
> > > > +	}
> > > 
> > > And certainly do "return -ENOMEM" explicitly here.
> > > 
> > 
> > ACK
> > 
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> I've done all amendments requested by Pavel and updated branch
> ib-leds-mfd-regulator on linux-leds.git, but in the same time

What do you mean by updated?  You cannot update an 'ib' (immutable
branch).  Immutable means that it cannot change, by definition.

> dropped the merge from the for-next.
> 
> We will proceed further once we clarify the issue of cross-merging
> recently raised again by Linus.
> 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

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

On Wed, May 29, 2019 at 06:51:32AM -0500, Dan Murphy wrote:

> Although I don't disagree with you I don't see how the interface is fragile
> with only these 3 regulators defined.

> Would it not be prudent to amend this driver if/when a new regulator is
> needed that has a different enable bit/register combination?   And if that

The fragility I'm worried about is someone forgetting to make suitable
updates, especially if they don't use the feature in their own system.

> was the case I would almost expect a different driver completely if the
> regmap did not line up correctly.  I only reused this driver because the
> registers and bits lined up and did not think it was necessary to create a
> whole new driver.

This is a single register bit which is set once on startup isn't it?  It
seems like exactly the sort of thing that a hardware designer might
change incompatibly, perhaps even for good reasons like adding more
flexibility over which pins can be used to control the enable and far
from something that would require a totally new driver if it was handled
differently.

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

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Dan Murphy]

Mark

On 5/29/19 10:10 AM, Mark Brown wrote:
> On Wed, May 29, 2019 at 06:51:32AM -0500, Dan Murphy wrote:
>
>> Although I don't disagree with you I don't see how the interface is fragile
>> with only these 3 regulators defined.
>> Would it not be prudent to amend this driver if/when a new regulator is
>> needed that has a different enable bit/register combination?   And if that
> The fragility I'm worried about is someone forgetting to make suitable
> updates, especially if they don't use the feature in their own system.

If they don't define the enable GPIO in the device tree then the gpio 
descriptor pointer is NULL and the register write does not occur.

The documentation indicates that this is only applicable for 3632 I need 
to add the LM36274.

>> was the case I would almost expect a different driver completely if the
>> regmap did not line up correctly.  I only reused this driver because the
>> registers and bits lined up and did not think it was necessary to create a
>> whole new driver.
> This is a single register bit which is set once on startup isn't it?  It
> seems like exactly the sort of thing that a hardware designer might
> change incompatibly, perhaps even for good reasons like adding more
> flexibility over which pins can be used to control the enable and far
> from something that would require a totally new driver if it was handled
> differently.

Currently I don't have a device in this family that requires this level 
of flexibility for this pin or configuration.

So if a need should arise should we not implement that flexibility at 
that point?

Not only this but the gpio descriptor is protected in 
lm363x_regulator_of_get_enable_gpio due to checking of the regulator 
ID.  As in patch #4 of this series if LM3632 or LM36274 check for enable 
definition in the DT and create the descriptor if found.  If it is any 
other regulator then don't do anything.

If the HW designer changes these bits we end up with a new part and then 
at that point we could add code to redefine the bit mask as well.

Dan

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

On 5/29/19 3:58 PM, Lee Jones wrote:
> On Fri, 24 May 2019, Jacek Anaszewski wrote:
> 
>> Hi,
>>
>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>> Pavel
>>>
>>> Thanks for the review
>>>
>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>
>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>> +{
>>>>> +	struct fwnode_handle *child = NULL;
>>>>> +	char label[LED_MAX_NAME_SIZE];
>>>>> +	struct device *dev = &lm36274_data->pdev->dev;
>>>>> +	const char *name;
>>>>> +	int child_cnt;
>>>>> +	int ret = -EINVAL;
>>>>> +
>>>>> +	/* There should only be 1 node */
>>>>> +	child_cnt = device_get_child_node_count(dev);
>>>>> +	if (child_cnt != 1)
>>>>> +		return ret;
>>>>
>>>> I'd do explicit "return -EINVAL" here.
>>>>
>>>
>>> ACK
>>>
>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>> +	struct lm36274 *lm36274_data;
>>>>> +	int ret;
>>>>> +
>>>>> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
>>>>> +				    GFP_KERNEL);
>>>>> +	if (!lm36274_data) {
>>>>> +		ret = -ENOMEM;
>>>>> +		return ret;
>>>>> +	}
>>>>
>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>
>>>
>>> ACK
>>>
>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>
>> I've done all amendments requested by Pavel and updated branch
>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
> 
> What do you mean by updated?  You cannot update an 'ib' (immutable
> branch).  Immutable means that it cannot change, by definition.

We have already talked about that. Nobody has pulled so the branch
could have been safely updated.

-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Lee Jones]

On Wed, 29 May 2019, Jacek Anaszewski wrote:

> On 5/29/19 3:58 PM, Lee Jones wrote:
> > On Fri, 24 May 2019, Jacek Anaszewski wrote:
> > 
> > > Hi,
> > > 
> > > On 5/23/19 9:09 PM, Dan Murphy wrote:
> > > > Pavel
> > > > 
> > > > Thanks for the review
> > > > 
> > > > On 5/23/19 7:50 AM, Pavel Machek wrote:
> > > > > Hi!
> > > > > 
> > > > > > +++ b/drivers/leds/leds-lm36274.c
> > > > > 
> > > > > > +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
> > > > > > +{
> > > > > > +	struct fwnode_handle *child = NULL;
> > > > > > +	char label[LED_MAX_NAME_SIZE];
> > > > > > +	struct device *dev = &lm36274_data->pdev->dev;
> > > > > > +	const char *name;
> > > > > > +	int child_cnt;
> > > > > > +	int ret = -EINVAL;
> > > > > > +
> > > > > > +	/* There should only be 1 node */
> > > > > > +	child_cnt = device_get_child_node_count(dev);
> > > > > > +	if (child_cnt != 1)
> > > > > > +		return ret;
> > > > > 
> > > > > I'd do explicit "return -EINVAL" here.
> > > > > 
> > > > 
> > > > ACK
> > > > 
> > > > > > +static int lm36274_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
> > > > > > +	struct lm36274 *lm36274_data;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
> > > > > > +				    GFP_KERNEL);
> > > > > > +	if (!lm36274_data) {
> > > > > > +		ret = -ENOMEM;
> > > > > > +		return ret;
> > > > > > +	}
> > > > > 
> > > > > And certainly do "return -ENOMEM" explicitly here.
> > > > > 
> > > > 
> > > > ACK
> > > > 
> > > > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > I've done all amendments requested by Pavel and updated branch
> > > ib-leds-mfd-regulator on linux-leds.git, but in the same time
> > 
> > What do you mean by updated?  You cannot update an 'ib' (immutable
> > branch).  Immutable means that it cannot change, by definition.
> 
> We have already talked about that. Nobody has pulled so the branch
> could have been safely updated.

You have no sure way to know that.  And since I have no way to know,
or faith that you won't update it again, pulling it now/at all would
seem like a foolish thing to do.

Until you can provide me with an assurance that you will not keep
updating/changing the supposedly immutable pull-requests you send out,
I won't be pulling any more in.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

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

On Wed, May 29, 2019 at 03:47:08PM -0500, Dan Murphy wrote:
> On 5/29/19 10:10 AM, Mark Brown wrote:
> > On Wed, May 29, 2019 at 06:51:32AM -0500, Dan Murphy wrote:

> > > Although I don't disagree with you I don't see how the interface is fragile
> > > with only these 3 regulators defined.
> > > Would it not be prudent to amend this driver if/when a new regulator is
> > > needed that has a different enable bit/register combination?   And if that

> > The fragility I'm worried about is someone forgetting to make suitable
> > updates, especially if they don't use the feature in their own system.

> If they don't define the enable GPIO in the device tree then the gpio
> descriptor pointer is NULL and the register write does not occur.

> The documentation indicates that this is only applicable for 3632 I need to
> add the LM36274.

This isn't so much about people's DTs (though that's a definite concern
as well) as it is about support for any future devices in the driver, a
user might see that the driver supports GPIO enables, correctly set up
their device tree and have things fall over because the driver silently
tries to configure the registers incorrectly.

> Currently I don't have a device in this family that requires this level of
> flexibility for this pin or configuration.

> So if a need should arise should we not implement that flexibility at that
> point?

This isn't about implementing support for some theoretical thing, this
is about making the implementation of the current support more robust
and making the driver more maintainable going forwards.

> Not only this but the gpio descriptor is protected in
> lm363x_regulator_of_get_enable_gpio due to checking of the regulator ID.  As
> in patch #4 of this series if LM3632 or LM36274 check for enable definition
> in the DT and create the descriptor if found.  If it is any other regulator
> then don't do anything.

> If the HW designer changes these bits we end up with a new part and then at
> that point we could add code to redefine the bit mask as well.

That code is rather far away from the code you're changing and it's
really not clear that this will do the right thing for new devices, it
already appears that we're handling two devices without obvious code for
that (the regulator IDs get reused AFAICT).  If there were a switch
statement right there it'd be clearer.

Part of this is a reviewability thing - I initially stopped on the patch
because it looked like it was using the enable field for something other
than the intended purpose and now there's all this chasing around to see
if the code is OK.

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

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

On 5/30/19 9:38 AM, Lee Jones wrote:
> On Wed, 29 May 2019, Jacek Anaszewski wrote:
> 
>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>
>>>> Hi,
>>>>
>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>> Pavel
>>>>>
>>>>> Thanks for the review
>>>>>
>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>> Hi!
>>>>>>
>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>
>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>> +{
>>>>>>> +	struct fwnode_handle *child = NULL;
>>>>>>> +	char label[LED_MAX_NAME_SIZE];
>>>>>>> +	struct device *dev = &lm36274_data->pdev->dev;
>>>>>>> +	const char *name;
>>>>>>> +	int child_cnt;
>>>>>>> +	int ret = -EINVAL;
>>>>>>> +
>>>>>>> +	/* There should only be 1 node */
>>>>>>> +	child_cnt = device_get_child_node_count(dev);
>>>>>>> +	if (child_cnt != 1)
>>>>>>> +		return ret;
>>>>>>
>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>
>>>>>
>>>>> ACK
>>>>>
>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>> +	struct lm36274 *lm36274_data;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
>>>>>>> +				    GFP_KERNEL);
>>>>>>> +	if (!lm36274_data) {
>>>>>>> +		ret = -ENOMEM;
>>>>>>> +		return ret;
>>>>>>> +	}
>>>>>>
>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>
>>>>>
>>>>> ACK
>>>>>
>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>
>>>> I've done all amendments requested by Pavel and updated branch
>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>
>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>> branch).  Immutable means that it cannot change, by definition.
>>
>> We have already talked about that. Nobody has pulled so the branch
>> could have been safely updated.
> 
> You have no sure way to know that.  And since I have no way to know,
> or faith that you won't update it again, pulling it now/at all would
> seem like a foolish thing to do.

Sorry, but you are simply unjust. You're pretending to portray the
situation as if I have been notoriously causing merge conflicts in
linux-next which did not take place.

Just to recap what this discussion is about:

On 7 Apr 2019:

1. I sent pull request [0].
2. 45 minutes later I updated it after discovering one omission [1].
    It was rather small chance for it to be pulled as quickly as that.
    And even if it happened it wouldn't have been much harmful - we
    wouldn't have lost e.g. weeks of testing in linux-next due to that
    fact.

On 21 May 2019:

3. I sent another pull request [2] to you and REGULATOR maintainers.
    After it turned out that lack of feedback from REGULATOR maintainers
    was caused by failing to send them the exact copies of patches to
    review, I informed you about possible need for updating the branch.
    Afterwards I received a reply from you saying that you hadn't pulled
    the branch anyway. At that point I was sure that neither MFD nor
    REGULATOR tree contains the patches. And only after that I updated
    the branch.

> Until you can provide me with an assurance that you will not keep
> updating/changing the supposedly immutable pull-requests you send out,
> I won't be pulling any more in.

I can just uphold the assurance which is implicitly assumed for anyone
who has never broken acclaimed rules. As justified above.

[0] https://lore.kernel.org/patchwork/patch/1059075/
[1] https://lore.kernel.org/patchwork/patch/1059080/
[2] https://lore.kernel.org/patchwork/patch/1077066/
-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Lee Jones]

On Thu, 30 May 2019, Jacek Anaszewski wrote:

> On 5/30/19 9:38 AM, Lee Jones wrote:
> > On Wed, 29 May 2019, Jacek Anaszewski wrote:
> > 
> > > On 5/29/19 3:58 PM, Lee Jones wrote:
> > > > On Fri, 24 May 2019, Jacek Anaszewski wrote:
> > > > 
> > > > > Hi,
> > > > > 
> > > > > On 5/23/19 9:09 PM, Dan Murphy wrote:
> > > > > > Pavel
> > > > > > 
> > > > > > Thanks for the review
> > > > > > 
> > > > > > On 5/23/19 7:50 AM, Pavel Machek wrote:
> > > > > > > Hi!
> > > > > > > 
> > > > > > > > +++ b/drivers/leds/leds-lm36274.c
> > > > > > > 
> > > > > > > > +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
> > > > > > > > +{
> > > > > > > > +	struct fwnode_handle *child = NULL;
> > > > > > > > +	char label[LED_MAX_NAME_SIZE];
> > > > > > > > +	struct device *dev = &lm36274_data->pdev->dev;
> > > > > > > > +	const char *name;
> > > > > > > > +	int child_cnt;
> > > > > > > > +	int ret = -EINVAL;
> > > > > > > > +
> > > > > > > > +	/* There should only be 1 node */
> > > > > > > > +	child_cnt = device_get_child_node_count(dev);
> > > > > > > > +	if (child_cnt != 1)
> > > > > > > > +		return ret;
> > > > > > > 
> > > > > > > I'd do explicit "return -EINVAL" here.
> > > > > > > 
> > > > > > 
> > > > > > ACK
> > > > > > 
> > > > > > > > +static int lm36274_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
> > > > > > > > +	struct lm36274 *lm36274_data;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
> > > > > > > > +				    GFP_KERNEL);
> > > > > > > > +	if (!lm36274_data) {
> > > > > > > > +		ret = -ENOMEM;
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > 
> > > > > > > And certainly do "return -ENOMEM" explicitly here.
> > > > > > > 
> > > > > > 
> > > > > > ACK
> > > > > > 
> > > > > > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > > > 
> > > > > I've done all amendments requested by Pavel and updated branch
> > > > > ib-leds-mfd-regulator on linux-leds.git, but in the same time
> > > > 
> > > > What do you mean by updated?  You cannot update an 'ib' (immutable
> > > > branch).  Immutable means that it cannot change, by definition.
> > > 
> > > We have already talked about that. Nobody has pulled so the branch
> > > could have been safely updated.
> > 
> > You have no sure way to know that.  And since I have no way to know,
> > or faith that you won't update it again, pulling it now/at all would
> > seem like a foolish thing to do.
> 
> Sorry, but you are simply unjust. You're pretending to portray the
> situation as if I have been notoriously causing merge conflicts in
> linux-next which did not take place.
> 
> Just to recap what this discussion is about:
> 
> On 7 Apr 2019:
> 
> 1. I sent pull request [0].
> 2. 45 minutes later I updated it after discovering one omission [1].
>    It was rather small chance for it to be pulled as quickly as that.
>    And even if it happened it wouldn't have been much harmful - we
>    wouldn't have lost e.g. weeks of testing in linux-next due to that
>    fact.
> 
> On 21 May 2019:
> 
> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>    After it turned out that lack of feedback from REGULATOR maintainers
>    was caused by failing to send them the exact copies of patches to
>    review, I informed you about possible need for updating the branch.
>    Afterwards I received a reply from you saying that you hadn't pulled
>    the branch anyway. At that point I was sure that neither MFD nor
>    REGULATOR tree contains the patches. And only after that I updated
>    the branch.

Here are 2 examples where you have changed immutable branches, which
is 100% of the Pull Requests I have received from you.  Using that
record as a benchmark, the situation hardly seems unjust.

> > Until you can provide me with an assurance that you will not keep
> > updating/changing the supposedly immutable pull-requests you send out,
> > I won't be pulling any more in.
> 
> I can just uphold the assurance which is implicitly assumed for anyone
> who has never broken acclaimed rules. As justified above.

You have broken the rules every (100% of the) time.

> [0] https://lore.kernel.org/patchwork/patch/1059075/
> [1] https://lore.kernel.org/patchwork/patch/1059080/
> [2] https://lore.kernel.org/patchwork/patch/1077066/

So we have 2 choices moving forward; you can either provide me with
assurance that you have learned from this experience and will never
change an *immutable* branch again, or I can continue to handle them,
which has been the preference for some years.

If you choose the former and adaptions need to be made in the future,
the correct thing to do is create a *new*, different pull-request
which has its own *new*, different tag, but uses the original tag as a
base.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

On 5/31/19 8:23 AM, Lee Jones wrote:
> On Thu, 30 May 2019, Jacek Anaszewski wrote:
> 
>> On 5/30/19 9:38 AM, Lee Jones wrote:
>>> On Wed, 29 May 2019, Jacek Anaszewski wrote:
>>>
>>>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>>>> Pavel
>>>>>>>
>>>>>>> Thanks for the review
>>>>>>>
>>>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>>>
>>>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>>>> +{
>>>>>>>>> +	struct fwnode_handle *child = NULL;
>>>>>>>>> +	char label[LED_MAX_NAME_SIZE];
>>>>>>>>> +	struct device *dev = &lm36274_data->pdev->dev;
>>>>>>>>> +	const char *name;
>>>>>>>>> +	int child_cnt;
>>>>>>>>> +	int ret = -EINVAL;
>>>>>>>>> +
>>>>>>>>> +	/* There should only be 1 node */
>>>>>>>>> +	child_cnt = device_get_child_node_count(dev);
>>>>>>>>> +	if (child_cnt != 1)
>>>>>>>>> +		return ret;
>>>>>>>>
>>>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>>>
>>>>>>>
>>>>>>> ACK
>>>>>>>
>>>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>>>> +{
>>>>>>>>> +	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>>>> +	struct lm36274 *lm36274_data;
>>>>>>>>> +	int ret;
>>>>>>>>> +
>>>>>>>>> +	lm36274_data = devm_kzalloc(&pdev->dev, sizeof(*lm36274_data),
>>>>>>>>> +				    GFP_KERNEL);
>>>>>>>>> +	if (!lm36274_data) {
>>>>>>>>> +		ret = -ENOMEM;
>>>>>>>>> +		return ret;
>>>>>>>>> +	}
>>>>>>>>
>>>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>>>
>>>>>>>
>>>>>>> ACK
>>>>>>>
>>>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>>>
>>>>>> I've done all amendments requested by Pavel and updated branch
>>>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>>>
>>>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>>>> branch).  Immutable means that it cannot change, by definition.
>>>>
>>>> We have already talked about that. Nobody has pulled so the branch
>>>> could have been safely updated.
>>>
>>> You have no sure way to know that.  And since I have no way to know,
>>> or faith that you won't update it again, pulling it now/at all would
>>> seem like a foolish thing to do.
>>
>> Sorry, but you are simply unjust. You're pretending to portray the
>> situation as if I have been notoriously causing merge conflicts in
>> linux-next which did not take place.
>>
>> Just to recap what this discussion is about:
>>
>> On 7 Apr 2019:
>>
>> 1. I sent pull request [0].
>> 2. 45 minutes later I updated it after discovering one omission [1].
>>     It was rather small chance for it to be pulled as quickly as that.
>>     And even if it happened it wouldn't have been much harmful - we
>>     wouldn't have lost e.g. weeks of testing in linux-next due to that
>>     fact.
>>
>> On 21 May 2019:
>>
>> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>>     After it turned out that lack of feedback from REGULATOR maintainers
>>     was caused by failing to send them the exact copies of patches to
>>     review, I informed you about possible need for updating the branch.
>>     Afterwards I received a reply from you saying that you hadn't pulled
>>     the branch anyway. At that point I was sure that neither MFD nor
>>     REGULATOR tree contains the patches. And only after that I updated
>>     the branch.
> 
> Here are 2 examples where you have changed immutable branches, which
> is 100% of the Pull Requests I have received from you.  Using that
> record as a benchmark, the situation hardly seems unjust.
> 
>>> Until you can provide me with an assurance that you will not keep
>>> updating/changing the supposedly immutable pull-requests you send out,
>>> I won't be pulling any more in.
>>
>> I can just uphold the assurance which is implicitly assumed for anyone
>> who has never broken acclaimed rules. As justified above.
> 
> You have broken the rules every (100% of the) time.

Yes, I admit, I would lose in court.

>> [0] https://lore.kernel.org/patchwork/patch/1059075/
>> [1] https://lore.kernel.org/patchwork/patch/1059080/
>> [2] https://lore.kernel.org/patchwork/patch/1077066/
> 
> So we have 2 choices moving forward; you can either provide me with
> assurance that you have learned from this experience and will never
> change an *immutable* branch again, or I can continue to handle them,
> which has been the preference for some years.
> 
> If you choose the former and adaptions need to be made in the future,
> the correct thing to do is create a *new*, different pull-request
> which has its own *new*, different tag, but uses the original tag as a
> base.

I choose the former. That being said:

Hereby I solemnly declare never ever change an immutable branch again.

-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Dan Murphy]

Hello

On 5/31/19 2:44 PM, Jacek Anaszewski wrote:
> On 5/31/19 8:23 AM, Lee Jones wrote:
>> On Thu, 30 May 2019, Jacek Anaszewski wrote:
>>
>>> On 5/30/19 9:38 AM, Lee Jones wrote:
>>>> On Wed, 29 May 2019, Jacek Anaszewski wrote:
>>>>
>>>>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>>>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>>>>> Pavel
>>>>>>>>
>>>>>>>> Thanks for the review
>>>>>>>>
>>>>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>>>>
>>>>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>>>>> +{
>>>>>>>>>> +    struct fwnode_handle *child = NULL;
>>>>>>>>>> +    char label[LED_MAX_NAME_SIZE];
>>>>>>>>>> +    struct device *dev = &lm36274_data->pdev->dev;
>>>>>>>>>> +    const char *name;
>>>>>>>>>> +    int child_cnt;
>>>>>>>>>> +    int ret = -EINVAL;
>>>>>>>>>> +
>>>>>>>>>> +    /* There should only be 1 node */
>>>>>>>>>> +    child_cnt = device_get_child_node_count(dev);
>>>>>>>>>> +    if (child_cnt != 1)
>>>>>>>>>> +        return ret;
>>>>>>>>>
>>>>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>>>>
>>>>>>>>
>>>>>>>> ACK
>>>>>>>>
>>>>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>>>>> +{
>>>>>>>>>> +    struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>>>>> +    struct lm36274 *lm36274_data;
>>>>>>>>>> +    int ret;
>>>>>>>>>> +
>>>>>>>>>> +    lm36274_data = devm_kzalloc(&pdev->dev, 
>>>>>>>>>> sizeof(*lm36274_data),
>>>>>>>>>> +                    GFP_KERNEL);
>>>>>>>>>> +    if (!lm36274_data) {
>>>>>>>>>> +        ret = -ENOMEM;
>>>>>>>>>> +        return ret;
>>>>>>>>>> +    }
>>>>>>>>>
>>>>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>>>>
>>>>>>>>
>>>>>>>> ACK
>>>>>>>>
>>>>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>>>>
>>>>>>> I've done all amendments requested by Pavel and updated branch
>>>>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>>>>
>>>>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>>>>> branch).  Immutable means that it cannot change, by definition.
>>>>>
>>>>> We have already talked about that. Nobody has pulled so the branch
>>>>> could have been safely updated.
>>>>
>>>> You have no sure way to know that.  And since I have no way to know,
>>>> or faith that you won't update it again, pulling it now/at all would
>>>> seem like a foolish thing to do.
>>>
>>> Sorry, but you are simply unjust. You're pretending to portray the
>>> situation as if I have been notoriously causing merge conflicts in
>>> linux-next which did not take place.
>>>
>>> Just to recap what this discussion is about:
>>>
>>> On 7 Apr 2019:
>>>
>>> 1. I sent pull request [0].
>>> 2. 45 minutes later I updated it after discovering one omission [1].
>>>     It was rather small chance for it to be pulled as quickly as that.
>>>     And even if it happened it wouldn't have been much harmful - we
>>>     wouldn't have lost e.g. weeks of testing in linux-next due to that
>>>     fact.
>>>
>>> On 21 May 2019:
>>>
>>> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>>>     After it turned out that lack of feedback from REGULATOR 
>>> maintainers
>>>     was caused by failing to send them the exact copies of patches to
>>>     review, I informed you about possible need for updating the branch.
>>>     Afterwards I received a reply from you saying that you hadn't 
>>> pulled
>>>     the branch anyway. At that point I was sure that neither MFD nor
>>>     REGULATOR tree contains the patches. And only after that I updated
>>>     the branch.
>>
>> Here are 2 examples where you have changed immutable branches, which
>> is 100% of the Pull Requests I have received from you.  Using that
>> record as a benchmark, the situation hardly seems unjust.
>>
>>>> Until you can provide me with an assurance that you will not keep
>>>> updating/changing the supposedly immutable pull-requests you send out,
>>>> I won't be pulling any more in.
>>>
>>> I can just uphold the assurance which is implicitly assumed for anyone
>>> who has never broken acclaimed rules. As justified above.
>>
>> You have broken the rules every (100% of the) time.
>
> Yes, I admit, I would lose in court.
>
>>> [0] https://lore.kernel.org/patchwork/patch/1059075/
>>> [1] https://lore.kernel.org/patchwork/patch/1059080/
>>> [2] https://lore.kernel.org/patchwork/patch/1077066/
>>
>> So we have 2 choices moving forward; you can either provide me with
>> assurance that you have learned from this experience and will never
>> change an *immutable* branch again, or I can continue to handle them,
>> which has been the preference for some years.
>>
>> If you choose the former and adaptions need to be made in the future,
>> the correct thing to do is create a *new*, different pull-request
>> which has its own *new*, different tag, but uses the original tag as a
>> base.
>
> I choose the former. That being said:
>
> Hereby I solemnly declare never ever change an immutable branch again.
>
So how do I proceed with the requested change by Mark B on the LM36274 
driver.

Do I add a patch on top?

Or do I submit a patch to the regulator tree once the PR is pulled?

Dan

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

Dan,

On 5/31/19 11:07 PM, Dan Murphy wrote:
> Hello
> 
> On 5/31/19 2:44 PM, Jacek Anaszewski wrote:
>> On 5/31/19 8:23 AM, Lee Jones wrote:
>>> On Thu, 30 May 2019, Jacek Anaszewski wrote:
>>>
>>>> On 5/30/19 9:38 AM, Lee Jones wrote:
>>>>> On Wed, 29 May 2019, Jacek Anaszewski wrote:
>>>>>
>>>>>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>>>>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>>>>>> Pavel
>>>>>>>>>
>>>>>>>>> Thanks for the review
>>>>>>>>>
>>>>>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>>>>>
>>>>>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct fwnode_handle *child = NULL;
>>>>>>>>>>> +    char label[LED_MAX_NAME_SIZE];
>>>>>>>>>>> +    struct device *dev = &lm36274_data->pdev->dev;
>>>>>>>>>>> +    const char *name;
>>>>>>>>>>> +    int child_cnt;
>>>>>>>>>>> +    int ret = -EINVAL;
>>>>>>>>>>> +
>>>>>>>>>>> +    /* There should only be 1 node */
>>>>>>>>>>> +    child_cnt = device_get_child_node_count(dev);
>>>>>>>>>>> +    if (child_cnt != 1)
>>>>>>>>>>> +        return ret;
>>>>>>>>>>
>>>>>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ACK
>>>>>>>>>
>>>>>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>>>>>> +{
>>>>>>>>>>> +    struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>>>>>> +    struct lm36274 *lm36274_data;
>>>>>>>>>>> +    int ret;
>>>>>>>>>>> +
>>>>>>>>>>> +    lm36274_data = devm_kzalloc(&pdev->dev, 
>>>>>>>>>>> sizeof(*lm36274_data),
>>>>>>>>>>> +                    GFP_KERNEL);
>>>>>>>>>>> +    if (!lm36274_data) {
>>>>>>>>>>> +        ret = -ENOMEM;
>>>>>>>>>>> +        return ret;
>>>>>>>>>>> +    }
>>>>>>>>>>
>>>>>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ACK
>>>>>>>>>
>>>>>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>>>>>
>>>>>>>> I've done all amendments requested by Pavel and updated branch
>>>>>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>>>>>
>>>>>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>>>>>> branch).  Immutable means that it cannot change, by definition.
>>>>>>
>>>>>> We have already talked about that. Nobody has pulled so the branch
>>>>>> could have been safely updated.
>>>>>
>>>>> You have no sure way to know that.  And since I have no way to know,
>>>>> or faith that you won't update it again, pulling it now/at all would
>>>>> seem like a foolish thing to do.
>>>>
>>>> Sorry, but you are simply unjust. You're pretending to portray the
>>>> situation as if I have been notoriously causing merge conflicts in
>>>> linux-next which did not take place.
>>>>
>>>> Just to recap what this discussion is about:
>>>>
>>>> On 7 Apr 2019:
>>>>
>>>> 1. I sent pull request [0].
>>>> 2. 45 minutes later I updated it after discovering one omission [1].
>>>>     It was rather small chance for it to be pulled as quickly as that.
>>>>     And even if it happened it wouldn't have been much harmful - we
>>>>     wouldn't have lost e.g. weeks of testing in linux-next due to that
>>>>     fact.
>>>>
>>>> On 21 May 2019:
>>>>
>>>> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>>>>     After it turned out that lack of feedback from REGULATOR 
>>>> maintainers
>>>>     was caused by failing to send them the exact copies of patches to
>>>>     review, I informed you about possible need for updating the branch.
>>>>     Afterwards I received a reply from you saying that you hadn't 
>>>> pulled
>>>>     the branch anyway. At that point I was sure that neither MFD nor
>>>>     REGULATOR tree contains the patches. And only after that I updated
>>>>     the branch.
>>>
>>> Here are 2 examples where you have changed immutable branches, which
>>> is 100% of the Pull Requests I have received from you.  Using that
>>> record as a benchmark, the situation hardly seems unjust.
>>>
>>>>> Until you can provide me with an assurance that you will not keep
>>>>> updating/changing the supposedly immutable pull-requests you send out,
>>>>> I won't be pulling any more in.
>>>>
>>>> I can just uphold the assurance which is implicitly assumed for anyone
>>>> who has never broken acclaimed rules. As justified above.
>>>
>>> You have broken the rules every (100% of the) time.
>>
>> Yes, I admit, I would lose in court.
>>
>>>> [0] https://lore.kernel.org/patchwork/patch/1059075/
>>>> [1] https://lore.kernel.org/patchwork/patch/1059080/
>>>> [2] https://lore.kernel.org/patchwork/patch/1077066/
>>>
>>> So we have 2 choices moving forward; you can either provide me with
>>> assurance that you have learned from this experience and will never
>>> change an *immutable* branch again, or I can continue to handle them,
>>> which has been the preference for some years.
>>>
>>> If you choose the former and adaptions need to be made in the future,
>>> the correct thing to do is create a *new*, different pull-request
>>> which has its own *new*, different tag, but uses the original tag as a
>>> base.
>>
>> I choose the former. That being said:
>>
>> Hereby I solemnly declare never ever change an immutable branch again.
>>
> So how do I proceed with the requested change by Mark B on the LM36274 
> driver.
> 
> Do I add a patch on top?
> 
> Or do I submit a patch to the regulator tree once the PR is pulled?

Won't the change be a dependency for [PATCH v4 1/6] ?

In each case, having all the commits in one set (and branch) should
simplify the integration.

-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Dan Murphy]

Jacek

On 5/31/19 4:57 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 5/31/19 11:07 PM, Dan Murphy wrote:
>> Hello
>>
>> On 5/31/19 2:44 PM, Jacek Anaszewski wrote:
>>> On 5/31/19 8:23 AM, Lee Jones wrote:
>>>> On Thu, 30 May 2019, Jacek Anaszewski wrote:
>>>>
>>>>> On 5/30/19 9:38 AM, Lee Jones wrote:
>>>>>> On Wed, 29 May 2019, Jacek Anaszewski wrote:
>>>>>>
>>>>>>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>>>>>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>>>>>>> Pavel
>>>>>>>>>>
>>>>>>>>>> Thanks for the review
>>>>>>>>>>
>>>>>>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>>>>>>> Hi!
>>>>>>>>>>>
>>>>>>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>>>>>>
>>>>>>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    struct fwnode_handle *child = NULL;
>>>>>>>>>>>> +    char label[LED_MAX_NAME_SIZE];
>>>>>>>>>>>> +    struct device *dev = &lm36274_data->pdev->dev;
>>>>>>>>>>>> +    const char *name;
>>>>>>>>>>>> +    int child_cnt;
>>>>>>>>>>>> +    int ret = -EINVAL;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    /* There should only be 1 node */
>>>>>>>>>>>> +    child_cnt = device_get_child_node_count(dev);
>>>>>>>>>>>> +    if (child_cnt != 1)
>>>>>>>>>>>> +        return ret;
>>>>>>>>>>>
>>>>>>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ACK
>>>>>>>>>>
>>>>>>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>>>>>>> +    struct lm36274 *lm36274_data;
>>>>>>>>>>>> +    int ret;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    lm36274_data = devm_kzalloc(&pdev->dev, 
>>>>>>>>>>>> sizeof(*lm36274_data),
>>>>>>>>>>>> +                    GFP_KERNEL);
>>>>>>>>>>>> +    if (!lm36274_data) {
>>>>>>>>>>>> +        ret = -ENOMEM;
>>>>>>>>>>>> +        return ret;
>>>>>>>>>>>> +    }
>>>>>>>>>>>
>>>>>>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> ACK
>>>>>>>>>>
>>>>>>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>>>>>>
>>>>>>>>> I've done all amendments requested by Pavel and updated branch
>>>>>>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>>>>>>
>>>>>>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>>>>>>> branch).  Immutable means that it cannot change, by definition.
>>>>>>>
>>>>>>> We have already talked about that. Nobody has pulled so the branch
>>>>>>> could have been safely updated.
>>>>>>
>>>>>> You have no sure way to know that.  And since I have no way to know,
>>>>>> or faith that you won't update it again, pulling it now/at all would
>>>>>> seem like a foolish thing to do.
>>>>>
>>>>> Sorry, but you are simply unjust. You're pretending to portray the
>>>>> situation as if I have been notoriously causing merge conflicts in
>>>>> linux-next which did not take place.
>>>>>
>>>>> Just to recap what this discussion is about:
>>>>>
>>>>> On 7 Apr 2019:
>>>>>
>>>>> 1. I sent pull request [0].
>>>>> 2. 45 minutes later I updated it after discovering one omission [1].
>>>>>     It was rather small chance for it to be pulled as quickly as 
>>>>> that.
>>>>>     And even if it happened it wouldn't have been much harmful - we
>>>>>     wouldn't have lost e.g. weeks of testing in linux-next due to 
>>>>> that
>>>>>     fact.
>>>>>
>>>>> On 21 May 2019:
>>>>>
>>>>> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>>>>>     After it turned out that lack of feedback from REGULATOR 
>>>>> maintainers
>>>>>     was caused by failing to send them the exact copies of patches to
>>>>>     review, I informed you about possible need for updating the 
>>>>> branch.
>>>>>     Afterwards I received a reply from you saying that you hadn't 
>>>>> pulled
>>>>>     the branch anyway. At that point I was sure that neither MFD nor
>>>>>     REGULATOR tree contains the patches. And only after that I 
>>>>> updated
>>>>>     the branch.
>>>>
>>>> Here are 2 examples where you have changed immutable branches, which
>>>> is 100% of the Pull Requests I have received from you. Using that
>>>> record as a benchmark, the situation hardly seems unjust.
>>>>
>>>>>> Until you can provide me with an assurance that you will not keep
>>>>>> updating/changing the supposedly immutable pull-requests you send 
>>>>>> out,
>>>>>> I won't be pulling any more in.
>>>>>
>>>>> I can just uphold the assurance which is implicitly assumed for 
>>>>> anyone
>>>>> who has never broken acclaimed rules. As justified above.
>>>>
>>>> You have broken the rules every (100% of the) time.
>>>
>>> Yes, I admit, I would lose in court.
>>>
>>>>> [0] https://lore.kernel.org/patchwork/patch/1059075/
>>>>> [1] https://lore.kernel.org/patchwork/patch/1059080/
>>>>> [2] https://lore.kernel.org/patchwork/patch/1077066/
>>>>
>>>> So we have 2 choices moving forward; you can either provide me with
>>>> assurance that you have learned from this experience and will never
>>>> change an *immutable* branch again, or I can continue to handle them,
>>>> which has been the preference for some years.
>>>>
>>>> If you choose the former and adaptions need to be made in the future,
>>>> the correct thing to do is create a *new*, different pull-request
>>>> which has its own *new*, different tag, but uses the original tag as a
>>>> base.
>>>
>>> I choose the former. That being said:
>>>
>>> Hereby I solemnly declare never ever change an immutable branch again.
>>>
>> So how do I proceed with the requested change by Mark B on the 
>> LM36274 driver.
>>
>> Do I add a patch on top?
>>
>> Or do I submit a patch to the regulator tree once the PR is pulled?
>
> Won't the change be a dependency for [PATCH v4 1/6] ?
>
Yes thats why I am asking as we would need to change the branch.


Dan

> In each case, having all the commits in one set (and branch) should
> simplify the integration.
>

Re: [RESEND PATCH v4 6/6] leds: lm36274: Introduce the TI LM36274 LED driver

[Jacek Anaszewski]

Dan,

On 6/1/19 12:41 AM, Dan Murphy wrote:
> Jacek
> 
> On 5/31/19 4:57 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 5/31/19 11:07 PM, Dan Murphy wrote:
>>> Hello
>>>
>>> On 5/31/19 2:44 PM, Jacek Anaszewski wrote:
>>>> On 5/31/19 8:23 AM, Lee Jones wrote:
>>>>> On Thu, 30 May 2019, Jacek Anaszewski wrote:
>>>>>
>>>>>> On 5/30/19 9:38 AM, Lee Jones wrote:
>>>>>>> On Wed, 29 May 2019, Jacek Anaszewski wrote:
>>>>>>>
>>>>>>>> On 5/29/19 3:58 PM, Lee Jones wrote:
>>>>>>>>> On Fri, 24 May 2019, Jacek Anaszewski wrote:
>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On 5/23/19 9:09 PM, Dan Murphy wrote:
>>>>>>>>>>> Pavel
>>>>>>>>>>>
>>>>>>>>>>> Thanks for the review
>>>>>>>>>>>
>>>>>>>>>>> On 5/23/19 7:50 AM, Pavel Machek wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>>> +++ b/drivers/leds/leds-lm36274.c
>>>>>>>>>>>>
>>>>>>>>>>>>> +static int lm36274_parse_dt(struct lm36274 *lm36274_data)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +    struct fwnode_handle *child = NULL;
>>>>>>>>>>>>> +    char label[LED_MAX_NAME_SIZE];
>>>>>>>>>>>>> +    struct device *dev = &lm36274_data->pdev->dev;
>>>>>>>>>>>>> +    const char *name;
>>>>>>>>>>>>> +    int child_cnt;
>>>>>>>>>>>>> +    int ret = -EINVAL;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    /* There should only be 1 node */
>>>>>>>>>>>>> +    child_cnt = device_get_child_node_count(dev);
>>>>>>>>>>>>> +    if (child_cnt != 1)
>>>>>>>>>>>>> +        return ret;
>>>>>>>>>>>>
>>>>>>>>>>>> I'd do explicit "return -EINVAL" here.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ACK
>>>>>>>>>>>
>>>>>>>>>>>>> +static int lm36274_probe(struct platform_device *pdev)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +    struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
>>>>>>>>>>>>> +    struct lm36274 *lm36274_data;
>>>>>>>>>>>>> +    int ret;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    lm36274_data = devm_kzalloc(&pdev->dev, 
>>>>>>>>>>>>> sizeof(*lm36274_data),
>>>>>>>>>>>>> +                    GFP_KERNEL);
>>>>>>>>>>>>> +    if (!lm36274_data) {
>>>>>>>>>>>>> +        ret = -ENOMEM;
>>>>>>>>>>>>> +        return ret;
>>>>>>>>>>>>> +    }
>>>>>>>>>>>>
>>>>>>>>>>>> And certainly do "return -ENOMEM" explicitly here.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> ACK
>>>>>>>>>>>
>>>>>>>>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>>>>>>>>>
>>>>>>>>>> I've done all amendments requested by Pavel and updated branch
>>>>>>>>>> ib-leds-mfd-regulator on linux-leds.git, but in the same time
>>>>>>>>>
>>>>>>>>> What do you mean by updated?  You cannot update an 'ib' (immutable
>>>>>>>>> branch).  Immutable means that it cannot change, by definition.
>>>>>>>>
>>>>>>>> We have already talked about that. Nobody has pulled so the branch
>>>>>>>> could have been safely updated.
>>>>>>>
>>>>>>> You have no sure way to know that.  And since I have no way to know,
>>>>>>> or faith that you won't update it again, pulling it now/at all would
>>>>>>> seem like a foolish thing to do.
>>>>>>
>>>>>> Sorry, but you are simply unjust. You're pretending to portray the
>>>>>> situation as if I have been notoriously causing merge conflicts in
>>>>>> linux-next which did not take place.
>>>>>>
>>>>>> Just to recap what this discussion is about:
>>>>>>
>>>>>> On 7 Apr 2019:
>>>>>>
>>>>>> 1. I sent pull request [0].
>>>>>> 2. 45 minutes later I updated it after discovering one omission [1].
>>>>>>     It was rather small chance for it to be pulled as quickly as 
>>>>>> that.
>>>>>>     And even if it happened it wouldn't have been much harmful - we
>>>>>>     wouldn't have lost e.g. weeks of testing in linux-next due to 
>>>>>> that
>>>>>>     fact.
>>>>>>
>>>>>> On 21 May 2019:
>>>>>>
>>>>>> 3. I sent another pull request [2] to you and REGULATOR maintainers.
>>>>>>     After it turned out that lack of feedback from REGULATOR 
>>>>>> maintainers
>>>>>>     was caused by failing to send them the exact copies of patches to
>>>>>>     review, I informed you about possible need for updating the 
>>>>>> branch.
>>>>>>     Afterwards I received a reply from you saying that you hadn't 
>>>>>> pulled
>>>>>>     the branch anyway. At that point I was sure that neither MFD nor
>>>>>>     REGULATOR tree contains the patches. And only after that I 
>>>>>> updated
>>>>>>     the branch.
>>>>>
>>>>> Here are 2 examples where you have changed immutable branches, which
>>>>> is 100% of the Pull Requests I have received from you. Using that
>>>>> record as a benchmark, the situation hardly seems unjust.
>>>>>
>>>>>>> Until you can provide me with an assurance that you will not keep
>>>>>>> updating/changing the supposedly immutable pull-requests you send 
>>>>>>> out,
>>>>>>> I won't be pulling any more in.
>>>>>>
>>>>>> I can just uphold the assurance which is implicitly assumed for 
>>>>>> anyone
>>>>>> who has never broken acclaimed rules. As justified above.
>>>>>
>>>>> You have broken the rules every (100% of the) time.
>>>>
>>>> Yes, I admit, I would lose in court.
>>>>
>>>>>> [0] https://lore.kernel.org/patchwork/patch/1059075/
>>>>>> [1] https://lore.kernel.org/patchwork/patch/1059080/
>>>>>> [2] https://lore.kernel.org/patchwork/patch/1077066/
>>>>>
>>>>> So we have 2 choices moving forward; you can either provide me with
>>>>> assurance that you have learned from this experience and will never
>>>>> change an *immutable* branch again, or I can continue to handle them,
>>>>> which has been the preference for some years.
>>>>>
>>>>> If you choose the former and adaptions need to be made in the future,
>>>>> the correct thing to do is create a *new*, different pull-request
>>>>> which has its own *new*, different tag, but uses the original tag as a
>>>>> base.
>>>>
>>>> I choose the former. That being said:
>>>>
>>>> Hereby I solemnly declare never ever change an immutable branch again.
>>>>
>>> So how do I proceed with the requested change by Mark B on the 
>>> LM36274 driver.
>>>
>>> Do I add a patch on top?
>>>
>>> Or do I submit a patch to the regulator tree once the PR is pulled?
>>
>> Won't the change be a dependency for [PATCH v4 1/6] ?
>>
> Yes thats why I am asking as we would need to change the branch.

I will need to send another pull request anyway - I haven't created
new one after updating the branch so far, so for now we are free
to change it.

-- 
Best regards,
Jacek Anaszewski

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Dan Murphy]

Mark

On 5/30/19 10:26 AM, Mark Brown wrote:
>
> That code is rather far away from the code you're changing and it's
> really not clear that this will do the right thing for new devices, it
> already appears that we're handling two devices without obvious code for
> that (the regulator IDs get reused AFAICT).  If there were a switch
> statement right there it'd be clearer.
>
> Part of this is a reviewability thing - I initially stopped on the patch
> because it looked like it was using the enable field for something other
> than the intended purpose and now there's all this chasing around to see
> if the code is OK.

I am going to introduce this update in patch 4 of this series when the 
LM36274 is introduced to the regulator driver.

It makes more sense to add it there as in patch 1 there is not really a 
need to extend the value or mask as it only supports the LM3632 device.  
It makes sense to add the condition when adding another device to support

Thoughts?

Dan

Re: [RESEND PATCH v4 1/6] regulator: lm363x: Make the gpio register enable flexible

[Mark Brown]

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

On Tue, Jun 04, 2019 at 10:14:42AM -0500, Dan Murphy wrote:

> I am going to introduce this update in patch 4 of this series when the
> LM36274 is introduced to the regulator driver.

> It makes more sense to add it there as in patch 1 there is not really a need
> to extend the value or mask as it only supports the LM3632 device.  It makes
> sense to add the condition when adding another device to support

Sure, add it along with the rest of the support for the new device.

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