Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Thierry Reding 
On Fri, Nov 09, 2012 at 10:59:30AM +, Philip, Avinash wrote:
> On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
> > On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> > > +#define ECAPCLK_EN   BIT(0)
> > > +#define ECAPCLK_STOP_REQ BIT(1)
> > 
> > This one doesn't seem to align with the rest. Also, why is bit 0 called
> > _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
> > _START and _STOP? Or _ENABLE and _DISABLE?
> 
> Ok I will change to PWMSS_ECAPCLK_EN & PWMSS_ECAPCLK_STPO_REQ

While at it, maybe move these defines to the pwm-tipwmss.h header file?
After all that's the module that accesses the register that contains
these bits.



pgpAYVt81lNu2.pgp
Description: PGP signature

RE: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Philip, Avinash 
On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> > This patch
> > 1. Add support for device-tree binding for ECAP APWM driver.
> > 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
> >period & polarity configuration from device tree.
> > 3. Add enable/disable clock gating in PWM subsystem common config space.
> > 4. When here set .owner member in platform_driver structure to
> >THIS_MODULE.
> > 
> > Signed-off-by: Philip, Avinash 
> > Cc: Grant Likely 
> > Cc: Rob Herring 
> > Cc: Rob Landley 
> > ---
> > Changes since v1:
> > - Add separate patch for pinctrl support
> > - Add conditional check for PWM subsystem clock enable.
> > - Combined with HWMOD changes & DT bindings.
> > - Remove the custom of xlate support.
> > 
> > :00 100644 000... fe24cac... A  
> > Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > :100644 100644 d6d4cf0... 0d43266... M  drivers/pwm/pwm-tiecap.c
> >  .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
> >  drivers/pwm/pwm-tiecap.c   |   48 
> > +++-
> >  2 files changed, 69 insertions(+), 1 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
> > b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > new file mode 100644
> > index 000..fe24cac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> > @@ -0,0 +1,22 @@
> > +TI SOC ECAP based APWM controller
> > +
> > +Required properties:
> > +- compatible: Must be "ti,am33xx-ecap"
> > +- #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 bit 0 in the third cell is
> 
> I think this should be "period in nanoseconds". I haven't heard "period
> cycle" before.

Ok

> 
> > +  used to encode the polarity of PWM output.
> 
> Maybe you should explicitly say how this is encoded.

Ok I will add details

> 
...
> >  
> > +#define ECAPCLK_EN BIT(0)
> > +#define ECAPCLK_STOP_REQ   BIT(1)
> 
> This one doesn't seem to align with the rest. Also, why is bit 0 called
> _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
> _START and _STOP? Or _ENABLE and _DISABLE?

Ok I will change to PWMSS_ECAPCLK_EN & PWMSS_ECAPCLK_STPO_REQ

> 
> > +
> > +#define ECAPCLK_EN_ACK BIT(0)
> > +
> > +#define PWM_CELL_SIZE  3
> 
> You don't need a define for this.

I remove.

> 
> > +
> >  struct ecap_pwm_chip {
> > struct pwm_chip chip;
> > unsigned intclk_rate;
> > @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
> > .owner  = THIS_MODULE,
> >  };
> >  
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id ecap_of_match[] = {
> > +   {
> > +   .compatible = "ti,am33xx-ecap",
> > +   },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, ecap_of_match);
> > +#endif
> > +
> 
> I'm not sure if I remember correctly, but wasn't AM33xx support supposed
> to be DT only? In that case you don't need the CONFIG_OF guards.

I will remove

> 
...
> > pm_runtime_enable(>dev);
> > +   pm_runtime_get_sync(>dev);
> 
> Maybe put a blank line after this for readability.

Ok

> 
> > +   if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
> > +   ECAPCLK_EN_ACK)) {
> 
> This is very hard to read, can you split this up into something like the
> following please?
> 
>   status = pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN);
>   if (!(status & ECAPCLK_EN_ACK)) {
>   ...
>   }
> 

Ok I will correct it.

> > +   dev_err(>dev, "PWMSS config space clock enable 
> > failure\n");
> > +   ret = -EINVAL;
> > +   goto pwmss_clk_failure;
> > +   }
> > +   pm_runtime_put_sync(>dev);
> 
> Another blank line between the two above would be good.

