[PATCH 0/2] regulator: pca9450: enable restart handler for I2C

[PATCH 0/2] regulator: pca9450: enable restart handler for I2C

[Holger Assmann]

This series introduces a restart handler for the pca9450. This is an
optional feature and can be activated by assigning the handler priority
in the device tree.

Best regards,
Holger

Holger Assmann (2):
  regulator: pca9450: enable restart handler for I2C operation
  dt-bindings: regulator: pca9450: add restart handler priority

 .../regulator/nxp,pca9450-regulator.yaml      |  3 ++
 drivers/regulator/pca9450-regulator.c         | 54 +++++++++++++++++++
 include/linux/regulator/pca9450.h             |  7 +++
 3 files changed, 64 insertions(+)

-- 
2.39.2

[PATCH 1/2] regulator: pca9450: enable restart handler for I2C operation

[Holger Assmann]

The NXP PCA9450 can perform various kinds of power cycles when triggered
by I2C-command.
We therefore make this functionality accessible by introducing a
respective restart handler. It will be used after a priority has been
defined within the devicetree.

Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 drivers/regulator/pca9450-regulator.c | 54 +++++++++++++++++++++++++++
 include/linux/regulator/pca9450.h     |  7 ++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/regulator/pca9450-regulator.c b/drivers/regulator/pca9450-regulator.c
index 2ab365d2749f9..e623323599964 100644
--- a/drivers/regulator/pca9450-regulator.c
+++ b/drivers/regulator/pca9450-regulator.c
@@ -16,6 +16,7 @@
 #include <linux/regulator/machine.h>
 #include <linux/regulator/of_regulator.h>
 #include <linux/regulator/pca9450.h>
