Message-ID: <6dcd724a-a55c-4cba-a45b-21e76b1973b0@gmail.com>
Date: Tue, 29 Oct 2024 12:26:10 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
 linux-kernel@...r.kernel.org, Fabio Estevam <festevam@...il.com>,
 devicetree@...r.kernel.org, Pengutronix Kernel Team <kernel@...gutronix.de>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 linux-arm-kernel@...ts.infradead.org, Sascha Hauer <s.hauer@...gutronix.de>,
 Alexander Stein <alexander.stein@...tq-group.com>, imx@...ts.linux.dev,
 sre@...nel.org
Subject: Re: [PATCH v2 2/3] ARM: dts: imx: Add devicetree for Kobo Clara 2E

On 29/10/2024 10:44, Andreas Kemnade wrote:
> Am Tue, 29 Oct 2024 09:53:33 +0200
> schrieb Matti Vaittinen <mazziesaccount@...il.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@...nade.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

Powered by blists - more mailing lists