Ok

> 
...
> > +   .owner  = THIS_MODULE,
> > +   .of_match_table = of_match_ptr(ecap_of_match),
> 
> Here as well, if AM33xx is DT-only, then the of_match_ptr() can be
> dropped.

Ok I will drop.

Thanks
Avinash
> 
> > },
> > .probe = ecap_pwm_probe,
> > .remove = __devexit_p(ecap_pwm_remove),
> 
> No __devexit_p() please.
> 
> Thierry
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Thierry Reding 
On Fri, Nov 09, 2012 at 08:52:19AM +0100, Thierry Reding wrote:
> On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
[...]
> >  static int __devinit ecap_pwm_probe(struct platform_device *pdev)
> 
> __devinit can go away.
[...]
> >  static int __devexit ecap_pwm_remove(struct platform_device *pdev)
> 
> No __devexit please.
[...]
> > .remove = __devexit_p(ecap_pwm_remove),
> 
> No __devexit_p() please.

Okay, ignore these comments. The annotation were not added in this patch
so they can be removed in a separate patch. Or when Greg finally removes
all traces of them.

Thierry


pgpWo93u7PMsR.pgp
Description: PGP signature

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Thierry Reding 
On Fri, Nov 09, 2012 at 08:52:19AM +0100, Thierry Reding wrote:
 On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
[...]
   static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 
 __devinit can go away.
[...]
   static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 
 No __devexit please.
[...]
  .remove = __devexit_p(ecap_pwm_remove),
 
 No __devexit_p() please.

Okay, ignore these comments. The annotation were not added in this patch
so they can be removed in a separate patch. Or when Greg finally removes
all traces of them.

Thierry


pgpWo93u7PMsR.pgp
Description: PGP signature

RE: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Philip, Avinash 
On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
 On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
  This patch
  1. Add support for device-tree binding for ECAP APWM driver.
  2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
 period  polarity configuration from device tree.
  3. Add enable/disable clock gating in PWM subsystem common config space.
  4. When here set .owner member in platform_driver structure to
 THIS_MODULE.
  
  Signed-off-by: Philip, Avinash avinashphi...@ti.com
  Cc: Grant Likely grant.lik...@secretlab.ca
  Cc: Rob Herring rob.herr...@calxeda.com
  Cc: Rob Landley r...@landley.net
  ---
  Changes since v1:
  - Add separate patch for pinctrl support
  - Add conditional check for PWM subsystem clock enable.
  - Combined with HWMOD changes  DT bindings.
  - Remove the custom of xlate support.
  
  :00 100644 000... fe24cac... A  
  Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
  :100644 100644 d6d4cf0... 0d43266... M  drivers/pwm/pwm-tiecap.c
   .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
   drivers/pwm/pwm-tiecap.c   |   48 
  +++-
   2 files changed, 69 insertions(+), 1 deletions(-)
  
  diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
  b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
  new file mode 100644
  index 000..fe24cac
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
  @@ -0,0 +1,22 @@
  +TI SOC ECAP based APWM controller
  +
  +Required properties:
  +- compatible: Must be ti,am33xx-ecap
  +- #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 bit 0 in the third cell is
 
 I think this should be period in nanoseconds. I haven't heard period
 cycle before.

Ok

 
  +  used to encode the polarity of PWM output.
 
 Maybe you should explicitly say how this is encoded.

Ok I will add details

 
...
   
  +#define ECAPCLK_EN BIT(0)
  +#define ECAPCLK_STOP_REQ   BIT(1)
 
 This one doesn't seem to align with the rest. Also, why is bit 0 called
 _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
 _START and _STOP? Or _ENABLE and _DISABLE?

Ok I will change to PWMSS_ECAPCLK_EN  PWMSS_ECAPCLK_STPO_REQ

 
  +
  +#define ECAPCLK_EN_ACK BIT(0)
  +
  +#define PWM_CELL_SIZE  3
 
 You don't need a define for this.

I remove.

 
  +
   struct ecap_pwm_chip {
  struct pwm_chip chip;
  unsigned intclk_rate;
  @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
  .owner  = THIS_MODULE,
   };
   
  +#ifdef CONFIG_OF
  +static const struct of_device_id ecap_of_match[] = {
  +   {
  +   .compatible = ti,am33xx-ecap,
  +   },
  +   {},
  +};
  +MODULE_DEVICE_TABLE(of, ecap_of_match);
  +#endif
  +
 
 I'm not sure if I remember correctly, but wasn't AM33xx support supposed
 to be DT only? In that case you don't need the CONFIG_OF guards.