+#include <linux/reboot.h>
 
 struct pc9450_dvs_config {
 	unsigned int run_reg; /* dvs0 */
@@ -33,6 +34,7 @@ struct pca9450 {
 	struct device *dev;
 	struct regmap *regmap;
 	struct gpio_desc *sd_vsel_gpio;
+	struct notifier_block restart_handler;
 	enum pca9450_chip_type type;
 	unsigned int rcnt;
 	int irq;
@@ -700,6 +702,34 @@ static irqreturn_t pca9450_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static int pca9450_restart(struct notifier_block *nb,
+			   unsigned long mode, void *cmd)
+{
+	struct pca9450 *pmic = container_of(nb, struct pca9450,
+					    restart_handler);
+	struct i2c_client *client = container_of(pmic->dev,
+						 struct i2c_client, dev);
+	int ret = 0;
+	u32 rtype;
+
+	switch (mode) {
+	case REBOOT_WARM:
+		/* Warm reset (Toggle POR_B for 20 ms) */
+		rtype = 0x35;
+		break;
+	default:
+		/* Cold reset (Power recycle all regulators) */
+		rtype = 0x64;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, PCA9450_REG_SWRST, rtype);
+	if (ret < 0)
+		dev_alert(pmic->dev, "Failed to shutdown (err = %d)\n", ret);
+
+	mdelay(500);
+	return NOTIFY_DONE;
+}
+
 static int pca9450_i2c_probe(struct i2c_client *i2c)
 {
 	enum pca9450_chip_type type = (unsigned int)(uintptr_t)
@@ -845,12 +875,35 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
 		return PTR_ERR(pca9450->sd_vsel_gpio);
 	}
 
+	/* Register I2C restart handler if one is defined by device tree */
+	if (!of_property_read_u32(i2c->dev.of_node, "priority",
+				  &pca9450->restart_handler.priority)) {
+		pca9450->restart_handler.notifier_call = pca9450_restart;
+
+		ret = register_restart_handler(&pca9450->restart_handler);
+		if (ret)
+			return dev_err_probe(&i2c->dev, ret,
+					     "cannot register restart handler.\n");
+		else
+			dev_info(&i2c->dev,
+				 "registered restart handler with priority %d.\n",
+				 pca9450->restart_handler.priority);
+	}
+
 	dev_info(&i2c->dev, "%s probed.\n",
 		type == PCA9450_TYPE_PCA9450A ? "pca9450a" : "pca9450bc");
 
 	return 0;
 }
 
+static void pca9450_i2c_remove(struct i2c_client *i2c)
+{
+	struct pca9450 *pca9450 = dev_get_drvdata(&i2c->dev);
+
+	if (pca9450->restart_handler.notifier_call)
+		unregister_restart_handler(&pca9450->restart_handler);
+}
+
 static const struct of_device_id pca9450_of_match[] = {
 	{
 		.compatible = "nxp,pca9450a",
@@ -875,6 +928,7 @@ static struct i2c_driver pca9450_i2c_driver = {
 		.of_match_table = pca9450_of_match,
 	},
 	.probe = pca9450_i2c_probe,
+	.remove = pca9450_i2c_remove,
 };
 
 module_i2c_driver(pca9450_i2c_driver);
diff --git a/include/linux/regulator/pca9450.h b/include/linux/regulator/pca9450.h
index 505c908dbb817..2ee38bab23402 100644
--- a/include/linux/regulator/pca9450.h
+++ b/include/linux/regulator/pca9450.h
@@ -226,6 +226,13 @@ enum {
 #define WDOG_B_CFG_COLD_LDO12		0x80
 #define WDOG_B_CFG_COLD			0xC0
 
+/* PCA9450_REG_SWRST bits */
+#define SW_RST_NONE			0x00
+#define SW_RST_DEFAULTS			0x05
+#define SW_RST_COLD_LDO12_CLK32		0x14
+#define SW_RST_WARM			0x35
+#define SW_RST_COLD			0x64
+
 /* PCA9450_REG_CONFIG2 bits */
 #define I2C_LT_MASK			0x03
 #define I2C_LT_FORCE_DISABLE		0x00
-- 
2.39.2

Re: [PATCH 1/2] regulator: pca9450: enable restart handler for I2C operation

[Krzysztof Kozlowski]

On 19/04/2024 10:31, Holger Assmann wrote:
> The NXP PCA9450 can perform various kinds of power cycles when triggered
> by I2C-command.
> We therefore make this functionality accessible by introducing a
> respective restart handler. It will be used after a priority has been
> defined within the devicetree.
> 


...

> +
>  static int pca9450_i2c_probe(struct i2c_client *i2c)
>  {
>  	enum pca9450_chip_type type = (unsigned int)(uintptr_t)
> @@ -845,12 +875,35 @@ static int pca9450_i2c_probe(struct i2c_client *i2c)
>  		return PTR_ERR(pca9450->sd_vsel_gpio);
>  	}
>  
> +	/* Register I2C restart handler if one is defined by device tree */
> +	if (!of_property_read_u32(i2c->dev.of_node, "priority",
> +				  &pca9450->restart_handler.priority)) {

Priority property does not define whether this is or is not restart
handler. In case of missing priority, you should use just default:
SYS_OFF_PRIO_DEFAULT. Not skip the registering.

Best regards,
Krzysztof

Re: [PATCH 1/2] regulator: pca9450: enable restart handler for I2C operation

[Holger Assmann]

Hello Krzysztof,

Am 19.04.24 um 15:41 schrieb Krzysztof Kozlowski:
> 
> Priority property does not define whether this is or is not restart
> handler. In case of missing priority, you should use just default:
> SYS_OFF_PRIO_DEFAULT. Not skip the registering.
> 

Thank you for your fast response and the feedback. I will work it in and
send a v2 later.

Kind regards,
Holger

-- 
Pengutronix e.K.                         | Holger Assmann              |
Steuerwalder Str. 21                     | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |

[PATCH 2/2] dt-bindings: regulator: pca9450: add restart handler priority

[Holger Assmann]

This is an optional property. If set, the pca9450 will be registered as
a reset device with the chosen priority level.

Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>
---
 .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml   | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
index 3d469b8e97748..7cc2d6636cf52 100644
--- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
@@ -35,6 +35,9 @@ properties:
   interrupts:
     maxItems: 1
 
+  priority:
+    $ref: /schemas/power/reset/restart-handler.yaml#
+
   regulators:
     type: object
     description: |
-- 
2.39.2

Re: [PATCH 2/2] dt-bindings: regulator: pca9450: add restart handler priority

[Krzysztof Kozlowski]

On 19/04/2024 10:31, Holger Assmann wrote:
> This is an optional property. If set, the pca9450 will be registered as
> a reset device with the chosen priority level.
> 
> Signed-off-by: Holger Assmann <h.assmann@pengutronix.de>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

> ---
>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml   | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> index 3d469b8e97748..7cc2d6636cf52 100644
> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
> @@ -35,6 +35,9 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  priority:
> +    $ref: /schemas/power/reset/restart-handler.yaml#

You defined object, which is not explained in commit msg. This code does
not look correct or it does not implement what you said.

Please look at existing code - do you see anything like this? No, there
is no such code and this should raise question.

You probably want to annotate that device is a restart handler?

Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: regulator: pca9450: add restart handler priority

[Holger Assmann]

Hello Krzysztof,

also thanks for the feedback on this one.

Am 19.04.24 um 15:39 schrieb Krzysztof Kozlowski:
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.

Short note: I did that prior submitting, but I did it directly for the
yaml-file and not for the directory - Those do not look the same
regarding their prefix scheme.

I will change it for my v2 and use a subject like for the directory.


> 
>> ---
>>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml   | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> index 3d469b8e97748..7cc2d6636cf52 100644
>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>> @@ -35,6 +35,9 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> +  priority:
>> +    $ref: /schemas/power/reset/restart-handler.yaml#
> 
> You defined object, which is not explained in commit msg. This code does
> not look correct or it does not implement what you said.
> 
> Please look at existing code - do you see anything like this? No, there
> is no such code and this should raise question.

I am a bit lost on that one to be honest.

The only other instances where a "priority" for restart handling is
described are "gpio-poweroff.yaml" and "syscon-reboot.yaml". These files
are dedicated documentation for the reset bindings, so I tried to
transfer the respective entry over for my commit.

Do you suggest I should replace

+  priority:
+    $ref: /schemas/power/reset/restart-handler.yaml#

with

+allOf:
+  - $ref: /schemas/power/reset/restart-handler.yaml#

in order to properly include the context for the restart handling?
Running dt_binding_check does not indicate an issue with any of those two.


> 
> You probably want to annotate that device is a restart handler?

You mean by adding to the "description" part of the file?


Kind regards,
Holger

-- 
Pengutronix e.K.                         | Holger Assmann              |
Steuerwalder Str. 21                     | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] dt-bindings: regulator: pca9450: add restart handler priority

[Krzysztof Kozlowski]

On 22/04/2024 10:24, Holger Assmann wrote:
> Hello Krzysztof,
> 
> also thanks for the feedback on this one.
> 
> Am 19.04.24 um 15:39 schrieb Krzysztof Kozlowski:
>>
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> 
> Short note: I did that prior submitting, but I did it directly for the
> yaml-file and not for the directory - Those do not look the same
> regarding their prefix scheme.

No, if you run it per file or per directory, prefix is the same. Starts with regulator, doesn't it?

> 
> I will change it for my v2 and use a subject like for the directory.

regulator: dt-bindings: nxp,pca9450: add foo bar


> 
> 
>>
>>> ---
>>>  .../devicetree/bindings/regulator/nxp,pca9450-regulator.yaml   | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>> index 3d469b8e97748..7cc2d6636cf52 100644
>>> --- a/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pca9450-regulator.yaml
>>> @@ -35,6 +35,9 @@ properties:
>>>    interrupts:
>>>      maxItems: 1
>>>  
>>> +  priority:
>>> +    $ref: /schemas/power/reset/restart-handler.yaml#
>>
>> You defined object, which is not explained in commit msg. This code does
>> not look correct or it does not implement what you said.
>>
>> Please look at existing code - do you see anything like this? No, there
>> is no such code and this should raise question.
> 
> I am a bit lost on that one to be honest.
> 
> The only other instances where a "priority" for restart handling is
> described are "gpio-poweroff.yaml" and "syscon-reboot.yaml". These files
> are dedicated documentation for the reset bindings, so I tried to
> transfer the respective entry over for my commit.

Where do you see such syntax?

> 
> Do you suggest I should replace
> 
> +  priority:
> +    $ref: /schemas/power/reset/restart-handler.yaml#

I don't understand what you want to express with such syntax. That's why I suggested you to look for existing examples. There is no such code as above, so you are the first one to add it. And then the obvious question: why are you doing things quite different than others? That's pretty often a hint that something is odd.

> 
> with
> 
> +allOf:
> +  - $ref: /schemas/power/reset/restart-handler.yaml#
> 
> in order to properly include the context for the restart handling?
> Running dt_binding_check does not indicate an issue with any of those two.

Again, what do you want to achieve?

> 
> 
>>
>> You probably want to annotate that device is a restart handler?
> 
> You mean by adding to the "description" part of the file?

No. What do you want to achieve?

Best regards,
Krzysztof