[RFC PATCH 0/2] mfd: rohm-bd71828: Add power off

[RFC PATCH 0/2] mfd: rohm-bd71828: Add power off

[Andreas Kemnade]

Add power off functionality. Marked as RFC because of magic numbers
without a good source and strange delays. The only information source is
only a vendor kernel.

Andreas Kemnade (2):
  dt-bindings: mfd: Add ROHM BD71828 system-power-controller property
  mfd: rohm-bd71828: Add power off functionality

 .../bindings/mfd/rohm,bd71828-pmic.yaml       |  2 ++
 drivers/mfd/rohm-bd71828.c                    | 31 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.39.2

[RFC PATCH 1/2] dt-bindings: mfd: Add ROHM BD71828 system-power-controller property

[Andreas Kemnade]

As the PMIC can power off the system, add the corresponding property.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
index 11089aa89ec6..0b62f854bf6b 100644
--- a/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
+++ b/Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml
@@ -73,6 +73,8 @@ properties:
       used to mark the pins which should not be configured for GPIO. Please see
       the ../gpio/gpio.txt for more information.
 
+  system-power-controller: true
+
 required:
   - compatible
   - reg
-- 
2.39.2

[RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Andreas Kemnade]

Since the chip can power off the system, add the corresponding
functionality.
Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
No information source about the magic numbers found.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/mfd/rohm-bd71828.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
index 594718f7e8e1..5a55aa3620d0 100644
--- a/drivers/mfd/rohm-bd71828.c
+++ b/drivers/mfd/rohm-bd71828.c
@@ -464,6 +464,24 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
 				  OUT32K_MODE_CMOS);
 }
 
+static struct i2c_client *bd71828_dev;
+static void bd71828_power_off(void)
+{
+	i2c_smbus_write_byte_data(bd71828_dev, 0x03, 0xff);
+	mdelay(500);
+	i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_INT_DCIN2, 0x02);
+	mdelay(500);
+	while (true) {
+		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);
+		mdelay(500);
+	}
+}
+
+static void bd71828_remove_poweroff(void *data)
+{
+	bd71828_dev = NULL;
+}
+
 static int bd71828_i2c_probe(struct i2c_client *i2c)
 {
 	struct regmap_irq_chip_data *irq_data;
@@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
 	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
 				   NULL, 0, regmap_irq_get_domain(irq_data));
 	if (ret)
-		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
+		return	dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
+
+	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
+		if (!pm_power_off) {
+			bd71828_dev = i2c;
+			pm_power_off = bd71828_power_off;
+			ret = devm_add_action_or_reset(&i2c->dev,
+						       bd71828_remove_poweroff,
+						       NULL);
+		} else
+			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");
+	}
 
 	return ret;
 }
-- 
2.39.2

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Matti Vaittinen]

On 3/24/24 22:12, Andreas Kemnade wrote:
> Since the chip can power off the system, add the corresponding
> functionality.
> Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> No information source about the magic numbers found.

Oh, interesting repository :) Thanks for linking to it! I didn't know 
someone had reworked this driver...

I have access to the data-sheets so I also have some pieces of 
information. I hope I can clarify part of the puzzle. Unfortunately I 
have no information about the magic delays. I guess I could try asking 
though.

Oh, it seems to me this handler is only working on BD71828, not on 
BD71815. So, it should be tied to the ROHM_CHIP_TYPE_BD71828.

> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   drivers/mfd/rohm-bd71828.c | 31 ++++++++++++++++++++++++++++++-
>   1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index 594718f7e8e1..5a55aa3620d0 100644
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -464,6 +464,24 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
>   				  OUT32K_MODE_CMOS);
>   }
>   
> +static struct i2c_client *bd71828_dev;

I'm not sure why to store pointer to the device and not a pointer to the 
regmap?

> +static void bd71828_power_off(void)
> +{
> +	i2c_smbus_write_byte_data(bd71828_dev, 0x03, 0xff);

0x03 is a "reset reason" - register. Spec I have states that the 
register should clear when a reset occurs - but it also says the bits 
are "write '1' to clear". So, for some reason(?), this clears the 
previous reset reason. I am unsure why i2c_smbus_write_byte_data() and 
not regmap()?

> +	mdelay(500);
> +	i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_INT_DCIN2, 0x02);

This clears the DCIN monitoring status bit from the IRQ status register. 
I don't understand the purpose though.

