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

[PATCH v2 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
a vendor kernel.

Changes in v2:
- style corrections
- remove unnecessary writes and delays
- correctly unregister handler

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                    | 30 ++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH v2 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>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 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

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

[Matti Vaittinen]

On 3/26/24 21:22, Andreas Kemnade wrote:
> As the PMIC can power off the system, add the corresponding property.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>

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

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

[PATCH v2 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 | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
index 594718f7e8e1..8fd994664bbf 100644
--- a/drivers/mfd/rohm-bd71828.c
+++ b/drivers/mfd/rohm-bd71828.c
@@ -464,6 +464,21 @@ 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)
+{
+	while (true) {
+		/* We are not allowed to sleep, so do not use regmap involving mutexes here. */
+		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);
+		mdelay(500);
+	}
+}
+
+static void bd71828_remove_poweroff(void *data)
+{
+	pm_power_off = NULL;
+}
+
 static int bd71828_i2c_probe(struct i2c_client *i2c)
 {
 	struct regmap_irq_chip_data *irq_data;
@@ -542,7 +557,20 @@ 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) &&
+	    chip_type == ROHM_CHIP_TYPE_BD71828) {
+		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: [PATCH v2 2/2] mfd: rohm-bd71828: Add power off functionality

[Matti Vaittinen]

On 3/26/24 21:22, 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.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   drivers/mfd/rohm-bd71828.c | 30 +++++++++++++++++++++++++++++-
>   1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/rohm-bd71828.c b/drivers/mfd/rohm-bd71828.c
> index 594718f7e8e1..8fd994664bbf 100644
> --- a/drivers/mfd/rohm-bd71828.c
> +++ b/drivers/mfd/rohm-bd71828.c
> @@ -464,6 +464,21 @@ 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)
> +{
> +	while (true) {
> +		/* We are not allowed to sleep, so do not use regmap involving mutexes here. */
> +		i2c_smbus_write_byte_data(bd71828_dev, BD71828_REG_PS_CTRL_1, 0x02);

We need a read-modify-write here. All this should do is setting the 
bit[1] - which is 'state transition to HBNT_MODE'. The register has some 
other controls as well - like transitions to other low-power states 
(bit[0] = SHIP_MODE, bit[2] = LPSR_MODE and bit[3] = IDLE_MODE). These 
aren't a problem though because transition to the HBNT has the highest 
priority.

(It may be that someone somewhere would like to use some other low-power 
mode as the power-off target, but if this is needed then we can probably 
add another DT-property to tell which low-power mode the hardware is 
built to work with. In any case, implementing the HBNT is far better 
than not implementing power-off at all).

What might cause a problem is that the high bits control how the 
voltages of the output rails are controlled. (The PMIC has different 
options on this - and changing the control may change the voltages when 
PMIC is on RUN state. The write to bit[1] should start transition to 
HBNT, but I'm not 100% sure if voltage changes may take some effect 
before state transition is done, or if these settings may be carried 
over to next boot.) Better to not risk anything and leave the rest of 
the bits unchanged. Also, now that the magic value 0x02 is explained - 
we could use BD71828_MASK_STATE_HBNT BIT(1) - instead of a magic number ;)

I should've noticed this when I looked at the v1. Sorry for that :/

> +		mdelay(500);
> +	}
> +}
> +
> +static void bd71828_remove_poweroff(void *data)
> +{
> +	pm_power_off = NULL;
> +}
> +
>   static int bd71828_i2c_probe(struct i2c_client *i2c)
>   {
>   	struct regmap_irq_chip_data *irq_data;
> @@ -542,7 +557,20 @@ 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) &&
> +	    chip_type == ROHM_CHIP_TYPE_BD71828) {

It's worth noting that there is another PMIC, BD71879, which, from the 
driver software point of view, should be (almost?) identical to the 
BD71828. I believe the BD71828 drivers should work with it as well - if 
not out of the box, at least with very minor modifications. 
Unfortunately I don't know products where the BD71879 is used or if it 
is sold via distributors - so I don't know if adding a DT 
compatible/chip type define for it would be beneficial. If someone is 
looking for information about the BD71879 and finds this mail - feel 
free to ping me :] (I expect no action from you here Andreas, just 
mentioned it for the record).

