Message-ID:
 <BM1PR01MB2545319120D9B3932C8C8806FEE12@BM1PR01MB2545.INDPRD01.PROD.OUTLOOK.COM>
Date: Wed, 22 Jan 2025 16:58:26 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Uwe Kleine-König <ukleinek@...nel.org>,
 Chen Wang <unicornxw@...il.com>
Cc: robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 inochiama@...look.com, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
 linux-riscv@...ts.infradead.org, chao.wei@...hgo.com,
 haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com, chunzhi.lin@...hgo.com,
 Sean Young <sean@...s.org>
Subject: Re: [PATCH v6 2/3] pwm: sophgo: add driver for Sophgo SG2042 PWM


On 2025/1/21 19:14, Uwe Kleine-König wrote:
> On Wed, Dec 04, 2024 at 11:17:18AM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@...look.com>
>>
>> Add a PWM driver for PWM controller in Sophgo SG2042 SoC.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
>> Signed-off-by: Sean Young <sean@...s.org>
>> ---
>>   drivers/pwm/Kconfig             |  10 ++
>>   drivers/pwm/Makefile            |   1 +
>>   drivers/pwm/pwm-sophgo-sg2042.c | 194 ++++++++++++++++++++++++++++++++
>>   3 files changed, 205 insertions(+)
>>   create mode 100644 drivers/pwm/pwm-sophgo-sg2042.c
[......]
>> diff --git a/drivers/pwm/pwm-sophgo-sg2042.c b/drivers/pwm/pwm-sophgo-sg2042.c
>> new file mode 100644
>> index 000000000000..a3d12505e4aa
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sophgo-sg2042.c
>> @@ -0,0 +1,194 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Sophgo SG2042 PWM Controller Driver
>> + *
>> + * Copyright (C) 2024 Sophgo Technology Inc.
>> + * Copyright (C) 2024 Chen Wang <unicorn_wang@...look.com>
>> + *
>> + * Limitations:
>> + * - After reset, the output of the PWM channel is always high.
>> + *   The value of HLPERIOD/PERIOD is 0.
>> + * - When HLPERIOD or PERIOD is reconfigured, PWM will start to
>> + *   output waveforms with the new configuration after completing
>> + *   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.
> Maybe I already asked: If there is a public manual for this chip, please
> add a link to it here.
There is a TRM on line, I will add the link here in next version.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +
>> +#include <asm/div64.h>
> The canonical include for that is <linux/math64.h>. This is also the
> header that defines mul_u64_u64_div_u64(). Your driver seems to compile
> only because clk.h includes math64.h via <linux/notifier.h> ->
> <linux/srcu.h> -> <linux/workqueue.h> -> <linux/timer.h> ->
> <linux/ktime.h> -> <linux/jiffies.h> -> <linux/math64.h>.
Thanks, I will correct this.
>> +/*
>> + * Offset RegisterName
>> + * 0x0000 HLPERIOD0
>> + * 0x0004 PERIOD0
>> + * 0x0008 HLPERIOD1
>> + * 0x000C PERIOD1
>> + * 0x0010 HLPERIOD2
>> + * 0x0014 PERIOD2
>> + * 0x0018 HLPERIOD3
>> + * 0x001C PERIOD3
>> + * Four groups and every group is composed of HLPERIOD & PERIOD
>> + */
>> +#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
>> +#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
> s/SG2042_/SG2042_PWM_/ to match the function prefix and driver name?
Accepted.
>> +
>> +#define SG2042_PWM_CHANNELNUM	4
>> +
>> +/**
>> + * struct sg2042_pwm_ddata - private driver data
>> + * @base:		base address of mapped PWM registers
>> + * @clk_rate_hz:	rate of base clock in HZ
>> + */
>> +struct sg2042_pwm_ddata {
>> +	void __iomem *base;
>> +	unsigned long clk_rate_hz;
>> +};
>> +
>> +static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
> Maybe pass ddata here instead of base and add
>
> 	void __iomem *base = ddata->base;
>
> to the function body. Then the calls simplify from
>
> 	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
>
> to
>
> 	pwm_sg2042_config(ddata, pwm->hwpwm, period, hlperiod);
>
> .

ok

