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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7467 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]