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 > 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 > > > > > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer. > > > > > > Signed-off-by: Daniel Palmer > > > Co-developed-by: Romain Perier > > > Signed-off-by: Romain Perier > > > --- > > > 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 > > > + * Copyright (C) 2022 Romain Perier > > > + */ > > > + > > > +#include > > > +#include > > > +#include > > > +#include > > > + > > > +#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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |