linux-kernel - Re: [PATCH v6 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

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:   Wed, 13 Feb 2019 13:39:02 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Yash Shah <yash.shah@...ive.com>, palmer@...ive.com,
        linux-pwm@...r.kernel.org, linux-riscv@...ts.infradead.org,
        robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        sachin.ghadi@...ive.com, paul.walmsley@...ive.com
Subject: Re: [PATCH v6 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

On Wed, Feb 13, 2019 at 11:34:59AM +0100, Uwe Kleine-König wrote:
> On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
[...]
> > +	unsigned long scale_pow =
> > +			div64_ul(pwm->real_period * (u64)rate, NSEC_PER_SEC);
> > +	int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf);
> > +
> > +	writel((1 << PWM_SIFIVE_PWMCFG_EN_ALWAYS) | (scale <<
> > +	       PWM_SIFIVE_PWMCFG_SCALE), pwm->regs + PWM_SIFIVE_PWMCFG);
> 
> I think this would be better readable with the newline after the |. With
> my editor's configuration when broken like this, the 2nd line would be
> intended with the opening ( after the |.
> 
> > +
> > +	/* As scale <= 15 the shift operation cannot overflow. */
> > +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +				    scale), rate);
> 
> ditto. Maybe break after the =?
> 
> > +	dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period);
> > +}
> > +
> > +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev,
> > +				 struct pwm_state *state)
> > +{
> > +	struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > +	u32 duty;
> > +	int val;
> > +
> > +	duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > +		     PWM_SIFIVE_SIZE_PWMCMP);
> > +
> > +	val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > +	state->enabled = (val & BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS)) > 0;
> > +
> > +	val &= 0x0F;
> > +	pwm->real_period = div64_ul(1000000000ULL << (PWM_SIFIVE_CMPWIDTH +
> > +				    val), clk_get_rate(pwm->clk));
> 
> Another bad line break.

Maybe just split all of these very long expressions up by introducing
temporary variables to make things more palatable?

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)