Hello, On Fri, Aug 02, 2024 at 05:44:08PM +0200, Dimitri Fedrau wrote: > The MC33XS2410 is a four channel high-side switch. Featuring advanced > monitoring and control function, the device is operational from 3.0 V to > 60 V. The device is controlled by SPI port for configuration. > > Signed-off-by: Dimitri Fedrau <dima.fedrau@xxxxxxxxx> > --- > drivers/pwm/Kconfig | 12 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-mc33xs2410.c | 419 +++++++++++++++++++++++++++++++++++ > 3 files changed, 432 insertions(+) > create mode 100644 drivers/pwm/pwm-mc33xs2410.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 1dd7921194f5..1e873a19a1cf 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -380,6 +380,18 @@ config PWM_LPSS_PLATFORM > To compile this driver as a module, choose M here: the module > will be called pwm-lpss-platform. > > +config PWM_MC33XS2410 > + tristate "MC33XS2410 PWM support" > + depends on OF > + depends on SPI > + help > + NXP MC33XS2410 high-side switch driver. The MC33XS2410 is a four > + channel high-side switch. The device is operational from 3.0 V > + to 60 V. The device is controlled by SPI port for configuration. > + > + To compile this driver as a module, choose M here: the module > + will be called pwm-mc33xs2410. > + > config PWM_MESON > tristate "Amlogic Meson PWM driver" > depends on ARCH_MESON || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 90913519f11a..b9b202f7fe7e 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -33,6 +33,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o > obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o > obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o > obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o > +obj-$(CONFIG_PWM_MC33XS2410) += pwm-mc33xs2410.o > obj-$(CONFIG_PWM_MESON) += pwm-meson.o > obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o > obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o > diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c > new file mode 100644 > index 000000000000..63e6a48b0d02 > --- /dev/null > +++ b/drivers/pwm/pwm-mc33xs2410.c > @@ -0,0 +1,419 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH > + * Please add a link to the manual here. I found https://www.nxp.com/docs/en/data-sheet/MC33XS2410.pdf. > + * Limitations: > + * - Supports frequencies between 0.5Hz and 2048Hz with following steps: > + * - 0.5 Hz steps from 0.5 Hz to 32 Hz > + * - 2 Hz steps from 2 Hz to 128 Hz > + * - 8 Hz steps from 8 Hz to 512 Hz > + * - 32 Hz steps from 32 Hz to 2048 Hz > + * - Cannot generate a 0 % duty cycle. > + * - Always produces low output if disabled. > + * - Configuration isn't atomic. When changing polarity, duty cycle or period > + * the data is taken immediately, counters not being affected, resulting in a > + * behavior of the output pin that is neither the old nor the new state, > + * rather something in between. > + */ > + > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/math64.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/pwm.h> > + > +#include <asm/unaligned.h> > + > +#include <linux/spi/spi.h> > + > +#define MC33XS2410_GLB_CTRL 0x00 > +#define MC33XS2410_GLB_CTRL_MODE_MASK GENMASK(7, 6) > +#define MC33XS2410_GLB_CTRL_NORMAL_MODE BIT(6) I would have defined these as: #define MC33XS2410_GLB_CTRL_MODE GENMASK(7, 6) #define MC33XS2410_GLB_CTRL_MODE_NORMAL FIELD_PREP(MC33XS2410_GLB_CTRL_MODE, 1) > +#define MC33XS2410_PWM_CTRL1 0x05 > +#define MC33XS2410_PWM_CTRL1_POL_INV(x) BIT(x) > +#define MC33XS2410_PWM_CTRL3 0x07 > +/* x in { 0 ... 3 } */ > +#define MC33XS2410_PWM_CTRL3_EN(x) BIT(4 + (x)) > +#define MC33XS2410_PWM_FREQ1 0x08 > +/* x in { 1 ... 4 } */ > +#define MC33XS2410_PWM_FREQ(x) (MC33XS2410_PWM_FREQ1 + (x - 1)) > +#define MC33XS2410_PWM_FREQ_STEP_MASK GENMASK(7, 6) > +#define MC33XS2410_PWM_FREQ_COUNT_MASK GENMASK(5, 0) > +#define MC33XS2410_PWM_DC1 0x0c > +/* x in { 1 ... 4 } */ > +#define MC33XS2410_PWM_DC(x) (MC33XS2410_PWM_DC1 + (x - 1)) > +#define MC33XS2410_WDT 0x14 > + > +#define MC33XS2410_WR BIT(7) > +#define MC33XS2410_RD_CTRL BIT(7) > +#define MC33XS2410_RD_DATA_MASK GENMASK(13, 0) > + > +#define MC33XS2410_MIN_PERIOD_STEP0 31250000 > +#define MC33XS2410_MAX_PERIOD_STEP0 2000000000 > +/* x in { 0 ... 3 } */ > +#define MC33XS2410_MIN_PERIOD_STEP(x) (MC33XS2410_MIN_PERIOD_STEP0 >> (2 * x)) > +/* x in { 0 ... 3 } */ > +#define MC33XS2410_MAX_PERIOD_STEP(x) (MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x)) So MC33XS2410_MIN_PERIOD_STEP(3) = 31250000 >> 6 which is mathematically 488281.25. I haven't thought deeply about it, but I wonder if that .25 is relevant in the calculation of the step to select. > + > +#define MC33XS2410_MAX_TRANSFERS 5 > +#define MC33XS2410_WORD_LEN 2 > + > +struct mc33xs2410_pwm { > + struct spi_device *spi; > +}; > + > +static > +inline struct mc33xs2410_pwm *to_pwm_mc33xs2410_chip(struct pwm_chip *chip) personally I'd prefer to call this mc33xs2410_from_chip() or something similar to have it use the same prefix as the other functions. But given there is some inconsistency and other people feel strong here and (rightly) claim this type of function is often called "to_*", I won't insist. > +{ > + return pwmchip_get_drvdata(chip); > +} > [...] > +static u8 mc33xs2410_pwm_get_freq(u64 period) > +{ > + u8 step, count; > + > + /* > + * Check which step is appropriate for the given period, starting with > + * the highest frequency(lowest period). Higher frequencies are > + * represented with better resolution by the device. Therefore favor > + * frequency range with the better resolution to minimize error > + * introduced by the frequency steps. > + */ > + > + switch (period) { > + case MC33XS2410_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3): > + step = 3; > + break; > + case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2): > + step = 2; > + break; > + case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1): > + step = 1; > + break; > + case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0): > + step = 0; > + break; > + } > + > + count = DIV_ROUND_UP((u32)MC33XS2410_MAX_PERIOD_STEP(step), (u32)period); It took me a while to verify that DIV_ROUND_UP is right here. The reasoning is that a higher count results in a higher frequency and so a smaller period. > + return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | > + FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count - 1); > +} > + > +static u64 mc33xs2410_pwm_get_period(u8 reg) > +{ > + u32 freq, code, steps; > + > + /* > + * steps: > + * - 0 = 0.5Hz > + * - 1 = 2Hz > + * - 2 = 8Hz > + * - 3 = 32Hz > + * frequency = (code + 1) x steps. > + * > + * To avoid division in case steps value is zero we scale the steps Technically you don't avoid a division, but "only" avoid loosing precision in case you have to do (integer) division by 0.5. > + * value for now by two and keep it in mind when calculating the period > + * that we have doubled the frequency. Maybe reflect that doubling in the variable naming? "doubled_steps"? > + */ > + steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, reg) * 2); > + code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT_MASK, reg); > + freq = (code + 1) * steps; > + > + /* Convert frequency to period, considering the doubled frequency. */ > + return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq); > +} > + > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) > +{ > + /* > + * duty_cycle cannot overflow and period is not zero, since this is > + * guaranteed by the caller. > + */ > + duty_cycle *= 256; > + duty_cycle = div64_u64(duty_cycle, period); > + > + return duty_cycle - 1; > +} > + > +static void mc33xs2410_pwm_set_relative_duty_cycle(struct pwm_state *state, > + u16 duty_cycle) > +{ > + if (!duty_cycle && !state->enabled) > + state->duty_cycle = 0; > + else > + state->duty_cycle = DIV_ROUND_UP_ULL((u64)(duty_cycle + 1) * state->period, 256); Why does !duty_cycle matter in the if condition. I would have expected if (!state->enabled) state->duty_cycle = 0; else state->duty_cycle = DIV_ROUND_UP_ULL....) That cast to (u64) in the last line can be dropped. > +} > + > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct mc33xs2410_pwm *mc33xs2410 = to_pwm_mc33xs2410_chip(chip); > + struct spi_device *spi = mc33xs2410->spi; > + u8 reg[4] = { > + MC33XS2410_PWM_FREQ(pwm->hwpwm + 1), > + MC33XS2410_PWM_DC(pwm->hwpwm + 1), > + MC33XS2410_PWM_CTRL1, > + MC33XS2410_PWM_CTRL3 > + }; > + bool ctrl[2] = { true, true }; > + u64 period, duty_cycle; > + int ret, rel_dc; > + u16 val[4]; > + u8 mask; > + > + period = min(state->period, MC33XS2410_MAX_PERIOD_STEP(0)); > + if (period < MC33XS2410_MIN_PERIOD_STEP(3) + 1) > + return -EINVAL; That is only right because in the expression for MC33XS2410_MIN_PERIOD_STEP(3) the shift results in a one being shifted out. If there were only zeros, the right check would be if (period < MC33XS2410_MIN_PERIOD_STEP(3)) . That's a bit unfortunate because it's unintuitive and at first sight I'd expect that MC33XS2410_MIN_PERIOD_STEP(3) is a possible period. Hmm, you could only fix that by doing scaled math or a good code comment. > + ret = mc33xs2410_read_regs(spi, ®[2], &ctrl[0], &val[2], 2); > + if (ret < 0) > + return ret; > + > + /* frequency */ > + val[0] = mc33xs2410_pwm_get_freq(period); > + /* Continue calculations with the possibly truncated period */ > + period = mc33xs2410_pwm_get_period(val[0]); > + > + /* duty cycle */ > + duty_cycle = min(period, state->duty_cycle); > + rel_dc = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle); > + val[1] = rel_dc < 0 ? 0 : rel_dc; > + > + /* polarity */ > + mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm); > + val[2] = (state->polarity == PWM_POLARITY_INVERSED) ? > + (val[2] | mask) : (val[2] & ~mask); > + > + /* enable output */ > + mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm); > + val[3] = (state->enabled && rel_dc >= 0) ? (val[3] | mask) : > + (val[3] & ~mask); > + > + return mc33xs2410_write_regs(spi, reg, val, 4); > +} Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