> +		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;
>   }


Just out of the curiosity... How on earth did you end up looking at this 
PMIC or kobo reader code? Do you work with a product using this PMIC, or 
do you just have the reader? No matter what the case is, I think this is 
really cool and seeing people using/improving these drivers warms my 
"open source heart" :) Almost feeling like my work has a purpose XD.

Anyways, if you add a define and change the write to rmw, and if Lee is 
ok with MFD driver hosting the power-off handler...

Acked-by: Matti Vaittinen <mazziesaccount@gmail.com>

Yours,
	-- Matti

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

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

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

[Andreas Kemnade]

Hi,

On Wed, 27 Mar 2024 09:32:29 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> It's worth noting that there is another PMIC, BD71879, which, from the 
> driver software point of view, should be (almost?) identical to the 
> BD71828. I believe the BD71828 drivers should work with it as well - if 
> not out of the box, at least with very minor modifications. 
> Unfortunately I don't know products where the BD71879 is used or if it 
> is sold via distributors - so I don't know if adding a DT 
> compatible/chip type define for it would be beneficial.

yes, you already told we thet the BD71828 drivers are compatible with
the BD71879 and I am using the latter. 
But that at least should be commented somewhere, so that
people do not raise questions, like: Do I have some strange board revision,
etc?
The most terse form to comment it is a separate dt compatible so we are
prepare any "almost identical" surprises.

Regards,
Andreas

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

[Matti Vaittinen]

On 3/27/24 15:04, Andreas Kemnade wrote:
> Hi,
> 
> On Wed, 27 Mar 2024 09:32:29 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> It's worth noting that there is another PMIC, BD71879, which, from the
>> driver software point of view, should be (almost?) identical to the
>> BD71828. I believe the BD71828 drivers should work with it as well - if
>> not out of the box, at least with very minor modifications.
>> Unfortunately I don't know products where the BD71879 is used or if it
>> is sold via distributors - so I don't know if adding a DT
>> compatible/chip type define for it would be beneficial.
> 
> yes, you already told we thet the BD71828 drivers are compatible with
> the BD71879 and I am using the latter.
> But that at least should be commented somewhere, so that
> people do not raise questions, like: Do I have some strange board revision,
> etc?
> The most terse form to comment it is a separate dt compatible so we are
> prepare any "almost identical" surprises.

I agree. Reason why I haven't done this already is that I don't always 
(like in this case) know which of the variant are eventually sold. So, 
it's balancing dance between adding compatibles for ICs that will never 
been seen by large audience, and missing compatibles for some of the 
variants.

This is also why I was interested in knowing which variant you had, and 
where was it used.

But yes, I think that as the BD71879 has obviously been found by a 
community linux kernel user - it would make sense to add a compatible 
for it!

Do you feel like adding the compatible 'rohm,bd71879' in 
rohm,bd71828-pmic.yaml as part of this series(?)

Yours,
	-- Matti

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

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

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

[Andreas Kemnade]

Hi Matti,

On Wed, 27 Mar 2024 16:11:36 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/27/24 15:04, Andreas Kemnade wrote:
> > Hi,
> > 
> > On Wed, 27 Mar 2024 09:32:29 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> It's worth noting that there is another PMIC, BD71879, which, from the
> >> driver software point of view, should be (almost?) identical to the
> >> BD71828. I believe the BD71828 drivers should work with it as well - if
> >> not out of the box, at least with very minor modifications.
> >> Unfortunately I don't know products where the BD71879 is used or if it
> >> is sold via distributors - so I don't know if adding a DT
> >> compatible/chip type define for it would be beneficial.  
> > 
> > yes, you already told we thet the BD71828 drivers are compatible with
> > the BD71879 and I am using the latter.
> > But that at least should be commented somewhere, so that
> > people do not raise questions, like: Do I have some strange board revision,
> > etc?
> > The most terse form to comment it is a separate dt compatible so we are
> > prepare any "almost identical" surprises.  
> 
> I agree. Reason why I haven't done this already is that I don't always 
> (like in this case) know which of the variant are eventually sold. So, 
> it's balancing dance between adding compatibles for ICs that will never 
> been seen by large audience, and missing compatibles for some of the 
> variants.
> 
> This is also why I was interested in knowing which variant you had, and 
> where was it used.
> 
I have found it in the Kobo Clara 2E ebook reader.
Kobo seems to switch from RC5T619 to BD71879.
The Kobo Nia rev C also has that one.
Kobo Libra 2 has several hardware revs out in the wild, some of them
with the BD71879.

