| Message ID | 1363761714-15034-4-git-send-email-avinashphilip@ti.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 3/20/2013 12:11 PM, Philip Avinash wrote: > Add da850 EHRPWM & ECAP DT node. > Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP > clock. > > Signed-off-by: Philip Avinash <avinashphilip@ti.com> > --- > Changes since v1: > - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are > same with am33xx platform and da850 has no platform specific > dependency. Which is fine, but I think the binding documentation still needs to be updated to document the ti,da850-ehrpwm binding. Looping Peter (it is always a good idea to CC folks who reviewed your patch last time around). Also, please Cc the DT folks and devicetree-discuss list too for their opinion. Thanks, Sekhar > > :100644 100644 3ec1bda... 62fd2d4... M arch/arm/boot/dts/da850.dtsi > :100644 100644 6b7a0a2... 89ee974... M arch/arm/mach-davinci/da8xx-dt.c > arch/arm/boot/dts/da850.dtsi | 30 ++++++++++++++++++++++++++++++ > arch/arm/mach-davinci/da8xx-dt.c | 5 +++++ > 2 files changed, 35 insertions(+) > > diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi > index 3ec1bda..62fd2d4 100644 > --- a/arch/arm/boot/dts/da850.dtsi > +++ b/arch/arm/boot/dts/da850.dtsi > @@ -107,6 +107,36 @@ > reg = <0x21000 0x1000>; > status = "disabled"; > }; > + ehrpwm0: ehrpwm@01f00000 { > + compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm"; > + #pwm-cells = <3>; > + reg = <0x300000 0x2000>; > + status = "disabled"; > + }; > + ehrpwm1: ehrpwm@01f02000 { > + compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm"; > + #pwm-cells = <3>; > + reg = <0x302000 0x2000>; > + status = "disabled"; > + }; > + ecap0: ecap@01f06000 { > + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; > + #pwm-cells = <3>; > + reg = <0x306000 0x80>; > + status = "disabled"; > + }; > + ecap1: ecap@01f07000 { > + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; > + #pwm-cells = <3>; > + reg = <0x307000 0x80>; > + status = "disabled"; > + }; > + ecap2: ecap@01f08000 { > + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; > + #pwm-cells = <3>; > + reg = <0x308000 0x80>; > + status = "disabled"; > + }; > }; > nand_cs3@62000000 { > compatible = "ti,davinci-nand"; > diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c > index 6b7a0a2..89ee974 100644 > --- a/arch/arm/mach-davinci/da8xx-dt.c > +++ b/arch/arm/mach-davinci/da8xx-dt.c > @@ -40,6 +40,11 @@ static void __init da8xx_init_irq(void) > struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { > OF_DEV_AUXDATA("ti,davinci-i2c", 0x01c22000, "i2c_davinci.1", NULL), > OF_DEV_AUXDATA("ti,davinci-wdt", 0x01c21000, "watchdog", NULL), > + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm", NULL), > + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm", NULL), > + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f06000, "ecap", NULL), > + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f07000, "ecap", NULL), > + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f08000, "ecap", NULL), > {} > }; > >
>>>>> "Sekhar" == Sekhar Nori <nsekhar@ti.com> writes: Sekhar> On 3/20/2013 12:11 PM, Philip Avinash wrote: >> Add da850 EHRPWM & ECAP DT node. >> Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP >> clock. >> >> Signed-off-by: Philip Avinash <avinashphilip@ti.com> >> --- >> Changes since v1: >> - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are >> same with am33xx platform and da850 has no platform specific >> dependency. Sekhar> Which is fine, but I think the binding documentation still needs to be Sekhar> updated to document the ti,da850-ehrpwm binding. Looping Peter (it is Sekhar> always a good idea to CC folks who reviewed your patch last time Sekhar> around). Also, please Cc the DT folks and devicetree-discuss list too Sekhar> for their opinion. Yes, thanks for CC'ing me. I also think da850-* should be documented. See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for an similar (old) example.
On Wed, Mar 20, 2013 at 18:17:59, Peter Korsgaard wrote: > >>>>> "Sekhar" == Sekhar Nori <nsekhar@ti.com> writes: > > Sekhar> On 3/20/2013 12:11 PM, Philip Avinash wrote: > >> Add da850 EHRPWM & ECAP DT node. > >> Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP > >> clock. > >> > >> Signed-off-by: Philip Avinash <avinashphilip@ti.com> > >> --- > >> Changes since v1: > >> - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are > >> same with am33xx platform and da850 has no platform specific > >> dependency. > > Sekhar> Which is fine, but I think the binding documentation still needs to be > Sekhar> updated to document the ti,da850-ehrpwm binding. Looping Peter (it is > Sekhar> always a good idea to CC folks who reviewed your patch last time > Sekhar> around). Also, please Cc the DT folks and devicetree-discuss list too > Sekhar> for their opinion. > > Yes, thanks for CC'ing me. I also think da850-* should be > documented. See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for > an similar (old) example. Peter, In this binding file also, only initial compatible field is updated. Later on many compatible were added in driver. But not update back to binding doc. <Documentation/devicetree/bindings/gpio/8xxx_gpio.txt> --- followed by "fsl,mpc8349-gpio" for 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. --- <drivers/gpio/gpio-mpc8xxx.c> --- static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { { .compatible = "fsl,mpc8349-gpio", }, { .compatible = "fsl,mpc8572-gpio", }, { .compatible = "fsl,mpc8610-gpio", }, { .compatible = "fsl,mpc5121-gpio", .data = mpc512x_irq_set_type, }, { .compatible = "fsl,pq3-gpio", }, { .compatible = "fsl,qoriq-gpio", }, {} }; --- Grant/Rob, With respect peters explanation (below), what is your opinion on adding information to binding doc for da850 support? Peter --> if the hardware block is identical the dts should simply list: Peter --> compatible = "ti,da850-ecap", "ti,am33xx-ecap" Peter --> And the driver only bind to ti,am33xx-ecap (unless there ever needs to Peter --> be a da850 specific workaround. Or Should I update both binding doc & the driver to use new compatible option "ti,da830-ecap" (as da830 platform has initial IP support)? Even with this, platforms da830, da850 and am33xx can reuse "ti,da830-ecap" compatible field. Thanks Avinash > > -- > Bye, Peter Korsgaard > _______________________________________________ > Davinci-linux-open-source mailing list > Davinci-linux-open-source@linux.davincidsp.com > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source >
On 3/21/2013 1:31 PM, Philip, Avinash wrote: > On Wed, Mar 20, 2013 at 18:17:59, Peter Korsgaard wrote: >>>>>>> "Sekhar" == Sekhar Nori <nsekhar@ti.com> writes: >> >> Sekhar> On 3/20/2013 12:11 PM, Philip Avinash wrote: >> >> Add da850 EHRPWM & ECAP DT node. >> >> Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP >> >> clock. >> >> >> >> Signed-off-by: Philip Avinash <avinashphilip@ti.com> >> >> --- >> >> Changes since v1: >> >> - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are >> >> same with am33xx platform and da850 has no platform specific >> >> dependency. >> >> Sekhar> Which is fine, but I think the binding documentation still needs to be >> Sekhar> updated to document the ti,da850-ehrpwm binding. Looping Peter (it is >> Sekhar> always a good idea to CC folks who reviewed your patch last time >> Sekhar> around). Also, please Cc the DT folks and devicetree-discuss list too >> Sekhar> for their opinion. >> >> Yes, thanks for CC'ing me. I also think da850-* should be >> documented. See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for >> an similar (old) example. > > Peter, > > In this binding file also, only initial compatible field is updated. Later on many > compatible were added in driver. But not update back to binding doc. Probably someone forgot to keep updating the binding doc after a point. > > <Documentation/devicetree/bindings/gpio/8xxx_gpio.txt> > --- > followed by "fsl,mpc8349-gpio" for 83xx, "fsl,mpc8572-gpio" for 85xx and > "fsl,mpc8610-gpio" for 86xx. > --- > <drivers/gpio/gpio-mpc8xxx.c> > --- > static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { > { .compatible = "fsl,mpc8349-gpio", }, > { .compatible = "fsl,mpc8572-gpio", }, > { .compatible = "fsl,mpc8610-gpio", }, > { .compatible = "fsl,mpc5121-gpio", .data = mpc512x_irq_set_type, }, > { .compatible = "fsl,pq3-gpio", }, > { .compatible = "fsl,qoriq-gpio", }, > {} > }; > --- > > Grant/Rob, > With respect peters explanation (below), what is your opinion on adding information to > binding doc for da850 support? > > Peter --> if the hardware block is identical the dts should simply list: > Peter --> compatible = "ti,da850-ecap", "ti,am33xx-ecap" > Peter --> And the driver only bind to ti,am33xx-ecap (unless there ever needs to > Peter --> be a da850 specific workaround. > > Or > Should I update both binding doc & the driver to use new compatible option "ti,da830-ecap" > (as da830 platform has initial IP support)? > Even with this, platforms da830, da850 and am33xx can reuse "ti,da830-ecap" compatible field. To me updating the driver to keep adding a .compatible even when not using it elsewhere in code is not required. Adding the new binding in .dts file is must since you may need to do something specific to da830 at a later time (and the .dtb should be considered ROM'ed so you wont be able to change it then). Adding documentation for the binding is also required to help anyone wanting to know more about the binding after reading the .dts file. Thanks, Sekhar
On Fri, Mar 22, 2013 at 11:23:32, Nori, Sekhar wrote: > On 3/21/2013 1:31 PM, Philip, Avinash wrote: > > On Wed, Mar 20, 2013 at 18:17:59, Peter Korsgaard wrote: > >>>>>>> "Sekhar" == Sekhar Nori <nsekhar@ti.com> writes: > >> > >> Sekhar> On 3/20/2013 12:11 PM, Philip Avinash wrote: > >> >> Add da850 EHRPWM & ECAP DT node. > >> >> Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP > >> >> clock. > >> >> > >> >> Signed-off-by: Philip Avinash <avinashphilip@ti.com> > >> >> --- > >> >> Changes since v1: > >> >> - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are > >> >> same with am33xx platform and da850 has no platform specific > >> >> dependency. > >> > >> Sekhar> Which is fine, but I think the binding documentation still needs to be > >> Sekhar> updated to document the ti,da850-ehrpwm binding. Looping Peter (it is > >> Sekhar> always a good idea to CC folks who reviewed your patch last time > >> Sekhar> around). Also, please Cc the DT folks and devicetree-discuss list too > >> Sekhar> for their opinion. > >> > >> Yes, thanks for CC'ing me. I also think da850-* should be > >> documented. See Documentation/devicetree/bindings/gpio/8xxx_gpio.txt for > >> an similar (old) example. > > > > Peter, > > > > In this binding file also, only initial compatible field is updated. Later on many > > compatible were added in driver. But not update back to binding doc. > > Probably someone forgot to keep updating the binding doc after a point. > > > > > <Documentation/devicetree/bindings/gpio/8xxx_gpio.txt> > > --- > > followed by "fsl,mpc8349-gpio" for 83xx, "fsl,mpc8572-gpio" for 85xx and > > "fsl,mpc8610-gpio" for 86xx. > > --- > > <drivers/gpio/gpio-mpc8xxx.c> > > --- > > static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { > > { .compatible = "fsl,mpc8349-gpio", }, > > { .compatible = "fsl,mpc8572-gpio", }, > > { .compatible = "fsl,mpc8610-gpio", }, > > { .compatible = "fsl,mpc5121-gpio", .data = mpc512x_irq_set_type, }, > > { .compatible = "fsl,pq3-gpio", }, > > { .compatible = "fsl,qoriq-gpio", }, > > {} > > }; > > --- > > > > Grant/Rob, > > With respect peters explanation (below), what is your opinion on adding information to > > binding doc for da850 support? > > > > Peter --> if the hardware block is identical the dts should simply list: > > Peter --> compatible = "ti,da850-ecap", "ti,am33xx-ecap" > > Peter --> And the driver only bind to ti,am33xx-ecap (unless there ever needs to > > Peter --> be a da850 specific workaround. > > > > Or > > Should I update both binding doc & the driver to use new compatible option "ti,da830-ecap" > > (as da830 platform has initial IP support)? > > Even with this, platforms da830, da850 and am33xx can reuse "ti,da830-ecap" compatible field. > > To me updating the driver to keep adding a .compatible even when not > using it elsewhere in code is not required. Adding the new binding in > .dts file is must since you may need to do something specific to da830 > at a later time (and the .dtb should be considered ROM'ed so you wont be > able to change it then). Thanks for the explanation. I will add "ti,da850-ecap" for da850.dtsi along with "ti,am33xx-ecap" as compatible fields. > Adding documentation for the binding is also > required to help anyone wanting to know more about the binding after > reading the .dts file. I will adding documentation part for "ti,da850-ecap". Thanks Avinash > > Thanks, > Sekhar >
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi index 3ec1bda..62fd2d4 100644 --- a/arch/arm/boot/dts/da850.dtsi +++ b/arch/arm/boot/dts/da850.dtsi @@ -107,6 +107,36 @@ reg = <0x21000 0x1000>; status = "disabled"; }; + ehrpwm0: ehrpwm@01f00000 { + compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm"; + #pwm-cells = <3>; + reg = <0x300000 0x2000>; + status = "disabled"; + }; + ehrpwm1: ehrpwm@01f02000 { + compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm"; + #pwm-cells = <3>; + reg = <0x302000 0x2000>; + status = "disabled"; + }; + ecap0: ecap@01f06000 { + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; + #pwm-cells = <3>; + reg = <0x306000 0x80>; + status = "disabled"; + }; + ecap1: ecap@01f07000 { + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; + #pwm-cells = <3>; + reg = <0x307000 0x80>; + status = "disabled"; + }; + ecap2: ecap@01f08000 { + compatible = "ti,da850-ecap", "ti,am33xx-ecap"; + #pwm-cells = <3>; + reg = <0x308000 0x80>; + status = "disabled"; + }; }; nand_cs3@62000000 { compatible = "ti,davinci-nand"; diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c index 6b7a0a2..89ee974 100644 --- a/arch/arm/mach-davinci/da8xx-dt.c +++ b/arch/arm/mach-davinci/da8xx-dt.c @@ -40,6 +40,11 @@ static void __init da8xx_init_irq(void) struct of_dev_auxdata da850_auxdata_lookup[] __initdata = { OF_DEV_AUXDATA("ti,davinci-i2c", 0x01c22000, "i2c_davinci.1", NULL), OF_DEV_AUXDATA("ti,davinci-wdt", 0x01c21000, "watchdog", NULL), + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f00000, "ehrpwm", NULL), + OF_DEV_AUXDATA("ti,da850-ehrpwm", 0x01f02000, "ehrpwm", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f06000, "ecap", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f07000, "ecap", NULL), + OF_DEV_AUXDATA("ti,da850-ecap", 0x01f08000, "ecap", NULL), {} };
Add da850 EHRPWM & ECAP DT node. Also adds OF_DEV_AUXDATA for EHRPWM & ECAP driver to use EHRPWM & ECAP clock. Signed-off-by: Philip Avinash <avinashphilip@ti.com> --- Changes since v1: - Reusing ti,am33xx<ecap/ehrpwm> as compatible field as both IP's are same with am33xx platform and da850 has no platform specific dependency. :100644 100644 3ec1bda... 62fd2d4... M arch/arm/boot/dts/da850.dtsi :100644 100644 6b7a0a2... 89ee974... M arch/arm/mach-davinci/da8xx-dt.c arch/arm/boot/dts/da850.dtsi | 30 ++++++++++++++++++++++++++++++ arch/arm/mach-davinci/da8xx-dt.c | 5 +++++ 2 files changed, 35 insertions(+)