Re: [PATCH 2/2] pwm: sophgo: add driver for Sophgo SG2042 PWM

[Chen Wang]

On 2024/9/6 0:03, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Sep 05, 2024 at 08:10:42PM +0800, Chen Wang wrote:
[......]
> > +static void pwm_sg2042_config(void __iomem *base, unsigned int channo, u32 period, u32 hlperiod)
> > +{
> > +	writel(period, base + REG_GROUP * channo + REG_PERIOD);
> > +	writel(hlperiod, base + REG_GROUP * channo + REG_HLPERIOD);
> > +}
> I suggest to use the following instead:
>
> 	#define SG2042_HLPERIOD(chan) ((chan) * 8 + 0)
> 	#define SG2042_PERIOD(chan) ((chan) * 8 + 4)
>
> 	...
>
> 	static void pwm_sg2042_config(void __iomem *base, unsigned int chan, u32 period, u32 hlperiod)
> 	{
> 		writel(period, base + SG2042_PERIOD(chan));
> 		writel(hlperiod, base + SG2042_HLPERIOD(chan));
> 	}
>
> The (subjective?) advantage is that the definition of SG2042_HLPERIOD
> contains information about all channel's HLPERIOD register and the usage
> in pwm_sg2042_config is obviously(?) right.
>
> (Another advantage is that the register names have a prefix unique to
> the driver, so there is no danger of mixing it up with some other
> hardware's driver that might also have a register named "PERIOD".)
Agree, will fix this in next version.
> Is this racy? i.e. can it happen that between the two writel the output
> is defined by the new period and old duty_cycle?
>
> How does the hardware behave on reconfiguration? Does it complete the
> currently running period? Please document that in a comment at the start
> of the driver like many other drivers do. (See
>
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
>
> )

When hlperiod or period is modified, PWM will start to output waveforms 
with the new configuration after completing the running period. Will 
document in a comment as other drivers do.

> > +static int pwm_sg2042_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			    const struct pwm_state *state)
> > +{
> > +	struct sg2042_pwm_chip *sg2042_pwm = to_sg2042_pwm_chip(chip);
> > +	u32 hlperiod;
> > +	u32 period;
> > +	u64 f_clk;
> > +	u64 p;
> > +
> > +	if (!state->enabled) {
> > +		pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, 0, 0);
> > +		return 0;
> > +	}
> Here you're missing (I guess):
>
> 	if (state->polarity == PWM_POLARITY_INVERSED)
> 		return -EINVAL;
Yes, it is required, will add this in next version, thanks.
> > +	/*
> > +	 * Period of High level (duty_cycle) = HLPERIOD x Period_clk
> > +	 * Period of One Cycle (period) = PERIOD x Period_clk
> > +	 */
> > +	f_clk = clk_get_rate(sg2042_pwm->base_clk);
> > +
> > +	p = f_clk * state->period;
> This might overflow.
>
> > +	do_div(p, NSEC_PER_SEC);
> > +	period = (u32)p;
> This gets very wrong if p happens to be bigger than U32_MAX.
>
> If you ensure f_clk <= NSEC_PER_SEC in .probe() (in combination with a
> call to clk_rate_exclusive_get()), you can do:
>
> 	period_cycles = min(mul_u64_u64_div_u64(f_clk, state->period, NSEC_PER_SEC), U32_MAX);
> 	duty_cycles = min(mul_u64_u64_div_u64(f_clk, state->duty_cycle, NSEC_PER_SEC), U32_MAX);
>
> This would also allow to store the clkrate in the driver private struct
> and drop the pointer to the clk from there.
f_clk should be 100M and <=NSEC_PER_SEC, will improve the code as your 
suggestion, thanks.
> > +	p = f_clk * state->duty_cycle;
> > +	do_div(p, NSEC_PER_SEC);
> > +	hlperiod = (u32)p;
> > +
> > +	dev_dbg(pwmchip_parent(chip), "chan[%d]: period=%u, hlperiod=%u\n",
> > +		pwm->hwpwm, period, hlperiod);
> pwm->hwpwm is an unsigned int, so use %u for it.
Ok, will fix.
> > +	pwm_sg2042_config(sg2042_pwm->base, pwm->hwpwm, period, hlperiod);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_sg2042_ops = {
> > +	.apply		= pwm_sg2042_apply,
> No .get_state() possible? Please use a single space before =.
Will add .get_state() and improve the format.
> > +};
> > +
> > +static const struct of_device_id sg2042_pwm_match[] = {
> > +	{ .compatible = "sophgo,sg2042-pwm" },
> > +	{ },
> Please drop the , after the sentinel entry.
OK
> > +};
> > +MODULE_DEVICE_TABLE(of, sg2042_pwm_match);
> > +
> > +static int pwm_sg2042_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct sg2042_pwm_chip *sg2042_pwm;
> > +	struct pwm_chip *chip;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(&pdev->dev, SG2042_PWM_CHANNELNUM, sizeof(*sg2042_pwm));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> > +	sg2042_pwm = to_sg2042_pwm_chip(chip);
> > +
> > +	chip->ops = &pwm_sg2042_ops;
> > +
> > +	sg2042_pwm->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(sg2042_pwm->base))
> > +		return PTR_ERR(sg2042_pwm->base);
> > +
> > +	sg2042_pwm->base_clk = devm_clk_get_enabled(&pdev->dev, "apb");
> > +	if (IS_ERR(sg2042_pwm->base_clk))
> > +		return dev_err_probe(dev, PTR_ERR(sg2042_pwm->base_clk),
> > +				     "failed to get base clk\n");
> > +
> > +	ret = devm_pwmchip_add(&pdev->dev, chip);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to register PWM chip\n");
> > +
> > +	platform_set_drvdata(pdev, chip);
> This is unused and should/can be dropped.
OK, will drop this, thanks for point it out. I should have cleaned it up.
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver pwm_sg2042_driver = {
> > +	.driver	= {
> > +		.name	= "sg2042-pwm",
> > +		.of_match_table = of_match_ptr(sg2042_pwm_match),
> > +	},
> > +	.probe = pwm_sg2042_probe,
> > +};
> > +module_platform_driver(pwm_sg2042_driver);
> Please use a single space before =.
Ok, will improve the format.
> > +MODULE_AUTHOR("Chen Wang");
> > +MODULE_DESCRIPTION("Sophgo SG2042 PWM driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> Best regards
> Uwe