linux-kernel - Re: [PATCH v2 2/4] pwm: atmel: add support for controllers with 32 bit counters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Feb 2019 09:10:15 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <u.kleine-koenig@...gutronix.de>
CC:     <thierry.reding@...il.com>, <robh+dt@...nel.org>,
        <mark.rutland@....com>, <Nicolas.Ferre@...rochip.com>,
        <alexandre.belloni@...tlin.com>, <Ludovic.Desroches@...rochip.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-pwm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/4] pwm: atmel: add support for controllers with 32
 bit counters



On 21.02.2019 22:42, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Feb 19, 2019 at 10:08:57AM +0000, Claudiu.Beznea@...rochip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@...rochip.com>
>>
>> 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@...rochip.com>
>> ---
>>  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
>