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

[Romain Perier <romain.perier@gmail.com>]

Hi,

Le dim. 19 juin 2022 à 23:35, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> a écrit :
> > diff --git a/drivers/pwm/pwm-msc313e.c b/drivers/pwm/pwm-msc313e.c
> > new file mode 100644
> > index 000000000000..f20419c6b9be
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-msc313e.c
> > @@ -0,0 +1,242 @@
> > +// 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)
> > +{
> > +     regmap_field_write(low, value);
> > +     regmap_field_write(high, value >> 16);
>
> Is this racy? E.g. if the hw is running and the low register overflows
> before the high register is updated?
>

Ack, I am re-working most of the stuff you noticed. The problem with
this IP blocks (and others...) is we have close registers
but we only need to write or read 16 bits in each of these (it is
mainly reverse engineered from vendor source or runtime most of the
time) . You cannot really do a single read (except by doing an obscur
thing via readq ? ...)

. We had exactly the same issue with the rtc driver see [1]

1. https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/rtc/rtc-msc313.c#n50
.

Regards,
Romain