Re: [PATCH 2/2] drivers: pwm: pwm-atmel: add support to allow run time changing of pwm parameters

[m18063 <Claudiu.Beznea@microchip.com>]

Hi,


On 23.02.2017 11:21, Alexandre Belloni wrote:
> On 23/02/2017 at 10:38:40 +0200, Claudiu Beznea wrote:
>> sama5d2 supports changing of pwm parameters like period and
>> duty factor without first to disable pwm. Since pwm code
>> is supported by more than one SoC add allow_runtime_cfg
>> parameter to atmel_pwm_chip data structure. This will be
>> filled statically for every SoC, saved in pwm specific
>> structure at probing time and checked while configuring
>> the device. Based on this, pwm clock will not be
>> enabled/disabled while configuring if it still enabled.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/pwm/pwm-atmel.c | 24 ++++++++++++++++++------
>>  1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index 4406639..9e1dece 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -68,6 +68,8 @@ struct atmel_pwm_chip {
>>  
>>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>>  		       unsigned long dty, unsigned long prd);
>> +
>> +	bool allow_runtime_cfg;
>>  };
>>  
>>  static inline struct atmel_pwm_chip *to_atmel_pwm_chip(struct pwm_chip *chip)
>> @@ -114,7 +116,8 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>  	u32 val;
>>  	int ret;
>>  
>> -	if (pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
>> +	if (!atmel_pwm->allow_runtime_cfg &&
>> +	    pwm_is_enabled(pwm) && (period_ns != pwm_get_period(pwm))) {
>>  		dev_err(chip->dev, "cannot change PWM period while enabled\n");
>>  		return -EBUSY;
>>  	}
>> @@ -139,10 +142,12 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>  	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;
>> +	if (!pwm_is_enabled(pwm)) {
>> +		ret = clk_enable(atmel_pwm->clk);
>> +		if (ret) {
>> +			dev_err(chip->dev, "failed to enable PWM clock\n");
>> +			return ret;
>> +		}
>>  	}
>>  
> It is probably worth switching to atomic PWM instead of changing this
> function. This would simplify the whole driver.
I was thinking to switch to atomic PWM in a future patch.
>
>>  	/* It is necessary to preserve CPOL, inside CMR */
>> @@ -155,7 +160,9 @@ static int atmel_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>  	atmel_pwm->updated_pwms &= ~(1 << pwm->hwpwm);
>>  	mutex_unlock(&atmel_pwm->isr_lock);
>>  
>> -	clk_disable(atmel_pwm->clk);
>> +	if (!pwm_is_enabled(pwm))
>> +		clk_disable(atmel_pwm->clk);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -294,18 +301,22 @@ static const struct pwm_ops atmel_pwm_ops = {
>>  struct atmel_pwm_data {
>>  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
>>  		       unsigned long dty, unsigned long prd);
>> +	bool allow_runtime_cfg;
>>  };
>>  
>>  static const struct atmel_pwm_data atmel_pwm_data_v1 = {
>>  	.config = atmel_pwm_config_v1,
>> +	.allow_runtime_cfg = false,
> This is useless as it is false even if not explicitly set.
Ok. I will do it in v2.
>
>>  };
>>  
>>  static const struct atmel_pwm_data atmel_pwm_data_v2 = {
>>  	.config = atmel_pwm_config_v2,
>> +	.allow_runtime_cfg = false,
> ditto.
>
>>  };
>>  
>>  static const struct atmel_pwm_data atmel_pwm_data_v3 = {
>>  	.config = atmel_pwm_config_v3,
>> +	.allow_runtime_cfg = true,
>>  };
>>  
>>  static const struct platform_device_id atmel_pwm_devtypes[] = {
>> @@ -399,6 +410,7 @@ static int atmel_pwm_probe(struct platform_device *pdev)
>>  	atmel_pwm->chip.npwm = 4;
>>  	atmel_pwm->chip.can_sleep = true;
>>  	atmel_pwm->config = data->config;
>> +	atmel_pwm->allow_runtime_cfg = data->allow_runtime_cfg;
> It is probably worth having a pointer to the atmel_pwm_data instead of
> having to copy all the members.
Ok. I will do it in v2.
>
>>  	atmel_pwm->updated_pwms = 0;
>>  	mutex_init(&atmel_pwm->isr_lock);
>>  
>> -- 
>> 2.7.4
>>

Thank you,
Claudiu Beznea