> But yes, I think that as the BD71879 has obviously been found by a 
> community linux kernel user - it would make sense to add a compatible 
> for it!
> 
> Do you feel like adding the compatible 'rohm,bd71879' in 
> rohm,bd71828-pmic.yaml as part of this series(?)

Do we want a separate chip_type now? Or do we want to add it later if
we ever see a difference. My personal opinion is to wait until there is
really a need.
If we do not need it, then it is a different series I think but sure
I will produce such a patch.

Regards,
Andreas

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

[Matti Vaittinen]

Morning Andreas,

On 3/28/24 00:02, Andreas Kemnade wrote:
> Hi Matti,
> 
> On Wed, 27 Mar 2024 16:11:36 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 3/27/24 15:04, Andreas Kemnade wrote:
>>> Hi,
>>>
>>> On Wed, 27 Mar 2024 09:32:29 +0200
>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>>    
>>>> It's worth noting that there is another PMIC, BD71879, which, from the
>>>> driver software point of view, should be (almost?) identical to the
>>>> BD71828. I believe the BD71828 drivers should work with it as well - if
>>>> not out of the box, at least with very minor modifications.
>>>> Unfortunately I don't know products where the BD71879 is used or if it
>>>> is sold via distributors - so I don't know if adding a DT
>>>> compatible/chip type define for it would be beneficial.
>>>
>>> yes, you already told we thet the BD71828 drivers are compatible with
>>> the BD71879 and I am using the latter.
>>> But that at least should be commented somewhere, so that
>>> people do not raise questions, like: Do I have some strange board revision,
>>> etc?
>>> The most terse form to comment it is a separate dt compatible so we are
>>> prepare any "almost identical" surprises.
>>
>> I agree. Reason why I haven't done this already is that I don't always
>> (like in this case) know which of the variant are eventually sold. So,
>> it's balancing dance between adding compatibles for ICs that will never
>> been seen by large audience, and missing compatibles for some of the
>> variants.
>>
>> This is also why I was interested in knowing which variant you had, and
>> where was it used.
>>
> I have found it in the Kobo Clara 2E ebook reader.
> Kobo seems to switch from RC5T619 to BD71879.
> The Kobo Nia rev C also has that one.
> Kobo Libra 2 has several hardware revs out in the wild, some of them
> with the BD71879.

Thanks for the info :) It's a shame we so rarely know where things we 
work for are used. I always find news like this interesting.

>> But yes, I think that as the BD71879 has obviously been found by a
>> community linux kernel user - it would make sense to add a compatible
>> for it!
>>
>> Do you feel like adding the compatible 'rohm,bd71879' in
>> rohm,bd71828-pmic.yaml as part of this series(?)
> 
> Do we want a separate chip_type now? Or do we want to add it later if
> we ever see a difference. My personal opinion is to wait until there is
> really a need.

Using the BD71828 chip_id for BD71879 in the MFD driver is fine to me. A 
comment saying they seem "functionally equivalent" can be added to 
explain this choice.

> If we do not need it, then it is a different series I think but sure
> I will produce such a patch.

Great, thanks! I think it's clearer to have it as own patch, but I think 
it fits in the same series - what suits you best. (Don't know if Lee or 
DT peeps have different opinion.)

Yours,
	-- Matti

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

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