[PATCH v2 3/3] ARM: davinci: da850: add EHRPWM & ECAP DT node

[Sekhar Nori]

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 at 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 at 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