| Message ID | 1365623505-6309-1-git-send-email-rklein@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
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
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.
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
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?
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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?
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
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.
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
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";
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(+)