linux-kernel - Aw: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration

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: <trinity-d6d039f6-44e5-4c30-ad17-7fa4dbedcf7e-1607520005953@3c-app-gmx-bap29>
Date:   Wed, 9 Dec 2020 14:20:05 +0100
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     thierry.reding@...il.com, lee.jones@...aro.org,
        nsaenzjulienne@...e.de, f.fainelli@...il.com, rjui@...adcom.com,
        sean@...s.org, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com, linux-pwm@...r.kernel.org,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Aw: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic
 configuration

Hi Uwe

> Hello Lino,
>
> On Tue, Dec 08, 2020 at 11:01:45PM +0100, Lino Sanfilippo wrote:
> > Use the newer .apply function of pwm_ops instead of .config, .enable,
> > .disable and .set_polarity. This guarantees atomic changes of the pwm
> > controller configuration. It also reduces the size of the driver.
> >
> > Since now period is a 64 bit value, add an extra check to reject periods
> > that exceed the possible max value for the 32 bit register.
> >
> > This has been tested on a Raspberry PI 4.
>
> This looks right, just two small nitpicks below.
>

>
> This cast isn't necessary. (And if it was, I *think* the space between
> "(u32)" and "period" is wrong. But my expectation that checkpatch warns
> about this is wrong, so take this with a grain of salt.)

OK, I will omit the cast in the next patch version (it was primarily
meant for documentation purposes but now it seems to me rather
unusual for kernel code)

>
> > -	value = readl(pc->base + PWM_CONTROL);
> > -	value &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > -	writel(value, pc->base + PWM_CONTROL);
> > -}
> > +	/* set duty cycle */
> > +	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, scaler);
> > +	writel(val, pc->base + DUTY(pwm->hwpwm));
> >
> > -static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > -				enum pwm_polarity polarity)
> > -{
> > -	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
> > -	u32 value;
> > +	/* set polarity */
> > +	val = readl(pc->base + PWM_CONTROL);
> >
> > -	value = readl(pc->base + PWM_CONTROL);
> > +	if (state->polarity == PWM_POLARITY_NORMAL)
> > +		val &= ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	else
> > +		val |= PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >
> > -	if (polarity == PWM_POLARITY_NORMAL)
> > -		value &= ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	/* enable/disable */
> > +	if (state->enabled)
> > +		val |= PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >  	else
> > -		value |= PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> > +		val &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
> >
> > -	writel(value, pc->base + PWM_CONTROL);
> > +	writel(val, pc->base + PWM_CONTROL);
> >
> >  	return 0;
> >  }
> >
> > +
>
> I wouldn't have added this empty line. But I guess that's subjective. Or
> did you add this by mistake?

I cannot remember that the line was added by intention, so I am fine to remove it.

Thanks and regards,
Lino