I will remove

 
...
  pm_runtime_enable(pdev-dev);
  +   pm_runtime_get_sync(pdev-dev);
 
 Maybe put a blank line after this for readability.

Ok

 
  +   if (!(pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_EN) 
  +   ECAPCLK_EN_ACK)) {
 
 This is very hard to read, can you split this up into something like the
 following please?
 
   status = pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_EN);
   if (!(status  ECAPCLK_EN_ACK)) {
   ...
   }
 

Ok I will correct it.

  +   dev_err(pdev-dev, PWMSS config space clock enable 
  failure\n);
  +   ret = -EINVAL;
  +   goto pwmss_clk_failure;
  +   }
  +   pm_runtime_put_sync(pdev-dev);
 
 Another blank line between the two above would be good.

Ok

 
...
  +   .owner  = THIS_MODULE,
  +   .of_match_table = of_match_ptr(ecap_of_match),
 
 Here as well, if AM33xx is DT-only, then the of_match_ptr() can be
 dropped.

Ok I will drop.

Thanks
Avinash
 
  },
  .probe = ecap_pwm_probe,
  .remove = __devexit_p(ecap_pwm_remove),
 
 No __devexit_p() please.
 
 Thierry
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-09 Thread Thierry Reding 
On Fri, Nov 09, 2012 at 10:59:30AM +, Philip, Avinash wrote:
 On Fri, Nov 09, 2012 at 13:22:19, Thierry Reding wrote:
  On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
   +#define ECAPCLK_EN   BIT(0)
   +#define ECAPCLK_STOP_REQ BIT(1)
  
  This one doesn't seem to align with the rest. Also, why is bit 0 called
  _EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
  _START and _STOP? Or _ENABLE and _DISABLE?
 
 Ok I will change to PWMSS_ECAPCLK_EN  PWMSS_ECAPCLK_STPO_REQ

While at it, maybe move these defines to the pwm-tipwmss.h header file?
After all that's the module that accesses the register that contains
these bits.



pgpAYVt81lNu2.pgp
Description: PGP signature

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-08 Thread Thierry Reding 
On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
> This patch
> 1. Add support for device-tree binding for ECAP APWM driver.
> 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
>period & polarity configuration from device tree.
> 3. Add enable/disable clock gating in PWM subsystem common config space.
> 4. When here set .owner member in platform_driver structure to
>THIS_MODULE.
> 
> Signed-off-by: Philip, Avinash 
> Cc:   Grant Likely 
> Cc: Rob Herring 
> Cc: Rob Landley 
> ---
> Changes since v1:
>   - Add separate patch for pinctrl support
>   - Add conditional check for PWM subsystem clock enable.
>   - Combined with HWMOD changes & DT bindings.
>   - Remove the custom of xlate support.
> 
> :00 100644 000... fe24cac... A
> Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> :100644 100644 d6d4cf0... 0d43266... Mdrivers/pwm/pwm-tiecap.c
>  .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
>  drivers/pwm/pwm-tiecap.c   |   48 
> +++-
>  2 files changed, 69 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
> b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> new file mode 100644
> index 000..fe24cac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
> @@ -0,0 +1,22 @@
> +TI SOC ECAP based APWM controller
> +
> +Required properties:
> +- compatible: Must be "ti,am33xx-ecap"
> +- #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 bit 0 in the third cell is

I think this should be "period in nanoseconds". I haven't heard "period
cycle" before.

> +  used to encode the polarity of PWM output.

Maybe you should explicitly say how this is encoded.

