On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
> Add support for device-tree binding and of_xlate for EHRWPM driver.
> of_xlate provides EHRPWM polarity configuration from client driver
> device-tree.
> Also size of pwm-cells set to 3 to support pwm channel number, pwm
> period & polarity configuration from device tree.
Oh, I forgot to mention this in my reply to the previous patch, but you
should consistently use PWM to refer to PWM devices, so the above should
be "PWM channel number" and "PWM period & polarity". And the property is
named "#pwm-cells".
> Signed-off-by: Philip, Avinash <[email protected]>
> ---
> :000000 100644 0000000... 05d9d63... A Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> :100644 100644 caf00fe... ae23c2b... M drivers/pwm/pwm-tiehrpwm.c
> .../devicetree/bindings/pwm/pwm-tiehrpwm.txt | 26 +++++
> drivers/pwm/pwm-tiehrpwm.c | 107 ++++++++++++++++++++
> 2 files changed, 133 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> new file mode 100644
> index 0000000..05d9d63
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> @@ -0,0 +1,26 @@
> +TI SOC EHRPWM based PWM controller
> +
> +Required properties:
> +- compatible : Must be "ti,am33xx-ehrpwm"
> +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> + First cell specifies the per-chip index of the PWM to use, the second
> + cell is the period cycle in nanoseconds and the third cell is the
"period cycle" doesn't make much sense. It should be "period" only. This
also applies to the previous patch.
> + polarity of PWM output. Polarity 0 gives normal polarity and 1 gives
> + inversed polarity (inverse duty cycle)
I don't think "inversed" exists. It should be either "inverted polarity"
or "inverse polarity".
> +- reg: physical base address and size of the registers map. For am33xx,
> + 2 register maps are present (EHRPWM register space & PWM subsystem common
> + config space). Order should be maintained with EHRPWM register map as first
> + entry & PWM subsystem common config space as second entry.
> +
> +Optional properties:
> +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> + "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
I don't see where this property is used. There is no code in this patch
that parses it.
> +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> + const struct of_phandle_args *args)
> +{
> + struct pwm_device *pwm;
> +
> + if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> + return ERR_PTR(-EINVAL);
> +
> + if (args->args[0] >= chip->npwm)
> + return ERR_PTR(-EINVAL);
> +
> + pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(pwm))
> + return pwm;
> +
> + pwm_set_period(pwm, args->args[1]);
> + pwm_set_polarity(pwm, args->args[2]);
> + return pwm;
> +}
This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
make this part of the PWM core. If so it is probably safer to define the
values for the third cell as flags, where the polarity is encoded in bit
0, and make the function handle this accordingly to allow other bits to
be added in the future.
The same comments as for patch 1 apply to the rest of this patch as
well.
Thierry
On Tue, Oct 02, 2012 at 11:41:43, Thierry Reding wrote:
> On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
> > Add support for device-tree binding and of_xlate for EHRWPM driver.
> > of_xlate provides EHRPWM polarity configuration from client driver
> > device-tree.
> > Also size of pwm-cells set to 3 to support pwm channel number, pwm
> > period & polarity configuration from device tree.
>
> Oh, I forgot to mention this in my reply to the previous patch, but you
> should consistently use PWM to refer to PWM devices, so the above should
> be "PWM channel number" and "PWM period & polarity". And the property is
> named "#pwm-cells".
I will correct it.
>
> > Signed-off-by: Philip, Avinash <[email protected]>
> > ---
> > :000000 100644 0000000... 05d9d63... A Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > :100644 100644 caf00fe... ae23c2b... M drivers/pwm/pwm-tiehrpwm.c
> > .../devicetree/bindings/pwm/pwm-tiehrpwm.txt | 26 +++++
> > drivers/pwm/pwm-tiehrpwm.c | 107 ++++++++++++++++++++
> > 2 files changed, 133 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > new file mode 100644
> > index 0000000..05d9d63
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> > @@ -0,0 +1,26 @@
> > +TI SOC EHRPWM based PWM controller
> > +
> > +Required properties:
> > +- compatible : Must be "ti,am33xx-ehrpwm"
> > +- #pwm-cells: Should be 3. Number of cells being used to specify PWM property.
> > + First cell specifies the per-chip index of the PWM to use, the second
> > + cell is the period cycle in nanoseconds and the third cell is the
>
> "period cycle" doesn't make much sense. It should be "period" only. This
> also applies to the previous patch.
I will correct it.
>
> > + polarity of PWM output. Polarity 0 gives normal polarity and 1 gives
> > + inversed polarity (inverse duty cycle)
>
> I don't think "inversed" exists. It should be either "inverted polarity"
> or "inverse polarity".
>
> > +- reg: physical base address and size of the registers map. For am33xx,
> > + 2 register maps are present (EHRPWM register space & PWM subsystem common
> > + config space). Order should be maintained with EHRPWM register map as first
> > + entry & PWM subsystem common config space as second entry.
> > +
> > +Optional properties:
> > +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> > + "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
>
> I don't see where this property is used. There is no code in this patch
> that parses it.
This data used by omap_hwmod layer to create platform devices. This is part
of omap hwmod implementation.
>
> > +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> > + const struct of_phandle_args *args)
> > +{
> > + struct pwm_device *pwm;
> > +
> > + if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> > + return ERR_PTR(-EINVAL);
> > +
> > + if (args->args[0] >= chip->npwm)
> > + return ERR_PTR(-EINVAL);
> > +
> > + pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(pwm))
> > + return pwm;
> > +
> > + pwm_set_period(pwm, args->args[1]);
> > + pwm_set_polarity(pwm, args->args[2]);
> > + return pwm;
> > +}
>
> This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
> make this part of the PWM core. If so it is probably safer to define the
> values for the third cell as flags, where the polarity is encoded in bit
> 0, and make the function handle this accordingly to allow other bits to
> be added in the future.
Custom of_xlate support is provided as suggested while the discussion of
"Adding support for configuring polarity in PWM framework".
https://lkml.org/lkml/2012/7/16/177
without custom of_xlate() support, PWM drivers has to populate
chip->of_pwm_n_cells = x;
as this is hard coded to 2 in pwm/core.c.
if (!chip->of_xlate) {
chip->of_xlate = of_pwm_simple_xlate;
chip->of_pwm_n_cells = 2;
Thanks
Avinash
>
> The same comments as for patch 1 apply to the rest of this patch as
> well.
>
> Thierry
>
On Mon, Oct 08, 2012 at 01:31:20PM +0000, Philip, Avinash wrote:
> On Tue, Oct 02, 2012 at 11:41:43, Thierry Reding wrote:
> > On Wed, Sep 26, 2012 at 04:57:43PM +0530, Philip, Avinash wrote:
[...]
> > > +- reg: physical base address and size of the registers map. For am33xx,
> > > + 2 register maps are present (EHRPWM register space & PWM subsystem common
> > > + config space). Order should be maintained with EHRPWM register map as first
> > > + entry & PWM subsystem common config space as second entry.
> > > +
> > > +Optional properties:
> > > +- ti,hwmods: Name of the hwmod associated to the EHRPWM:
> > > + "ehrpwm<x>", <x> being the 0-based instance number from the HW spec
> >
> > I don't see where this property is used. There is no code in this patch
> > that parses it.
>
> This data used by omap_hwmod layer to create platform devices. This is part
> of omap hwmod implementation.
Okay. I've heard about hwmod but I wasn't aware of how it was used in
the context of device tree.
> > > +static struct pwm_device *of_ehrpwm_xlate(struct pwm_chip *chip,
> > > + const struct of_phandle_args *args)
> > > +{
> > > + struct pwm_device *pwm;
> > > +
> > > + if (chip->of_pwm_n_cells < PWM_CELL_SIZE)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (args->args[0] >= chip->npwm)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > > + if (IS_ERR(pwm))
> > > + return pwm;
> > > +
> > > + pwm_set_period(pwm, args->args[1]);
> > > + pwm_set_polarity(pwm, args->args[2]);
> > > + return pwm;
> > > +}
> >
> > This is an exact duplicate of the ECAP's of_xlate(). Maybe we should
> > make this part of the PWM core. If so it is probably safer to define the
> > values for the third cell as flags, where the polarity is encoded in bit
> > 0, and make the function handle this accordingly to allow other bits to
> > be added in the future.
>
> Custom of_xlate support is provided as suggested while the discussion of
> "Adding support for configuring polarity in PWM framework".
> https://lkml.org/lkml/2012/7/16/177
>
> without custom of_xlate() support, PWM drivers has to populate
> chip->of_pwm_n_cells = x;
> as this is hard coded to 2 in pwm/core.c.
>
> if (!chip->of_xlate) {
> chip->of_xlate = of_pwm_simple_xlate;
> chip->of_pwm_n_cells = 2;
It's absolutely fine to provide a custom implementation. All I'm saying
is that we should add a 3-cell variant of of_pwm_simple_xlate() instead
of having to duplicate it for every chip that supports inversion of the
polarity.
Maybe something like of_pwm_xlate_with_flags()?
Thierry