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. > +#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 */ > [...] > +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. > + 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. > + 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. > + } > + > + 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; > +}
Attachment:
signature.asc
Description: PGP signature
