| Message ID | 20240927125745.38367-3-dima.fedrau@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: add support for NXPs high-side switch MC33XS2410 | expand |
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
Hello Uwe, Am Tue, Oct 22, 2024 at 09:54:50AM +0200 schrieb Uwe Kleine-König: [...] > > + > > +#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. > Adding an empty line seems reasonable to me but the additional tab doesn't help me to improve readability. > MC33XS2410_PWM_DC1 is only used once, I'd hard-code it into the > definition of MC33XS2410_PWM_DC. > Ok. Should I do the same for MC33XS2410_PWM_FREQ1 and MC33XS2410_MAX_PERIOD_STEP0 ? > 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.) > Will fix it. Should I do the same for MC33XS2410_PWM_CTRL1_POL_INV ? > Also I'd drop all _MASK suffixes. > Ok. > 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. > What about MC33XS2410_PWM_MAX_PERIOD(x) ? > > +#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? > I switch to unsigned. > Unless I missed something all ctrl[x] are always identical. If so > represent that by a single bool. > Yes, they are identical. I added the crtl[x] to be able read from ctrl and diag registers. I will change it so it is represented by a single bool, if the feature is needed in the future I can still add it. > > +{ > > + 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. > It is used throughout in the function and improves readability overall, maybe not here but for consistency I would stick to it. > 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)? > Without cs_change after every 16 bit, requests aren't processed by the device. Reading/writing from/to device fails. The SPI controller therefore must support cs_change. Single transfer is not possible because of the cs_change after every 16bit. > > + } > > + > > + 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. > Ok. > > +{ > > + 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. > Will fix it. > > +} > > [...] > > +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. > Will add an explicit if block, should I do the same for the value for MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm) ? > mc33xs2410_pwm_get_relative_duty_cycle() is simple enough and only used > once that I'd unroll it here. > You are right, will fix it. > > + /* 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. > Function can be unrolled, but the check for state->enabled is needed. The device is unable to generate a 0% duty cycle, so it is turned off to generate a 0% duty cylce. > > + 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? > When its setup is done in the bootloader and the watchdog is disabled in the bootloader it shouldn't have any impact. > > + > > + 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 Best regards Dimitri
Hello Dimitri, On Wed, Oct 23, 2024 at 02:52:21PM +0200, Dimitri Fedrau wrote: > Am Tue, Oct 22, 2024 at 09:54:50AM +0200 schrieb Uwe Kleine-König: > > [...] > > > + > > > +#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. > > Adding an empty line seems reasonable to me but the additional tab doesn't > help me to improve readability. OK, fine for me. > > MC33XS2410_PWM_DC1 is only used once, I'd hard-code it into the > > definition of MC33XS2410_PWM_DC. > > Ok. Should I do the same for MC33XS2410_PWM_FREQ1 and > MC33XS2410_MAX_PERIOD_STEP0 ? yepp. > > 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.) > > Will fix it. Should I do the same for MC33XS2410_PWM_CTRL1_POL_INV ? I guess so, otherwise you don't get consistent ranges. > > 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. > > What about MC33XS2410_PWM_MAX_PERIOD(x) ? Consistency is trump. > > > +#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) > > > > Unless I missed something all ctrl[x] are always identical. If so > > represent that by a single bool. > > Yes, they are identical. I added the crtl[x] to be able read from ctrl and > diag registers. I will change it so it is represented by a single bool, if > the feature is needed in the future I can still add it. ack. > > > +{ > > > + 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. > > It is used throughout in the function and improves readability overall, > maybe not here but for consistency I would stick to it. Seems to be subjective. > > 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)? > > Without cs_change after every 16 bit, requests aren't processed by the > device. Reading/writing from/to device fails. The SPI controller therefore > must support cs_change. Single transfer is not possible because of the > cs_change after every 16bit. There is SPI_CS_WORD for this usecase. > > > + /* 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. > > Function can be unrolled, but the check for state->enabled is needed. The > device is unable to generate a 0% duty cycle, so it is turned off to > generate a 0% duty cylce. What breaks if you drop the check for state->enabled? > > > [...] > > > +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? > > > > When its setup is done in the bootloader and the watchdog is disabled in > the bootloader it shouldn't have any impact. ok. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 0915c1e7df16..f513513f9b2f 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -411,6 +411,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 9081e0c0e9e0..c75deeeace40 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -36,6 +36,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..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)) + +#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) +{ + 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; + } + + 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_write_regs(struct spi_device *spi, u8 *reg, u16 *val, int len) +{ + + return mc33xs2410_xfer_regs(spi, false, reg, val, NULL, len); +} + +static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, bool *ctrl, + u16 *val, u8 len) +{ + return mc33xs2410_xfer_regs(spi, true, reg, val, ctrl, len); +} + + +static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u16 val) +{ + return mc33xs2410_write_regs(spi, ®, &val, 1); +} + +static +int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl) +{ + return mc33xs2410_read_regs(spi, ®, &ctrl, val, 1); +} + +static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val) +{ + return mc33xs2410_read_reg(spi, reg, val, true); +} + +static +int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u16 mask, u16 val) +{ + u16 tmp; + int ret; + + ret = mc33xs2410_read_reg_ctrl(spi, reg, &tmp); + if (ret < 0) + return ret; + + tmp &= ~mask; + tmp |= val & mask; + + return mc33xs2410_write_reg(spi, reg, tmp); +} + +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 ... 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; + } + + /* + * Round up here because a higher count results in a higher frequency + * and so a smaller period. + */ + count = DIV_ROUND_UP((u32)MC33XS2410_MAX_PERIOD_STEP(step), (u32)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, doubled_steps; + + /* + * steps: + * - 0 = 0.5Hz + * - 1 = 2Hz + * - 2 = 8Hz + * - 3 = 32Hz + * frequency = (code + 1) x steps. + * + * To avoid losing precision in case steps value is zero, scale the + * steps value for now by two and keep it in mind when calculating the + * period that the frequency had been doubled. + */ + doubled_steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP_MASK, reg) * 2); + code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT_MASK, reg); + freq = (code + 1) * doubled_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 (!state->enabled) + state->duty_cycle = 0; + else + state->duty_cycle = DIV_ROUND_UP_ULL((duty_cycle + 1) * state->period, 256); +} + +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_from_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) + return -EINVAL; + + 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); +} + +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct mc33xs2410_pwm *mc33xs2410 = mc33xs2410_from_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[4] = { true, true, true, true }; + u16 val[4]; + int ret; + + ret = mc33xs2410_read_regs(spi, reg, ctrl, val, 4); + if (ret < 0) + return ret; + + 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]); + return 0; +} + +static const struct pwm_ops mc33xs2410_pwm_ops = { + .apply = mc33xs2410_pwm_apply, + .get_state = mc33xs2410_pwm_get_state, +}; + +static int mc33xs2410_reset(struct device *dev) +{ + struct gpio_desc *reset_gpio; + + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR_OR_NULL(reset_gpio)) + return PTR_ERR_OR_ZERO(reset_gpio); + + fsleep(1000); + gpiod_set_value_cansleep(reset_gpio, 0); + /* Wake-up time */ + fsleep(10000); + + return 0; +} + +static int mc33xs2410_probe(struct spi_device *spi) +{ + struct mc33xs2410_pwm *mc33xs2410; + struct device *dev = &spi->dev; + struct pwm_chip *chip; + int ret; + + chip = devm_pwmchip_alloc(dev, 4, sizeof(*mc33xs2410)); + if (IS_ERR(chip)) + return PTR_ERR(chip); + + mc33xs2410 = mc33xs2410_from_chip(chip); + mc33xs2410->spi = spi; + chip->ops = &mc33xs2410_pwm_ops; + + ret = mc33xs2410_reset(dev); + if (ret) + return ret; + + /* + * Disable watchdog and keep in mind that the watchdog won't trigger a + * reset of the machine when running into an timeout, instead the + * control over the outputs is handed over to the INx input logic + * signals of the device. Disabling it here just deactivates this + * feature until a proper solution is found. + */ + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); + + /* 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"); + + ret = devm_pwmchip_add(dev, chip); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to add pwm chip\n"); + + return 0; +} + +static const struct spi_device_id mc33xs2410_spi_id[] = { + { "mc33xs2410" }, + { } +}; +MODULE_DEVICE_TABLE(spi, mc33xs2410_spi_id); + +static const struct of_device_id mc33xs2410_of_match[] = { + { .compatible = "nxp,mc33xs2410" }, + { } +}; +MODULE_DEVICE_TABLE(of, mc33xs2410_of_match); + +static struct spi_driver mc33xs2410_driver = { + .driver = { + .name = "mc33xs2410-pwm", + .of_match_table = mc33xs2410_of_match, + }, + .probe = mc33xs2410_probe, + .id_table = mc33xs2410_spi_id, +}; +module_spi_driver(mc33xs2410_driver); + +MODULE_DESCRIPTION("NXP MC33XS2410 high-side switch driver"); +MODULE_AUTHOR("Dimitri Fedrau <dima.fedrau@gmail.com>"); +MODULE_LICENSE("GPL");
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@gmail.com> --- drivers/pwm/Kconfig | 12 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-mc33xs2410.c | 422 +++++++++++++++++++++++++++++++++++ 3 files changed, 435 insertions(+) create mode 100644 drivers/pwm/pwm-mc33xs2410.c