Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[carl]

> I think the patch is also wrong, since the information is already 
> described in DT - just the other way around: The battery references 
> the charger. This provides all required information to the kernel 
> for a 1:1 link. 
>
> -- Sebastian 

I added this so that cpcap_charger may become aware of the battery insertion state by querying the battery driver.
Would you thus recommend that instead of adding this phandle i should amend the series such that cpcap_charger walks the tree looking for a cpcap_battery compatible node and then determines if the charger phandle points to itself? Is there some recommended way performing this reverse search?

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

Hi,

On Wed, Mar 24, 2021 at 05:08:58PM +0100, carl@uvos.xyz wrote:
> > I think the patch is also wrong, since the information is already 
> > described in DT - just the other way around: The battery references 
> > the charger. This provides all required information to the kernel 
> > for a 1:1 link. 
> 
> I added this so that cpcap_charger may become aware of the battery
> insertion state by querying the battery driver.

> Would you thus recommend that instead of adding this phandle i
> should amend the series such that cpcap_charger walks the tree
> looking for a cpcap_battery compatible node and then determines if
> the charger phandle points to itself? Is there some recommended
> way performing this reverse search?

I was thinking of creating a new core function to loop over all
supplied batteries of a power_supply (using psy->supplied_from),
but in this specific case it might be enough to just use
power_supply_get_by_name(). As I said, I have not yet properly
reviewed the full patchset.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]

Hi,

On Wed, Mar 24, 2021 at 05:21:52PM +0100, H. Nikolaus Schaller wrote:
> > Am 24.03.2021 um 16:42 schrieb Sebastian Reichel <sre@kernel.org>:
> > I think the patch is also wrong, since the information is already
> > described in DT - just the other way around: The battery references
> > the charger.
> 
> Just curious for other devices to be properly defined:
> 
> Does a battery have its own driver?
> Can it be addressed (through I2C or similar mechanisms)?

Linux power-supply subsystem expects chargers and batteries
with battery being smart battery. I guess this has been
designed following the ACPI specs. On embedded devices this
usually means battery = fuel gauge.

> Is it closer to the processor (being the root node of DTS) or
> farther away than the chargers?

It depends? :) There are systems with smart batteries reachable
via I2C and gpio-charger device, which cannot be controlled and
just having an enable gpio (and thus being in the root node).
OTOH there are systems, that lack a proper fuel gauge and have
advanced I2C chargers.

> My observations is that usually chargers have drivers and need to
> reference battery information to adapt their behaviour.

I believe you are talking about chemistry information e.t.c.
that's available from the simple-battery node?

> So IMHO it would be more natural to have a charger reference the
> battery.

Direction was not something I came up with. I took over the
subsystem years ago when this was already in place. It's a
core thing in the subsystem and definetly cannot be changed
anymore:

Documentation/devicetree/bindings/power/supply/power-supply.yaml

In general sometimes the battery needs charger info and sometimes
charger needs fuel gauge info. The thing is, that this does not
mean we need phandles in both directions. One phandle is enough
to have the required information, everything else can be handled
by kernel frameworks. That's preferred, since it does not create
ABI.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Tony Lindgren]

Hi,

* Sebastian Reichel <sre@kernel.org> [210324 15:43]:
> On Wed, Mar 24, 2021 at 01:54:02PM +0200, Tony Lindgren wrote:
> > * Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> > > --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > > +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > > @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
> > >  			io-channel-names = "battdetb", "battp",
> > >  					   "vbus", "chg_isense",
> > >  					   "batti";
> > > +			battery = <&cpcap_battery>;
> > >  		};
> > 
> > This seems like good standard stuff to have, picking up this patch into
> > omap-for-v5.13/dt thanks.
> 
> This was btw. one of the patches that finally made me convert all
> the binding files to YAML. This patch will now result in warning
> being printed when you run 'make dtbs_check', since the binding has
> not been updated.
> 
> I did not yet have time to review the patchset properly, so I have
> not yet replied (partely, because of being busy with the YAML
> stuff).
> 
> I think the patch is also wrong, since the information is already
> described in DT - just the other way around: The battery references
> the charger. This provides all required information to the kernel
> for a 1:1 link.

OK, I will drop this patch.

Regards,

Tony

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Hi,

On Wed, Mar 24, 2021 at 01:54:02PM +0200, Tony Lindgren wrote:
> * Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> > --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> > @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
> >  			io-channel-names = "battdetb", "battp",
> >  					   "vbus", "chg_isense",
> >  					   "batti";
> > +			battery = <&cpcap_battery>;
> >  		};
> 
> This seems like good standard stuff to have, picking up this patch into
> omap-for-v5.13/dt thanks.
> 
> Tony

This was btw. one of the patches that finally made me convert all
the binding files to YAML. This patch will now result in warning
being printed when you run 'make dtbs_check', since the binding has
not been updated.

I did not yet have time to review the patchset properly, so I have
not yet replied (partely, because of being busy with the YAML
stuff).

I think the patch is also wrong, since the information is already
described in DT - just the other way around: The battery references
the charger. This provides all required information to the kernel
for a 1:1 link.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Tony Lindgren]

* Carl Philipp Klemm <philipp@uvos.xyz> [210117 23:45]:
> --- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> +++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
> @@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
>  			io-channel-names = "battdetb", "battp",
>  					   "vbus", "chg_isense",
>  					   "batti";
> +			battery = <&cpcap_battery>;
>  		};

This seems like good standard stuff to have, picking up this patch into
omap-for-v5.13/dt thanks.

Tony

[PATCH 2/5] ARM: dts: add battery phandle to cpcap_charger

[Carl Philipp Klemm]

This is required for 
	power: supply: cpcap-charger: get the battery inserted 
		infomation from cpcap-battery

Signed-off-by: Carl Philipp Klemm <philipp@uvos.xyz>
---
 arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
index 08a7d3ce383f..842eaa7b89e5 100644
--- a/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
+++ b/arch/arm/boot/dts/motorola-cpcap-mapphone.dtsi
@@ -61,6 +61,7 @@ &cpcap_adc 2 &cpcap_adc 5
 			io-channel-names = "battdetb", "battp",
 					   "vbus", "chg_isense",
 					   "batti";
+			battery = <&cpcap_battery>;
 		};
 
 		cpcap_regulator: regulator {
-- 
2.29.2