Hi Andreas,
On 3/25/24 14:16, Andreas Kemnade wrote:
> On Mon, 25 Mar 2024 13:31:15 +0200
> Matti Vaittinen <[email protected]> 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 <[email protected]>
>>> ---
>>> 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! ~~
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 <[email protected]> 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 [李琼斯]
Hi Matti,
On Mon, 25 Mar 2024 14:44:43 +0200
Matti Vaittinen <[email protected]> wrote:
> Hi Andreas,
>
> On 3/25/24 14:16, Andreas Kemnade wrote:
> > On Mon, 25 Mar 2024 13:31:15 +0200
> > Matti Vaittinen <[email protected]> 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
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 <[email protected]> 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 [李琼斯]