> +- reg: physical base address and size of the registers map.
> +
> +Optional properties:
> +- ti,hwmods: Name of the hwmod associated to the ECAP:
> +  "ecap",  being the 0-based instance number from the HW spec
> +
> +Example:
> +
> +ecap0: ecap@0 {
> + compatible = "ti,am33xx-ecap";
> + #pwm-cells = <3>;
> + reg = <0x48300100 0x80>;
> + ti,hwmods = "ecap0";
> +};
> diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
> index d6d4cf0..0d43266 100644
> --- a/drivers/pwm/pwm-tiecap.c
> +++ b/drivers/pwm/pwm-tiecap.c
> @@ -25,6 +25,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +
> +#include "tipwmss.h"
>  
>  /* ECAP registers and bits definitions */
>  #define CAP1 0x08
> @@ -37,6 +40,13 @@
>  #define ECCTL2_SYNC_SEL_DISA (BIT(7) | BIT(6))
>  #define ECCTL2_TSCTR_FREERUN BIT(4)
>  
> +#define ECAPCLK_EN   BIT(0)
> +#define ECAPCLK_STOP_REQ BIT(1)

This one doesn't seem to align with the rest. Also, why is bit 0 called
_EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
_START and _STOP? Or _ENABLE and _DISABLE?

> +
> +#define ECAPCLK_EN_ACK   BIT(0)
> +
> +#define PWM_CELL_SIZE3

You don't need a define for this.

> +
>  struct ecap_pwm_chip {
>   struct pwm_chip chip;
>   unsigned intclk_rate;
> @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
>   .owner  = THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ecap_of_match[] = {
> + {
> + .compatible = "ti,am33xx-ecap",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_of_match);
> +#endif
> +

I'm not sure if I remember correctly, but wasn't AM33xx support supposed
to be DT only? In that case you don't need the CONFIG_OF guards.

>  static int __devinit ecap_pwm_probe(struct platform_device *pdev)

__devinit can go away.

>  {
>   int ret;
> @@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct 
> platform_device *pdev)
>  
>   pc->chip.dev = >dev;
>   pc->chip.ops = _pwm_ops;
> + pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
>   pc->chip.base = -1;
>   pc->chip.npwm = 1;
>  
> @@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct 
> platform_device *pdev)
>   }
>  
>   pm_runtime_enable(>dev);
> + pm_runtime_get_sync(>dev);

Maybe put a blank line after this for readability.

> + if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
> + ECAPCLK_EN_ACK)) {

This is very hard to read, can you split this up into something like the
following please?

status = pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN);
if (!(status & ECAPCLK_EN_ACK)) {
...
}

> + dev_err(>dev, "PWMSS config space clock enable 
> failure\n");
> + ret = -EINVAL;
> + goto pwmss_clk_failure;
> + }
> + pm_runtime_put_sync(>dev);

Another blank line between the two

