| Message ID | 1552378289-27245-3-git-send-email-yash.shah@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM support for HiFive Unleashed | expand |
Hello, there are just a few minor things left I commented below. On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote: > +#define div_u64_round(a, b) \ > + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); }) Parenthesis around b please. I guess I didn't had them in my suggestion either, sorry for that. > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable) > +{ > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); > + int ret; > + > + if (enable) { > + ret = clk_enable(pwm->clk); > + if (ret) { > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); > + return ret; > + } > + } > + > + if (!enable) > + clk_disable(pwm->clk); > + > + return 0; > +} There is only a single caller for this function. I wonder if it is trivial enough to fold it into pwm_sifive_apply. > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev, > + struct pwm_state *state) > +{ > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); > + unsigned int duty_cycle; > + u32 frac; > + struct pwm_state cur_state; > + bool enabled; > + int ret = 0; > + unsigned long num; > + > + if (state->polarity != PWM_POLARITY_INVERSED) > + return -EINVAL; > + > + ret = clk_enable(pwm->clk); > + if (ret) { > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); > + return ret; > + } > + > + mutex_lock(&pwm->lock); > + pwm_get_state(dev, &cur_state); > + > + enabled = cur_state.enabled; > + > + if (state->period != pwm->approx_period) { > + if (pwm->user_count != 1) { > + ret = -EBUSY; > + goto exit; > + } > + pwm->approx_period = state->period; > + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk)); > + } > + > + duty_cycle = state->duty_cycle; > + if (!state->enabled) > + duty_cycle = 0; > + > + num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH); > + frac = div_u64_round(num, state->period); > + /* The hardware cannot generate a 100% duty cycle */ > + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > + > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + > + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP); > + > + if (state->enabled != enabled) { > + ret = pwm_sifive_enable(chip, state->enabled); > + if (ret) > + goto exit; This goto is a noop. > + } > + > +exit: > + clk_disable(pwm->clk); > + mutex_unlock(&pwm->lock); > + return ret; > +} There are a few other things that could be improved, but I think they could be addressed later. For some of these I don't even know what to suggest, for some Thierry might not agree it is worth fixing: - rounding how to round? When should a request declined, when is rounding ok? There is still "if (state->period != pwm->approx_period) return -EBUSY" in this driver. This is better than before, but if state-period == pwm->approx_period + 1 the result (if accepted) might be the same as without the +1 and so returning -EBUSY for one case and accepting the other is strange. - don't call PWM API functions designed for consumers (here: pwm_get_state) - Move div_u64_round to include/linux/math64.h Best regards Uwe
On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote: > Hello, > > there are just a few minor things left I commented below. > > On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote: > > +#define div_u64_round(a, b) \ > > + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); }) > > Parenthesis around b please. I guess I didn't had them in my suggestion > either, sorry for that. We don't really need the parentheses here, do we? The only operator that has lower priority than the assignment is the comma operator, and that's not going to work in the macro anyway, unless you put it inside a pair of parentheses, in which case, well, you have the parentheses already. > > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable) > > +{ > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); > > + int ret; > > + > > + if (enable) { > > + ret = clk_enable(pwm->clk); > > + if (ret) { > > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); > > + return ret; > > + } > > + } > > + > > + if (!enable) > > + clk_disable(pwm->clk); > > + > > + return 0; > > +} > > There is only a single caller for this function. I wonder if it is > trivial enough to fold it into pwm_sifive_apply. I think this is fine. pwm_sifive_apply() is already fairly long at this point, so might as well split things up a little. > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev, > > + struct pwm_state *state) > > +{ > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); > > + unsigned int duty_cycle; > > + u32 frac; > > + struct pwm_state cur_state; > > + bool enabled; > > + int ret = 0; > > + unsigned long num; > > + > > + if (state->polarity != PWM_POLARITY_INVERSED) > > + return -EINVAL; > > + > > + ret = clk_enable(pwm->clk); > > + if (ret) { > > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); > > + return ret; > > + } > > + > > + mutex_lock(&pwm->lock); > > + pwm_get_state(dev, &cur_state); > > + > > + enabled = cur_state.enabled; > > + > > + if (state->period != pwm->approx_period) { > > + if (pwm->user_count != 1) { > > + ret = -EBUSY; > > + goto exit; > > + } > > + pwm->approx_period = state->period; > > + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk)); > > + } > > + > > + duty_cycle = state->duty_cycle; > > + if (!state->enabled) > > + duty_cycle = 0; > > + > > + num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH); > > + frac = div_u64_round(num, state->period); > > + /* The hardware cannot generate a 100% duty cycle */ > > + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); > > + > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + > > + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP); > > + > > + if (state->enabled != enabled) { > > + ret = pwm_sifive_enable(chip, state->enabled); > > + if (ret) > > + goto exit; > > This goto is a noop. > > > + } > > + > > +exit: > > + clk_disable(pwm->clk); > > + mutex_unlock(&pwm->lock); > > + return ret; > > +} > > There are a few other things that could be improved, but I think they > could be addressed later. For some of these I don't even know what to > suggest, for some Thierry might not agree it is worth fixing: > > - rounding > how to round? When should a request declined, when is rounding ok? > There is still "if (state->period != pwm->approx_period) return -EBUSY" > in this driver. This is better than before, but if state-period == > pwm->approx_period + 1 the result (if accepted) might be the same as > without the +1 and so returning -EBUSY for one case and accepting the > other is strange. Perhaps a good idea would be to reject a configuration only after we've determined that it is incompatible? If we're really going to end up with the same configuration within a given margin of period or duty cycle and we can't do much about it, there's little point in rejecting such configurations. > - don't call PWM API functions designed for consumers (here: pwm_get_state) Agreed. The driver can just access pwm_device.state directly. > - Move div_u64_round to include/linux/math64.h Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this. The only difference that I can see is that the divisor is 32-bit, but since we pass in state->period, that already works fine. Thierry
On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote: > On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote: > > Hello, > > > > there are just a few minor things left I commented below. > > > > On Tue, Mar 12, 2019 at 01:41:29PM +0530, Yash Shah wrote: > > > +#define div_u64_round(a, b) \ > > > + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); }) > > > > Parenthesis around b please. I guess I didn't had them in my suggestion > > either, sorry for that. > > We don't really need the parentheses here, do we? The only operator that > has lower priority than the assignment is the comma operator, and that's > not going to work in the macro anyway, unless you put it inside a pair > of parentheses, in which case, well, you have the parentheses already. I thought that, too, but using parenthesis just always is a safe bet and prevents people stumbling over this and spending time to come to the conclusion that it is actually safe without them. > > > +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable) > > > +{ > > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); > > > + int ret; > > > + > > > + if (enable) { > > > + ret = clk_enable(pwm->clk); > > > + if (ret) { > > > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); > > > + return ret; > > > + } > > > + } > > > + > > > + if (!enable) > > > + clk_disable(pwm->clk); > > > + > > > + return 0; > > > +} > > > > There is only a single caller for this function. I wonder if it is > > trivial enough to fold it into pwm_sifive_apply. > > I think this is fine. pwm_sifive_apply() is already fairly long at this > point, so might as well split things up a little. I don't have a strong opinion here, so keeping as is is fine for me. > > There are a few other things that could be improved, but I think they > > could be addressed later. For some of these I don't even know what to > > suggest, for some Thierry might not agree it is worth fixing: > > > > - rounding > > how to round? When should a request declined, when is rounding ok? > > There is still "if (state->period != pwm->approx_period) return -EBUSY" > > in this driver. This is better than before, but if state-period == > > pwm->approx_period + 1 the result (if accepted) might be the same as > > without the +1 and so returning -EBUSY for one case and accepting the > > other is strange. > > Perhaps a good idea would be to reject a configuration only after we've > determined that it is incompatible? If we're really going to end up with > the same configuration within a given margin of period or duty cycle and > we can't do much about it, there's little point in rejecting such > configurations. It seems we agree here. Is this important enough to delay taking this driver further? Currently the driver rejects too broad so if it annoys someone this can still be fixed later and there is only little harm (assuming correct error handling in the consumers). > > - don't call PWM API functions designed for consumers (here: pwm_get_state) > > Agreed. The driver can just access pwm_device.state directly. I wouldn't do this either. IMHO the driver should only look into its hardware registers instead of using framework interna (or consumer API calls). > > - Move div_u64_round to include/linux/math64.h > > Looks to me like DIV_ROUND_CLOSEST_ULL() is already pretty much this. > The only difference that I can see is that the divisor is 32-bit, but > since we pass in state->period, that already works fine. num (i.e. the divident) should be a u64, but it isn't. This needs fixing. But I agree that DIV_ROUND_CLOSEST_ULL should be good enough then. Best regards Uwe
On Tue, Mar 12, 2019 at 02:17:12PM +0100, Uwe Kleine-König wrote: > On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote: > > On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote: [...] > > > There are a few other things that could be improved, but I think they > > > could be addressed later. For some of these I don't even know what to > > > suggest, for some Thierry might not agree it is worth fixing: > > > > > > - rounding > > > how to round? When should a request declined, when is rounding ok? > > > There is still "if (state->period != pwm->approx_period) return -EBUSY" > > > in this driver. This is better than before, but if state-period == > > > pwm->approx_period + 1 the result (if accepted) might be the same as > > > without the +1 and so returning -EBUSY for one case and accepting the > > > other is strange. > > > > Perhaps a good idea would be to reject a configuration only after we've > > determined that it is incompatible? If we're really going to end up with > > the same configuration within a given margin of period or duty cycle and > > we can't do much about it, there's little point in rejecting such > > configurations. > > It seems we agree here. Is this important enough to delay taking this > driver further? Currently the driver rejects too broad so if it annoys > someone this can still be fixed later and there is only little harm > (assuming correct error handling in the consumers). I don't think it has to be a blocker. As you said, we'd be giving users more flexibility, not restricting them, so it should be fine to do later on. > > > - don't call PWM API functions designed for consumers (here: pwm_get_state) > > > > Agreed. The driver can just access pwm_device.state directly. > > I wouldn't do this either. IMHO the driver should only look into its > hardware registers instead of using framework interna (or consumer API > calls). The current hardware state is already the software representation of what the hardware has programmed. I think it's fair for drivers to make use of that in order to avoid having to read back from hardware. Especially if reading back from hardware might require switching on the module to access registers. Thierry
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index a8f47df..4a61d1a 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -380,6 +380,17 @@ config PWM_SAMSUNG To compile this driver as a module, choose M here: the module will be called pwm-samsung. +config PWM_SIFIVE + tristate "SiFive PWM support" + depends on OF + depends on COMMON_CLK + depends on RISCV || COMPILE_TEST + help + Generic PWM framework driver for SiFive SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sifive. + config PWM_SPEAR tristate "STMicroelectronics SPEAr PWM support" depends on PLAT_SPEAR diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 9c676a0..30089ca 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -37,6 +37,7 @@ obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o +obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o obj-$(CONFIG_PWM_STI) += pwm-sti.o obj-$(CONFIG_PWM_STM32) += pwm-stm32.o diff --git a/drivers/pwm/pwm-sifive.c b/drivers/pwm/pwm-sifive.c new file mode 100644 index 0000000..4233557 --- /dev/null +++ b/drivers/pwm/pwm-sifive.c @@ -0,0 +1,338 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017-2018 SiFive + * For SiFive's PWM IP block documentation please refer Chapter 14 of + * Reference Manual : https://static.dev.sifive.com/FU540-C000-v1.0.pdf + * + * Limitations: + * - When changing both duty cycle and period, we cannot prevent in + * software that the output might produce a period with mixed + * settings (new period length and old duty cycle). + * - The hardware cannot generate a 100% duty cycle. + * - The hardware generates only inverted output. + */ +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/bitfield.h> + +/* Register offsets */ +#define PWM_SIFIVE_PWMCFG 0x0 +#define PWM_SIFIVE_PWMCOUNT 0x8 +#define PWM_SIFIVE_PWMS 0x10 +#define PWM_SIFIVE_PWMCMP0 0x20 + +/* PWMCFG fields */ +#define PWM_SIFIVE_PWMCFG_SCALE GENMASK(3, 0) +#define PWM_SIFIVE_PWMCFG_STICKY BIT(8) +#define PWM_SIFIVE_PWMCFG_ZERO_CMP BIT(9) +#define PWM_SIFIVE_PWMCFG_DEGLITCH BIT(10) +#define PWM_SIFIVE_PWMCFG_EN_ALWAYS BIT(12) +#define PWM_SIFIVE_PWMCFG_EN_ONCE BIT(13) +#define PWM_SIFIVE_PWMCFG_CENTER BIT(16) +#define PWM_SIFIVE_PWMCFG_GANG BIT(24) +#define PWM_SIFIVE_PWMCFG_IP BIT(28) + +/* PWM_SIFIVE_SIZE_PWMCMP is used to calculate offset for pwmcmpX registers */ +#define PWM_SIFIVE_SIZE_PWMCMP 4 +#define PWM_SIFIVE_CMPWIDTH 16 +#define PWM_SIFIVE_DEFAULT_PERIOD 10000000 + +#define div_u64_round(a, b) \ + ({typeof(b) __b = b; div_u64((a) + __b / 2, __b); }) + +struct pwm_sifive_ddata { + struct pwm_chip chip; + struct mutex lock; /* lock to protect user_count */ + struct notifier_block notifier; + struct clk *clk; + void __iomem *regs; + unsigned int real_period; + unsigned int approx_period; + int user_count; +}; + +static inline +struct pwm_sifive_ddata *pwm_sifive_chip_to_ddata(struct pwm_chip *c) +{ + return container_of(c, struct pwm_sifive_ddata, chip); +} + +static int pwm_sifive_request(struct pwm_chip *chip, struct pwm_device *dev) +{ + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); + + mutex_lock(&pwm->lock); + pwm->user_count++; + mutex_unlock(&pwm->lock); + + return 0; +} + +static void pwm_sifive_free(struct pwm_chip *chip, struct pwm_device *dev) +{ + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); + + mutex_lock(&pwm->lock); + pwm->user_count--; + mutex_unlock(&pwm->lock); +} + +static void pwm_sifive_update_clock(struct pwm_sifive_ddata *pwm, + unsigned long rate) +{ + u32 val; + unsigned long long num; + /* + * The PWM unit is used with pwmzerocmp=0, so the only way to modify the + * period length is using pwmscale which provides the number of bits the + * counter is shifted before being feed to the comparators. A period + * lasts (1 << (PWM_SIFIVE_CMPWIDTH + pwmscale)) clock ticks. + * (1 << (PWM_SIFIVE_CMPWIDTH + scale)) * 10^9/rate = period + */ + unsigned long scale_pow = + div64_ul(pwm->approx_period * (u64)rate, NSEC_PER_SEC); + int scale = clamp(ilog2(scale_pow) - PWM_SIFIVE_CMPWIDTH, 0, 0xf); + + val = PWM_SIFIVE_PWMCFG_EN_ALWAYS | + FIELD_PREP(PWM_SIFIVE_PWMCFG_SCALE, scale); + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG); + + /* As scale <= 15 the shift operation cannot overflow. */ + num = (unsigned long long)NSEC_PER_SEC << (PWM_SIFIVE_CMPWIDTH + scale); + pwm->real_period = div64_ul(num, rate); + dev_dbg(pwm->chip.dev, "New real_period = %u ns\n", pwm->real_period); +} + +static void pwm_sifive_get_state(struct pwm_chip *chip, struct pwm_device *dev, + struct pwm_state *state) +{ + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); + u32 duty, val; + + duty = readl(pwm->regs + PWM_SIFIVE_PWMCMP0 + + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP); + + state->enabled = duty > 0; + + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG); + if (!(val & PWM_SIFIVE_PWMCFG_EN_ALWAYS)) + state->enabled = false; + + state->period = pwm->real_period; + state->duty_cycle = + (u64)duty * pwm->real_period >> PWM_SIFIVE_CMPWIDTH; + state->polarity = PWM_POLARITY_INVERSED; +} + +static int pwm_sifive_enable(struct pwm_chip *chip, bool enable) +{ + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); + int ret; + + if (enable) { + ret = clk_enable(pwm->clk); + if (ret) { + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); + return ret; + } + } + + if (!enable) + clk_disable(pwm->clk); + + return 0; +} + +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev, + struct pwm_state *state) +{ + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip); + unsigned int duty_cycle; + u32 frac; + struct pwm_state cur_state; + bool enabled; + int ret = 0; + unsigned long num; + + if (state->polarity != PWM_POLARITY_INVERSED) + return -EINVAL; + + ret = clk_enable(pwm->clk); + if (ret) { + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret); + return ret; + } + + mutex_lock(&pwm->lock); + pwm_get_state(dev, &cur_state); + + enabled = cur_state.enabled; + + if (state->period != pwm->approx_period) { + if (pwm->user_count != 1) { + ret = -EBUSY; + goto exit; + } + pwm->approx_period = state->period; + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk)); + } + + duty_cycle = state->duty_cycle; + if (!state->enabled) + duty_cycle = 0; + + num = (u64)duty_cycle * (1U << PWM_SIFIVE_CMPWIDTH); + frac = div_u64_round(num, state->period); + /* The hardware cannot generate a 100% duty cycle */ + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1); + + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + + dev->hwpwm * PWM_SIFIVE_SIZE_PWMCMP); + + if (state->enabled != enabled) { + ret = pwm_sifive_enable(chip, state->enabled); + if (ret) + goto exit; + } + +exit: + clk_disable(pwm->clk); + mutex_unlock(&pwm->lock); + return ret; +} + +static const struct pwm_ops pwm_sifive_ops = { + .request = pwm_sifive_request, + .free = pwm_sifive_free, + .get_state = pwm_sifive_get_state, + .apply = pwm_sifive_apply, + .owner = THIS_MODULE, +}; + +static int pwm_sifive_clock_notifier(struct notifier_block *nb, + unsigned long event, void *data) +{ + struct clk_notifier_data *ndata = data; + struct pwm_sifive_ddata *pwm = + container_of(nb, struct pwm_sifive_ddata, notifier); + + if (event == POST_RATE_CHANGE) + pwm_sifive_update_clock(pwm, ndata->new_rate); + + return NOTIFY_OK; +} + +static int pwm_sifive_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct pwm_sifive_ddata *pwm; + struct pwm_chip *chip; + struct resource *res; + int ret; + + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + + mutex_init(&pwm->lock); + chip = &pwm->chip; + chip->dev = dev; + chip->ops = &pwm_sifive_ops; + chip->of_pwm_n_cells = 3; + chip->base = -1; + chip->npwm = 4; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + pwm->regs = devm_ioremap_resource(dev, res); + if (IS_ERR(pwm->regs)) { + dev_err(dev, "Unable to map IO resources\n"); + return PTR_ERR(pwm->regs); + } + + pwm->clk = devm_clk_get(dev, NULL); + if (IS_ERR(pwm->clk)) { + if (PTR_ERR(pwm->clk) != -EPROBE_DEFER) + dev_err(dev, "Unable to find controller clock\n"); + return PTR_ERR(pwm->clk); + } + + ret = clk_prepare_enable(pwm->clk); + if (ret) { + dev_err(dev, "failed to enable clock for pwm: %d\n", ret); + return ret; + } + + /* Watch for changes to underlying clock frequency */ + pwm->notifier.notifier_call = pwm_sifive_clock_notifier; + ret = clk_notifier_register(pwm->clk, &pwm->notifier); + if (ret) { + dev_err(dev, "failed to register clock notifier: %d\n", ret); + goto disable_clk; + } + + /* Initialize PWM */ + pwm->approx_period = PWM_SIFIVE_DEFAULT_PERIOD; + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk)); + + ret = pwmchip_add(chip); + if (ret < 0) { + dev_err(dev, "cannot register PWM: %d\n", ret); + goto unregister_clk; + } + + platform_set_drvdata(pdev, pwm); + dev_dbg(dev, "SiFive PWM chip registered %d PWMs\n", chip->npwm); + + return 0; + +unregister_clk: + clk_notifier_unregister(pwm->clk, &pwm->notifier); +disable_clk: + clk_disable_unprepare(pwm->clk); + + return ret; +} + +static int pwm_sifive_remove(struct platform_device *dev) +{ + struct pwm_sifive_ddata *pwm = platform_get_drvdata(dev); + int ret, ch; + bool is_enabled = false; + + ret = pwmchip_remove(&pwm->chip); + clk_notifier_unregister(pwm->clk, &pwm->notifier); + + for (ch = 0; ch < pwm->chip.npwm; ch++) { + if (pwm_is_enabled(&pwm->chip.pwms[ch])) { + is_enabled = true; + break; + } + } + if (is_enabled) + clk_disable(pwm->clk); + clk_unprepare(pwm->clk); + return ret; +} + +static const struct of_device_id pwm_sifive_of_match[] = { + { .compatible = "sifive,pwm0" }, + {}, +}; +MODULE_DEVICE_TABLE(of, pwm_sifive_of_match); + +static struct platform_driver pwm_sifive_driver = { + .probe = pwm_sifive_probe, + .remove = pwm_sifive_remove, + .driver = { + .name = "pwm-sifive", + .of_match_table = pwm_sifive_of_match, + }, +}; +module_platform_driver(pwm_sifive_driver); + +MODULE_DESCRIPTION("SiFive PWM driver"); +MODULE_LICENSE("GPL v2");