Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

2017-03-27 Thread m18063

On 27.03.2017 16:15, Alexandre Belloni wrote:
> On 27/03/2017 at 15:02:37 +0200, Boris Brezillon wrote:
>> Hi Claudiu,
>>
>> On Wed, 22 Mar 2017 15:29:34 +0200
>> Claudiu Beznea  wrote:
>>
>>>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>>> {
>>> .name = "at91sam9rl-pwm",
>>> -   .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
>>> +   .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
>>> }, {
>>> .name = "sama5d3-pwm",
>>> -   .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
>>> +   .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
>>> }, {
>>> /* sentinel */
>>> },
>> Unrelated to this series, but can you prepare a patch to get rid of
>> this platform id table (AT91 platforms have completely switched to DT
>> for quite some time now).
>>
> Please, don't until AVR32 is gone.

Sure, I will wait for it.

>
>> You can also get rid of the "if (pdev->dev.of_node)" condition in
>> atmel_pwm_probe() since it's guaranteed to be true, otherwise the
>> ->probe() method wouldn't be called.
>>
>> Thanks,
>>
>> Boris



Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

2017-03-27 Thread Alexandre Belloni
On 27/03/2017 at 15:02:37 +0200, Boris Brezillon wrote:
> Hi Claudiu,
> 
> On Wed, 22 Mar 2017 15:29:34 +0200
> Claudiu Beznea  wrote:
> 
> >  static const struct platform_device_id atmel_pwm_devtypes[] = {
> > {
> > .name = "at91sam9rl-pwm",
> > -   .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
> > +   .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
> > }, {
> > .name = "sama5d3-pwm",
> > -   .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
> > +   .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
> > }, {
> > /* sentinel */
> > },
> 
> Unrelated to this series, but can you prepare a patch to get rid of
> this platform id table (AT91 platforms have completely switched to DT
> for quite some time now).
> 

Please, don't until AVR32 is gone.

> You can also get rid of the "if (pdev->dev.of_node)" condition in
> atmel_pwm_probe() since it's guaranteed to be true, otherwise the
> ->probe() method wouldn't be called.
> 
> Thanks,
> 
> Boris

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

2017-03-27 Thread Boris Brezillon
Hi Claudiu,

On Wed, 22 Mar 2017 15:29:34 +0200
Claudiu Beznea  wrote:

>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>   {
>   .name = "at91sam9rl-pwm",
> - .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1,
> + .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1,
>   }, {
>   .name = "sama5d3-pwm",
> - .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2,
> + .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2,
>   }, {
>   /* sentinel */
>   },

Unrelated to this series, but can you prepare a patch to get rid of
this platform id table (AT91 platforms have completely switched to DT
for quite some time now).

You can also get rid of the "if (pdev->dev.of_node)" condition in
atmel_pwm_probe() since it's guaranteed to be true, otherwise the
->probe() method wouldn't be called.

Thanks,

Boris


Re: [PATCH v3 1/2] drivers: pwm: pwm-atmel: switch to atomic PWM

2017-03-27 Thread Boris Brezillon
On Wed, 22 Mar 2017 15:29:34 +0200
Claudiu Beznea  wrote:

> The currently Atmel PWM controllers supported by this driver
> could change period or duty factor without channel disable,
> for regular channels (sama5d3 support this by using period
> or duty factor update registers, sam9rl support this by
> writing channel update register and select the corresponding
> update: period or duty factor). The chip doesn't support run
> time changings of signal polarity. To take advantage of
> atomic PWM framework and let controller works without glitches,
> in this patch only the duty factor could be changed without
> disabling PWM channel. For period and signal polarity the
> atomic PWM is simulated by disabling + enabling the right PWM channel.
> 
> Signed-off-by: Claudiu Beznea 

Reviewed-by: Boris Brezillon 

> 
> ---
>  drivers/pwm/pwm-atmel.c | 273 
> +++-
>  1 file changed, 129 insertions(+), 144 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 67a7023..f147154 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -58,17 +58,22 @@
>  #define PWM_MAX_PRD  0x
>  #define PRD_MAX_PRES 10
>  
> +struct atmel_pwm_registers {
> + u8 period;
> + u8 period_upd;
> + u8 duty;
> + u8 duty_upd;
> +};
> +
>  struct atmel_pwm_chip {
>   struct pwm_chip chip;
>   struct clk *clk;
>   void __iomem *base;
> + const struct atmel_pwm_registers *regs;
>  
>   unsigned int updated_pwms;
>   /* ISR is cleared when read, ensure only one thread does that */
>   struct mutex isr_lock;
> -
> - void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
> -unsigned long dty, unsigned long prd);
>  };
>  
>  static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
> @@ -105,153 +110,71 @@ static inline void atmel_pwm_ch_writel(struct 
> atmel_pwm_chip *chip,
>   writel_relaxed(val, chip->base + base + offset);
>  }
>  
> -static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> - int duty_ns, int period_ns)
> +static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
> +  const struct pwm_state *state,
> +  unsigned long *cprd, u32 *pres)
>  {
>   struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> - unsigned long prd, dty;
> - unsigned long long div;
> - unsigned int pres = 0;
> - u32 val;
> - int ret;
> -
> - if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
> - dev_err(chip->dev, "cannot change PWM period while enabled\n");
> - return -EBUSY;
> - }
> + unsigned long long cycles = state->period;
>  
>   /* Calculate the period cycles and prescale value */
> - div = (unsigned long long)clk_get_rate(atmel_pwm->clk) * period_ns;
> - do_div(div, NSEC_PER_SEC);
> + cycles *= clk_get_rate(atmel_pwm->clk);
> + do_div(cycles, NSEC_PER_SEC);
>  
> - while (div > PWM_MAX_PRD) {
> - div >>= 1;
> - pres++;
> - }
> + for (*pres = 0; cycles > PWM_MAX_PRD; cycles >>= 1)
> + (*pres)++;
>  
> - if (pres > PRD_MAX_PRES) {
> + if (*pres > PRD_MAX_PRES) {
>   dev_err(chip->dev, "pres exceeds the maximum value\n");
>   return -EINVAL;
>   }
>  
> - /* Calculate the duty cycles */
> - prd = div;
> - div *= duty_ns;
> - do_div(div, period_ns);
> - dty = prd - div;
> -
> - ret = clk_enable(atmel_pwm->clk);
> - if (ret) {
> - dev_err(chip->dev, "failed to enable PWM clock\n");
> - return ret;
> - }
> -
> - /* It is necessary to preserve CPOL, inside CMR */
> - val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> - val = (val & ~PWM_CMR_CPRE_MSK) | (pres & PWM_CMR_CPRE_MSK);
> - atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val);
> - atmel_pwm->config(chip, pwm, dty, prd);
> - mutex_lock(&atmel_pwm->isr_lock);
> - atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
> - atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
> - mutex_unlock(&atmel_pwm->isr_lock);
> + *cprd = cycles;
>  
> - clk_disable(atmel_pwm->clk);
> - return ret;
> + return 0;
>  }
>  
> -static void atmel_pwm_config_v1(struct pwm_chip *chip, struct pwm_device 
> *pwm,
> - unsigned long dty, unsigned long prd)
> +static void atmel_pwm_calculate_cdty(const struct pwm_state *state,
> +  unsigned long cprd, unsigned long *cdty)
>  {
> - struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> - unsigned int val;
> + unsigned long long cycles = state->duty_cycle;
>  
> -
> - atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWMV1_CUPD, dty);
> -
> - val = atmel