Re: [PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-08 Thread Thierry Reding 
On Thu, Nov 08, 2012 at 01:23:11PM +0530, Philip, Avinash wrote:
 This patch
 1. Add support for device-tree binding for ECAP APWM driver.
 2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
period  polarity configuration from device tree.
 3. Add enable/disable clock gating in PWM subsystem common config space.
 4. When here set .owner member in platform_driver structure to
THIS_MODULE.
 
 Signed-off-by: Philip, Avinash avinashphi...@ti.com
 Cc:   Grant Likely grant.lik...@secretlab.ca
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Rob Landley r...@landley.net
 ---
 Changes since v1:
   - Add separate patch for pinctrl support
   - Add conditional check for PWM subsystem clock enable.
   - Combined with HWMOD changes  DT bindings.
   - Remove the custom of xlate support.
 
 :00 100644 000... fe24cac... A
 Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
 :100644 100644 d6d4cf0... 0d43266... Mdrivers/pwm/pwm-tiecap.c
  .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
  drivers/pwm/pwm-tiecap.c   |   48 
 +++-
  2 files changed, 69 insertions(+), 1 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
 b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
 new file mode 100644
 index 000..fe24cac
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
 @@ -0,0 +1,22 @@
 +TI SOC ECAP based APWM controller
 +
 +Required properties:
 +- compatible: Must be ti,am33xx-ecap
 +- #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 bit 0 in the third cell is

I think this should be period in nanoseconds. I haven't heard period
cycle before.

 +  used to encode the polarity of PWM output.

Maybe you should explicitly say how this is encoded.

 +- reg: physical base address and size of the registers map.
 +
 +Optional properties:
 +- ti,hwmods: Name of the hwmod associated to the ECAP:
 +  ecapx, x being the 0-based instance number from the HW spec
 +
 +Example:
 +
 +ecap0: ecap@0 {
 + compatible = ti,am33xx-ecap;
 + #pwm-cells = 3;
 + reg = 0x48300100 0x80;
 + ti,hwmods = ecap0;
 +};
 diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
 index d6d4cf0..0d43266 100644
 --- a/drivers/pwm/pwm-tiecap.c
 +++ b/drivers/pwm/pwm-tiecap.c
 @@ -25,6 +25,9 @@
  #include linux/clk.h
  #include linux/pm_runtime.h
  #include linux/pwm.h
 +#include linux/of_device.h
 +
 +#include tipwmss.h
  
  /* ECAP registers and bits definitions */
  #define CAP1 0x08
 @@ -37,6 +40,13 @@
  #define ECCTL2_SYNC_SEL_DISA (BIT(7) | BIT(6))
  #define ECCTL2_TSCTR_FREERUN BIT(4)
  
 +#define ECAPCLK_EN   BIT(0)
 +#define ECAPCLK_STOP_REQ BIT(1)

This one doesn't seem to align with the rest. Also, why is bit 0 called
_EN and bit 1 _STOP_REQ? Couldn't they be made more consistent, i.e.
_START and _STOP? Or _ENABLE and _DISABLE?

 +
 +#define ECAPCLK_EN_ACK   BIT(0)
 +
 +#define PWM_CELL_SIZE3

You don't need a define for this.

 +
  struct ecap_pwm_chip {
   struct pwm_chip chip;
   unsigned intclk_rate;
 @@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
   .owner  = THIS_MODULE,
  };
  
 +#ifdef CONFIG_OF
 +static const struct of_device_id ecap_of_match[] = {
 + {
 + .compatible = ti,am33xx-ecap,
 + },
 + {},
 +};
 +MODULE_DEVICE_TABLE(of, ecap_of_match);
 +#endif
 +

I'm not sure if I remember correctly, but wasn't AM33xx support supposed
to be DT only? In that case you don't need the CONFIG_OF guards.

  static int __devinit ecap_pwm_probe(struct platform_device *pdev)

__devinit can go away.

  {
   int ret;
 @@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct 
 platform_device *pdev)
  
   pc-chip.dev = pdev-dev;
   pc-chip.ops = ecap_pwm_ops;
 + pc-chip.of_pwm_n_cells = PWM_CELL_SIZE;
   pc-chip.base = -1;
   pc-chip.npwm = 1;
  
 @@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct 
 platform_device *pdev)
   }
  
   pm_runtime_enable(pdev-dev);
 + pm_runtime_get_sync(pdev-dev);

Maybe put a blank line after this for readability.

 + if (!(pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_EN) 
 + ECAPCLK_EN_ACK)) {

This is very hard to read, can you split this up into something like the
following please?

status = pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_EN);
if (!(status  ECAPCLK_EN_ACK)) {
...
}

 + dev_err(pdev-dev, PWMSS config space clock enable 
 failure\n);
 + ret = -EINVAL;
 + goto pwmss_clk_failure;
 + }
 + pm_runtime_put_sync(pdev-dev);

Another blank line between

[PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-07 Thread Philip, Avinash 
This patch
1. Add support for device-tree binding for ECAP APWM driver.
2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
   period & polarity configuration from device tree.
3. Add enable/disable clock gating in PWM subsystem common config space.
4. When here set .owner member in platform_driver structure to
   THIS_MODULE.

Signed-off-by: Philip, Avinash 
Cc: Grant Likely 
Cc: Rob Herring 
Cc: Rob Landley 
---
Changes since v1:
- Add separate patch for pinctrl support
- Add conditional check for PWM subsystem clock enable.
- Combined with HWMOD changes & DT bindings.
- Remove the custom of xlate support.

:00 100644 000... fe24cac... A  
Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
:100644 100644 d6d4cf0... 0d43266... M  drivers/pwm/pwm-tiecap.c
 .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
 drivers/pwm/pwm-tiecap.c   |   48 +++-
 2 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
new file mode 100644
index 000..fe24cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
@@ -0,0 +1,22 @@
+TI SOC ECAP based APWM controller
+
+Required properties:
+- compatible: Must be "ti,am33xx-ecap"
+- #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 bit 0 in the third cell is
+  used to encode the polarity of PWM output.
+- reg: physical base address and size of the registers map.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the ECAP:
+  "ecap",  being the 0-based instance number from the HW spec
+
+Example:
+
+ecap0: ecap@0 {
+   compatible = "ti,am33xx-ecap";
+   #pwm-cells = <3>;
+   reg = <0x48300100 0x80>;
+   ti,hwmods = "ecap0";
+};
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index d6d4cf0..0d43266 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -25,6 +25,9 @@
 #include 
 #include 
 #include 
+#include 
+
+#include "tipwmss.h"
 
 /* ECAP registers and bits definitions */
 #define CAP1   0x08
@@ -37,6 +40,13 @@
 #define ECCTL2_SYNC_SEL_DISA   (BIT(7) | BIT(6))
 #define ECCTL2_TSCTR_FREERUN   BIT(4)
 
+#define ECAPCLK_EN BIT(0)
+#define ECAPCLK_STOP_REQ   BIT(1)
+
+#define ECAPCLK_EN_ACK BIT(0)
+
+#define PWM_CELL_SIZE  3
+
 struct ecap_pwm_chip {
struct pwm_chip chip;
unsigned intclk_rate;
@@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
.owner  = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id ecap_of_match[] = {
+   {
+   .compatible = "ti,am33xx-ecap",
+   },
+   {},
+};
+MODULE_DEVICE_TABLE(of, ecap_of_match);
+#endif
+
 static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 {
int ret;
@@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct platform_device 
*pdev)
 
pc->chip.dev = >dev;
pc->chip.ops = _pwm_ops;
+   pc->chip.of_pwm_n_cells = PWM_CELL_SIZE;
pc->chip.base = -1;
pc->chip.npwm = 1;
 
@@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct 
platform_device *pdev)
}
 
