Hi Uwe:
Thanks for your detailed review.
On Thu, May 23, 2024 at 10:31 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> Hello,
>
> sorry for taking so long to get back to your patch. reviewing new
> drivers is quite time consuming which makes me often fail to review in a
> timely manner.
>
> On Tue, Apr 16, 2024 at 09:55:15AM +0800, Binbin Zhou wrote:
> > This commit adds a generic PWM framework driver for the PWM controller
> > found on Loongson family chips.
> >
> > Co-developed-by: Juxin Gao <gaojuxin@xxxxxxxxxxx>
> > Signed-off-by: Juxin Gao <gaojuxin@xxxxxxxxxxx>
> > Signed-off-by: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > drivers/pwm/Kconfig | 10 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-loongson.c | 298 +++++++++++++++++++++++++++++++++++++
> > 4 files changed, 310 insertions(+)
> > create mode 100644 drivers/pwm/pwm-loongson.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ecef2744726d..d32da7c77f0e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12756,6 +12756,7 @@ M: Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > L: linux-pwm@xxxxxxxxxxxxxxx
> > S: Maintained
> > F: Documentation/devicetree/bindings/pwm/loongson,ls7a-pwm.yaml
> > +F: drivers/pwm/pwm-loongson.c
> >
> > LOONGSON-2 SOC SERIES CLOCK DRIVER
> > M: Yinbo Zhu <zhuyinbo@xxxxxxxxxxx>
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 4b956d661755..bb163c65e5ae 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -324,6 +324,16 @@ config PWM_KEEMBAY
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-keembay.
> >
> > +config PWM_LOONGSON
> > + tristate "Loongson PWM support"
> > + depends on MACH_LOONGSON64
>
> Something with || COMPILE_TEST would be nice.
OK..
>
> > + help
> > + Generic PWM framework driver for Loongson family.
> > + It can be found on Loongson-2K series cpu and Loongson LS7A bridge chips.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called pwm-loongson.
> > +
> > config PWM_LP3943
> > tristate "TI/National Semiconductor LP3943 PWM support"
> > depends on MFD_LP3943
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index c5ec9e168ee7..bffa49500277 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -28,6 +28,7 @@ obj-$(CONFIG_PWM_INTEL_LGM) += pwm-intel-lgm.o
> > obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o
> > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o
> > obj-$(CONFIG_PWM_KEEMBAY) += pwm-keembay.o
> > +obj-$(CONFIG_PWM_LOONGSON) += pwm-loongson.o
> > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o
> > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o
> > obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c
> > new file mode 100644
> > index 000000000000..5ac79a69acd3
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-loongson.c
> > @@ -0,0 +1,298 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Loongson PWM driver
> > + *
> > + * Author: Juxin Gao <gaojuxin@xxxxxxxxxxx>
> > + * Further cleanup and restructuring by:
> > + * Binbin Zhou <zhoubinbin@xxxxxxxxxxx>
> > + *
> > + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited.
>
> A paragraph about the hardware capabilities here please. Please answer
> the following questions:
>
> - How does the hardware behave on disable? (Does it complete the
> currently running period? Is the output still driven then? If yes,
> which level?)
>
> - How does the hardware behave on configuration changes? (Does it
> complete the currently running period? Are there some glitches
> expected (like driving an output corresponding to the old period
> length but the new duty_cycle or similar).
>
> - Are there any restrictions like: Cannot do 100% relative duty (or
> 0%)?
>
> Stick to the format used in most other drivers such that
>
> sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-loongson.c
>
> emits the requested info.
>
OK, I will add it.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/units.h>
> > +
> > +/* Loongson PWM registers */
> > +#define PWM_DUTY 0x4 /* Low Pulse Buffer Register */
> > +#define PWM_PERIOD 0x8 /* Pulse Period Buffer Register */
> > +#define PWM_CTRL 0xc /* Control Register */
>
> Please use a driver specific prefix for these defines. PWM_DUTY is quite
> generic otherwise.
OK, I will rename these defines as LOONGSON_*.
>
> > +
> > +/* Control register bits */
> > +#define PWM_CTRL_EN BIT(0) /* Counter Enable Bit */
> > +#define PWM_CTRL_OE BIT(3) /* Pulse Output Enable Control Bit, Valid Low */
> > +#define PWM_CTRL_SINGLE BIT(4) /* Single Pulse Control Bit */
> > +#define PWM_CTRL_INTE BIT(5) /* Interrupt Enable Bit */
> > +#define PWM_CTRL_INT BIT(6) /* Interrupt Bit */
> > +#define PWM_CTRL_RST BIT(7) /* Counter Reset Bit */
> > +#define PWM_CTRL_CAPTE BIT(8) /* Measurement Pulse Enable Bit */
> > +#define PWM_CTRL_INVERT BIT(9) /* Output flip-flop Enable Bit */
> > +#define PWM_CTRL_DZONE BIT(10) /* Anti-dead Zone Enable Bit */
> > +
> > +#define PWM_FREQ_STD (50 * HZ_PER_KHZ)
> > +
> > +struct pwm_loongson_ddata {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + void __iomem *base;
> > + /* The following for PM */
> > + u32 ctrl;
> > + u32 duty;
> > + u32 period;
>
> This needs updating to cope for commit 05947224ff46 ("pwm: Ensure that
> pwm_chips are allocated using pwmchip_alloc()")
>
> Also I'm not a fan of aligning the member names. If you feel strong
> about it, keep it as is, however.
>
Yes, I have refactored this part based on commit 05947224ff46 ("pwm:
Ensure that pwm_chips are allocated using pwmchip_alloc()").
> > +};
> > +
> > +static inline struct pwm_loongson_ddata *to_pwm_loongson_ddata(struct pwm_chip *chip)
> > +{
> > + return container_of(chip, struct pwm_loongson_ddata, chip);
> > +}
> > +
> > +static inline u32 pwm_loongson_readl(struct pwm_loongson_ddata *ddata, u64 offset)
>
> I don't know about the calling convention on loongson, but I'd expect
> offset to be an unsigned int only, given that (I guess) only PWM_CTRL
> and friends are passed here.
>
Emm...
Actually, unsigned int should be enough.
> > +{
> > + return readl(ddata->base + offset);
> > +}
> > +
> > +static inline void pwm_loongson_writel(struct pwm_loongson_ddata *ddata,
> > + u32 val, u64 offset)
> > +{
> > + writel(val, ddata->base + offset);
> > +}
> > +
> > +static int pwm_loongson_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> > + enum pwm_polarity polarity)
> > +{
> > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > + u16 val;
> > +
> > + val = pwm_loongson_readl(ddata, PWM_CTRL);
> > +
> > + if (polarity == PWM_POLARITY_INVERSED)
> > + /* Duty cycle defines LOW period of PWM */
> > + val |= PWM_CTRL_INVERT;
> > + else
> > + /* Duty cycle defines HIGH period of PWM */
> > + val &= ~PWM_CTRL_INVERT;
> > +
> > + pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static void pwm_loongson_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > + u32 val;
> > +
> > + if (pwm->state.polarity == PWM_POLARITY_NORMAL)
> > + pwm_loongson_writel(ddata, ddata->period, PWM_DUTY);
> > + else if (pwm->state.polarity == PWM_POLARITY_INVERSED)
> > + pwm_loongson_writel(ddata, 0, PWM_DUTY);
> > +
> > + val = pwm_loongson_readl(ddata, PWM_CTRL);
> > + val &= ~PWM_CTRL_EN;
> > + pwm_loongson_writel(ddata, val, PWM_CTRL);
>
> Technically it's not needed to configure the duty. A consumer who
> expects a certain behaviour is supposed to not disable the PWM.
>
Emm, this is really not necessary.
> > +}
> > +
> > +static int pwm_loongson_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > + u32 val;
> > +
> > + pwm_loongson_writel(ddata, ddata->duty, PWM_DUTY);
> > + pwm_loongson_writel(ddata, ddata->period, PWM_PERIOD);
>
> pwm_loongson_enable() is called from pwm_loongson_apply() and PWM_DUTY and
> PWM_PERIOD were already written there. So please either only write it
> once, or add a code comment about why writing twice is needed.
>
Sorry, it's my fault. I will keep these regs written only once.
> > + val = pwm_loongson_readl(ddata, PWM_CTRL);
> > + val |= PWM_CTRL_EN;
> > + pwm_loongson_writel(ddata, val, PWM_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static u32 pwm_loongson_set_config(struct pwm_loongson_ddata *ddata, int ns,
> > + u64 clk_rate, u64 offset)
> > +{
> > + u32 val;
> > + u64 c;
> > +
> > + c = clk_rate * ns;
>
> That migth overflow?!
>
> > + do_div(c, NSEC_PER_SEC);
> > + val = c < 1 ? 1 : c;
>
> That smells fishy. If a period (or duty_cycle) is requested that is too
> small to be implemented, let .apply() return -EINVAL.
>
In fact, I'm going to rewrite this part, drop the
pwm_loongson_set_config(), and calulate the duty and period in
pwm_loongson_config(), something like:
/* duty = duty_ns * ddata->clk_rate / NSEC_PER_SEC */
ctx.duty = mul_u64_u64_div_u64(duty_ns, ddata->clk_rate, NSEC_PER_SEC);
pwm_loongson_writel(ddata, ctx.duty, LOONGSON_PWM_REG_DUTY);
/* period = period_ns * ddata->clk_rate / NSEC_PER_SEC */
ctx.period = mul_u64_u64_div_u64(period_ns, ddata->clk_rate,
NSEC_PER_SEC);
pwm_loongson_writel(ddata, ctx.period, LOONGSON_PWM_REG_PERIOD);
> > + pwm_loongson_writel(ddata, val, offset);
> > +
> > + return val;
> > +}
> > +
> > +static int pwm_loongson_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > + int duty_ns, int period_ns)
> > +{
> > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > + struct device *dev = chip->dev;
> > + u64 clk_rate;
> > +
> > + if (duty_ns > NANOHZ_PER_HZ || period_ns > NANOHZ_PER_HZ)
> > + return -ERANGE;
>
> Nope, that's wrong. Please configure the biggest possible period not
> bigger than the requested period. So something like:
>
> period_ns = min(period_ns, NANOHZ_PER_HZ);
>
> ; ditto for duty_cycle.
OK.. I will do it in .apply().
>
> > + clk_rate = has_acpi_companion(dev) ? PWM_FREQ_STD : clk_get_rate(ddata->clk);
> > +
> > + ddata->duty = pwm_loongson_set_config(ddata, duty_ns, clk_rate, PWM_DUTY);
> > + ddata->period = pwm_loongson_set_config(ddata, period_ns, clk_rate, PWM_PERIOD);
> > +
> > + return 0;
> > +}
> > +
> > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + int err;
> > + bool enabled = pwm->state.enabled;
> > +
> > + if (state->polarity != pwm->state.polarity) {
> > + if (enabled) {
> > + pwm_loongson_disable(chip, pwm);
> > + enabled = false;
> > + }
> > +
> > + err = pwm_loongson_set_polarity(chip, pwm, state->polarity);
> > + if (err)
> > + return err;
> > + }
> > +
> > + if (!state->enabled) {
> > + if (enabled)
> > + pwm_loongson_disable(chip, pwm);
> > + return 0;
> > + }
> > +
> > + err = pwm_loongson_config(chip, pwm, state->duty_cycle, state->period);
>
> state->duty_cycle is an u64, however it's truncated to an int here.
>
OK, I will fix it.
> > + if (err)
> > + return err;
> > +
> > + if (!enabled)
> > + err = pwm_loongson_enable(chip, pwm);
> > +
> > + return err;
> > +}
> > +
> > +static int pwm_loongson_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct pwm_loongson_ddata *ddata = to_pwm_loongson_ddata(chip);
> > + u32 period, duty, ctrl;
> > + u64 ns;
> > +
> > + ctrl = pwm_loongson_readl(ddata, PWM_CTRL);
> > + state->polarity = (ctrl & PWM_CTRL_INVERT) ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > + state->enabled = (ctrl & PWM_CTRL_EN) ? true : false;
> > +
> > + duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > + ns = duty * NSEC_PER_SEC;
> > + state->duty_cycle = do_div(ns, duty);
> > +
> > + period = pwm_loongson_readl(ddata, PWM_PERIOD);
> > + ns = period * NSEC_PER_SEC;
> > + state->period = do_div(ns, period);
> > +
> > + ddata->ctrl = ctrl;
> > + ddata->duty = pwm_loongson_readl(ddata, PWM_DUTY);
> > + ddata->period = pwm_loongson_readl(ddata, PWM_PERIOD);
>
> The rounding looks wrong. Did you test with PWM_DEBUG enabled?
>
> I think the value assigned to ddata->period and the other members isn't
> used. Unless I'm mistaken, please drop the assignment.
>
The period, duty and ctrl are prepared for PM. I plan to put these
three parameters separately into the pwm_loongson_context structure. I
think it will look clearer:
struct pwm_loongson_context {
u32 ctrl;
u32 duty;
u32 period;
};
> > + return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_loongson_ops = {
> > + .apply = pwm_loongson_apply,
> > + .get_state = pwm_loongson_get_state,
> > +};
> > +
> > +static int pwm_loongson_probe(struct platform_device *pdev)
> > +{
> > + struct pwm_loongson_ddata *ddata;
> > + struct device *dev = &pdev->dev;
> > +
> > + ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> > + if (!ddata)
> > + return -ENOMEM;
> > +
> > + ddata->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(ddata->base))
> > + return PTR_ERR(ddata->base);
> > +
> > + if (!has_acpi_companion(dev)) {
> > + ddata->clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(ddata->clk))
> > + return PTR_ERR(ddata->clk);
>
> error message with dev_err_probe() please.
OK, I will do it.
>
> > + }
> > +
> > + ddata->chip.dev = dev;
> > + ddata->chip.ops = &pwm_loongson_ops;
> > + ddata->chip.npwm = 1;
> > + platform_set_drvdata(pdev, ddata);
>
> The effect of platform_set_drvdata is used in .suspend below, however
> there you use dev_get_drvdata on &pdev->dev. For symmetry I suggest to
> use dev_set_drvdata(dev, ddata) here.
>
> > + return devm_pwmchip_add(dev, &ddata->chip);
>
> error message iwth dev_err_probe() please (if it fails).
OK, I will add it.
>
> > +}
> > +
> > [...]
> > +static struct platform_driver pwm_loongson_driver = {
> > + .probe = pwm_loongson_probe,
> > + .driver = {
> > + .name = "loongson-pwm",
> > + .pm = pm_ptr(&pwm_loongson_pm_ops),
> > + .of_match_table = pwm_loongson_of_ids,
> > + .acpi_match_table = pwm_loongson_acpi_ids,
>
> This alignment looks really ugly. Please use a single space before the
> =. (Or if you must, properly align the =.)
>
OK., I will keep a single space before the =.
Thanks.
Binbin
> > + },
> > +};
> > +module_platform_driver(pwm_loongson_driver);
> > +
> > +MODULE_DESCRIPTION("Loongson PWM driver");
> > +MODULE_AUTHOR("Loongson Technology Corporation Limited.");
> > +MODULE_LICENSE("GPL");
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
