| 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
| ||
|
Message-ID: <xgk2ac6zkfs34coefjcrol3aklob7sn7ob6qerhzf7yfa3dqdo@b54syid35obp>
Date: Tue, 27 May 2025 09:33:20 +0800
From: Longbin Li <looong.bin@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Chen Wang <unicorn_wang@...look.com>, Inochi Amaoto <inochiama@...il.com>,
Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, linux-pwm@...r.kernel.org,
devicetree@...r.kernel.org, sophgo@...ts.linux.dev, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v4 3/3] pwm: sophgo: add driver for SG2044
On Mon, May 26, 2025 at 05:06:25PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 28, 2025 at 09:34:50AM +0800, Longbin Li wrote:
> > Add PWM controller for SG2044 on base of SG2042.
> >
> > Signed-off-by: Longbin Li <looong.bin@...il.com>
> > Reviewed-by: Chen Wang <unicorn_wang@...look.com>
> > ---
> > drivers/pwm/pwm-sophgo-sg2042.c | 89 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 87 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
> > index 23a83843ba53..5bb92c910540 100644
> > --- a/drivers/pwm/pwm-sophgo-sg2042.c
> > +++ b/drivers/pwm/pwm-sophgo-sg2042.c
> > @@ -13,6 +13,9 @@
> > * the running period.
> > * - When PERIOD and HLPERIOD is set to 0, the PWM wave output will
> > * be stopped and the output is pulled to high.
> > + * - SG2044 support polarity while SG2042 does not. When PWMSTART is
> > + * false, POLARITY being NORMAL will make output being low,
> > + * POLARITY being INVERSED will make output being high.
>
> Without detailed knowledge about the hardware this isn't understandable.
> What is PWMSTART? I think this can just read:
>
> SG2044 supports both polarities, SG2042 only normal polarity.
>
> The rest is an implementation detail.
>
I will modify that, thanks.
> > * See the datasheet [1] for more details.
> > * [1]:https://github.com/sophgo/sophgo-doc/tree/main/SG2042/TRM
>
> This only describes SG2042, right? Can you please add a link for the new
> variant?
>
Sorry, there is no public document for SG2044.
> > */
> > @@ -26,6 +29,10 @@
> > #include <linux/pwm.h>
> > #include <linux/reset.h>
> >
> > +#define SG2044_PWM_POLARITY 0x40
> > +#define SG2044_PWM_PWMSTART 0x44
> > +#define SG2044_PWM_OE 0xd0
> > +
> > #define SG2042_PWM_HLPERIOD(chan) ((chan) * 8 + 0)
> > #define SG2042_PWM_PERIOD(chan) ((chan) * 8 + 4)
> >
> > @@ -72,8 +79,8 @@ static void pwm_set_dutycycle(struct pwm_chip *chip, struct pwm_device *pwm,
> > period_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
> > hlperiod_ticks = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
> >
> > - dev_dbg(pwmchip_parent(chip), "chan[%u]: PERIOD=%u, HLPERIOD=%u\n",
> > - pwm->hwpwm, period_ticks, hlperiod_ticks);
> > + dev_dbg(pwmchip_parent(chip), "chan[%u]: ENABLE=%u, PERIOD=%u, HLPERIOD=%u, POLARITY=%u\n",
> > + pwm->hwpwm, state->enabled, period_ticks, hlperiod_ticks, state->polarity);
> >
> > pwm_sg2042_config(ddata, pwm->hwpwm, period_ticks, hlperiod_ticks);
> > }
> > @@ -123,6 +130,74 @@ static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > return 0;
> > }
> >
> > +static void pwm_sg2044_set_start(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
> > + bool enabled)
> > +{
> > + u32 pwm_value;
> > +
> > + pwm_value = readl(ddata->base + SG2044_PWM_PWMSTART);
>
> Please use a variable name that matches the register this is used for.
>
I will fix that.
> > + if (enabled)
> > + pwm_value |= BIT(pwm->hwpwm);
> > + else
> > + pwm_value &= ~BIT(pwm->hwpwm);
> > +
> > + writel(pwm_value, ddata->base + SG2044_PWM_PWMSTART);
> > +}
> > +
> > +static void pwm_sg2044_set_outputdir(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
> > + bool enabled)
> > +{
> > + u32 pwm_value;
> > +
> > + pwm_value = readl(ddata->base + SG2044_PWM_OE);
> > +
> > + if (enabled)
> > + pwm_value |= BIT(pwm->hwpwm);
> > + else
> > + pwm_value &= ~BIT(pwm->hwpwm);
> > +
> > + writel(pwm_value, ddata->base + SG2044_PWM_OE);
> > +}
> > +
> > +static void pwm_sg2044_set_polarity(struct sg2042_pwm_ddata *ddata, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + u32 pwm_value;
> > +
> > + pwm_value = readl(ddata->base + SG2044_PWM_POLARITY);
> > +
> > + if (state->polarity == PWM_POLARITY_NORMAL)
> > + pwm_value &= ~BIT(pwm->hwpwm);
> > + else
> > + pwm_value |= BIT(pwm->hwpwm);
> > +
> > + writel(pwm_value, ddata->base + SG2044_PWM_POLARITY);
> > +}
> > +
> > +static int pwm_sg2044_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
> > +
> > + pwm_sg2044_set_polarity(ddata, pwm, state);
> > +
> > + pwm_set_dutycycle(chip, pwm, state);
> > +
> > + /*
> > + * re-enable PWMSTART to refresh the register period
> > + */
> > + pwm_sg2044_set_start(ddata, pwm, false);
>
> I don't understand the effect of the START register.
>
PWMSTART[3:0] enable the output of the corresponding channel. Must re-enable
it to refresh the register period when changed the duty cycle. I will find
a better name, thanks.
> > + if (!state->enabled)
> > + return 0;
> > +
> > + pwm_sg2044_set_outputdir(ddata, pwm, true);
> > + pwm_sg2044_set_start(ddata, pwm, true);
> > +
> > + return 0;
> > +}
> > +
> > static const struct sg2042_chip_data sg2042_chip_data = {
> > .ops = {
> > .apply = pwm_sg2042_apply,
>
> Best regards
> Uwe
Powered by blists - more mailing lists