From: Lino Sanfilippo <LinoSanfilippo@gmx.de> To: "Uwe Kleine-König" <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 Date: Wed, 9 Dec 2020 14:20:05 +0100 [thread overview] Message-ID: <trinity-d6d039f6-44e5-4c30-ad17-7fa4dbedcf7e-1607520005953@3c-app-gmx-bap29> (raw) In-Reply-To: <20201209070516.yw5bpsh474k7mnfx@pengutronix.de> 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
WARNING: multiple messages have this Message-ID (diff)
From: Lino Sanfilippo <LinoSanfilippo@gmx.de> To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de> Cc: linux-arm-kernel@lists.infradead.org, linux-pwm@vger.kernel.org, f.fainelli@gmail.com, sbranden@broadcom.com, sean@mess.org, rjui@broadcom.com, linux-kernel@vger.kernel.org, thierry.reding@gmail.com, bcm-kernel-feedback-list@broadcom.com, linux-rpi-kernel@lists.infradead.org, lee.jones@linaro.org, nsaenzjulienne@suse.de Subject: Aw: Re: [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration Date: Wed, 9 Dec 2020 14:20:05 +0100 [thread overview] Message-ID: <trinity-d6d039f6-44e5-4c30-ad17-7fa4dbedcf7e-1607520005953@3c-app-gmx-bap29> (raw) In-Reply-To: <20201209070516.yw5bpsh474k7mnfx@pengutronix.de> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-12-09 13:22 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-08 22:01 [PATCH v3] pwm: bcm2835: Support apply function for atomic configuration Lino Sanfilippo 2020-12-08 22:01 ` Lino Sanfilippo 2020-12-09 7:05 ` Uwe Kleine-König 2020-12-09 7:05 ` Uwe Kleine-König 2020-12-09 13:20 ` Lino Sanfilippo [this message] 2020-12-09 13:20 ` Aw: " Lino Sanfilippo 2020-12-10 11:43 ` Uwe Kleine-König 2020-12-10 11:43 ` Uwe Kleine-König 2020-12-11 9:28 ` Aw: " Lino Sanfilippo 2020-12-11 9:28 ` Lino Sanfilippo 2020-12-11 9:53 ` Uwe Kleine-König 2020-12-11 9:53 ` Uwe Kleine-König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=trinity-d6d039f6-44e5-4c30-ad17-7fa4dbedcf7e-1607520005953@3c-app-gmx-bap29 \ --to=linosanfilippo@gmx.de \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=f.fainelli@gmail.com \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux-rpi-kernel@lists.infradead.org \ --cc=nsaenzjulienne@suse.de \ --cc=rjui@broadcom.com \ --cc=sbranden@broadcom.com \ --cc=sean@mess.org \ --cc=thierry.reding@gmail.com \ --cc=u.kleine-koenig@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.