[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

The charger is now represented by a distinct subnode of the tps65090
device. Add this node and enable low current charging with it.

Signed-off-by: Rhyland Klein <rklein@nvidia.com>
---
- This patch is dependent upon the following fixup-patch.
  http://patchwork.ozlabs.org/patch/235434/

 arch/arm/boot/dts/tegra114-dalmore.dts |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
index 72c1f27..d257441 100644
--- a/arch/arm/boot/dts/tegra114-dalmore.dts
+++ b/arch/arm/boot/dts/tegra114-dalmore.dts
@@ -763,6 +763,11 @@
 			vsys-l1-supply = <&vdd_ac_bat_reg>;
 			vsys-l2-supply = <&vdd_ac_bat_reg>;
 
+			charger {
+				compatible = "ti,tps65090-charger";
+				ti,enable-low-current-chrg;
+			};
+
 			regulators {
 				tps65090_dcdc1_reg: dcdc1 {
 					regulator-name = "vdd-sys-5v0";
-- 
1.7.9.5

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Stephen Warren]

On 04/10/2013 01:51 PM, Rhyland Klein wrote:
> The charger is now represented by a distinct subnode of the tps65090
> device. Add this node and enable low current charging with it.

> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts

> +			charger {
> +				compatible = "ti,tps65090-charger";
> +				ti,enable-low-current-chrg;

So does the TPS65090 driver scan all its sub-nodes like a bus an
instantiate anything? That's a little odd since the regulators node is
at the same level, yet doesn't represent a device on that same internal
bus...

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

On 4/10/2013 6:30 PM, Stephen Warren wrote:
> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>> The charger is now represented by a distinct subnode of the tps65090
>> device. Add this node and enable low current charging with it.
> 
>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
> 
>> +			charger {
>> +				compatible = "ti,tps65090-charger";
>> +				ti,enable-low-current-chrg;
> 
> So does the TPS65090 driver scan all its sub-nodes like a bus an
> instantiate anything? That's a little odd since the regulators node is
> at the same level, yet doesn't represent a device on that same internal
> bus...
> 

It doesn't scan so much as have a list of subdev's it tries to register.
The charger node uses the of_compatible string defined (in the tps65090
driver and the charger driver) to match the subnode to the mfd_cell
device for the charger. The regulators are currently under a different
subdevice which is not using the of_compatible string. I was planning on
a follow up patch series to update the tps65090-regulator driver to use
the dt type approach with a subnode to be consistent.

-rhyland

-- 
nvpublic

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Stephen Warren]

On 04/11/2013 09:38 AM, Rhyland Klein wrote:
> On 4/10/2013 6:30 PM, Stephen Warren wrote:
>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>> The charger is now represented by a distinct subnode of the tps65090
>>> device. Add this node and enable low current charging with it.
>>
>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>
>>> +			charger {
>>> +				compatible = "ti,tps65090-charger";
>>> +				ti,enable-low-current-chrg;
>>
>> So does the TPS65090 driver scan all its sub-nodes like a bus an
>> instantiate anything? That's a little odd since the regulators node is
>> at the same level, yet doesn't represent a device on that same internal
>> bus...
>>
> 
> It doesn't scan so much as have a list of subdev's it tries to register.
> The charger node uses the of_compatible string defined (in the tps65090
> driver and the charger driver) to match the subnode to the mfd_cell
> device for the charger.

That sounds very odd. If the main driver hard-codes the list of children
it expects, there's no point using compatible for the children. Using a
bus-structure and compatible values would only be appropriate when the
top-level driver is generic, and simply scans all its children as a
generic bus, without having any idea what's there.

> The regulators are currently under a different
> subdevice which is not using the of_compatible string. I was planning on
> a follow up patch series to update the tps65090-regulator driver to use
> the dt type approach with a subnode to be consistent.

But that's changing the DT bindings. That shouldn't be done.

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

On 4/11/2013 12:17 PM, Stephen Warren wrote:
> On 04/11/2013 09:38 AM, Rhyland Klein wrote:
>> On 4/10/2013 6:30 PM, Stephen Warren wrote:
>>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>>> The charger is now represented by a distinct subnode of the tps65090
>>>> device. Add this node and enable low current charging with it.
>>>
>>>> diff --git a/arch/arm/boot/dts/tegra114-dalmore.dts b/arch/arm/boot/dts/tegra114-dalmore.dts
>>>
>>>> +			charger {
>>>> +				compatible = "ti,tps65090-charger";
>>>> +				ti,enable-low-current-chrg;
>>>
>>> So does the TPS65090 driver scan all its sub-nodes like a bus an
>>> instantiate anything? That's a little odd since the regulators node is
>>> at the same level, yet doesn't represent a device on that same internal
>>> bus...
>>>
>>
>> It doesn't scan so much as have a list of subdev's it tries to register.
>> The charger node uses the of_compatible string defined (in the tps65090
>> driver and the charger driver) to match the subnode to the mfd_cell
>> device for the charger.
> 
> That sounds very odd. If the main driver hard-codes the list of children
> it expects, there's no point using compatible for the children. Using a
> bus-structure and compatible values would only be appropriate when the
> top-level driver is generic, and simply scans all its children as a
> generic bus, without having any idea what's there.

Agreed, but this seems to mfd children are handled right now, perhaps a
rework can be done for dt, but for non-dt routes, there is no bus to
scan for children.

> 
>> The regulators are currently under a different
>> subdevice which is not using the of_compatible string. I was planning on
>> a follow up patch series to update the tps65090-regulator driver to use
>> the dt type approach with a subnode to be consistent.
> 
> But that's changing the DT bindings. That shouldn't be done.
> 

We chose to the do the subnode structure partly to make the
Documentation layout more logical. This follows the design of the palmas
mfd and its sub-components. I can leave the regulators as is, but to me
the inconsistency between the two is more of a concern.

-rhyland

-- 
nvpublic

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Stephen Warren]

On 04/10/2013 01:51 PM, Rhyland Klein wrote:
> The charger is now represented by a distinct subnode of the tps65090
> device. Add this node and enable low current charging with it.

What's the status of the TPS60590 bindings; are they agreed upon by
NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
something I should apply for 3.11, or does it need to be reworked?

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

On 5/17/2013 7:57 PM, Stephen Warren wrote:
> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>> The charger is now represented by a distinct subnode of the tps65090
>> device. Add this node and enable low current charging with it.
> 
> What's the status of the TPS60590 bindings; are they agreed upon by
> NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
> something I should apply for 3.11, or does it need to be reworked?

I haven't seen any discussion with slimlogic or TI about the tps65090.
As far as I know the bindings for this driver haven't changed.

Laxman, do you think the work on the palmas driver will impact the
design of the bindings for the tps65090?

-rhyland

> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
nvpublic

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Stephen Warren]

On 05/20/2013 09:24 AM, Rhyland Klein wrote:
> On 5/17/2013 7:57 PM, Stephen Warren wrote:
>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>> The charger is now represented by a distinct subnode of the tps65090
>>> device. Add this node and enable low current charging with it.
>>
>> What's the status of the TPS60590 bindings; are they agreed upon by
>> NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
>> something I should apply for 3.11, or does it need to be reworked?
> 
> I haven't seen any discussion with slimlogic or TI about the tps65090.
> As far as I know the bindings for this driver haven't changed.
> 
> Laxman, do you think the work on the palmas driver will impact the
> design of the bindings for the tps65090?

Sorry, perhaps I'm confusing two different chips. If TPS65090 isn't
Palmas, then ignore my question. In which case, I suppose I should just
apply your patch then?

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

On 5/20/2013 11:28 AM, Stephen Warren wrote:
> On 05/20/2013 09:24 AM, Rhyland Klein wrote:
>> On 5/17/2013 7:57 PM, Stephen Warren wrote:
>>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>>> The charger is now represented by a distinct subnode of the tps65090
>>>> device. Add this node and enable low current charging with it.
>>>
>>> What's the status of the TPS60590 bindings; are they agreed upon by
>>> NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
>>> something I should apply for 3.11, or does it need to be reworked?
>>
>> I haven't seen any discussion with slimlogic or TI about the tps65090.
>> As far as I know the bindings for this driver haven't changed.
>>
>> Laxman, do you think the work on the palmas driver will impact the
>> design of the bindings for the tps65090?
> 
> Sorry, perhaps I'm confusing two different chips. If TPS65090 isn't
> Palmas, then ignore my question. In which case, I suppose I should just
> apply your patch then?
> 

I would say yes. The design of having the child node this way was how we
had agreed worked best. The only reason I would see a significant reason
to change this, is if something was decided that all mfd devices should
start to follow some pattern which differed, which would mean changing
existing bindings and therefore is unlikely.

-rhyland

-- 
nvpublic

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Stephen Warren]

On 05/20/2013 09:35 AM, Rhyland Klein wrote:
> On 5/20/2013 11:28 AM, Stephen Warren wrote:
>> On 05/20/2013 09:24 AM, Rhyland Klein wrote:
>>> On 5/17/2013 7:57 PM, Stephen Warren wrote:
>>>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>>>> The charger is now represented by a distinct subnode of the tps65090
>>>>> device. Add this node and enable low current charging with it.
>>>>
>>>> What's the status of the TPS60590 bindings; are they agreed upon by
>>>> NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
>>>> something I should apply for 3.11, or does it need to be reworked?
>>>
>>> I haven't seen any discussion with slimlogic or TI about the tps65090.
>>> As far as I know the bindings for this driver haven't changed.
>>>
>>> Laxman, do you think the work on the palmas driver will impact the
>>> design of the bindings for the tps65090?
>>
>> Sorry, perhaps I'm confusing two different chips. If TPS65090 isn't
>> Palmas, then ignore my question. In which case, I suppose I should just
>> apply your patch then?
>>
> 
> I would say yes. The design of having the child node this way was how we
> had agreed worked best. The only reason I would see a significant reason
> to change this, is if something was decided that all mfd devices should
> start to follow some pattern which differed, which would mean changing
> existing bindings and therefore is unlikely.

OK, I have applied this patch to Tegra's for-3.11/dt branch. I also sent
a patch to actually enable the new driver in tegra_defconfig.

[PATCH] ARM: tegra: Add charger subnode to tps65090 node

[Rhyland Klein]

On 5/20/2013 3:15 PM, Stephen Warren wrote:
> On 05/20/2013 09:35 AM, Rhyland Klein wrote:
>> On 5/20/2013 11:28 AM, Stephen Warren wrote:
>>> On 05/20/2013 09:24 AM, Rhyland Klein wrote:
>>>> On 5/17/2013 7:57 PM, Stephen Warren wrote:
>>>>> On 04/10/2013 01:51 PM, Rhyland Klein wrote:
>>>>>> The charger is now represented by a distinct subnode of the tps65090
>>>>>> device. Add this node and enable low current charging with it.
>>>>>
>>>>> What's the status of the TPS60590 bindings; are they agreed upon by
>>>>> NVIDIA, TI, and SlimLogic yet? In other words, is this patch still
>>>>> something I should apply for 3.11, or does it need to be reworked?
>>>>
>>>> I haven't seen any discussion with slimlogic or TI about the tps65090.
>>>> As far as I know the bindings for this driver haven't changed.
>>>>
>>>> Laxman, do you think the work on the palmas driver will impact the
>>>> design of the bindings for the tps65090?
>>>
>>> Sorry, perhaps I'm confusing two different chips. If TPS65090 isn't
>>> Palmas, then ignore my question. In which case, I suppose I should just
>>> apply your patch then?
>>>
>>
>> I would say yes. The design of having the child node this way was how we
>> had agreed worked best. The only reason I would see a significant reason
>> to change this, is if something was decided that all mfd devices should
>> start to follow some pattern which differed, which would mean changing
>> existing bindings and therefore is unlikely.
> 
> OK, I have applied this patch to Tegra's for-3.11/dt branch. I also sent
> a patch to actually enable the new driver in tegra_defconfig.
> 

thanks, that was the next patch I was going to send :) Beat me to it.

-rhyland

-- 
nvpublic