Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Message-ID: <trinity-d6d039f6-44e5-4c30-ad17-7fa4dbedcf7e-1607520005953@3c-app-gmx-bap29>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
To:     =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>
Cc:     thierry.reding@gmail.com, lee.jones@linaro.org,
        nsaenzjulienne@suse.de, f.fainelli@gmail.com, rjui@broadcom.com,
        sean@mess.org, sbranden@broadcom.com,
        bcm-kernel-feedback-list@broadcom.com, linux-pwm@vger.kernel.org,
        linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Aw: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic
 configuration
Content-Type: text/plain; charset=UTF-8
Date:   Wed, 9 Dec 2020 14:20:05 +0100
Importance: normal
Sensitivity: Normal
In-Reply-To: <20201209070516.yw5bpsh474k7mnfx@pengutronix.de>
References: <1607464905-16630-1-git-send-email-LinoSanfilippo@gmx.de>
 <20201209070516.yw5bpsh474k7mnfx@pengutronix.de>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 perio=
ds
> > 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 =3D readl(pc->base + PWM_CONTROL);
> > -	value &=3D ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > -	writel(value, pc->base + PWM_CONTROL);
> > -}
> > +	/* set duty cycle */
> > +	val =3D 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_dev=
ice *pwm,
> > -				enum pwm_polarity polarity)
> > -{
> > -	struct bcm2835_pwm *pc =3D to_bcm2835_pwm(chip);
> > -	u32 value;
> > +	/* set polarity */
> > +	val =3D readl(pc->base + PWM_CONTROL);
> >
> > -	value =3D readl(pc->base + PWM_CONTROL);
> > +	if (state->polarity =3D=3D PWM_POLARITY_NORMAL)
> > +		val &=3D ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	else
> > +		val |=3D PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >
> > -	if (polarity =3D=3D PWM_POLARITY_NORMAL)
> > -		value &=3D ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
> > +	/* enable/disable */
> > +	if (state->enabled)
> > +		val |=3D PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm);
> >  	else
> > -		value |=3D PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
> > +		val &=3D ~(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 re=
move it.

Thanks and regards,
Lino