pm_runtime_enable(>dev);
+   pm_runtime_get_sync(>dev);
+   if (!(pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_EN) &
+   ECAPCLK_EN_ACK)) {
+   dev_err(>dev, "PWMSS config space clock enable 
failure\n");
+   ret = -EINVAL;
+   goto pwmss_clk_failure;
+   }
+   pm_runtime_put_sync(>dev);
+
platform_set_drvdata(pdev, pc);
return 0;
+
+pwmss_clk_failure:
+   pm_runtime_put_sync(>dev);
+   pm_runtime_disable(>dev);
+   pwmchip_remove(>chip);
+   return ret;
 }
 
 static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 {
struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
 
+   pm_runtime_get_sync(>dev);
+   /*
+* Due to hardware misbehaviour, acknowledge of the stop_req
+* is missing. Hence checking of the status bit skipped.
+*/
+   pwmss_submodule_state_change(pdev->dev.parent, ECAPCLK_STOP_REQ);
+   pm_runtime_put_sync(>dev);
+
pm_runtime_put_sync(>dev);
pm_runtime_disable(>dev);
return pwmchip_remove(>chip);
@@ -246,7 +290,9 @@ static int __devexit ecap_pwm_remove(struct platform_device 
*pdev)
 
 static struct platform_driver ecap_pwm_driver = {
.driver = {
-   .name = "ecap",
+   .name   = "ecap",
+   .owner  = THIS_MODULE,
+   .of_match_table = of_match_ptr(ecap_of_match),
},
.probe = ecap_pwm_probe,

[PATCH v2 04/10] pwm: pwm-tiecap: Add device-tree binding support for APWM driver

2012-11-07 Thread Philip, Avinash 
This patch
1. Add support for device-tree binding for ECAP APWM driver.
2. Set size of pwm-cells set to 3 to support PWM channel number, PWM
   period  polarity configuration from device tree.
3. Add enable/disable clock gating in PWM subsystem common config space.
4. When here set .owner member in platform_driver structure to
   THIS_MODULE.

Signed-off-by: Philip, Avinash avinashphi...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Rob Landley r...@landley.net
---
Changes since v1:
- Add separate patch for pinctrl support
- Add conditional check for PWM subsystem clock enable.
- Combined with HWMOD changes  DT bindings.
- Remove the custom of xlate support.

:00 100644 000... fe24cac... A  
Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
:100644 100644 d6d4cf0... 0d43266... M  drivers/pwm/pwm-tiecap.c
 .../devicetree/bindings/pwm/pwm-tiecap.txt |   22 +
 drivers/pwm/pwm-tiecap.c   |   48 +++-
 2 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt 
b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
new file mode 100644
index 000..fe24cac
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiecap.txt
@@ -0,0 +1,22 @@
+TI SOC ECAP based APWM controller
+
+Required properties:
+- compatible: Must be ti,am33xx-ecap
+- #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 bit 0 in the third cell is
+  used to encode the polarity of PWM output.
+- reg: physical base address and size of the registers map.
+
+Optional properties:
+- ti,hwmods: Name of the hwmod associated to the ECAP:
+  ecapx, x being the 0-based instance number from the HW spec
+
+Example:
+
+ecap0: ecap@0 {
+   compatible = ti,am33xx-ecap;
+   #pwm-cells = 3;
+   reg = 0x48300100 0x80;
+   ti,hwmods = ecap0;
+};
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index d6d4cf0..0d43266 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -25,6 +25,9 @@
 #include linux/clk.h
 #include linux/pm_runtime.h
 #include linux/pwm.h
+#include linux/of_device.h
+
+#include tipwmss.h
 
 /* ECAP registers and bits definitions */
 #define CAP1   0x08
@@ -37,6 +40,13 @@
 #define ECCTL2_SYNC_SEL_DISA   (BIT(7) | BIT(6))
 #define ECCTL2_TSCTR_FREERUN   BIT(4)
 
+#define ECAPCLK_EN BIT(0)
+#define ECAPCLK_STOP_REQ   BIT(1)
+
+#define ECAPCLK_EN_ACK BIT(0)
+
+#define PWM_CELL_SIZE  3
+
 struct ecap_pwm_chip {
struct pwm_chip chip;
unsigned intclk_rate;
@@ -184,6 +194,16 @@ static const struct pwm_ops ecap_pwm_ops = {
.owner  = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id ecap_of_match[] = {
+   {
+   .compatible = ti,am33xx-ecap,
+   },
+   {},
+};
+MODULE_DEVICE_TABLE(of, ecap_of_match);
+#endif
+
 static int __devinit ecap_pwm_probe(struct platform_device *pdev)
 {
int ret;
@@ -211,6 +231,7 @@ static int __devinit ecap_pwm_probe(struct platform_device 
*pdev)
 
pc-chip.dev = pdev-dev;
pc-chip.ops = ecap_pwm_ops;
+   pc-chip.of_pwm_n_cells = PWM_CELL_SIZE;
pc-chip.base = -1;
pc-chip.npwm = 1;
 
@@ -231,14 +252,37 @@ static int __devinit ecap_pwm_probe(struct 
platform_device *pdev)
}
 
pm_runtime_enable(pdev-dev);
+   pm_runtime_get_sync(pdev-dev);
+   if (!(pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_EN) 
+   ECAPCLK_EN_ACK)) {
+   dev_err(pdev-dev, PWMSS config space clock enable 
failure\n);
+   ret = -EINVAL;
+   goto pwmss_clk_failure;
+   }
+   pm_runtime_put_sync(pdev-dev);
+
platform_set_drvdata(pdev, pc);
return 0;
+
+pwmss_clk_failure:
+   pm_runtime_put_sync(pdev-dev);
+   pm_runtime_disable(pdev-dev);
+   pwmchip_remove(pc-chip);
+   return ret;
 }
 
 static int __devexit ecap_pwm_remove(struct platform_device *pdev)
 {
struct ecap_pwm_chip *pc = platform_get_drvdata(pdev);
 
+   pm_runtime_get_sync(pdev-dev);
+   /*
+* Due to hardware misbehaviour, acknowledge of the stop_req
+* is missing. Hence checking of the status bit skipped.
+*/
+   pwmss_submodule_state_change(pdev-dev.parent, ECAPCLK_STOP_REQ);
+   pm_runtime_put_sync(pdev-dev);
+
pm_runtime_put_sync(pdev-dev);
pm_runtime_disable(pdev-dev);
return pwmchip_remove(pc-chip);
@@ -246,7 +290,9 @@ static int __devexit ecap_pwm_remove(struct platform_device 
*pdev)
 
 static struct platform_driver ecap_pwm_driver = {
.driver = {
-   .name = ecap,
+