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