Re: [PATCH v2 2/3] ARM: dts: imx: Add devicetree for Kobo Clara 2E

Matti Vaittinen
2024-10-29T10:26:17Z

On 29/10/2024 10:44, Andreas Kemnade wrote:
> Am Tue, 29 Oct 2024 09:53:33 +0200
> schrieb Matti Vaittinen <mazziesaccount@gmail.com>:
>
>> On 24/10/2024 17:22, Andreas Kemnade wrote:
>>> Adds a devicetree for the Kobo Clara 2E Ebook reader. It is based
>>> on boards marked with "37NB-E60K2M+4A2" or "37NB-E60K2M+4B0". It is
>>> equipped with an i.MX6SLL SoC.
>>>
>>> Expected to work:
>>> - Buttons
>>> - Wifi
>>> - Bluetooth
>>> (if Wifi is initialized first, driver does not handle
>>> regulators yet)
>>> - LED
>>> - uSD
>>> - USB
>>> - RTC
>>> - Touchscreen
>>>
>>> Add human-readable comments for devices without mainlined driver and
>>> binding. Such comments can e.g. be help to find testers if someone
>>> starts to work on the missing drivers.
>>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
>>
>> ...
>>
>>> +
>>> + pmic@4b {
>>> + compatible = "rohm,bd71879", "rohm,bd71828";
>>> + reg = <0x4b>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pinctrl_bd71828>;
>>> +
>>> + interrupt-parent = <&gpio4>;
>>> + interrupts = <19 IRQ_TYPE_LEVEL_LOW>;
>>> + system-power-controller;
>>> +
>>> + clocks = <&clks 0>;
>>> + #clock-cells = <0>;
>>> + clock-output-names = "bd71828-32k-out";
>>> +
>>> + gpio-controller;
>>> + #gpio-cells = <2>;
>>> + gpio-reserved-ranges = <0 1>, <2 1>;
>>> +
>>> + rohm,charger-sense-resistor-ohms = <30000000>;
>>
>> Well, now that I got this out - I suppose this could be
>> rohm,charger-sense-resistor-milli-ohms = <30>;
>> or
>> rohm,charger-sense-resistor-micro-ohms = <30000>;
>>
>> I further guess there is no upstreamn binding doc for this property.
>
> The binding doc is upstream. So an impressive amount of maintainers
> had a look at it...

Oh. So I have upstreamed it at some point... I suppose there is no users
though.

> Well, everyone seem to entrust Rohm Semiconductors to do magic...
> wonderful reputation.

:)

> So how to proceed? As this property is not required, I can simply
> remove it and add a comment.

I would like to suggest adding:
rohm,charger-sense-resistor-milli-ohms = <30>;

and a binding with minimum 1 and maximum 50. Those should be sane enough
limits. At the same time the 'rohm,charger-sense-resistor-ohms' could
probably be deprecated.

This, however, is an extra mile for you. So, if you don't feel like
doing it, then dropping the entry is Ok as well.

>
>> I think there is also no upstream charger driver for the
>> BD71828/BD71879 - only an early RFC and some downstream mess - but
>> stil it'd be nice to have the property in place as the size of the
>> sense resistor is needed when converting coulomb counter register
>> values to current.
>>
> What are you upstreaming plans here? For all:
> I rebased the charger stuff to v6.11 on
> https://github.com/akemnade/linux branch kobo/power-6.11

Excellent question.

The reason why this driver is not upstream is that (as far as I know)
the PMIC variants supported by the driver have always been tailored for
a specific customer's needs. I don't think these PMICs have been sold
for anyone else besides the specific customers. Hence, there has been
little benefit for creating an upstream driver.

For the charger there has been additional complexity because the
bd71827-charger driver implements also an in-kernel battery fuel gauge
which computes the SOC values and sends them to the user-space. Like you
know, this is very battery specific and requires correct battery
parameters to be given.

I think your work changes things a bit. Seems we will be having an
upstream board (kobo) using the PMIC drivers - and it might also use
some information from the charger block. I know you also implemented
some control logic to get the charging working with limited power-supplies.

(I think the BD71815 is also used in some projects which wanted to have
upstream drivers - but I haven't seen upstream dts using it).

Furthermore, we still seem to be having new variants - I am right now
writing drivers for another customer specific PMIC using somewhat
similar charging logic. Having upstream driver would help in work like
that. (By the way, I took your rebased kobo patches and continued my
work on top of those - so thanks for the rebasing!)

So - in my opinion, upstreaming would be beneficial and I will push for
this direction - but it will be slow process.

Also, I think that maybe the downstream driver shouldn't be upstreamed
as such. I am not 100% convinced the SOC should be computed in-kernel.
It'd might be easier to implement the SOC computation in a user-space
library which just gets the details from the kernel. On the other hand,
I think the driver should support a few currently unsupported features
like setting the currents for different charge phases based on
device-tree properties.

Giving I am the only guy at ROHM working on these Linux drivers (and not
only these drivers), and knowing I don't have a system which is equipped
with a real battery - I wouldn't hold my breath waiting :(

Yours,
-- Matti

> Regards,
> Andreas