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] [day] [month] [year] [list]

Message-ID: <20170424130809.GA3998@mint-host>
Date:   Mon, 24 Apr 2017 21:08:09 +0800
From:   Yang Ling <gnaygnil@...il.com>
To:     Marcin Nowakowski <marcin.nowakowski@...tec.com>
Cc:     Yang Ling <gnaygnil@...il.com>, 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, Marcin,

I am sorry for the late reply.

On Thu, Feb 16, 2017 at 10:27:15AM +0100, Marcin Nowakowski wrote:
> 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 ?
> 
Indeed, NSEC_PER_SEC should be used.

> >>>+	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?
>
The macro definition of the register here is misleading.
I will fix these problems afterwards.

Thanks for your friendly reminder.

Yang