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

[Thierry Reding]

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

Attachment: signature.asc
Description: PGP signature