linux-kernel - Re: [PATCH v4 3/3] pwm: sophgo: add driver for SG2044

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: <jo4ustzbkswmeive5s4tepwhzeoeswa4knfihkzh6eylujeid4@dj2a2nzp2a5v>
Date: Mon, 26 May 2025 17:06:25 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Longbin Li <looong.bin@...il.com>
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, 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.

>   * 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?

>   */
> @@ -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.

> +	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. 

> +	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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)