>> +{
>> +	writel(period, base + SG2042_PERIOD(chan));
>> +	writel(hlperiod, base + SG2042_HLPERIOD(chan));
>> +}
>> +
>> +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			    const struct pwm_state *state)
>> +{
>> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
>> +	u32 hlperiod;
>> +	u32 period;
> state->period is measured in ns, the local variable period however
> holds a value measured in clock ticks. To make this still clearer than
> it already is, I suggest to rename the variable to period_ticks. Ditto
> for hlperiod.
Agree, changing the name would indeed make it clearer
>> +	if (state->polarity == PWM_POLARITY_INVERSED)
>> +		return -EINVAL;
>> +
>> +	if (!state->enabled) {
>> +		pwm_sg2042_config(ddata->base, pwm->hwpwm, 0, 0);
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
>> +	 * Period of One Cycle (period) = PERIOD x Period_clk
> s/Period/Duration/ ? What is Period_clk?
Period_clk is period of the input clock, i.e. 1/(ddata->clk_rate_hz).
>
>> +	 */
>> +	period = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->period, NSEC_PER_SEC), U32_MAX);
>> +	hlperiod = min(mul_u64_u64_div_u64(ddata->clk_rate_hz, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
>> +
>> +	if (hlperiod > period) {
>> +		dev_err(pwmchip_parent(chip), "period < hlperiod, failed to apply current setting\n");
>> +		return -EINVAL;
> No need to check for that, .apply() is only called with
> state->duty_cycle <= state->period.
ok.
>> +	}
>> +
>> +	dev_dbg(pwmchip_parent(chip), "chan[%u]: period=%u, hlperiod=%u\n",
>> +		pwm->hwpwm, period, hlperiod);
>> +
>> +	pwm_sg2042_config(ddata->base, pwm->hwpwm, period, hlperiod);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pwm_sg2042_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>> +				struct pwm_state *state)
>> +{
>> +	struct sg2042_pwm_ddata *ddata = pwmchip_get_drvdata(chip);
>> +	unsigned int chan = pwm->hwpwm;
>> +	u32 hlperiod;
>> +	u32 period;
>> +
>> +	period = readl(ddata->base + SG2042_PERIOD(chan));
>> +	hlperiod = readl(ddata->base + SG2042_HLPERIOD(chan));
>> +
>> +	if (!period && !hlperiod)
>> +		state->enabled = false;
>> +	else
>> +		state->enabled = true;
> What happens if hlperiod > period? Isn't period==0 enough for
> state->enabled = false? Also if period==0 there is no use in determining
> state->period and state->duty_cycle.
>
> So I would expect here:
>
> 	period_ticks = readl(ddata->base + SG2042_PERIOD(chan));
> 	hlperiod_ticks = readl(ddata->base + SG2042_HLPERIOD(chan));
>
> 	if (!period_ticks) {
> 		state->enabled = false;
> 		return 0;
> 	}
>
> 	if (hlperiod_ticks > period_ticks)
> 		hlperiod_ticks = period_ticks;
>
> 	state->enabled = true;
> 	state->period = DIV_ROUND_UP_ULL((u64)period_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
> 	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod_ticks * NSEC_PER_SEC, ddata->clk_rate_hz);
> 	state->polarity = PWM_POLARITY_NORMAL;
Thanks, I will improve this.
>> +	state->period = DIV_ROUND_UP_ULL((u64)period * NSEC_PER_SEC, ddata->clk_rate_hz);
>> +	state->duty_cycle = DIV_ROUND_UP_ULL((u64)hlperiod * NSEC_PER_SEC, ddata->clk_rate_hz);
>> +
>> +	state->polarity = PWM_POLARITY_NORMAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops pwm_sg2042_ops = {
>> +	.apply = pwm_sg2042_apply,
>> +	.get_state = pwm_sg2042_get_state,
>> +};
>> +
>> +static const struct of_device_id sg2042_pwm_ids[] = {
>> +	{ .compatible = "sophgo,sg2042-pwm" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sg2042_pwm_ids);
>> +
>> +static int pwm_sg2042_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct sg2042_pwm_ddata *ddata;
>> +	struct reset_control *rst;
>> +	struct pwm_chip *chip;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	chip = devm_pwmchip_alloc(dev, SG2042_PWM_CHANNELNUM, sizeof(*ddata));
>> +	if (IS_ERR(chip))
>> +		return PTR_ERR(chip);
>> +	ddata = pwmchip_get_drvdata(chip);
>> +
>> +	ddata->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(ddata->base))
>> +		return PTR_ERR(ddata->base);
>> +
>> +	clk = devm_clk_get_enabled(dev, "apb");
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get base clk\n");
> I like error messages to start consistently with a capital letter.
ok.
>
>> +	ret = devm_clk_rate_exclusive_get(dev, clk);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to get exclusive rate\n");
>> +
>> +	ddata->clk_rate_hz = clk_get_rate(clk);
>> +	if (!ddata->clk_rate_hz || ddata->clk_rate_hz > NSEC_PER_SEC)
> Please add a comment about why you check for > NSEC_PER_SEC.
Seems no need to check this (> NSEC_PER_SEC ), I will remove it in next 
version.
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Invalid clock rate: %lu\n", ddata->clk_rate_hz);
>> +
>> +	rst = devm_reset_control_get_optional_shared(dev, NULL);
>> +	if (IS_ERR(rst))
>> +		return dev_err_probe(dev, PTR_ERR(rst), "failed to get reset\n");
>> +
>> +	/* Deassert reset */
>> +	ret = reset_control_deassert(rst);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to deassert\n");
> There is devm_reset_control_get_optional_shared_deasserted() that does
> the two calls devm_reset_control_get_optional_shared() and
> reset_control_deassert() together and also cares for reasserting the
> reset at remove time.
ok, I will check this out in next version.
>> +	chip->ops = &pwm_sg2042_ops;
>> +	chip->atomic = true;
>> +
>> +	ret = devm_pwmchip_add(dev, chip);
>> +	if (ret < 0) {
>> +		reset_control_assert(rst);
>> +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
>> +	}
>> +
>> +	return 0;
>> +}
> Best regards
> Uwe

Thanks,

Chen.

Powered by blists - more mailing lists