On 21.02.2019 22:42, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, Feb 19, 2019 at 10:08:57AM +0000, Claudiu.Beznea@xxxxxxxxxxxxx wrote:
>> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>>
>> New SAM9X60's PWM controller use 32 bits counters thus it could generate
>> signals with higher period and duty cycles than the old ones. Prepare the
>> current driver to be able to work with old controllers (that uses 16 bits
>> counters) and with the new SAM9X60's controller, by providing counters
>> information based on compatible.
>
> I'd write:
>
> The PWM controller of the new SAM9X60 SoC uses 32 bit wide
> counters compared to 16 bit wide counters in the earlier chips.
> To support this add a new structure to the compatibles' data
> that describe the counter width and precision and make use of
> them instead of the hard coded values.
>
> Other than that the commit looks fine.
>
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>> drivers/pwm/pwm-atmel.c | 34 +++++++++++++++++++++++-----------
>> 1 file changed, 23 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
>> index 7e86a5266eb6..647d063562db 100644
>> --- a/drivers/pwm/pwm-atmel.c
>> +++ b/drivers/pwm/pwm-atmel.c
>> @@ -48,15 +48,11 @@
>> #define PWMV2_CPRD 0x0C
>> #define PWMV2_CPRDUPD 0x10
>>
>> -/*
>> - * Max value for duty and period
>> - *
>> - * Although the duty and period register is 32 bit,
>> - * however only the LSB 16 bits are significant.
>> - */
>> -#define PWM_MAX_DTY 0xFFFF
>> -#define PWM_MAX_PRD 0xFFFF
>> -#define PRD_MAX_PRES 10
>> +/* Max values for period and prescaler */
>> +
>> +/* Only the LSB 16 bits are significant. */
>> +#define PWM_MAXV1_PRD 0xFFFF
>> +#define PRD_MAXV1_PRES 10
>>
>> struct atmel_pwm_registers {
>> u8 period;
>> @@ -65,8 +61,14 @@ struct atmel_pwm_registers {
>> u8 duty_upd;
>> };
>>
>> +struct atmel_pwm_config {
>> + u32 max_period;
>> + u32 max_pres;
>> +};
>> +
>> struct atmel_pwm_data {
>> struct atmel_pwm_registers regs;
>> + struct atmel_pwm_config cfg;
>> };
>>
>> struct atmel_pwm_chip {
>> @@ -125,10 +127,10 @@ static int atmel_pwm_calculate_cprd_and_pres(struct pwm_chip *chip,
>> cycles *= clk_get_rate(atmel_pwm->clk);
>> do_div(cycles, NSEC_PER_SEC);
>>
>> - for (*pres = 0; cycles > PWM_MAX_PRD; cycles >>= 1)
>> + for (*pres = 0; cycles > atmel_pwm->data->cfg.max_period; cycles >>= 1)
>> (*pres)++;
>
> Orthogonal to this patch, this could be calculated without a loop.
> Something like
>
> pres = roundup_pow_of_two(cycles);
> if (pres > bitwidth_of_counter_register)
> pres -= bitwidth_of_counter_register;
> else
> pres = 0
>
> (where bitwidth_of_counter_register is 16 for the older PWMs and 32 for
> the new one). Maybe it would make more sense to put 16 into the
> structure describing the PWM then instead of 0xffff (which is easily
> calculated from 16)? If picking this up, you might have to pay
> attention to pick functions that support long long arguments.
> roundup_pow_of_two() doesn't as of now.
I will take it into account for future patches.
Thank you,
Claudiu Beznea
>
> Best regards
> Uwe
>
