Hi Uwe: Thanks for your detailed reply. On Sat, Nov 23, 2024 at 1:19 AM Uwe Kleine-König <ukleinek@xxxxxxxxxx> wrote: > > Hello, > > I now finally comne around to review your patch. Thanks for your > patience. > > On Tue, Oct 22, 2024 at 05:04:15PM +0800, Binbin Zhou wrote: > > diff --git a/drivers/pwm/pwm-loongson.c b/drivers/pwm/pwm-loongson.c > > new file mode 100644 > > index 000000000000..4c9b14efadc3 > > --- /dev/null > > +++ b/drivers/pwm/pwm-loongson.c > > @@ -0,0 +1,288 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (C) 2017-2024 Loongson Technology Corporation Limited. > > + * > > + * Loongson PWM driver > > + * > > + * For Loongson's PWM IP block documentation please refer Chapter 11 of > > + * Reference Manual: https://loongson.github.io/LoongArch-Documentation/Loongson-7A1000-usermanual-EN.pdf > > + * > > + * Author: Juxin Gao <gaojuxin@xxxxxxxxxxx> > > + * Further cleanup and restructuring by: > > + * Binbin Zhou <zhoubinbin@xxxxxxxxxxx> > > + * > > + * Limitations: > > + * - The buffer register value should be written before the CTRL register. > > This isn't an interesting point for the high level description. I'd hope > the driver cares for this implementation detail. > > > + * - When disabled the output is driven to 0 independent of the configured > > + * polarity. > > + */ > > + > > +#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 LOONGSON_PWM_REG_DUTY 0x4 /* Low Pulse Buffer Register */ > > +#define LOONGSON_PWM_REG_PERIOD 0x8 /* Pulse Period Buffer Register */ > > +#define LOONGSON_PWM_REG_CTRL 0xc /* Control Register */ > > + > > +/* Control register bits */ > > +#define LOONGSON_PWM_CTRL_EN BIT(0) /* Counter Enable Bit */ > > +#define LOONGSON_PWM_CTRL_OE BIT(3) /* Pulse Output Enable Control Bit, Valid Low */ > > +#define LOONGSON_PWM_CTRL_SINGLE BIT(4) /* Single Pulse Control Bit */ > > +#define LOONGSON_PWM_CTRL_INTE BIT(5) /* Interrupt Enable Bit */ > > +#define LOONGSON_PWM_CTRL_INT BIT(6) /* Interrupt Bit */ > > +#define LOONGSON_PWM_CTRL_RST BIT(7) /* Counter Reset Bit */ > > +#define LOONGSON_PWM_CTRL_CAPTE BIT(8) /* Measurement Pulse Enable Bit */ > > +#define LOONGSON_PWM_CTRL_INVERT BIT(9) /* Output flip-flop Enable Bit */ > > +#define LOONGSON_PWM_CTRL_DZONE BIT(10) /* Anti-dead Zone Enable Bit */ > > Most of these are unused. And you only ever access the CTRL register > using read-modify-write. So I guess the behaviour of the hardware > depends on how the bootloader (or boot rom) initialized these bits. I > would prefer if you could this more deterministic. Emm, I would explicitly initialize the CTRL register value to 0 in probe(). > > > +#define LOONGSON_PWM_FREQ_STD (50 * HZ_PER_KHZ) > > Maybe it's just me, but I think the HZ_PER_KHZ doesn't add readability > and I would have just done: > > /* default input clk frequency for the ACPI case */ > #define LOONGSON_PWM_FREQ_DEFAULT 50000 /* Hz */ OK.... > > > [...] > > +static int pwm_loongson_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + int ret; > > + u64 period, duty_cycle; > > + bool enabled = pwm->state.enabled; > > + > > + if (!state->enabled) { > > + if (enabled) > > + pwm_loongson_disable(chip, pwm); > > + return 0; > > + } > > + > > + ret = pwm_loongson_set_polarity(chip, pwm, state->polarity); > > + if (ret) > > + return ret; > > Is setting the polarity shadowed in hardware or does it take effect > immediately? If the latter please mention that the output might glitch > on reconfiguration in the Limitations section above.. Another > "opportunity" to glitch is in pwm_loongson_config() above when > LOONGSON_PWM_REG_DUTY is written but LOONGSON_PWM_REG_PERIOD isn't yet. The setting of the polarity is shadowed in hardware, maybe we don't need to worry. > > > + period = min(state->period, NANOHZ_PER_HZ); > > + duty_cycle = min(state->duty_cycle, NANOHZ_PER_HZ); > > period and duty_cycle are measured in nanoseconds. So NSEC_PER_SEC is > more natural to them than NANOHZ_PER_HZ. OK... > > > + ret = pwm_loongson_config(chip, pwm, duty_cycle, period); > > + if (ret) > > + return ret; > > + > > + if (!enabled && state->enabled) > > + ret = pwm_loongson_enable(chip, pwm); > > + > > + return ret; > > +} > > [...] > > +static int pwm_loongson_probe(struct platform_device *pdev) > > +{ > > + int ret; > > + struct pwm_chip *chip; > > + struct pwm_loongson_ddata *ddata; > > + struct device *dev = &pdev->dev; > > + > > + chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata)); > > + if (IS_ERR(chip)) > > + return PTR_ERR(chip); > > + ddata = to_pwm_loongson_ddata(chip); > > + > > + 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 dev_err_probe(dev, PTR_ERR(ddata->clk), > > + "failed to get pwm clock\n"); > > + ddata->clk_rate = clk_get_rate(ddata->clk); > > I guess you rely on the clockrate to not change. So please add a call to > devm_clk_rate_exclusive_get(). > > > + } else { > > + ddata->clk_rate = LOONGSON_PWM_FREQ_STD; > > I thought that clk_get() also works for devices described by ACPI? > > Maybe something like this gives more flexibility: > > ddata->clk = devm_clk_get_optional_enabled(dev, NULL); > if (IS_ERR(ddata->clk)) > return dev_err_probe(...); > > if (ddata->clk) { > ret = devm_clk_rate_exclusive_get(...); > if (ret) > return ret; > > ddata->clk_rate = clk_get_rate(ddata->clk); > } else { > ddata->clk_rate = LOONGSON_PWM_FREQ_STD; > } > > and it's conceptually easier given that it doesn't have to care about > the device being described in ACPI or dt. > > Just a suggestion. OK, I will try to do it. > > > + } > > + > > + chip->ops = &pwm_loongson_ops; > > + chip->atomic = true; > > + dev_set_drvdata(dev, chip); > > + > > + ret = devm_pwmchip_add(dev, chip); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > > + > > + return 0; > > +} -- Thanks. Binbin
