Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM

From: Uwe Kleine-König
Date: Thu Oct 27 2022 - 05:22:48 EST

Next message: Paolo Abeni: "Re: [PATCH net-next v8.1] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy"
Previous message: Paolo Bonzini: "Re: [PATCH] KVM: x86: Exempt pending triple fault from event injection sanity check"
In reply to: Romain Perier: "Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxxxxx> 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@xxxxxxxx>
> > >
> > > This adds support for the PWM block on the Mstar MSC313e SoCs and newer.
> > >
> > > Signed-off-by: Daniel Palmer <daniel@xxxxxxxx>
> > > Co-developed-by: Romain Perier <romain.perier@xxxxxxxxx>
> > > Signed-off-by: Romain Perier <romain.perier@xxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx>
> > > + * Copyright (C) 2022 Romain Perier <romain.perier@xxxxxxxxx>
> > > + */
> > > +
> > > +#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

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature

Next message: Paolo Abeni: "Re: [PATCH net-next v8.1] net: phy: Add driver for Motorcomm yt8521 gigabit ethernet phy"
Previous message: Paolo Bonzini: "Re: [PATCH] KVM: x86: Exempt pending triple fault from event injection sanity check"
In reply to: Romain Perier: "Re: [PATCH v2 2/5] pwm: Add support for the MSTAR MSC313 PWM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]