> +	mdelay(500);
> +	while (true) {
> +		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);

This write to PS_CTRL_1 initiates a state transition. 0x2 equals to HBNT 
state. Eg, in usual cases this should be a start of the power-off sequence.

> +		mdelay(500);
> +	}
> +}

If you have the hardware to test this on, then it'd be great to see if 
clearing the reset reason and IRQ status could be dropped. I can't 
immediately think of a reason for those.

> +static void bd71828_remove_poweroff(void *data)
> +{
> +	bd71828_dev = NULL;

This does not smell correct to me. Should we remove the 
bd71828_power_off() from the pm_power_off instead?

> +}
> +
>   static int bd71828_i2c_probe(struct i2c_client *i2c)
>   {
>   	struct regmap_irq_chip_data *irq_data;
> @@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
>   	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
>   				   NULL, 0, regmap_irq_get_domain(irq_data));
>   	if (ret)
> -		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> +		return	dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> +
> +	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
> +		if (!pm_power_off) {
> +			bd71828_dev = i2c;
> +			pm_power_off = bd71828_power_off;
> +			ret = devm_add_action_or_reset(&i2c->dev,
> +						       bd71828_remove_poweroff,
> +						       NULL);
> +		} else
> +			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");
> +	}
>   
>   	return ret;
>   }

Thanks for doing the digging Andreas! I think supporting the power-off 
for devices using this PMIC is nice!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Krzysztof Kozlowski]

On 24/03/2024 21:12, Andreas Kemnade wrote:
>  	struct regmap_irq_chip_data *irq_data;
> @@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
>  	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
>  				   NULL, 0, regmap_irq_get_domain(irq_data));
>  	if (ret)
> -		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> +		return	dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> +
> +	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
> +		if (!pm_power_off) {
> +			bd71828_dev = i2c;
> +			pm_power_off = bd71828_power_off;
> +			ret = devm_add_action_or_reset(&i2c->dev,
> +						       bd71828_remove_poweroff,
> +						       NULL);
> +		} else
> +			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");

Missing {}

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

Best regards,
Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: mfd: Add ROHM BD71828 system-power-controller property

[Krzysztof Kozlowski]

On 24/03/2024 21:12, Andreas Kemnade wrote:
> As the PMIC can power off the system, add the corresponding property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>  Documentation/devicetree/bindings/mfd/rohm,bd71828-pmic.yaml | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Andreas Kemnade]

On Mon, 25 Mar 2024 13:31:15 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/24/24 22:12, Andreas Kemnade wrote:
> > Since the chip can power off the system, add the corresponding
> > functionality.
> > Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> > No information source about the magic numbers found.  
> 
> Oh, interesting repository :) Thanks for linking to it! I didn't know 
> someone had reworked this driver...
> 
which btw: contains this interesting snippet (output from fdtdump)
  bd71828-i2c@4b {
                    reg = <0x0000004b>;
                    compatible = "rohm,bd71828";
                    gpio_int = <0x00000008 0x00000013 0x00000001>;
                    gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
                    #address-cells = <0x00000001>;
                    #size-cells = <0x00000000>;
                    pmic@4b {
                        compatible = "rohm,bd71828";
             	        regulators {
                        	BUCK1 {
                            		regulator-name = "buck1";


and to make it work since basically no regulators are registered 
instead just some regmap_write()s are done to configure something
in probe(). It is a pitfall to think that the information below pmic@4b
is used, especially since it is not that obvious in the source.

> I have access to the data-sheets so I also have some pieces of 
> information. I hope I can clarify part of the puzzle. Unfortunately I 
> have no information about the magic delays. I guess I could try asking 
> though.
> 
> Oh, it seems to me this handler is only working on BD71828, not on 
> BD71815. So, it should be tied to the ROHM_CHIP_TYPE_BD71828.
> 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >   drivers/mfd/rohm-bd71828.c | 31 ++++++++++++++++++++++++++++++-
> >   1 file changed, 30 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> > index 594718f7e8e1..5a55aa3620d0 100644
> > --- a/drivers/mfd/rohm-bd71828.c
> > +++ b/drivers/mfd/rohm-bd71828.c
> > @@ -464,6 +464,24 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
> >   				  OUT32K_MODE_CMOS);
> >   }
> >   
> > +static struct i2c_client *bd71828_dev;  
> 
> I'm not sure why to store pointer to the device and not a pointer to the 
> regmap?
> 
> > +static void bd71828_power_off(void)
> > +{
> > +	i2c_smbus_write_byte_data(bd71828_dev, 0x03, 0xff);  
> 
> 0x03 is a "reset reason" - register. Spec I have states that the 
> register should clear when a reset occurs - but it also says the bits 
> are "write '1' to clear". So, for some reason(?), this clears the 
> previous reset reason.

well, so just check in bootloader what the reset reason is and check if
there is anything odd.

> I am unsure why i2c_smbus_write_byte_data() and 
> not regmap()?
> 
regmap involves mutex_lock() and we are not allowed to sleep here.

> > +	mdelay(500);
> > +	i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_INT_DCIN2, 0x02);  
> 
> This clears the DCIN monitoring status bit from the IRQ status register. 
> I don't understand the purpose though.
> 
so maybe something to prevent power on by just plugging a usb cable? Will
experiment a bit with it.

> > +	mdelay(500);
> > +	while (true) {
> > +		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);  
> 
> This write to PS_CTRL_1 initiates a state transition. 0x2 equals to HBNT 
> state. Eg, in usual cases this should be a start of the power-off sequence.
> 
> > +		mdelay(500);
> > +	}
> > +}  
> 
> If you have the hardware to test this on, then it'd be great to see if 
> clearing the reset reason and IRQ status could be dropped. I can't 
> immediately think of a reason for those.
> 
I will to so. That will also remove the need for all those delays.

> > +static void bd71828_remove_poweroff(void *data)
> > +{
> > +	bd71828_dev = NULL;  
> 
> This does not smell correct to me. Should we remove the 
> bd71828_power_off() from the pm_power_off instead?
> 
oh, yes, that is not correct.

Regards,
Andreas

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Matti Vaittinen]

Hi Andreas,

On 3/25/24 14:16, Andreas Kemnade wrote:
> On Mon, 25 Mar 2024 13:31:15 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 3/24/24 22:12, Andreas Kemnade wrote:
>>> Since the chip can power off the system, add the corresponding
>>> functionality.
>>> Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
>>> No information source about the magic numbers found.
>>
>> Oh, interesting repository :) Thanks for linking to it! I didn't know
>> someone had reworked this driver...
>>
> which btw: contains this interesting snippet (output from fdtdump)
>    bd71828-i2c@4b {
>                      reg = <0x0000004b>;
>                      compatible = "rohm,bd71828";
>                      gpio_int = <0x00000008 0x00000013 0x00000001>;
>                      gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
>                      #address-cells = <0x00000001>;
>                      #size-cells = <0x00000000>;
>                      pmic@4b {
>                          compatible = "rohm,bd71828";
>               	        regulators {
>                          	BUCK1 {
>                              		regulator-name = "buck1";
> 
> 
> and to make it work since basically no regulators are registered
> instead just some regmap_write()s are done to configure something
> in probe(). It is a pitfall to think that the information below pmic@4b
> is used, especially since it is not that obvious in the source.

Just to ensure there will be no misunderstanding - I have not authored 
the modifications seen in "Kobo-Reader" repository. Upstream driver does 
register the regulators - and it does not use the oddly named gpio_int 
or the gpio_wdogb:

https://elixir.bootlin.com/linux/v6.9-rc1/source/drivers/regulator/bd71828-regulator.c#L750

>> I have access to the data-sheets so I also have some pieces of
>> information. I hope I can clarify part of the puzzle. Unfortunately I
>> have no information about the magic delays. I guess I could try asking
>> though.
>>
>> Oh, it seems to me this handler is only working on BD71828, not on
>> BD71815. So, it should be tied to the ROHM_CHIP_TYPE_BD71828.
>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>> ---
>>>    drivers/mfd/rohm-bd71828.c | 31 ++++++++++++++++++++++++++++++-
>>>    1 file changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
>>> index 594718f7e8e1..5a55aa3620d0 100644
>>> --- a/drivers/mfd/rohm-bd71828.c
>>> +++ b/drivers/mfd/rohm-bd71828.c
>>> @@ -464,6 +464,24 @@ static int set_clk_mode(struct device *dev, struct regmap *regmap,
>>>    				  OUT32K_MODE_CMOS);
>>>    }
>>>    
>>> +static struct i2c_client *bd71828_dev;
>>
>> I'm not sure why to store pointer to the device and not a pointer to the
>> regmap?
>>
>>> +static void bd71828_power_off(void)
>>> +{
>>> +	i2c_smbus_write_byte_data(bd71828_dev, 0x03, 0xff);
>>
>> 0x03 is a "reset reason" - register. Spec I have states that the
>> register should clear when a reset occurs - but it also says the bits
>> are "write '1' to clear". So, for some reason(?), this clears the
>> previous reset reason.
> 
> well, so just check in bootloader what the reset reason is and check if
> there is anything odd.

I understand why bootloader would check this register - but I don't 
understand why the driver would clear it as it should be cleared by 
reset (unless I missread the spec).

>> I am unsure why i2c_smbus_write_byte_data() and
>> not regmap()?
>>
> regmap involves mutex_lock() and we are not allowed to sleep here.

Right. It makes perfect sense now. Thanks! (Do you think it's worth a 
comment?)

>>> +	mdelay(500);
>>> +	i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_INT_DCIN2, 0x02);
>>
>> This clears the DCIN monitoring status bit from the IRQ status register.
>> I don't understand the purpose though.
>>
> so maybe something to prevent power on by just plugging a usb cable? Will
> experiment a bit with it.

I still think this is odd because, if I read it right, this is an IRQ 
status and not a mask register. Well, wouldn't be a first undocumented 
"feature" I see...

>>> +	mdelay(500);
>>> +	while (true) {
>>> +		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);
>>
>> This write to PS_CTRL_1 initiates a state transition. 0x2 equals to HBNT
>> state. Eg, in usual cases this should be a start of the power-off sequence.
>>
>>> +		mdelay(500);
>>> +	}
>>> +}
>>
>> If you have the hardware to test this on, then it'd be great to see if
>> clearing the reset reason and IRQ status could be dropped. I can't
>> immediately think of a reason for those.
>>
> I will to so. That will also remove the need for all those delays.

Thanks :) Very much appreciated!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Andreas Kemnade]

