LM36274 Introduction

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

---
 drivers/regulator/lm363x-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/regulator/lm363x-regulator.c +2 -2

index c876e161052a..382b1cecdd93 100644
@@ -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) {

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.

Quoted 6 lines · 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.

(quoted diff) +0 -0

@@ -263,8 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
 

drivers/regulator/lm363x-regulator.c static int lm363x_regulator_probe(struct platform_device *pdev) · line 270

Quoted 9 lines

drivers/regulator/lm363x-regulator.c +2 -2

 	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?

Quoted 8 lines · Mark Brown

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.

Quoted 11 lines · Dan Murphy

Quoted 7 lines · Mark Brown

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.

Quoted 4 lines · Dan Murphy

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.

Quoted 4 lines

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.

Quoted 6 lines · Mark Brown

Quoted 4 lines · Dan Murphy

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

Quoted 2 lines

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.

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.

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

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

Signed-off-by Dan Murphy <[email protected]>

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

Signed-off-by Dan Murphy <[email protected]>

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

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.

Add the LM36274 LED specific bindings.

Reviewed-by Rob Herring <[email protected]>

Signed-off-by Dan Murphy <[email protected]>

---
 .../devicetree/bindings/leds/leds-lm36274.txt | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm36274.txt

Documentation/devicetree/bindings/leds/leds-lm36274.txt new file +0 -0

new file mode 100644
index 000000000000..329393700191

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

drivers/leds/leds-lm36274.c new file +3 -0

+static int lm36274_parse_dt(struct lm36274 *lm36274_data)
+{
+	struct fwnode_handle *child = NULL;

Hi!

Quoted 1 line

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

drivers/leds/leds-lm36274.c line 82

Quoted 13 lines

drivers/leds/leds-lm36274.c new file +13 -0

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

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+static int lm36274_probe(struct platform_device *pdev)
+{
+	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct lm36274 *lm36274_data;
+	int ret;
+

drivers/leds/leds-lm36274.c line 135

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+	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 <[email protected]>

Pavel

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

drivers/leds/leds-lm36274.c line 82

Quoted 13 lines

drivers/leds/leds-lm36274.c new file +13 -0

+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

Quoted 6 lines

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+static int lm36274_probe(struct platform_device *pdev)
+{
+	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct lm36274 *lm36274_data;
+	int ret;
+

Quoted 9 lines

drivers/leds/leds-lm36274.c line 135

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+	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

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

Hi,

Quoted 44 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

drivers/leds/leds-lm36274.c line 82

Quoted 13 lines

drivers/leds/leds-lm36274.c new file +13 -0

+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

Quoted 15 lines

drivers/leds/leds-lm36274.c line 135

Quoted 12 lines

drivers/leds/leds-lm36274.c new file +12 -0

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

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 49 lines · Jacek Anaszewski

Hi,

Quoted 46 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

drivers/leds/leds-lm36274.c line 82

Quoted 13 lines

drivers/leds/leds-lm36274.c new file +13 -0

+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

Quoted 6 lines

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+static int lm36274_probe(struct platform_device *pdev)
+{
+	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct lm36274 *lm36274_data;
+	int ret;
+

Quoted 9 lines

drivers/leds/leds-lm36274.c line 135

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+	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

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 53 lines · Lee Jones

Quoted 51 lines · Jacek Anaszewski

Hi,

Quoted 47 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

drivers/leds/leds-lm36274.c line 82

Quoted 13 lines

drivers/leds/leds-lm36274.c new file +13 -0

+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

Quoted 6 lines

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+static int lm36274_probe(struct platform_device *pdev)
+{
+	struct ti_lmu *lmu = dev_get_drvdata(pdev->dev.parent);
+	struct lm36274 *lm36274_data;
+	int ret;
+

Quoted 9 lines

drivers/leds/leds-lm36274.c line 135

Quoted 6 lines

drivers/leds/leds-lm36274.c new file +6 -0

+	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

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

Quoted 5 lines

Quoted 2 lines

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.

Quoted 60 lines · Jacek Anaszewski

Quoted 58 lines · Lee Jones

Quoted 47 lines · Jacek Anaszewski

Hi,

Quoted 44 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +12 -0

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

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 2 lines

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.

Quoted 62 lines · Lee Jones

Quoted 60 lines · Jacek Anaszewski

Quoted 58 lines · Lee Jones

Quoted 47 lines · Jacek Anaszewski

Hi,

Quoted 44 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +12 -0

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

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 3 lines

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/

Quoted 87 lines · Jacek Anaszewski

Quoted 62 lines · Lee Jones

Quoted 60 lines · Jacek Anaszewski

Quoted 58 lines · Lee Jones

Quoted 47 lines · Jacek Anaszewski

Hi,

Quoted 44 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 15 lines

Review

Quoted 12 lines

(quoted diff) +12 -0

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

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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:

Quoted 8 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 103 lines · Lee Jones

Quoted 88 lines · Jacek Anaszewski

Quoted 63 lines · Lee Jones

Quoted 61 lines · Jacek Anaszewski

Quoted 59 lines · Lee Jones

Quoted 48 lines · Jacek Anaszewski

Hi,

Quoted 45 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 16 lines

Quoted 7 lines

(quoted diff) +7 -0

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

Quoted 6 lines

sizeof(*lm36274_data),
+                    GFP_KERNEL);
+    if (!lm36274_data) {
+        ret = -ENOMEM;
+        return ret;
+    }

And certainly do "return -ENOMEM" explicitly here.

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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

Quoted 4 lines

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.

Hello

Quoted 131 lines · Jacek Anaszewski

Quoted 103 lines · Lee Jones

Quoted 88 lines · Jacek Anaszewski

Quoted 63 lines · Lee Jones

Quoted 61 lines · Jacek Anaszewski

Quoted 59 lines · Lee Jones

Quoted 48 lines · Jacek Anaszewski

Hi,

Quoted 45 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 16 lines

Quoted 7 lines

(quoted diff) +7 -0

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

Quoted 6 lines

sizeof(*lm36274_data),
+                    GFP_KERNEL);
+    if (!lm36274_data) {
+        ret = -ENOMEM;
+        return ret;
+    }

And certainly do "return -ENOMEM" explicitly here.

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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

Quoted 4 lines

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:

Quoted 2 lines

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,

Quoted 136 lines · Dan Murphy

Hello

Quoted 133 lines · Jacek Anaszewski

Quoted 103 lines · Lee Jones

Quoted 88 lines · Jacek Anaszewski

Quoted 63 lines · Lee Jones

Quoted 61 lines · Jacek Anaszewski

Quoted 59 lines · Lee Jones

Quoted 48 lines · Jacek Anaszewski

Hi,

Quoted 45 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Review

Quoted 13 lines

(quoted diff) +13 -0

+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

Quoted 16 lines

Quoted 7 lines

(quoted diff) +7 -0

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

Quoted 6 lines

sizeof(*lm36274_data),
+                    GFP_KERNEL);
+    if (!lm36274_data) {
+        ret = -ENOMEM;
+        return ret;
+    }

And certainly do "return -ENOMEM" explicitly here.

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 5 lines

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.

Jacek

Quoted 141 lines · Jacek Anaszewski

Dan,

Quoted 138 lines · Dan Murphy

Hello

Quoted 124 lines · Jacek Anaszewski

Quoted 103 lines · Lee Jones

Quoted 88 lines · Jacek Anaszewski

Quoted 63 lines · Lee Jones

Quoted 61 lines · Jacek Anaszewski

Quoted 59 lines · Lee Jones

Quoted 48 lines · Jacek Anaszewski

Hi,

Quoted 45 lines · Dan Murphy

Pavel

Thanks for the review

Quoted 4 lines · Pavel Machek

Hi!

Quoted 1 line

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

Quoted 16 lines

Quoted 13 lines

+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

Quoted 16 lines

Quoted 13 lines

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

Quoted 6 lines

Quoted 3 lines

ACK

Quoted 1 line

Acked-by Pavel Machek <[email protected]>

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.

Quoted 6 lines

Quoted 2 lines

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:

Quoted 8 lines

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.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 3 lines

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

Quoted 9 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

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

Hi Dan,

Quoted 5 lines · 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.

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