| Message ID | 20220907131241.31941-3-romain.perier@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add PWM for MStar SoCs | expand |
Hello Romain, hello Daniel, adding Mark Brown to Cc: for the regmap stuff. On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote: > From: Daniel Palmer <daniel@0x0f.com> > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > Co-developed-by: Romain Perier <romain.perier@gmail.com> > Signed-off-by: Romain Perier <romain.perier@gmail.com> > --- > MAINTAINERS | 1 + > drivers/pwm/Kconfig | 9 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 280 insertions(+) > create mode 100644 drivers/pwm/pwm-msc313e.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9d7f64dc0efe..c3b39b09097c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2439,6 +2439,7 @@ F: arch/arm/mach-mstar/ > F: drivers/clk/mstar/ > F: drivers/clocksource/timer-msc313e.c > F: drivers/gpio/gpio-msc313.c > +F: drivers/pwm/pwm-msc313e.c > F: drivers/rtc/rtc-msc313.c > F: drivers/watchdog/msc313e_wdt.c > F: include/dt-bindings/clock/mstar-* > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 60d13a949bc5..8049fd03a821 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -372,6 +372,15 @@ config PWM_MESON > To compile this driver as a module, choose M here: the module > will be called pwm-meson. > > +config PWM_MSC313E > + tristate "MStar MSC313e PWM support" > + depends on ARCH_MSTARV7 || COMPILE_TEST > + help > + Generic PWM framework driver for MSTAR MSC313e. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-msc313e. > + > config PWM_MTK_DISP > tristate "MediaTek display PWM driver" > depends on ARCH_MEDIATEK || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 7bf1a29f02b8..bc285c054f2a 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > +obj-$(CONFIG_PWM_MSC313E) += pwm-msc313e.o > obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c > new file mode 100644 > index 000000000000..a71f39ba66c3 > --- /dev/null > +++ b/drivers/pwm/pwm-msc313e.c > @@ -0,0 +1,269 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/of_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > + > +#define DRIVER_NAME "msc313e-pwm" > + > +#define CHANNEL_OFFSET 0x80 > +#define REG_DUTY 0x8 > +#define REG_PERIOD 0x10 > +#define REG_DIV 0x18 > +#define REG_CTRL 0x1c > +#define REG_SWRST 0x1fc > + > +struct msc313e_pwm_channel { > + struct regmap_field *clkdiv; > + struct regmap_field *polarity; > + struct regmap_field *dutyl; > + struct regmap_field *dutyh; > + struct regmap_field *periodl; > + struct regmap_field *periodh; > + struct regmap_field *swrst; > +}; > + > +struct msc313e_pwm { > + struct regmap *regmap; > + struct pwm_chip pwmchip; > + struct clk *clk; > + struct msc313e_pwm_channel channels[]; > +}; > + > +struct msc313e_pwm_info { > + unsigned int channels; > +}; > + > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip) > + > +static const struct regmap_config msc313e_pwm_regmap_config = { > + .reg_bits = 16, > + .val_bits = 16, > + .reg_stride = 4, > +}; > + > +static const struct msc313e_pwm_info msc313e_data = { > + .channels = 8, > +}; > + > +static const struct msc313e_pwm_info ssd20xd_data = { > + .channels = 4, > +}; > + > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value) > +{ > + /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into Please fix the comment style to use /* on a line for itself. Also for comments staying below 80 chars per line is appreciated. > + * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter > + * we are about to write has this contrainst. s/contrainst/contraint/ I wonder if that could be abstracted by regmap?! > + */ > + regmap_field_write(low, value & 0xffff); > + regmap_field_write(high, value >> 16); > +} > + > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value) > +{ > + unsigned int val = 0; > + > + regmap_field_read(low, &val); > + *value = val; > + regmap_field_read(high, &val); > + *value = (val << 16) | *value; > +} > + > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device, > + int duty_ns, int period_ns) > +{ > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > + unsigned long long div = 1; > + > + /* Fit the period into the period register by prescaling the clk */ > + while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) { dividing by the result of a division looses precision. Also rounding down both divisions looks wrong. > + div++; > + if (div > (0xffff + 1)) { > + /* Force clk div to the maximum allowed value */ > + div = 0xffff; > + break; > + } > + nspertick = DIV_ROUND_DOWN_ULL(nspertick, div); > + } > + > + regmap_field_write(channel->clkdiv, div - 1); > + msc313e_pwm_writecounter(channel->dutyl, channel->dutyh, > + DIV_ROUND_DOWN_ULL(duty_ns, nspertick)); > + msc313e_pwm_writecounter(channel->periodl, channel->periodh, > + DIV_ROUND_DOWN_ULL(period_ns, nspertick)); > + return 0; > +}; > + > +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device, > + enum pwm_polarity polarity) > +{ > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > + unsigned int pol = 0; > + > + if (polarity == PWM_POLARITY_INVERSED) > + pol = 1; > + regmap_field_update_bits(channel->polarity, 1, pol); > + > + return 0; > +} > + > +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device) > +{ > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > + int ret; > + > + ret = clk_prepare_enable(pwm->clk); > + if (ret) > + return ret; > + return regmap_field_write(channel->swrst, 0); > +} > + > +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device) > +{ > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > + int ret; > + > + ret = regmap_field_write(channel->swrst, 1); > + clk_disable_unprepare(pwm->clk); > + return ret; > +} > + > +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + int ret; > + > + if (state->enabled) { > + if (!pwm->state.enabled) { > + ret = msc313e_pwm_enable(chip, pwm); > + if (ret) > + return ret; > + } > + msc313e_pwm_set_polarity(chip, pwm, state->polarity); > + msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period); Does the output emit a previous setting after msc313e_pwm_enable()? If so, please do the enable at the end. Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an immediate effect? Or is the currently running period completed? > + } else if (pwm->state.enabled) { > + ret = msc313e_pwm_disable(chip, pwm); How does the output behave on disable? Typical options are: - freezes where ever it currently is - goes to the inactive level Does it complete the currently running period? Please answer these in a "Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a. > + } > + return 0; > +} > + > +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device, Please s/device/pwm/ to match the naming used in other drivers. > + struct pwm_state *state) > +{ > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); > + unsigned int val = 0; > + > + regmap_field_read(channel->polarity, &val); > + state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; > + > + regmap_field_read(channel->swrst, &val); > + state->enabled = val == 0 ? true : false; > + > + msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val); > + state->duty_cycle = val * nspertick; Please do the division at the end. Consider val = 333335 and clk_get_rate() returning 666666667. Then the exact duty cycle would be 500002.49974999874 but you would calculate nspertick = 1 and so duty_cycle = 333335. Also you need to round up. Did you test your driver with PWM_DEBUG enabled? The idea is that .get_state should return 500003 here to have apply ∘ get_state idempotent. As apply is supposed to round down to the next available setting, get_state has to round up. > + msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val); > + state->period = val * nspertick; > +} > + > +static const struct pwm_ops msc313e_pwm_ops = { > + .apply = msc313e_apply, > + .get_state = msc313e_get_state, > + .owner = THIS_MODULE > +}; > + > +static int msc313e_pwm_probe(struct platform_device *pdev) > +{ > + const struct msc313e_pwm_info *match_data; > + struct device *dev = &pdev->dev; > + struct msc313e_pwm *pwm; > + __iomem void *base; > + int i; > + > + match_data = of_device_get_match_data(dev); > + if (!match_data) > + return -EINVAL; > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL); Please don't use "pwm" as a name for your driver data variable. That name is reserved for struct pwm_device pointers. Something like "ddata" would be fine, but there is a wide variety in pwm driver land to choose from. > + if (!pwm) > + return -ENOMEM; > + > + pwm->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(pwm->clk)) > + return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n"); > + > + pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config); > + if (IS_ERR(pwm->regmap)) > + return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n"); > + > + for (i = 0; i < match_data->channels; i++) { > + unsigned int offset = CHANNEL_OFFSET * i; > + struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7); > + struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4); > + struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15); > + struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2); > + struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15); > + struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2); > + struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i); > + > + pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap, > + div_clkdiv_field); > + pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap, > + ctrl_polarity_field); > + pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field); > + pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field); > + pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field); > + pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field); > + pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field); > + > + /* Channels are enabled on boot, disable it until the pwm subsystem re-enable it > + * explicitly You're supposed to keep the PWM running. (Consider a bootloader setting up a splash screen. You're probably killing the backlight here.) Note you need improved clk handling to do that right. > + */ > + regmap_field_write(pwm->channels[i].swrst, 1); > + } > + > + pwm->pwmchip.dev = dev; > + pwm->pwmchip.ops = &msc313e_pwm_ops; > + pwm->pwmchip.npwm = match_data->channels; > + > + platform_set_drvdata(pdev, pwm); This is unused, please drop. > + return devm_pwmchip_add(dev, &pwm->pwmchip); > +} > + > +static const struct of_device_id msc313e_pwm_dt_ids[] = { > + { .compatible = "mstar,msc313e-pwm", .data = &msc313e_data }, > + { .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids); > + > +static struct platform_driver msc313e_pwm_driver = { > + .probe = msc313e_pwm_probe, > + .driver = { > + .name = DRIVER_NAME, > + .of_match_table = msc313e_pwm_dt_ids, > + }, > +}; > +module_platform_driver(msc313e_pwm_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver"); > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>"); That address doesn't match the author of this patch. Is this on purpose? Best regards Uwe
Hi Uwe, On Wed, 28 Sept 2022 at 01:33, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Romain, hello Daniel, Romain is doing the fixes to this so I'll let him answer the other bits but on all of the "what does the hardware do?" questions I'll have to test that on the hardware and follow up. The only documentation we have is very rough listings of registers and bits and not much else. > > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver"); > > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>"); > > That address doesn't match the author of this patch. Is this on purpose? My intention is to put all of the stuff for this MStar (Now Sigmastar) platform under this domain including the kernel, u-boot and PCB files so that at some point I can handover the whole thing to someone else or maybe a community that want to take over by giving them the domain. I should really set my email address in git for these commits to match but keep forgetting. Cheers, Daniel
Hi, Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> a écrit : > > Hello Romain, hello Daniel, > > adding Mark Brown to Cc: for the regmap stuff. > > On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote: > > From: Daniel Palmer <daniel@0x0f.com> > > > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer. > > > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > Co-developed-by: Romain Perier <romain.perier@gmail.com> > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > > --- > > MAINTAINERS | 1 + > > drivers/pwm/Kconfig | 9 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 280 insertions(+) > > create mode 100644 drivers/pwm/pwm-msc313e.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 9d7f64dc0efe..c3b39b09097c 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -2439,6 +2439,7 @@ F: arch/arm/mach-mstar/ > > F: drivers/clk/mstar/ > > F: drivers/clocksource/timer-msc313e.c > > F: drivers/gpio/gpio-msc313.c > > +F: drivers/pwm/pwm-msc313e.c > > F: drivers/rtc/rtc-msc313.c > > F: drivers/watchdog/msc313e_wdt.c > > F: include/dt-bindings/clock/mstar-* > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 60d13a949bc5..8049fd03a821 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -372,6 +372,15 @@ config PWM_MESON > > To compile this driver as a module, choose M here: the module > > will be called pwm-meson. > > > > +config PWM_MSC313E > > + tristate "MStar MSC313e PWM support" > > + depends on ARCH_MSTARV7 || COMPILE_TEST > > + help > > + Generic PWM framework driver for MSTAR MSC313e. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-msc313e. > > + > > config PWM_MTK_DISP > > tristate "MediaTek display PWM driver" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 7bf1a29f02b8..bc285c054f2a 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > > obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o > > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > > +obj-$(CONFIG_PWM_MSC313E) += pwm-msc313e.o > > obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o > > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c > > new file mode 100644 > > index 000000000000..a71f39ba66c3 > > --- /dev/null > > +++ b/drivers/pwm/pwm-msc313e.c > > @@ -0,0 +1,269 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> > > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com> > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/of_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > + > > +#define DRIVER_NAME "msc313e-pwm" > > + > > +#define CHANNEL_OFFSET 0x80 > > +#define REG_DUTY 0x8 > > +#define REG_PERIOD 0x10 > > +#define REG_DIV 0x18 > > +#define REG_CTRL 0x1c > > +#define REG_SWRST 0x1fc > > + > > +struct msc313e_pwm_channel { > > + struct regmap_field *clkdiv; > > + struct regmap_field *polarity; > > + struct regmap_field *dutyl; > > + struct regmap_field *dutyh; > > + struct regmap_field *periodl; > > + struct regmap_field *periodh; > > + struct regmap_field *swrst; > > +}; > > + > > +struct msc313e_pwm { > > + struct regmap *regmap; > > + struct pwm_chip pwmchip; > > + struct clk *clk; > > + struct msc313e_pwm_channel channels[]; > > +}; > > + > > +struct msc313e_pwm_info { > > + unsigned int channels; > > +}; > > + > > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip) > > + > > +static const struct regmap_config msc313e_pwm_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 16, > > + .reg_stride = 4, > > +}; > > + > > +static const struct msc313e_pwm_info msc313e_data = { > > + .channels = 8, > > +}; > > + > > +static const struct msc313e_pwm_info ssd20xd_data = { > > + .channels = 4, > > +}; > > + > > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value) > > +{ > > + /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into > > Please fix the comment style to use /* on a line for itself. Also for > comments staying below 80 chars per line is appreciated. even if check-patch.pl --strict passed ? ^^ > > > + * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter > > + * we are about to write has this contrainst. > > s/contrainst/contraint/ > > I wonder if that could be abstracted by regmap?! I had the same thought, not from what I have read/found, but perhaps the regmap maintainer has an opinion. > > > + */ > > + regmap_field_write(low, value & 0xffff); > > + regmap_field_write(high, value >> 16); > > +} > > + > > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value) > > +{ > > + unsigned int val = 0; > > + > > + regmap_field_read(low, &val); > > + *value = val; > > + regmap_field_read(high, &val); > > + *value = (val << 16) | *value; > > +} > > + > > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device, > > + int duty_ns, int period_ns) > > +{ > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > + unsigned long long div = 1; > > + > > + /* Fit the period into the period register by prescaling the clk */ > > + while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) { > > dividing by the result of a division looses precision. Also rounding > down both divisions looks wrong. Such cases are not supposed to be covered by PWM_DEBUG ? (because everything passed with PWM_DEBUG) > > > + div++; > > + if (div > (0xffff + 1)) { > > + /* Force clk div to the maximum allowed value */ > > + div = 0xffff; > > + break; > > + } > > + nspertick = DIV_ROUND_DOWN_ULL(nspertick, div); > > + } > > + > > + regmap_field_write(channel->clkdiv, div - 1); > > + msc313e_pwm_writecounter(channel->dutyl, channel->dutyh, > > + DIV_ROUND_DOWN_ULL(duty_ns, nspertick)); > > + msc313e_pwm_writecounter(channel->periodl, channel->periodh, > > + DIV_ROUND_DOWN_ULL(period_ns, nspertick)); > > + return 0; > > +}; > > + > > +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device, > > + enum pwm_polarity polarity) > > +{ > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > + unsigned int pol = 0; > > + > > + if (polarity == PWM_POLARITY_INVERSED) > > + pol = 1; > > + regmap_field_update_bits(channel->polarity, 1, pol); > > + > > + return 0; > > +} > > + > > +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device) > > +{ > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > + int ret; > > + > > + ret = clk_prepare_enable(pwm->clk); > > + if (ret) > > + return ret; > > + return regmap_field_write(channel->swrst, 0); > > +} > > + > > +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device) > > +{ > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > + int ret; > > + > > + ret = regmap_field_write(channel->swrst, 1); > > + clk_disable_unprepare(pwm->clk); > > + return ret; > > +} > > + > > +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + int ret; > > + > > + if (state->enabled) { > > + if (!pwm->state.enabled) { > > + ret = msc313e_pwm_enable(chip, pwm); > > + if (ret) > > + return ret; > > + } > > + msc313e_pwm_set_polarity(chip, pwm, state->polarity); > > + msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period); > > Does the output emit a previous setting after msc313e_pwm_enable()? If > so, please do the enable at the end. > > Does calling msc313e_pwm_set_polarity() and msc313e_pwm_config() have an > immediate effect? Or is the currently running period completed? > > > + } else if (pwm->state.enabled) { > > + ret = msc313e_pwm_disable(chip, pwm); > > How does the output behave on disable? Typical options are: > > - freezes where ever it currently is > - goes to the inactive level I let Daniel answer these questions. > > Does it complete the currently running period? Please answer these in a > "Limitations:" section similar to e.g. pwm-sl28cpld.c and pwm-iqs620a. > > > + } > > + return 0; > > +} > > + > > +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device, > > Please s/device/pwm/ to match the naming used in other drivers. > > > + struct pwm_state *state) > > +{ > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); > > + unsigned int val = 0; > > + > > + regmap_field_read(channel->polarity, &val); > > + state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; > > + > > + regmap_field_read(channel->swrst, &val); > > + state->enabled = val == 0 ? true : false; > > + > > + msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val); > > + state->duty_cycle = val * nspertick; > > Please do the division at the end. Consider val = 333335 and > clk_get_rate() returning 666666667. > > Then the exact duty cycle would be 500002.49974999874 but you would > calculate nspertick = 1 and so duty_cycle = 333335. > > Also you need to round up. Did you test your driver with PWM_DEBUG > enabled? Take a look at my cover letter, yeah I have tested everything with PWM_DEBUG, everything passed ^^ The idea is that .get_state should return 500003 here to have > apply ∘ get_state idempotent. As apply is supposed to round down to the > next available setting, get_state has to round up. > > > + msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val); > > + state->period = val * nspertick; > > +} > > + > > +static const struct pwm_ops msc313e_pwm_ops = { > > + .apply = msc313e_apply, > > + .get_state = msc313e_get_state, > > + .owner = THIS_MODULE > > +}; > > + > > +static int msc313e_pwm_probe(struct platform_device *pdev) > > +{ > > + const struct msc313e_pwm_info *match_data; > > + struct device *dev = &pdev->dev; > > + struct msc313e_pwm *pwm; > > + __iomem void *base; > > + int i; > > + > > + match_data = of_device_get_match_data(dev); > > + if (!match_data) > > + return -EINVAL; > > + > > + base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(base)) > > + return PTR_ERR(base); > > + > > + pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL); > > Please don't use "pwm" as a name for your driver data variable. That > name is reserved for struct pwm_device pointers. Something like "ddata" > would be fine, but there is a wide variety in pwm driver land to choose > from. > > > + if (!pwm) > > + return -ENOMEM; > > + > > + pwm->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(pwm->clk)) > > + return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n"); > > + > > + pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config); > > + if (IS_ERR(pwm->regmap)) > > + return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n"); > > + > > + for (i = 0; i < match_data->channels; i++) { > > + unsigned int offset = CHANNEL_OFFSET * i; > > + struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7); > > + struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4); > > + struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15); > > + struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2); > > + struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15); > > + struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2); > > + struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i); > > + > > + pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap, > > + div_clkdiv_field); > > + pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap, > > + ctrl_polarity_field); > > + pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field); > > + pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field); > > + pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field); > > + pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field); > > + pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field); > > + > > + /* Channels are enabled on boot, disable it until the pwm subsystem re-enable it > > + * explicitly > > You're supposed to keep the PWM running. (Consider a bootloader setting > up a splash screen. You're probably killing the backlight here.) Note > you need improved clk handling to do that right. > > > + */ > > + regmap_field_write(pwm->channels[i].swrst, 1); > > + } > > + > > + pwm->pwmchip.dev = dev; > > + pwm->pwmchip.ops = &msc313e_pwm_ops; > > + pwm->pwmchip.npwm = match_data->channels; > > + > > + platform_set_drvdata(pdev, pwm); > > This is unused, please drop. > > > + return devm_pwmchip_add(dev, &pwm->pwmchip); > > +} > > + > > +static const struct of_device_id msc313e_pwm_dt_ids[] = { > > + { .compatible = "mstar,msc313e-pwm", .data = &msc313e_data }, > > + { .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids); > > + > > +static struct platform_driver msc313e_pwm_driver = { > > + .probe = msc313e_pwm_probe, > > + .driver = { > > + .name = DRIVER_NAME, > > + .of_match_table = msc313e_pwm_dt_ids, > > + }, > > +}; > > +module_platform_driver(msc313e_pwm_driver); > > + > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Mstar MSC313e PWM driver"); > > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>"); > > That address doesn't match the author of this patch. Is this on purpose? > > Best regards > Uwe Regards, Romain
Hello Romain, On Thu, Oct 27, 2022 at 10:36:10AM +0200, Romain Perier wrote: > Le mar. 27 sept. 2022 à 18:33, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> a écrit : > > > > Hello Romain, hello Daniel, > > > > adding Mark Brown to Cc: for the regmap stuff. > > > > On Wed, Sep 07, 2022 at 03:12:38PM +0200, Romain Perier wrote: > > > From: Daniel Palmer <daniel@0x0f.com> > > > > > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer. > > > > > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > > > Co-developed-by: Romain Perier <romain.perier@gmail.com> > > > Signed-off-by: Romain Perier <romain.perier@gmail.com> > > > --- > > > MAINTAINERS | 1 + > > > drivers/pwm/Kconfig | 9 ++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-msc313e.c | 269 ++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 280 insertions(+) > > > create mode 100644 drivers/pwm/pwm-msc313e.c > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index 9d7f64dc0efe..c3b39b09097c 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -2439,6 +2439,7 @@ F: arch/arm/mach-mstar/ > > > F: drivers/clk/mstar/ > > > F: drivers/clocksource/timer-msc313e.c > > > F: drivers/gpio/gpio-msc313.c > > > +F: drivers/pwm/pwm-msc313e.c > > > F: drivers/rtc/rtc-msc313.c > > > F: drivers/watchdog/msc313e_wdt.c > > > F: include/dt-bindings/clock/mstar-* > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index 60d13a949bc5..8049fd03a821 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -372,6 +372,15 @@ config PWM_MESON > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-meson. > > > > > > +config PWM_MSC313E > > > + tristate "MStar MSC313e PWM support" > > > + depends on ARCH_MSTARV7 || COMPILE_TEST > > > + help > > > + Generic PWM framework driver for MSTAR MSC313e. > > > + > > > + To compile this driver as a module, choose M here: the module > > > + will be called pwm-msc313e. > > > + > > > config PWM_MTK_DISP > > > tristate "MediaTek display PWM driver" > > > depends on ARCH_MEDIATEK || COMPILE_TEST > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > index 7bf1a29f02b8..bc285c054f2a 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o > > > obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o > > > obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o > > > obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o > > > +obj-$(CONFIG_PWM_MSC313E) += pwm-msc313e.o > > > obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o > > > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c > > > new file mode 100644 > > > index 000000000000..a71f39ba66c3 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-msc313e.c > > > @@ -0,0 +1,269 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> > > > + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com> > > > + */ > > > + > > > +#include <linux/clk.h> > > > +#include <linux/of_device.h> > > > +#include <linux/pwm.h> > > > +#include <linux/regmap.h> > > > + > > > +#define DRIVER_NAME "msc313e-pwm" > > > + > > > +#define CHANNEL_OFFSET 0x80 > > > +#define REG_DUTY 0x8 > > > +#define REG_PERIOD 0x10 > > > +#define REG_DIV 0x18 > > > +#define REG_CTRL 0x1c > > > +#define REG_SWRST 0x1fc > > > + > > > +struct msc313e_pwm_channel { > > > + struct regmap_field *clkdiv; > > > + struct regmap_field *polarity; > > > + struct regmap_field *dutyl; > > > + struct regmap_field *dutyh; > > > + struct regmap_field *periodl; > > > + struct regmap_field *periodh; > > > + struct regmap_field *swrst; > > > +}; > > > + > > > +struct msc313e_pwm { > > > + struct regmap *regmap; > > > + struct pwm_chip pwmchip; > > > + struct clk *clk; > > > + struct msc313e_pwm_channel channels[]; > > > +}; > > > + > > > +struct msc313e_pwm_info { > > > + unsigned int channels; > > > +}; > > > + > > > +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip) > > > + > > > +static const struct regmap_config msc313e_pwm_regmap_config = { > > > + .reg_bits = 16, > > > + .val_bits = 16, > > > + .reg_stride = 4, > > > +}; > > > + > > > +static const struct msc313e_pwm_info msc313e_data = { > > > + .channels = 8, > > > +}; > > > + > > > +static const struct msc313e_pwm_info ssd20xd_data = { > > > + .channels = 4, > > > +}; > > > + > > > +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value) > > > +{ > > > + /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into > > > > Please fix the comment style to use /* on a line for itself. Also for > > comments staying below 80 chars per line is appreciated. > > even if check-patch.pl --strict passed ? ^^ I also already wondered about check-patch not demanding this. *shrug* > > > + * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter > > > + * we are about to write has this contrainst. > > > > s/contrainst/contraint/ > > > > I wonder if that could be abstracted by regmap?! > > I had the same thought, not from what I have read/found, but perhaps > the regmap maintainer has an opinion. > > > > > > + */ > > > + regmap_field_write(low, value & 0xffff); > > > + regmap_field_write(high, value >> 16); > > > +} > > > + > > > +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value) > > > +{ > > > + unsigned int val = 0; > > > + > > > + regmap_field_read(low, &val); > > > + *value = val; > > > + regmap_field_read(high, &val); > > > + *value = (val << 16) | *value; > > > +} > > > + > > > +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device, > > > + int duty_ns, int period_ns) > > > +{ > > > + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); > > > + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); > > > + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; > > > + unsigned long long div = 1; > > > + > > > + /* Fit the period into the period register by prescaling the clk */ > > > + while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) { > > > > dividing by the result of a division looses precision. Also rounding > > down both divisions looks wrong. > > Such cases are not supposed to be covered by PWM_DEBUG ? (because > everything passed with PWM_DEBUG) Note that PWM_DEBUG being silent isn't an indicator that everything is fine. It cannot catch everything and so doesn't replace human review. If you tell me what clk_get_rate() returns for you, I might be able to tell you a procedure that makes PWM_DEBUG unhappy. Best regards Uwe
diff --git a/MAINTAINERS b/MAINTAINERS index 9d7f64dc0efe..c3b39b09097c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2439,6 +2439,7 @@ F: arch/arm/mach-mstar/ F: drivers/clk/mstar/ F: drivers/clocksource/timer-msc313e.c F: drivers/gpio/gpio-msc313.c +F: drivers/pwm/pwm-msc313e.c F: drivers/rtc/rtc-msc313.c F: drivers/watchdog/msc313e_wdt.c F: include/dt-bindings/clock/mstar-* diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 60d13a949bc5..8049fd03a821 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -372,6 +372,15 @@ config PWM_MESON To compile this driver as a module, choose M here: the module will be called pwm-meson. +config PWM_MSC313E + tristate "MStar MSC313e PWM support" + depends on ARCH_MSTARV7 || COMPILE_TEST + help + Generic PWM framework driver for MSTAR MSC313e. + + To compile this driver as a module, choose M here: the module + will be called pwm-msc313e. + config PWM_MTK_DISP tristate "MediaTek display PWM driver" depends on ARCH_MEDIATEK || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 7bf1a29f02b8..bc285c054f2a 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -62,4 +62,5 @@ obj-$(CONFIG_PWM_TWL) += pwm-twl.o obj-$(CONFIG_PWM_TWL_LED) += pwm-twl-led.o obj-$(CONFIG_PWM_VISCONTI) += pwm-visconti.o obj-$(CONFIG_PWM_VT8500) += pwm-vt8500.o +obj-$(CONFIG_PWM_MSC313E) += pwm-msc313e.o obj-$(CONFIG_PWM_XILINX) += pwm-xilinx.o diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c new file mode 100644 index 000000000000..a71f39ba66c3 --- /dev/null +++ b/drivers/pwm/pwm-msc313e.c @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> + * Copyright (C) 2022 Romain Perier <romain.perier@gmail.com> + */ + +#include <linux/clk.h> +#include <linux/of_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> + +#define DRIVER_NAME "msc313e-pwm" + +#define CHANNEL_OFFSET 0x80 +#define REG_DUTY 0x8 +#define REG_PERIOD 0x10 +#define REG_DIV 0x18 +#define REG_CTRL 0x1c +#define REG_SWRST 0x1fc + +struct msc313e_pwm_channel { + struct regmap_field *clkdiv; + struct regmap_field *polarity; + struct regmap_field *dutyl; + struct regmap_field *dutyh; + struct regmap_field *periodl; + struct regmap_field *periodh; + struct regmap_field *swrst; +}; + +struct msc313e_pwm { + struct regmap *regmap; + struct pwm_chip pwmchip; + struct clk *clk; + struct msc313e_pwm_channel channels[]; +}; + +struct msc313e_pwm_info { + unsigned int channels; +}; + +#define to_msc313e_pwm(ptr) container_of(ptr, struct msc313e_pwm, pwmchip) + +static const struct regmap_config msc313e_pwm_regmap_config = { + .reg_bits = 16, + .val_bits = 16, + .reg_stride = 4, +}; + +static const struct msc313e_pwm_info msc313e_data = { + .channels = 8, +}; + +static const struct msc313e_pwm_info ssd20xd_data = { + .channels = 4, +}; + +static void msc313e_pwm_writecounter(struct regmap_field *low, struct regmap_field *high, u32 value) +{ + /* The bus that connects the CPU to the peripheral registers splits 32 bit registers into + * two 16bit registers placed 4 bytes apart. It's the hardware design they used. The counter + * we are about to write has this contrainst. + */ + regmap_field_write(low, value & 0xffff); + regmap_field_write(high, value >> 16); +} + +static void msc313e_pwm_readcounter(struct regmap_field *low, struct regmap_field *high, u32 *value) +{ + unsigned int val = 0; + + regmap_field_read(low, &val); + *value = val; + regmap_field_read(high, &val); + *value = (val << 16) | *value; +} + +static int msc313e_pwm_config(struct pwm_chip *chip, struct pwm_device *device, + int duty_ns, int period_ns) +{ + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; + unsigned long long div = 1; + + /* Fit the period into the period register by prescaling the clk */ + while (DIV_ROUND_DOWN_ULL(period_ns, nspertick) > 0x3ffff) { + div++; + if (div > (0xffff + 1)) { + /* Force clk div to the maximum allowed value */ + div = 0xffff; + break; + } + nspertick = DIV_ROUND_DOWN_ULL(nspertick, div); + } + + regmap_field_write(channel->clkdiv, div - 1); + msc313e_pwm_writecounter(channel->dutyl, channel->dutyh, + DIV_ROUND_DOWN_ULL(duty_ns, nspertick)); + msc313e_pwm_writecounter(channel->periodl, channel->periodh, + DIV_ROUND_DOWN_ULL(period_ns, nspertick)); + return 0; +}; + +static int msc313e_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *device, + enum pwm_polarity polarity) +{ + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; + unsigned int pol = 0; + + if (polarity == PWM_POLARITY_INVERSED) + pol = 1; + regmap_field_update_bits(channel->polarity, 1, pol); + + return 0; +} + +static int msc313e_pwm_enable(struct pwm_chip *chip, struct pwm_device *device) +{ + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; + int ret; + + ret = clk_prepare_enable(pwm->clk); + if (ret) + return ret; + return regmap_field_write(channel->swrst, 0); +} + +static int msc313e_pwm_disable(struct pwm_chip *chip, struct pwm_device *device) +{ + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; + int ret; + + ret = regmap_field_write(channel->swrst, 1); + clk_disable_unprepare(pwm->clk); + return ret; +} + +static int msc313e_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + int ret; + + if (state->enabled) { + if (!pwm->state.enabled) { + ret = msc313e_pwm_enable(chip, pwm); + if (ret) + return ret; + } + msc313e_pwm_set_polarity(chip, pwm, state->polarity); + msc313e_pwm_config(chip, pwm, state->duty_cycle, state->period); + } else if (pwm->state.enabled) { + ret = msc313e_pwm_disable(chip, pwm); + } + return 0; +} + +static void msc313e_get_state(struct pwm_chip *chip, struct pwm_device *device, + struct pwm_state *state) +{ + struct msc313e_pwm *pwm = to_msc313e_pwm(chip); + struct msc313e_pwm_channel *channel = &pwm->channels[device->hwpwm]; + unsigned long long nspertick = DIV_ROUND_DOWN_ULL(NSEC_PER_SEC, clk_get_rate(pwm->clk)); + unsigned int val = 0; + + regmap_field_read(channel->polarity, &val); + state->polarity = val ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; + + regmap_field_read(channel->swrst, &val); + state->enabled = val == 0 ? true : false; + + msc313e_pwm_readcounter(channel->dutyl, channel->dutyh, &val); + state->duty_cycle = val * nspertick; + + msc313e_pwm_readcounter(channel->periodl, channel->periodh, &val); + state->period = val * nspertick; +} + +static const struct pwm_ops msc313e_pwm_ops = { + .apply = msc313e_apply, + .get_state = msc313e_get_state, + .owner = THIS_MODULE +}; + +static int msc313e_pwm_probe(struct platform_device *pdev) +{ + const struct msc313e_pwm_info *match_data; + struct device *dev = &pdev->dev; + struct msc313e_pwm *pwm; + __iomem void *base; + int i; + + match_data = of_device_get_match_data(dev); + if (!match_data) + return -EINVAL; + + base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(base)) + return PTR_ERR(base); + + pwm = devm_kzalloc(dev, struct_size(pwm, channels, match_data->channels), GFP_KERNEL); + if (!pwm) + return -ENOMEM; + + pwm->clk = devm_clk_get(dev, NULL); + if (IS_ERR(pwm->clk)) + return dev_err_probe(dev, PTR_ERR(pwm->clk), "Cannot get clk\n"); + + pwm->regmap = devm_regmap_init_mmio(dev, base, &msc313e_pwm_regmap_config); + if (IS_ERR(pwm->regmap)) + return dev_err_probe(dev, PTR_ERR(pwm->regmap), "Cannot get regmap\n"); + + for (i = 0; i < match_data->channels; i++) { + unsigned int offset = CHANNEL_OFFSET * i; + struct reg_field div_clkdiv_field = REG_FIELD(offset + REG_DIV, 0, 7); + struct reg_field ctrl_polarity_field = REG_FIELD(offset + REG_CTRL, 4, 4); + struct reg_field dutyl_field = REG_FIELD(offset + REG_DUTY, 0, 15); + struct reg_field dutyh_field = REG_FIELD(offset + REG_DUTY + 4, 0, 2); + struct reg_field periodl_field = REG_FIELD(offset + REG_PERIOD, 0, 15); + struct reg_field periodh_field = REG_FIELD(offset + REG_PERIOD + 4, 0, 2); + struct reg_field swrst_field = REG_FIELD(REG_SWRST, i, i); + + pwm->channels[i].clkdiv = devm_regmap_field_alloc(dev, pwm->regmap, + div_clkdiv_field); + pwm->channels[i].polarity = devm_regmap_field_alloc(dev, pwm->regmap, + ctrl_polarity_field); + pwm->channels[i].dutyl = devm_regmap_field_alloc(dev, pwm->regmap, dutyl_field); + pwm->channels[i].dutyh = devm_regmap_field_alloc(dev, pwm->regmap, dutyh_field); + pwm->channels[i].periodl = devm_regmap_field_alloc(dev, pwm->regmap, periodl_field); + pwm->channels[i].periodh = devm_regmap_field_alloc(dev, pwm->regmap, periodh_field); + pwm->channels[i].swrst = devm_regmap_field_alloc(dev, pwm->regmap, swrst_field); + + /* Channels are enabled on boot, disable it until the pwm subsystem re-enable it + * explicitly + */ + regmap_field_write(pwm->channels[i].swrst, 1); + } + + pwm->pwmchip.dev = dev; + pwm->pwmchip.ops = &msc313e_pwm_ops; + pwm->pwmchip.npwm = match_data->channels; + + platform_set_drvdata(pdev, pwm); + + return devm_pwmchip_add(dev, &pwm->pwmchip); +} + +static const struct of_device_id msc313e_pwm_dt_ids[] = { + { .compatible = "mstar,msc313e-pwm", .data = &msc313e_data }, + { .compatible = "mstar,ssd20xd-pwm", .data = &ssd20xd_data }, + {}, +}; +MODULE_DEVICE_TABLE(of, msc313e_pwm_dt_ids); + +static struct platform_driver msc313e_pwm_driver = { + .probe = msc313e_pwm_probe, + .driver = { + .name = DRIVER_NAME, + .of_match_table = msc313e_pwm_dt_ids, + }, +}; +module_platform_driver(msc313e_pwm_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Mstar MSC313e PWM driver"); +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>");