| Message ID | 1445014753-15450-2-git-send-email-afd@ti.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, 16 Oct 2015, Andrew F. Davis wrote: > The TPS65086 PMIC contains several regulators and a GPO controller. > Add bindings for the TPS65086 PMIC. > > Signed-off-by: Andrew F. Davis <afd@ti.com> > --- > .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ > Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ > .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ Please split these up into separate patches. There is no functional reason to bundle them up. > 3 files changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt > create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt > create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt [...] > diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt > new file mode 100644 > index 0000000..4b6aeb4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt > @@ -0,0 +1,46 @@ > +* TPS65086 Power Management Integrated Circuit bindings > + > +Required properties: > + - compatible : Should be "ti,tps65086". Any indication that it's a PMIC? > + - reg : Slave address. I2C/SPI? > + - interrupt-parent : The parent interrupt controller. Phandled to ... > + - interrupts : The interrupt line the device is connected to. > + - interrupt-controller : Marks the device node as an interrupt controller. > + - #interrupt-cells : The number of cells to describe an IRQ, this > + should be 2. The first cell is the IRQ number. > + The second cell is the flags, encoded as the trigger > + masks from ../interrupt-controller/interrupts.txt. Masks? What masks? Best to make a link to the header where the flags are defined here. > +Additional nodes defined in: > + - Regulators : ../regulator/tps65086-regulator.txt. > + - GPIO : ../gpio/gpio-tps65086.txt. I'd suggest removing the full stops from all of the lines above. Just treat them as bullet points like we normally do. > +Example: > + > + pmic: tps65086@5e { > + compatible = "ti,tps65086"; > + reg = <0x5e>; > + interrupt-parent = <&gpio1>; > + interrupts = <28 IRQ_TYPE_LEVEL_LOW>; > + interrupt-controller; > + #interrupt-cells = <2>; > + > + regulators { > + compatible = "ti,tps65086-regulator"; > + > + buck1 { > + regulator-name = "vcc1"; > + regulator-min-microvolt = <1600000>; > + regulator-max-microvolt = <1600000>; > + regulator-boot-on; > + ti,regulator-decay; > + ti,regulator-step-size-25mv; > + }; > + }; > + > + gpio4: tps65086_gpio { > + compatible = "ti,tps65086-gpio"; > + gpio-controller; > + #gpio-cells = <2>; > + }; > + }; [...]
On 10/19/2015 04:13 AM, Lee Jones wrote: > On Fri, 16 Oct 2015, Andrew F. Davis wrote: > >> The TPS65086 PMIC contains several regulators and a GPO controller. >> Add bindings for the TPS65086 PMIC. >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> >> --- >> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ >> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ >> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ > > Please split these up into separate patches. > > There is no functional reason to bundle them up. > ACK >> 3 files changed, 99 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt >> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt >> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt > > [...] > >> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt >> new file mode 100644 >> index 0000000..4b6aeb4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt >> @@ -0,0 +1,46 @@ >> +* TPS65086 Power Management Integrated Circuit bindings >> + >> +Required properties: >> + - compatible : Should be "ti,tps65086". > > Any indication that it's a PMIC? > In the compatible string? >> + - reg : Slave address. > > I2C/SPI? > ACK >> + - interrupt-parent : The parent interrupt controller. > > Phandled to ... > ACK >> + - interrupts : The interrupt line the device is connected to. >> + - interrupt-controller : Marks the device node as an interrupt controller. >> + - #interrupt-cells : The number of cells to describe an IRQ, this >> + should be 2. The first cell is the IRQ number. >> + The second cell is the flags, encoded as the trigger >> + masks from ../interrupt-controller/interrupts.txt. > > Masks? What masks? > > Best to make a link to the header where the flags are defined here. > ACK >> +Additional nodes defined in: >> + - Regulators : ../regulator/tps65086-regulator.txt. >> + - GPIO : ../gpio/gpio-tps65086.txt. > > I'd suggest removing the full stops from all of the lines above. > > Just treat them as bullet points like we normally do. > ACK >> +Example: >> + >> + pmic: tps65086@5e { >> + compatible = "ti,tps65086"; >> + reg = <0x5e>; >> + interrupt-parent = <&gpio1>; >> + interrupts = <28 IRQ_TYPE_LEVEL_LOW>; >> + interrupt-controller; >> + #interrupt-cells = <2>; >> + >> + regulators { >> + compatible = "ti,tps65086-regulator"; >> + >> + buck1 { >> + regulator-name = "vcc1"; >> + regulator-min-microvolt = <1600000>; >> + regulator-max-microvolt = <1600000>; >> + regulator-boot-on; >> + ti,regulator-decay; >> + ti,regulator-step-size-25mv; >> + }; >> + }; >> + >> + gpio4: tps65086_gpio { >> + compatible = "ti,tps65086-gpio"; >> + gpio-controller; >> + #gpio-cells = <2>; >> + }; >> + }; > > [...] >
On Mon, 19 Oct 2015, Andrew F. Davis wrote: > On 10/19/2015 04:13 AM, Lee Jones wrote: > >On Fri, 16 Oct 2015, Andrew F. Davis wrote: > > > >>The TPS65086 PMIC contains several regulators and a GPO controller. > >>Add bindings for the TPS65086 PMIC. > >> > >>Signed-off-by: Andrew F. Davis <afd@ti.com> > >>--- > >> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ > >> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ > >> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ > > > >Please split these up into separate patches. > > > >There is no functional reason to bundle them up. > > > > ACK > > >> 3 files changed, 99 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt > >> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt > >> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt > > > >[...] > > > >>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>new file mode 100644 > >>index 0000000..4b6aeb4 > >>--- /dev/null > >>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>@@ -0,0 +1,46 @@ > >>+* TPS65086 Power Management Integrated Circuit bindings > >>+ > >>+Required properties: > >>+ - compatible : Should be "ti,tps65086". > > > >Any indication that it's a PMIC? > > > > In the compatible string? Ya. > >>+ - reg : Slave address. > > > >I2C/SPI? > > > > ACK > > >>+ - interrupt-parent : The parent interrupt controller. > > > >Phandled to ... > > > > ACK > > >>+ - interrupts : The interrupt line the device is connected to. > >>+ - interrupt-controller : Marks the device node as an interrupt controller. > >>+ - #interrupt-cells : The number of cells to describe an IRQ, this > >>+ should be 2. The first cell is the IRQ number. > >>+ The second cell is the flags, encoded as the trigger > >>+ masks from ../interrupt-controller/interrupts.txt. > > > >Masks? What masks? > > > >Best to make a link to the header where the flags are defined here. > > > > ACK > > >>+Additional nodes defined in: > >>+ - Regulators : ../regulator/tps65086-regulator.txt. > >>+ - GPIO : ../gpio/gpio-tps65086.txt. > > > >I'd suggest removing the full stops from all of the lines above. > > > >Just treat them as bullet points like we normally do. > > > > ACK > > >>+Example: > >>+ > >>+ pmic: tps65086@5e { > >>+ compatible = "ti,tps65086"; > >>+ reg = <0x5e>; > >>+ interrupt-parent = <&gpio1>; > >>+ interrupts = <28 IRQ_TYPE_LEVEL_LOW>; > >>+ interrupt-controller; > >>+ #interrupt-cells = <2>; > >>+ > >>+ regulators { > >>+ compatible = "ti,tps65086-regulator"; > >>+ > >>+ buck1 { > >>+ regulator-name = "vcc1"; > >>+ regulator-min-microvolt = <1600000>; > >>+ regulator-max-microvolt = <1600000>; > >>+ regulator-boot-on; > >>+ ti,regulator-decay; > >>+ ti,regulator-step-size-25mv; > >>+ }; > >>+ }; > >>+ > >>+ gpio4: tps65086_gpio { > >>+ compatible = "ti,tps65086-gpio"; > >>+ gpio-controller; > >>+ #gpio-cells = <2>; > >>+ }; > >>+ }; > > > >[...] > > >
On 10/19/2015 10:21 AM, Lee Jones wrote: > On Mon, 19 Oct 2015, Andrew F. Davis wrote: > >> On 10/19/2015 04:13 AM, Lee Jones wrote: >>> On Fri, 16 Oct 2015, Andrew F. Davis wrote: >>> >>>> The TPS65086 PMIC contains several regulators and a GPO controller. >>>> Add bindings for the TPS65086 PMIC. >>>> >>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>> --- >>>> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ >>>> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ >>>> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ >>> >>> Please split these up into separate patches. >>> >>> There is no functional reason to bundle them up. >>> >> >> ACK >> >>>> 3 files changed, 99 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt >>>> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt >>>> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt >>> >>> [...] >>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt >>>> new file mode 100644 >>>> index 0000000..4b6aeb4 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt >>>> @@ -0,0 +1,46 @@ >>>> +* TPS65086 Power Management Integrated Circuit bindings >>>> + >>>> +Required properties: >>>> + - compatible : Should be "ti,tps65086". >>> >>> Any indication that it's a PMIC? >>> >> >> In the compatible string? > > Ya. > Not sure what you mean then?, no one else seems to be doing that, "xx,xxxxxxx-pmic" is usually used for matching the regulator node, not the device itself. >>>> + - reg : Slave address. >>> >>> I2C/SPI? >>> >> >> ACK >> >>>> + - interrupt-parent : The parent interrupt controller. >>> >>> Phandled to ... >>> >> >> ACK >> >>>> + - interrupts : The interrupt line the device is connected to. >>>> + - interrupt-controller : Marks the device node as an interrupt controller. >>>> + - #interrupt-cells : The number of cells to describe an IRQ, this >>>> + should be 2. The first cell is the IRQ number. >>>> + The second cell is the flags, encoded as the trigger >>>> + masks from ../interrupt-controller/interrupts.txt. >>> >>> Masks? What masks? >>> >>> Best to make a link to the header where the flags are defined here. >>> >> >> ACK >> >>>> +Additional nodes defined in: >>>> + - Regulators : ../regulator/tps65086-regulator.txt. >>>> + - GPIO : ../gpio/gpio-tps65086.txt. >>> >>> I'd suggest removing the full stops from all of the lines above. >>> >>> Just treat them as bullet points like we normally do. >>> >> >> ACK >> >>>> +Example: >>>> + >>>> + pmic: tps65086@5e { >>>> + compatible = "ti,tps65086"; >>>> + reg = <0x5e>; >>>> + interrupt-parent = <&gpio1>; >>>> + interrupts = <28 IRQ_TYPE_LEVEL_LOW>; >>>> + interrupt-controller; >>>> + #interrupt-cells = <2>; >>>> + >>>> + regulators { >>>> + compatible = "ti,tps65086-regulator"; >>>> + >>>> + buck1 { >>>> + regulator-name = "vcc1"; >>>> + regulator-min-microvolt = <1600000>; >>>> + regulator-max-microvolt = <1600000>; >>>> + regulator-boot-on; >>>> + ti,regulator-decay; >>>> + ti,regulator-step-size-25mv; >>>> + }; >>>> + }; >>>> + >>>> + gpio4: tps65086_gpio { >>>> + compatible = "ti,tps65086-gpio"; >>>> + gpio-controller; >>>> + #gpio-cells = <2>; >>>> + }; >>>> + }; >>> >>> [...] >>> >> >
On Mon, 19 Oct 2015, Andrew F. Davis wrote: > On 10/19/2015 10:21 AM, Lee Jones wrote: > >On Mon, 19 Oct 2015, Andrew F. Davis wrote: > > > >>On 10/19/2015 04:13 AM, Lee Jones wrote: > >>>On Fri, 16 Oct 2015, Andrew F. Davis wrote: > >>> > >>>>The TPS65086 PMIC contains several regulators and a GPO controller. > >>>>Add bindings for the TPS65086 PMIC. > >>>> > >>>>Signed-off-by: Andrew F. Davis <afd@ti.com> > >>>>--- > >>>> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ > >>>> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ > >>>> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ > >>> > >>>Please split these up into separate patches. > >>> > >>>There is no functional reason to bundle them up. > >>> > >> > >>ACK > >> > >>>> 3 files changed, 99 insertions(+) > >>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt > >>>> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt > >>>> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt > >>> > >>>[...] > >>> > >>>>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>>>new file mode 100644 > >>>>index 0000000..4b6aeb4 > >>>>--- /dev/null > >>>>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>>>@@ -0,0 +1,46 @@ > >>>>+* TPS65086 Power Management Integrated Circuit bindings > >>>>+ > >>>>+Required properties: > >>>>+ - compatible : Should be "ti,tps65086". > >>> > >>>Any indication that it's a PMIC? > >>> > >> > >>In the compatible string? > > > >Ya. > > > > Not sure what you mean then?, no one else seems to be doing that, > "xx,xxxxxxx-pmic" is usually used for matching the regulator node, > not the device itself. Either the driver is MFD is the PMIC or it's not. If it is, the compatible should reflect that, if isn't not then the description in the header comment and the one above is not correct. IMO, 'pmic' should not be used in the regulator compatible strings, as it's a general description of the overall device. The regulators are just a component of that device. > >>>>+ - reg : Slave address. > >>> > >>>I2C/SPI? > >>> > >> > >>ACK > >> > >>>>+ - interrupt-parent : The parent interrupt controller. > >>> > >>>Phandled to ... > >>> > >> > >>ACK > >> > >>>>+ - interrupts : The interrupt line the device is connected to. > >>>>+ - interrupt-controller : Marks the device node as an interrupt controller. > >>>>+ - #interrupt-cells : The number of cells to describe an IRQ, this > >>>>+ should be 2. The first cell is the IRQ number. > >>>>+ The second cell is the flags, encoded as the trigger > >>>>+ masks from ../interrupt-controller/interrupts.txt. > >>> > >>>Masks? What masks? > >>> > >>>Best to make a link to the header where the flags are defined here. > >>> > >> > >>ACK > >> > >>>>+Additional nodes defined in: > >>>>+ - Regulators : ../regulator/tps65086-regulator.txt. > >>>>+ - GPIO : ../gpio/gpio-tps65086.txt. > >>> > >>>I'd suggest removing the full stops from all of the lines above. > >>> > >>>Just treat them as bullet points like we normally do. > >>> > >> > >>ACK > >> > >>>>+Example: > >>>>+ > >>>>+ pmic: tps65086@5e { > >>>>+ compatible = "ti,tps65086"; > >>>>+ reg = <0x5e>; > >>>>+ interrupt-parent = <&gpio1>; > >>>>+ interrupts = <28 IRQ_TYPE_LEVEL_LOW>; > >>>>+ interrupt-controller; > >>>>+ #interrupt-cells = <2>; > >>>>+ > >>>>+ regulators { > >>>>+ compatible = "ti,tps65086-regulator"; > >>>>+ > >>>>+ buck1 { > >>>>+ regulator-name = "vcc1"; > >>>>+ regulator-min-microvolt = <1600000>; > >>>>+ regulator-max-microvolt = <1600000>; > >>>>+ regulator-boot-on; > >>>>+ ti,regulator-decay; > >>>>+ ti,regulator-step-size-25mv; > >>>>+ }; > >>>>+ }; > >>>>+ > >>>>+ gpio4: tps65086_gpio { > >>>>+ compatible = "ti,tps65086-gpio"; > >>>>+ gpio-controller; > >>>>+ #gpio-cells = <2>; > >>>>+ }; > >>>>+ }; > >>> > >>>[...] > >>> > >> > > >
On 10/20/2015 06:31 AM, Lee Jones wrote: > On Mon, 19 Oct 2015, Andrew F. Davis wrote: > >> On 10/19/2015 10:21 AM, Lee Jones wrote: >>> On Mon, 19 Oct 2015, Andrew F. Davis wrote: >>> >>>> On 10/19/2015 04:13 AM, Lee Jones wrote: >>>>> On Fri, 16 Oct 2015, Andrew F. Davis wrote: >>>>> >>>>>> The TPS65086 PMIC contains several regulators and a GPO controller. >>>>>> Add bindings for the TPS65086 PMIC. >>>>>> >>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com> >>>>>> --- >>>>>> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ >>>>>> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ >>>>>> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ >>>>> >>>>> Please split these up into separate patches. >>>>> >>>>> There is no functional reason to bundle them up. >>>>> >>>> >>>> ACK >>>> >>>>>> 3 files changed, 99 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt >>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt >>>>>> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt >>>>> >>>>> [...] >>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt >>>>>> new file mode 100644 >>>>>> index 0000000..4b6aeb4 >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt >>>>>> @@ -0,0 +1,46 @@ >>>>>> +* TPS65086 Power Management Integrated Circuit bindings >>>>>> + >>>>>> +Required properties: >>>>>> + - compatible : Should be "ti,tps65086". >>>>> >>>>> Any indication that it's a PMIC? >>>>> >>>> >>>> In the compatible string? >>> >>> Ya. >>> >> >> Not sure what you mean then?, no one else seems to be doing that, >> "xx,xxxxxxx-pmic" is usually used for matching the regulator node, >> not the device itself. > > Either the driver is MFD is the PMIC or it's not. > > If it is, the compatible should reflect that, if isn't not then the > description in the header comment and the one above is not correct. > > IMO, 'pmic' should not be used in the regulator compatible strings, as > it's a general description of the overall device. The regulators are > just a component of that device. > I agree about not using compatible in regulator nodes (Mark Brown and I had a discussion on this topic), what I mean with "xxxxxxx-pmic" is for the MFD core to match sub-drivers (mfd_cells). For example drivers/mfd/tps80031.c, matches the regulator with "tps80031-pmic", the clock with "tps80031-clock", the charger with "tps80031-charger", etc.. The core device itself is just matched with "ti,tps65086" through the I2C bus matching. I could change the core to be "tps65086-pmic", then call the regulator driver "tps65086-regulator" if this works for you, this seems to be the way new drivers name the regulator driver (max77843.c). >>>>>> + - reg : Slave address. >>>>> >>>>> I2C/SPI? >>>>> >>>> >>>> ACK >>>> >>>>>> + - interrupt-parent : The parent interrupt controller. >>>>> >>>>> Phandled to ... >>>>> >>>> >>>> ACK >>>> >>>>>> + - interrupts : The interrupt line the device is connected to. >>>>>> + - interrupt-controller : Marks the device node as an interrupt controller. >>>>>> + - #interrupt-cells : The number of cells to describe an IRQ, this >>>>>> + should be 2. The first cell is the IRQ number. >>>>>> + The second cell is the flags, encoded as the trigger >>>>>> + masks from ../interrupt-controller/interrupts.txt. >>>>> >>>>> Masks? What masks? >>>>> >>>>> Best to make a link to the header where the flags are defined here. >>>>> >>>> >>>> ACK >>>> >>>>>> +Additional nodes defined in: >>>>>> + - Regulators : ../regulator/tps65086-regulator.txt. >>>>>> + - GPIO : ../gpio/gpio-tps65086.txt. >>>>> >>>>> I'd suggest removing the full stops from all of the lines above. >>>>> >>>>> Just treat them as bullet points like we normally do. >>>>> >>>> >>>> ACK >>>> >>>>>> +Example: >>>>>> + >>>>>> + pmic: tps65086@5e { >>>>>> + compatible = "ti,tps65086"; >>>>>> + reg = <0x5e>; >>>>>> + interrupt-parent = <&gpio1>; >>>>>> + interrupts = <28 IRQ_TYPE_LEVEL_LOW>; >>>>>> + interrupt-controller; >>>>>> + #interrupt-cells = <2>; >>>>>> + >>>>>> + regulators { >>>>>> + compatible = "ti,tps65086-regulator"; >>>>>> + >>>>>> + buck1 { >>>>>> + regulator-name = "vcc1"; >>>>>> + regulator-min-microvolt = <1600000>; >>>>>> + regulator-max-microvolt = <1600000>; >>>>>> + regulator-boot-on; >>>>>> + ti,regulator-decay; >>>>>> + ti,regulator-step-size-25mv; >>>>>> + }; >>>>>> + }; >>>>>> + >>>>>> + gpio4: tps65086_gpio { >>>>>> + compatible = "ti,tps65086-gpio"; >>>>>> + gpio-controller; >>>>>> + #gpio-cells = <2>; >>>>>> + }; >>>>>> + }; >>>>> >>>>> [...] >>>>> >>>> >>> >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 20 Oct 2015, Andrew F. Davis wrote: > On 10/20/2015 06:31 AM, Lee Jones wrote: > >On Mon, 19 Oct 2015, Andrew F. Davis wrote: > > > >>On 10/19/2015 10:21 AM, Lee Jones wrote: > >>>On Mon, 19 Oct 2015, Andrew F. Davis wrote: > >>> > >>>>On 10/19/2015 04:13 AM, Lee Jones wrote: > >>>>>On Fri, 16 Oct 2015, Andrew F. Davis wrote: > >>>>> > >>>>>>The TPS65086 PMIC contains several regulators and a GPO controller. > >>>>>>Add bindings for the TPS65086 PMIC. > >>>>>> > >>>>>>Signed-off-by: Andrew F. Davis <afd@ti.com> > >>>>>>--- > >>>>>> .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ > >>>>>> Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ > >>>>>> .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ > >>>>> > >>>>>Please split these up into separate patches. > >>>>> > >>>>>There is no functional reason to bundle them up. > >>>>> > >>>> > >>>>ACK > >>>> > >>>>>> 3 files changed, 99 insertions(+) > >>>>>> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt > >>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt > >>>>>> create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt > >>>>> > >>>>>[...] > >>>>> > >>>>>>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>>>>>new file mode 100644 > >>>>>>index 0000000..4b6aeb4 > >>>>>>--- /dev/null > >>>>>>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt > >>>>>>@@ -0,0 +1,46 @@ > >>>>>>+* TPS65086 Power Management Integrated Circuit bindings > >>>>>>+ > >>>>>>+Required properties: > >>>>>>+ - compatible : Should be "ti,tps65086". > >>>>> > >>>>>Any indication that it's a PMIC? > >>>>> > >>>> > >>>>In the compatible string? > >>> > >>>Ya. > >>> > >> > >>Not sure what you mean then?, no one else seems to be doing that, > >>"xx,xxxxxxx-pmic" is usually used for matching the regulator node, > >>not the device itself. > > > >Either the driver is MFD is the PMIC or it's not. > > > >If it is, the compatible should reflect that, if isn't not then the > >description in the header comment and the one above is not correct. > > > >IMO, 'pmic' should not be used in the regulator compatible strings, as > >it's a general description of the overall device. The regulators are > >just a component of that device. > > > > I agree about not using compatible in regulator nodes (Mark Brown and I had > a discussion on this topic), what I mean with "xxxxxxx-pmic" is for the MFD > core to match sub-drivers (mfd_cells). For example drivers/mfd/tps80031.c, > matches the regulator with "tps80031-pmic", the clock with "tps80031-clock", > the charger with "tps80031-charger", etc.. > > The core device itself is just matched with "ti,tps65086" through the I2C bus > matching. > > I could change the core to be "tps65086-pmic", then call the regulator > driver "tps65086-regulator" if this works for you, this seems to be the > way new drivers name the regulator driver (max77843.c). Yes, this is what I was thinking of. ti,tps65086 is uninformative/undescriptive and tells me nothing. [...]
On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote: > On Tue, 20 Oct 2015, Andrew F. Davis wrote: > > I could change the core to be "tps65086-pmic", then call the regulator > > driver "tps65086-regulator" if this works for you, this seems to be the > > way new drivers name the regulator driver (max77843.c). > Yes, this is what I was thinking of. > ti,tps65086 is uninformative/undescriptive and tells me nothing. It is however the normal way we write compatible strings - the class information would normaly go in the node name (ie, i2c@7000c000 or whatever).
On Wed, 21 Oct 2015, Mark Brown wrote: > On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote: > > On Tue, 20 Oct 2015, Andrew F. Davis wrote: > > > > I could change the core to be "tps65086-pmic", then call the regulator > > > driver "tps65086-regulator" if this works for you, this seems to be the > > > way new drivers name the regulator driver (max77843.c). > > > Yes, this is what I was thinking of. > > > ti,tps65086 is uninformative/undescriptive and tells me nothing. > > It is however the normal way we write compatible strings - the class > information would normaly go in the node name (ie, i2c@7000c000 or > whatever). I didn't say it hasn't been done before, just that I didn't like it for the aforementioned reasons. I can also find 1000's of compatible strings which do append "-<device_type>", so it's not exactly an unheard of practice.
On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote: > On Wed, 21 Oct 2015, Mark Brown wrote: > > On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote: > > > On Tue, 20 Oct 2015, Andrew F. Davis wrote: > > It is however the normal way we write compatible strings - the class > > information would normaly go in the node name (ie, i2c@7000c000 or > > whatever). > I didn't say it hasn't been done before, just that I didn't like it > for the aforementioned reasons. I can also find 1000's of compatible > strings which do append "-<device_type>", so it's not exactly an > unheard of practice. It's a pretty substantial change in the way we make compatible strings that we probably want to discuss more widely if we want to adopt it - we've not been using that idiom and it's pretty surprising. I'm not really sure it help much and we do already have the pre-@ noise words for this purpose (as well as comments in the DT).
On Wed, 21 Oct 2015, Mark Brown wrote: > On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote: > > On Wed, 21 Oct 2015, Mark Brown wrote: > > > On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote: > > > > On Tue, 20 Oct 2015, Andrew F. Davis wrote: > > > > It is however the normal way we write compatible strings - the class > > > information would normaly go in the node name (ie, i2c@7000c000 or > > > whatever). > > > I didn't say it hasn't been done before, just that I didn't like it > > for the aforementioned reasons. I can also find 1000's of compatible > > strings which do append "-<device_type>", so it's not exactly an > > unheard of practice. > > It's a pretty substantial change in the way we make compatible strings > that we probably want to discuss more widely if we want to adopt it - > we've not been using that idiom and it's pretty surprising. I'm not > really sure it help much and we do already have the pre-@ noise words > for this purpose (as well as comments in the DT). I'm not *that* fussed about it to justify starting-up wider community discussions. My only point is that: compatible = "<vendor>,udw9283"; ... is meaningless gibberish and I think it'd be better to be more forthcoming which prevents having to dig around in DTS files for the node name/label for true device/type identification.
On Wed, Oct 21, 2015 at 04:26:59PM +0100, Lee Jones wrote: > On Wed, 21 Oct 2015, Mark Brown wrote: > > On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote: > > > I didn't say it hasn't been done before, just that I didn't like it > > > for the aforementioned reasons. I can also find 1000's of compatible > > > strings which do append "-<device_type>", so it's not exactly an > > > unheard of practice. > > It's a pretty substantial change in the way we make compatible strings > > that we probably want to discuss more widely if we want to adopt it - > > we've not been using that idiom and it's pretty surprising. I'm not > > really sure it help much and we do already have the pre-@ noise words > > for this purpose (as well as comments in the DT). > I'm not *that* fussed about it to justify starting-up wider community > discussions. > My only point is that: > compatible = "<vendor>,udw9283"; > ... is meaningless gibberish and I think it'd be better to be more > forthcoming which prevents having to dig around in DTS files for the > node name/label for true device/type identification. What I suspect most people actually do here is google the part name if it's not obvious from context (presumably people with a compatible string only are either looking at the driver or some DT source instantiating it).
diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt new file mode 100644 index 0000000..15e4063 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt @@ -0,0 +1,17 @@ +* TPS65086 GPO Controller bindings + +Required properties: + - compatible : Should be "ti,tps65086-gpio". + - gpio-controller : Marks the device node as a GPIO Controller. + - #gpio-cells : Should be two. The first cell is the pin number + and the second cell is used to specify flags. + See include/dt-bindings/gpio/gpio.h for possible + values. + +Example: + + gpio4: tps65086_gpio { + compatible = "ti,tps65086-gpio"; + gpio-controller; + #gpio-cells = <2>; + }; diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt new file mode 100644 index 0000000..4b6aeb4 --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt @@ -0,0 +1,46 @@ +* TPS65086 Power Management Integrated Circuit bindings + +Required properties: + - compatible : Should be "ti,tps65086". + - reg : Slave address. + - interrupt-parent : The parent interrupt controller. + - interrupts : The interrupt line the device is connected to. + - interrupt-controller : Marks the device node as an interrupt controller. + - #interrupt-cells : The number of cells to describe an IRQ, this + should be 2. The first cell is the IRQ number. + The second cell is the flags, encoded as the trigger + masks from ../interrupt-controller/interrupts.txt. + +Additional nodes defined in: + - Regulators : ../regulator/tps65086-regulator.txt. + - GPIO : ../gpio/gpio-tps65086.txt. + +Example: + + pmic: tps65086@5e { + compatible = "ti,tps65086"; + reg = <0x5e>; + interrupt-parent = <&gpio1>; + interrupts = <28 IRQ_TYPE_LEVEL_LOW>; + interrupt-controller; + #interrupt-cells = <2>; + + regulators { + compatible = "ti,tps65086-regulator"; + + buck1 { + regulator-name = "vcc1"; + regulator-min-microvolt = <1600000>; + regulator-max-microvolt = <1600000>; + regulator-boot-on; + ti,regulator-decay; + ti,regulator-step-size-25mv; + }; + }; + + gpio4: tps65086_gpio { + compatible = "ti,tps65086-gpio"; + gpio-controller; + #gpio-cells = <2>; + }; + }; diff --git a/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt b/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt new file mode 100644 index 0000000..023cd73 --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt @@ -0,0 +1,36 @@ +* TPS65086 regulator bindings + +Required properties: + - compatible : Should be "ti,tps65086-regulator". + - list of regulators provided by this controller, must be named + after their hardware counterparts: buck[1-6], ldoa[1-3], swa1, + swb[1-2], and vtt. + +Optional properties: + - Per-regulator optional properties are defined in regulator.txt + - ti,regulator-step-size-25mv : This is applicable for buck[1,2,6]. + Set this if the regulator is factory + set with a 25mv step voltage mapping. + - ti,regulator-decay : This is applicable for buck[1-6]. + Set this if the output needs to decay, + default is for the output to slew down. + +Example: + + regulators { + compatible = "ti,tps65086-regulator"; + + buck1 { + regulator-name = "vcc1"; + regulator-min-microvolt = <1600000>; + regulator-max-microvolt = <1600000>; + regulator-boot-on; + ti,regulator-step-size-25mv; + ti,regulator-decay; + }; + + swa1 { + regulator-name = "ls1"; + regulator-boot-on; + }; + };
The TPS65086 PMIC contains several regulators and a GPO controller. Add bindings for the TPS65086 PMIC. Signed-off-by: Andrew F. Davis <afd@ti.com> --- .../devicetree/bindings/gpio/gpio-tps65086.txt | 17 ++++++++ Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++ .../bindings/regulator/tps65086-regulator.txt | 36 +++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt