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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andreas,

On 3/25/24 14:16, Andreas Kemnade wrote:
On Mon, 25 Mar 2024 13:31:15 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxxxx>
---
   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! ~~





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux