Hello Dimitri, On Fri, Sep 27, 2024 at 02:57:45PM +0200, Dimitri Fedrau wrote: > diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c > new file mode 100644 > index 000000000000..f9a334a5e69b > --- /dev/null > +++ b/drivers/pwm/pwm-mc33xs2410.c > @@ -0,0 +1,422 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH > + * > + * Reference Manual : 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 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 488282 > +#define MC33XS2410_MAX_PERIOD_STEP0 2000000000 > +/* x in { 0 ... 3 } */ > +#define MC33XS2410_MAX_PERIOD_STEP(x) (MC33XS2410_MAX_PERIOD_STEP0 >> (2 * x)) Nitpick: These register definition become easier to parse for a human if you indent the RHS of register fields one tab further and add an empty line between the definitions for different registers. MC33XS2410_PWM_DC1 is only used once, I'd hard-code it into the definition of MC33XS2410_PWM_DC. The register fields [7:4] in MC33XS2410_PWM_CTRL3 are called PWM_ON4 .. PWM_ON1. So your x in { 0 ... 3 } is wrong. (Luckily, having some x range over { 0 ... 3 } and others orver { 1 ... 4 } is prone to error and confusion.) Also I'd drop all _MASK suffixes. For MC33XS2410_MAX_PERIOD_STEP maybe use a different variable name than for the others. For the register definitions the range is over hwpwm (which might be a good name there?), for MC33XS2410_MAX_PERIOD_STEP it's about MC33XS2410_PWM_FREQ_STEP. > +#define MC33XS2410_MAX_TRANSFERS 5 > +#define MC33XS2410_WORD_LEN 2 > + > +struct mc33xs2410_pwm { > + struct spi_device *spi; > +}; > + > +static inline struct mc33xs2410_pwm *mc33xs2410_from_chip(struct pwm_chip *chip) > +{ > + return pwmchip_get_drvdata(chip); > +} > + > +static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg, > + u16 *val, bool *ctrl, int len) Should len better be unsigned? Unless I missed something all ctrl[x] are always identical. If so represent that by a single bool. > +{ > + struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } }; > + u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN]; > + u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN]; > + int i, ret, reg_i, val_i; > + > + if (!len) > + return 0; > + > + if (read) > + len++; > + > + if (len > MC33XS2410_MAX_TRANSFERS) > + return -EINVAL; > + > + for (i = 0; i < len; i++) { > + reg_i = i * MC33XS2410_WORD_LEN; > + val_i = reg_i + 1; > + if (read) { > + if (i < len - 1) { > + tx[reg_i] = reg[i]; > + tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0; > + t[i].tx_buf = &tx[reg_i]; > + } > + > + if (i > 0) > + t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN]; > + } else { > + tx[reg_i] = reg[i] | MC33XS2410_WR; > + tx[val_i] = val[i]; > + t[i].tx_buf = &tx[reg_i]; > + } > + > + t[i].len = MC33XS2410_WORD_LEN; > + t[i].cs_change = 1; Not sure if MC33XS2410_WORD_LEN really improves readability here. Why is this done using $len transfers, wouldn't a single one do (and maybe be more performant and not rely on a spi controller that supports cs_change)? > + } > + > + t[len - 1].cs_change = 0; > + > + ret = spi_sync_transfer(spi, &t[0], len); > + if (ret < 0) > + return ret; > + > + if (read) { > + for (i = 0; i < len - 1; i++) { > + reg_i = i * MC33XS2410_WORD_LEN; > + val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK, > + get_unaligned_be16(&rx[reg_i])); > + } > + } > + > + return 0; > +} > [...] > +static > +int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl) My personal opinion: Better break the line in the argument list or not at all. Having a "static" on its own line looks ugly. > +{ > + return mc33xs2410_read_regs(spi, ®, &ctrl, val, 1); > +} > [...] > +static u64 mc33xs2410_pwm_get_period(u8 reg) > +{ > [...] > + /* Convert frequency to period, considering the doubled frequency. */ > + return DIV_ROUND_UP((u32)(2 * NSEC_PER_SEC), freq); That u32 cast isn't needed. > +} > [...] > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > [...] > + /* 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; Handling of the duty cycle is correct here, but misleading. I already added a comment here that using val[1] = 0 if rel_dc < 0 is wrong and just deleted it again after I saw (rel_dc >= 0) being used determining the value for MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm). An explicit if block would make this more obvious. mc33xs2410_pwm_get_relative_duty_cycle() is simple enough and only used once that I'd unroll it here. > + /* 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); > +} > + > +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > [...] > + state->period = mc33xs2410_pwm_get_period(val[0]); > + state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ? > + PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL; > + state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm)); > + mc33xs2410_pwm_set_relative_duty_cycle(state, val[1]); No need to set state->duty_cycle = 0 if state->enabled is false. This is another function I suggest to unroll as it hides more than it abstracts. > + return 0; > +} > + > [...] > +static int mc33xs2410_probe(struct spi_device *spi) > +{ > [...] > + /* Transition to normal mode */ > + ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL, > + MC33XS2410_GLB_CTRL_MODE, > + MC33XS2410_GLB_CTRL_MODE_NORMAL); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Failed to transition to normal mode\n"); What is the effect of this register write if the PWM was already setup by the bootloader? > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to add pwm chip\n"); > + > + return 0; > +} Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
