linux-kernel - Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC

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]

Message-ID: <0d0c43f5-1016-4cb5-01f8-9ca82860b8ad@imgtec.com>
Date:   Thu, 16 Feb 2017 10:27:15 +0100
From:   Marcin Nowakowski <marcin.nowakowski@...tec.com>
To:     Yang Ling <gnaygnil@...il.com>
CC:     <thierry.reding@...il.com>, <keguang.zhang@...il.com>,
        <linux-kernel@...r.kernel.org>, <linux-pwm@...r.kernel.org>,
        <linux-mips@...ux-mips.org>
Subject: Re: [PATCH 1/2] pwm: loongson1: Add PWM driver for Loongson1 SoC

Hi Yang,

On 15.02.2017 14:09, Yang Ling wrote:

>>> +	tmp = (unsigned long long)clk_get_rate(pc->clk) * period_ns;
>>> +	do_div(tmp, 1000000000);

NSEC_PER_SEC ?

>>> +	period = tmp;
>>> +
>>> +	tmp = (unsigned long long)period * duty_ns;
>>> +	do_div(tmp, period_ns);
>>> +	duty = period - tmp;
>>> +
>>> +	if (duty >= period)
>>> +		duty = period - 1;
>>> +
>>> +	if (duty >> 24 || period >> 24)
>>> +		return -EINVAL;
>>> +
>>> +	chan->period_ns = period_ns;
>>> +	chan->duty_ns = duty_ns;
>>> +
>>> +	writel(duty, pc->base + PWM_HRC(pwm->hwpwm));
>>> +	writel(period, pc->base + PWM_LRC(pwm->hwpwm));
>>> +	writel(0x00, pc->base + PWM_CNT(pwm->hwpwm));
>>> +
>>
>> PWM_HRC and PWM_LRC names suggest that you're using high/low state
>> counters here rather than duty/period - but with no documentation
>> I'm just guessing here.
>
> Indeed, the high/low state counters is used here.
> Change the name to duty_cnt/period_cnt.
>
>

What I was referring to here is that if you have a high/low value 
counters that you enter then these are not the same as duty/period, in 
simple terms:
high_cnt = duty_cnt
low_cnt = period_cnt - duty_cnt

so please double check that this is what you want to be doing? As the 
names used suggest that this code may be wrong. Or maybe what you're 
doing is correct but the register access macros have misleading names?

Marcin