Hello Ban, On Wed, Feb 03, 2021 at 08:53:17PM +0800, Ban Tao wrote: > v1->v2:
FTR: v1 wasn't sent to any list, so don't try to find it in some archive. > diff --git a/drivers/pwm/pwm-sun50i.c b/drivers/pwm/pwm-sun50i.c > new file mode 100644 > index 000000000000..37285d771924 > --- /dev/null > +++ b/drivers/pwm/pwm-sun50i.c > @@ -0,0 +1,348 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Driver for Allwinner sun50i Pulse Width Modulation Controller > + * > + * Copyright (C) 2020 Ban Tao <[email protected]> > + */ Please add a section here about how this PWM behaves. Things to cover: - Mention if the hardware cannot do 0% or 100% - Describe if changing the settings is racy, e.g. if it can happen when switching from duty_cycle=100 + period=1000 to duty_cycle=200 + period=2000 that we see a period with duty_cycle=100 and period=2000 or similar - Tell if on a reconfiguration the currently running period is completed. Please stick to the format used in other drivers to simplify grepping, see the Limitations section in drivers/pwm/pwm-sl28cpld.c for an example. > + > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/pwm.h> > +#include <linux/clk.h> > +#include <linux/reset.h> > + > +#define PWM_GET_CLK_OFFSET(chan) (0x20 + ((chan >> 1) * 0x4)) > +#define PWM_CLK_APB_SCR BIT(7) > +#define PWM_DIV_M 0 > +#define PWM_DIV_M_WIDTH 0x4 > + > +#define PWM_CLK_REG 0x40 > +#define PWM_CLK_GATING BIT(0) > + > +#define PWM_ENABLE_REG 0x80 > +#define PWM_EN BIT(0) > + > +#define PWM_CTL_REG(chan) (0x100 + 0x20 * chan) > +#define PWM_ACT_STA BIT(8) > +#define PWM_PRESCAL_K 0 > +#define PWM_PRESCAL_K_WIDTH 0x8 > + > +#define PWM_PERIOD_REG(chan) (0x104 + 0x20 * chan) > +#define PWM_ENTIRE_CYCLE 16 > +#define PWM_ENTIRE_CYCLE_WIDTH 0x10 > +#define PWM_ACT_CYCLE 0 > +#define PWM_ACT_CYCLE_WIDTH 0x10 Please use a driver specific prefix for these defines. > + > +#define BIT_CH(bit, chan) ((bit) << (chan)) > + > +#define SETMASK(width, shift) ((width?((-1U) >> > (32-width)):0) << (shift)) > +#define CLRMASK(width, shift) (~(SETMASK(width, shift))) > +#define GET_BITS(shift, width, reg) \ > + (((reg) & SETMASK(width, shift)) >> (shift)) > +#define SET_BITS(shift, width, reg, val) \ > + (((reg) & CLRMASK(width, shift)) | (val << (shift))) You're reinventing the stuff from <linux/bitfield.h> here. Please don't. > + > +#define PWM_OSC_CLK 24000000 > +#define PWM_PRESCALER_MAX 256 > +#define PWM_CLK_DIV_M__MAX 9 > +#define PWM_ENTIRE_CYCLE_MAX 65536 > + > +struct sun50i_pwm_data { > + unsigned int npwm; > +}; > + > +struct sun50i_pwm_chip { > + struct pwm_chip chip; > + struct clk *clk; > + struct reset_control *rst_clk; > + void __iomem *base; > + const struct sun50i_pwm_data *data; > +}; > + > +static inline struct sun50i_pwm_chip *sun50i_pwm_from_chip(struct pwm_chip > *chip) > +{ > + return container_of(chip, struct sun50i_pwm_chip, chip); > +} > + > +static inline u32 sun50i_pwm_readl(struct sun50i_pwm_chip *chip, > + unsigned long offset) > +{ > + return readl(chip->base + offset); > +} > + > +static inline void sun50i_pwm_writel(struct sun50i_pwm_chip *chip, > + u32 val, unsigned long offset) > +{ > + writel(val, chip->base + offset); > +} > + > +static int sun50i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device > *pwm, > + enum pwm_polarity polarity) > +{ > + struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip); > + u32 temp; > + > + temp = sun50i_pwm_readl(sun50i_pwm, PWM_CTL_REG(pwm->hwpwm)); > + > + if (polarity == PWM_POLARITY_NORMAL) > + temp |= PWM_ACT_STA; > + else > + temp &= ~PWM_ACT_STA; > + > + sun50i_pwm_writel(sun50i_pwm, temp, PWM_CTL_REG(pwm->hwpwm)); > + > + return 0; > +} For v1 I asked to test this driver with PWM_DEBUG enabled and to fix all emitted warnings. This didn't happen :-\ > +static int sun50i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > + int duty_ns, int period_ns) Please align to the opening brace in the line above. > +{ > + struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip); > + unsigned long long c; > + unsigned long entire_cycles, active_cycles; > + unsigned int div_m, prescaler; > + u32 tmp; > + > + if (period_ns > 334) { > + /* if freq < 3M, then select 24M clock */ > + c = PWM_OSC_CLK; > + tmp = sun50i_pwm_readl(sun50i_pwm, > PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + tmp &= ~PWM_CLK_APB_SCR; > + sun50i_pwm_writel(sun50i_pwm, tmp, > PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + } else { > + /* if freq > 3M, then select APB as clock */ > + c = clk_get_rate(sun50i_pwm->clk); > + tmp = sun50i_pwm_readl(sun50i_pwm, > PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + tmp |= PWM_CLK_APB_SCR; > + sun50i_pwm_writel(sun50i_pwm, tmp, > PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + } > + > + dev_dbg(chip->dev, "duty_ns=%d period_ns=%d c =%llu.\n", > + duty_ns, period_ns, c); Inconsistent spacing in the message. > + > + /* > + * (clk / div_m / prescaler) / entire_cycles = NSEC_PER_SEC / period_ns. > + * So, entire_cycles = clk * period_ns / NSEC_PER_SEC / div_m / > prescaler. > + */ > + c = c * period_ns; > + do_div(c, NSEC_PER_SEC); > + for (div_m = 0; div_m < PWM_CLK_DIV_M__MAX; div_m++) { > + for (prescaler = 0; prescaler < PWM_PRESCALER_MAX; prescaler++) > { The calculation could be done without this double-for loop. Something like: div_m = order_base_2(PWM_ENTIRE_CYCLE_MAX * PWM_PRESCALER_MAX / c); if (div_m >= PWM_CLK_DIV_M__MAX) bail out; prescaler = DIV_ROUND_UP(c << div_m, PWM_ENTIRE_CYCLE_MAX) - 1; (but please double check, I didn't.) Also note that this doesn't yield the best approximation for period in general. (But that's ok for now, maybe just mention it in a comment.) > + /* > + * actual prescaler = prescaler + 1. > + * actual div_m = 0x1 << div_m. > + */ > + entire_cycles = ((unsigned long)c/(0x1 << > div_m))/(prescaler + 1); c / (1 << div_m) can be written as c >> div_m which reads better and might result in better code. > + if (entire_cycles <= PWM_ENTIRE_CYCLE_MAX) { > + goto calc_end; > + } No { } for one line bodys please. > + } > + } Missing error handling for the case that c >> (PWM_CLK_DIV_M__MAX - 1) / PWM_PRESCALER_MAX > PWM_ENTIRE_CYCLE_MAX > + > +calc_end: > + /* > + * duty_ns / period_ns = active_cycles / entire_cycles. > + * So, active_cycles = entire_cycles * duty_ns / period_ns. active_cyles is the number of clock steps for duty_cycle, right? > + */ > + c = (unsigned long long)entire_cycles * duty_ns; > + do_div(c, period_ns); > + active_cycles = c; > + if (entire_cycles == 0) > + entire_cycles++; > + > + /* enable clk gating */ > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG); > + tmp |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG); > + > + /* config clk div_m*/ > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + tmp = SET_BITS(PWM_DIV_M, PWM_DIV_M_WIDTH, tmp, div_m); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_GET_CLK_OFFSET(pwm->hwpwm)); > + > + /* config prescal */ prescale > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CTL_REG(pwm->hwpwm)); > + tmp = SET_BITS(PWM_PRESCAL_K, PWM_PRESCAL_K_WIDTH, tmp, prescaler); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CTL_REG(pwm->hwpwm)); > + > + /* config active and period cycles */ > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_PERIOD_REG(pwm->hwpwm)); > + tmp = SET_BITS(PWM_ACT_CYCLE, PWM_ACT_CYCLE_WIDTH, tmp, active_cycles); > + tmp = SET_BITS(PWM_ENTIRE_CYCLE, PWM_ENTIRE_CYCLE_WIDTH, tmp, > (entire_cycles - 1)); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_PERIOD_REG(pwm->hwpwm)); > + > + dev_dbg(chip->dev, "active_cycles=%lu entire_cycles=%lu prescaler=%u > div_m=%u\n", > + active_cycles, entire_cycles, prescaler, div_m); align to opening brace. > + > + return 0; > +} > + > +static int sun50i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip); > + u32 tmp; > + > + /* enable pwm controller */ > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_ENABLE_REG); > + tmp |= BIT_CH(PWM_EN, pwm->hwpwm); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_ENABLE_REG); > + > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG); > + tmp |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG); > + > + return 0; > +} > + > +static void sun50i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct sun50i_pwm_chip *sun50i_pwm = sun50i_pwm_from_chip(chip); > + u32 tmp; > + > + /* disable pwm controller */ > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_ENABLE_REG); > + tmp &= ~BIT_CH(PWM_EN, pwm->hwpwm); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_ENABLE_REG); > + > + tmp = sun50i_pwm_readl(sun50i_pwm, PWM_CLK_REG); > + tmp &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm); > + sun50i_pwm_writel(sun50i_pwm, tmp, PWM_CLK_REG); > +} > + > +static const struct pwm_ops sun50i_pwm_ops = { > + .config = sun50i_pwm_config, > + .enable = sun50i_pwm_enable, > + .disable = sun50i_pwm_disable, > + .set_polarity = sun50i_pwm_set_polarity, > + .owner = THIS_MODULE, > +}; > + > +static const struct sun50i_pwm_data sun8i_pwm_data_c9 = { > + .npwm = 9, > +}; > + > +static const struct sun50i_pwm_data sun50i_pwm_data_c16 = { > + .npwm = 16, > +}; > + > +static const struct of_device_id sun50i_pwm_dt_ids[] = { > + { > + .compatible = "allwinner,sun8i-v833-pwm", > + .data = &sun8i_pwm_data_c9, > + }, { > + .compatible = "allwinner,sun8i-v536-pwm", > + .data = &sun8i_pwm_data_c9, > + }, { > + .compatible = "allwinner,sun50i-r818-pwm", > + .data = &sun50i_pwm_data_c16, > + }, { > + .compatible = "allwinner,sun50i-a133-pwm", > + .data = &sun50i_pwm_data_c16, > + }, { > + .compatible = "allwinner,sun50i-r329-pwm", > + .data = &sun8i_pwm_data_c9, > + }, { > + /* sentinel */ > + }, > +}; > +MODULE_DEVICE_TABLE(of, sun50i_pwm_dt_ids); > + > +static int sun50i_pwm_probe(struct platform_device *pdev) > +{ > + struct sun50i_pwm_chip *pwm; "pwm" isn't a good name, in PWM code this name is usually used for struct pwm_device pointers (and sometimes the global pwm id). I usually use "ddata" for driver data (and would have called "sun50i_pwm_chip" "sun50i_pwm_ddata" instead). Another common name is "priv". > + int ret; > + > + pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL); > + if (!pwm) > + return -ENOMEM; > + > + pwm->data = of_device_get_match_data(&pdev->dev); > + if (!pwm->data) Error message here > + return -ENODEV; > + > + pwm->base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(pwm->base)) > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->base), > + "can't remap pwm resource\n"); > + > + pwm->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pwm->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), > + "get unnamed clock failed\n"); s/unnamed/PWM/ ? > + > + pwm->rst_clk = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + if (IS_ERR(pwm->rst_clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(pwm->rst_clk), > + "get reset failed\n"); > + > + /* Deassert reset */ > + ret = reset_control_deassert(pwm->rst_clk); > + if (ret) { > + dev_err(&pdev->dev, "cannot deassert reset control: %pe\n", > + ERR_PTR(ret)); > + return ret; Even though reset_control_deassert probably will never return -EPROBE_DEFER, you should use dev_err_probe here to for consistency. > + } > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) { > + dev_err(&pdev->dev, "cannot prepare and enable clk %pe\n", > + ERR_PTR(ret)); > + goto err_clk; > + } > + > + pwm->chip.dev = &pdev->dev; > + pwm->chip.ops = &sun50i_pwm_ops; > + pwm->chip.npwm = pwm->data->npwm; > + pwm->chip.of_xlate = of_pwm_xlate_with_flags; > + pwm->chip.base = -1; > + pwm->chip.of_pwm_n_cells = 3; > + > [...] Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