Hi Matti,

On Mon, 25 Mar 2024 14:44:43 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Andreas,
> 
> On 3/25/24 14:16, Andreas Kemnade wrote:
> > On Mon, 25 Mar 2024 13:31:15 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 3/24/24 22:12, Andreas Kemnade wrote:  
> >>> Since the chip can power off the system, add the corresponding
> >>> functionality.
> >>> Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> >>> No information source about the magic numbers found.  
> >>
> >> Oh, interesting repository :) Thanks for linking to it! I didn't know
> >> someone had reworked this driver...
> >>  
> > which btw: contains this interesting snippet (output from fdtdump)
> >    bd71828-i2c@4b {
> >                      reg = <0x0000004b>;
> >                      compatible = "rohm,bd71828";
> >                      gpio_int = <0x00000008 0x00000013 0x00000001>;
> >                      gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
> >                      #address-cells = <0x00000001>;
> >                      #size-cells = <0x00000000>;
> >                      pmic@4b {
> >                          compatible = "rohm,bd71828";
> >               	        regulators {
> >                          	BUCK1 {
> >                              		regulator-name = "buck1";
> > 
> > 
> > and to make it work since basically no regulators are registered
> > instead just some regmap_write()s are done to configure something
> > in probe(). It is a pitfall to think that the information below pmic@4b
> > is used, especially since it is not that obvious in the source.  
> 
> Just to ensure there will be no misunderstanding - I have not authored 
> the modifications seen in "Kobo-Reader" repository. Upstream driver does 
> register the regulators - and it does not use the oddly named gpio_int 
> or the gpio_wdogb:

yes, I know it is not from you and I know the general quality in those repos.
But I feel not well pointing to those repositories without issuing the warning
that regulator information in dtb is not an authoritative source for required
regulator settings. Not that someone enables the smoke-generator
functionality...

But lets not go too much offtopic.

Regards,
Andreas

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Lee Jones]

On Mon, 25 Mar 2024, Matti Vaittinen wrote:

> Hi Andreas,
> 
> On 3/25/24 14:16, Andreas Kemnade wrote:
> > On Mon, 25 Mar 2024 13:31:15 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > 
> > > On 3/24/24 22:12, Andreas Kemnade wrote:
> > > > Since the chip can power off the system, add the corresponding
> > > > functionality.
> > > > Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> > > > No information source about the magic numbers found.
> > > 
> > > Oh, interesting repository :) Thanks for linking to it! I didn't know
> > > someone had reworked this driver...
> > > 
> > which btw: contains this interesting snippet (output from fdtdump)
> >    bd71828-i2c@4b {
> >                      reg = <0x0000004b>;
> >                      compatible = "rohm,bd71828";
> >                      gpio_int = <0x00000008 0x00000013 0x00000001>;
> >                      gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
> >                      #address-cells = <0x00000001>;
> >                      #size-cells = <0x00000000>;
> >                      pmic@4b {
> >                          compatible = "rohm,bd71828";
> >               	        regulators {
> >                          	BUCK1 {
> >                              		regulator-name = "buck1";
> > 
> > 
> > and to make it work since basically no regulators are registered
> > instead just some regmap_write()s are done to configure something
> > in probe(). It is a pitfall to think that the information below pmic@4b
> > is used, especially since it is not that obvious in the source.

Odd!  Not only did I not receive the original patch, I also did not
receive your response Andreas.  Spam is empty too.

LORE too: https://lore.kernel.org/all/?q=%22mfd:%20rohm-bd71828:%20Add%20power%20off%20functionality%22

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Lee Jones]

On Mon, 25 Mar 2024, Lee Jones wrote:

> On Mon, 25 Mar 2024, Matti Vaittinen wrote:
> 
> > Hi Andreas,
> > 
> > On 3/25/24 14:16, Andreas Kemnade wrote:
> > > On Mon, 25 Mar 2024 13:31:15 +0200
> > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > > 
> > > > On 3/24/24 22:12, Andreas Kemnade wrote:
> > > > > Since the chip can power off the system, add the corresponding
> > > > > functionality.
> > > > > Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> > > > > No information source about the magic numbers found.
> > > > 
> > > > Oh, interesting repository :) Thanks for linking to it! I didn't know
> > > > someone had reworked this driver...
> > > > 
> > > which btw: contains this interesting snippet (output from fdtdump)
> > >    bd71828-i2c@4b {
> > >                      reg = <0x0000004b>;
> > >                      compatible = "rohm,bd71828";
> > >                      gpio_int = <0x00000008 0x00000013 0x00000001>;
> > >                      gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
> > >                      #address-cells = <0x00000001>;
> > >                      #size-cells = <0x00000000>;
> > >                      pmic@4b {
> > >                          compatible = "rohm,bd71828";
> > >               	        regulators {
> > >                          	BUCK1 {
> > >                              		regulator-name = "buck1";
> > > 
> > > 
> > > and to make it work since basically no regulators are registered
> > > instead just some regmap_write()s are done to configure something
> > > in probe(). It is a pitfall to think that the information below pmic@4b
> > > is used, especially since it is not that obvious in the source.
> 
> Odd!  Not only did I not receive the original patch, I also did not
> receive your response Andreas.  Spam is empty too.
> 
> LORE too: https://lore.kernel.org/all/?q=%22mfd:%20rohm-bd71828:%20Add%20power%20off%20functionality%22

Super weird!  They just all came through.

The LORE link above is now working too.

I suspect an issue with kernel.org.

-- 
Lee Jones [李琼斯]

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Andreas Kemnade]

On Mon, 25 Mar 2024 16:51:34 +0000
Lee Jones <lee@kernel.org> wrote:

> On Mon, 25 Mar 2024, Lee Jones wrote:
> 
> > On Mon, 25 Mar 2024, Matti Vaittinen wrote:
> >   
> > > Hi Andreas,
> > > 
> > > On 3/25/24 14:16, Andreas Kemnade wrote:  
> > > > On Mon, 25 Mar 2024 13:31:15 +0200
> > > > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > > >   
> > > > > On 3/24/24 22:12, Andreas Kemnade wrote:  
> > > > > > Since the chip can power off the system, add the corresponding
> > > > > > functionality.
> > > > > > Based on https://github.com/kobolabs/Kobo-Reader/raw/master/hw/imx6sll-clara2e/kernel.tar.bz2
> > > > > > No information source about the magic numbers found.  
> > > > > 
> > > > > Oh, interesting repository :) Thanks for linking to it! I didn't know
> > > > > someone had reworked this driver...
> > > > >   
> > > > which btw: contains this interesting snippet (output from fdtdump)
> > > >    bd71828-i2c@4b {
> > > >                      reg = <0x0000004b>;
> > > >                      compatible = "rohm,bd71828";
> > > >                      gpio_int = <0x00000008 0x00000013 0x00000001>;
> > > >                      gpio_wdogb = <0x00000039 0x00000018 0x00000001>;
> > > >                      #address-cells = <0x00000001>;
> > > >                      #size-cells = <0x00000000>;
> > > >                      pmic@4b {
> > > >                          compatible = "rohm,bd71828";
> > > >               	        regulators {
> > > >                          	BUCK1 {
> > > >                              		regulator-name = "buck1";
> > > > 
> > > > 
> > > > and to make it work since basically no regulators are registered
> > > > instead just some regmap_write()s are done to configure something
> > > > in probe(). It is a pitfall to think that the information below pmic@4b
> > > > is used, especially since it is not that obvious in the source.  
> > 
> > Odd!  Not only did I not receive the original patch, I also did not
> > receive your response Andreas.  Spam is empty too.
> > 
> > LORE too: https://lore.kernel.org/all/?q=%22mfd:%20rohm-bd71828:%20Add%20power%20off%20functionality%22  
> 
> Super weird!  They just all came through.
> 
> The LORE link above is now working too.
> 
> I suspect an issue with kernel.org.
> 
Well, no, emails from me to everyone sitting behind a mail server without IPv6
address were affected. TCP ACK (3rd part of the handshake) was eaten up by
something. Whatever... So nothing to blame kernel.org besides not having IPv6.

Regards,
Andreas

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Andreas Kemnade]

On Mon, 25 Mar 2024 13:13:13 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 24/03/2024 21:12, Andreas Kemnade wrote:
> >  	struct regmap_irq_chip_data *irq_data;
> > @@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
> >  	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
> >  				   NULL, 0, regmap_irq_get_domain(irq_data));
> >  	if (ret)
> > -		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> > +		return	dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
> > +
> > +	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
> > +		if (!pm_power_off) {
> > +			bd71828_dev = i2c;
> > +			pm_power_off = bd71828_power_off;
> > +			ret = devm_add_action_or_reset(&i2c->dev,
> > +						       bd71828_remove_poweroff,
> > +						       NULL);
> > +		} else
> > +			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");  
> 
> Missing {}
> 
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
> 
No, it does not complain about the {}. I was a bit unsure whether it is
required or not, but I was sure that checkpatch.pl does catch such things.
Yes, documentation clearly says that braces are required in those cases.

Regards,
Andreas

Re: [RFC PATCH 2/2] mfd: rohm-bd71828: Add power off functionality

[Krzysztof Kozlowski]

On 25/03/2024 21:21, Andreas Kemnade wrote:
> On Mon, 25 Mar 2024 13:13:13 +0100
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 24/03/2024 21:12, Andreas Kemnade wrote:
>>>  	struct regmap_irq_chip_data *irq_data;
>>> @@ -542,7 +560,18 @@ static int bd71828_i2c_probe(struct i2c_client *i2c)
>>>  	ret = devm_mfd_add_devices(&i2c->dev, PLATFORM_DEVID_AUTO, mfd, cells,
>>>  				   NULL, 0, regmap_irq_get_domain(irq_data));
>>>  	if (ret)
>>> -		dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>> +		return	dev_err_probe(&i2c->dev, ret, "Failed to create subdevices\n");
>>> +
>>> +	if (of_device_is_system_power_controller(i2c->dev.of_node)) {
>>> +		if (!pm_power_off) {
>>> +			bd71828_dev = i2c;
>>> +			pm_power_off = bd71828_power_off;
>>> +			ret = devm_add_action_or_reset(&i2c->dev,
>>> +						       bd71828_remove_poweroff,
>>> +						       NULL);
>>> +		} else
>>> +			dev_warn(&i2c->dev, "Poweroff callback already assigned\n");  
>>
>> Missing {}
>>
>> Please run scripts/checkpatch.pl and fix reported warnings. Some
>> warnings can be ignored, but the code here looks like it needs a fix.
>> Feel free to get in touch if the warning is not clear.
>>
> No, it does not complain about the {}. I was a bit unsure whether it is
> required or not, but I was sure that checkpatch.pl does catch such things.
> Yes, documentation clearly says that braces are required in those cases.

"CHECK: braces {} should be used on all arms of this statement"

I will update my template-response to use --strict.


Best regards,
Krzysztof