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
