| Message ID | 20240515112034.298116-3-dima.fedrau@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: add support for NXPs high-side switch MC33XS2410 | expand |
Hi Dimitri, kernel test robot noticed the following build errors: [auto build test ERROR on robh/for-next] [also build test ERROR on linus/master v6.9 next-20240516] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/dt-bindings-pwm-add-support-for-MC33XS2410/20240515-192237 base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next patch link: https://lore.kernel.org/r/20240515112034.298116-3-dima.fedrau%40gmail.com patch subject: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410 config: openrisc-allmodconfig (https://download.01.org/0day-ci/archive/20240516/202405162306.aFLe0sSZ-lkp@intel.com/config) compiler: or1k-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240516/202405162306.aFLe0sSZ-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405162306.aFLe0sSZ-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/pwm/pwm-mc33xs2410.c: In function 'mc33xs2410_xfer_regs': >> drivers/pwm/pwm-mc33xs2410.c:123:34: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration] 123 | val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK, | ^~~~~~~~~ drivers/pwm/pwm-mc33xs2410.c: In function 'mc33xs2410_pwm_get_freq': >> drivers/pwm/pwm-mc33xs2410.c:206:16: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration] 206 | return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | | ^~~~~~~~~~ cc1: some warnings being treated as errors vim +/FIELD_GET +123 drivers/pwm/pwm-mc33xs2410.c 74 75 static int mc33xs2410_xfer_regs(struct spi_device *spi, bool read, u8 *reg, 76 u16 *val, bool *ctrl, int len) 77 { 78 struct spi_transfer t[MC33XS2410_MAX_TRANSFERS] = { { 0 } }; 79 u8 tx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN]; 80 u8 rx[MC33XS2410_MAX_TRANSFERS * MC33XS2410_WORD_LEN]; 81 int i, ret, reg_i, val_i; 82 83 if (!len) 84 return 0; 85 86 if (read) 87 len++; 88 89 if (len > MC33XS2410_MAX_TRANSFERS) 90 return -EINVAL; 91 92 for (i = 0; i < len; i++) { 93 reg_i = i * MC33XS2410_WORD_LEN; 94 val_i = reg_i + 1; 95 if (read) { 96 if (i < len - 1) { 97 tx[reg_i] = reg[i]; 98 tx[val_i] = ctrl[i] ? MC33XS2410_RD_CTRL : 0; 99 t[i].tx_buf = &tx[reg_i]; 100 } 101 102 if (i > 0) 103 t[i].rx_buf = &rx[reg_i - MC33XS2410_WORD_LEN]; 104 } else { 105 tx[reg_i] = reg[i] | MC33XS2410_WR; 106 tx[val_i] = val[i]; 107 t[i].tx_buf = &tx[reg_i]; 108 } 109 110 t[i].len = MC33XS2410_WORD_LEN; 111 t[i].cs_change = 1; 112 } 113 114 t[len - 1].cs_change = 0; 115 116 ret = spi_sync_transfer(spi, &t[0], len); 117 if (ret < 0) 118 return ret; 119 120 if (read) { 121 for (i = 0; i < len - 1; i++) { 122 reg_i = i * MC33XS2410_WORD_LEN; > 123 val[i] = FIELD_GET(MC33XS2410_RD_DATA_MASK, 124 get_unaligned_be16(&rx[reg_i])); 125 } 126 } 127 128 return 0; 129 } 130 131 static 132 int mc33xs2410_write_regs(struct spi_device *spi, u8 *reg, u16 *val, int len) 133 { 134 135 return mc33xs2410_xfer_regs(spi, false, reg, val, NULL, len); 136 } 137 138 static int mc33xs2410_read_regs(struct spi_device *spi, u8 *reg, bool *ctrl, 139 u16 *val, u8 len) 140 { 141 return mc33xs2410_xfer_regs(spi, true, reg, val, ctrl, len); 142 } 143 144 145 static int mc33xs2410_write_reg(struct spi_device *spi, u8 reg, u16 val) 146 { 147 return mc33xs2410_write_regs(spi, ®, &val, 1); 148 } 149 150 static 151 int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, bool ctrl) 152 { 153 return mc33xs2410_read_regs(spi, ®, &ctrl, val, 1); 154 } 155 156 static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val) 157 { 158 return mc33xs2410_read_reg(spi, reg, val, true); 159 } 160 161 static 162 int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u16 mask, u16 val) 163 { 164 u16 tmp; 165 int ret; 166 167 ret = mc33xs2410_read_reg_ctrl(spi, reg, &tmp); 168 if (ret < 0) 169 return ret; 170 171 tmp &= ~mask; 172 tmp |= val & mask; 173 174 return mc33xs2410_write_reg(spi, reg, tmp); 175 } 176 177 static u8 mc33xs2410_pwm_get_freq(u64 period) 178 { 179 u8 step, count; 180 181 /* 182 * Check if period is within the limits of each of the four frequency 183 * ranges, starting with the highest frequency(lowest period). Higher 184 * frequencies are represented with better resolution by the device. 185 * Therefore favor frequency range with the better resolution to 186 * minimize error introduced by the frequency steps. 187 */ 188 189 switch (period) { 190 case MC33XS2410_MIN_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(3): 191 step = 3; 192 break; 193 case MC33XS2410_MAX_PERIOD_STEP(3) + 1 ... MC33XS2410_MAX_PERIOD_STEP(2): 194 step = 2; 195 break; 196 case MC33XS2410_MAX_PERIOD_STEP(2) + 1 ... MC33XS2410_MAX_PERIOD_STEP(1): 197 step = 1; 198 break; 199 case MC33XS2410_MAX_PERIOD_STEP(1) + 1 ... MC33XS2410_MAX_PERIOD_STEP(0): 200 step = 0; 201 break; 202 } 203 204 count = DIV_ROUND_UP(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1; 205 > 206 return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | 207 FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count); 208 } 209
Hi Dimitri,
kernel test robot noticed the following build errors:
[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.9 next-20240516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dimitri-Fedrau/dt-bindings-pwm-add-support-for-MC33XS2410/20240515-192237
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240515112034.298116-3-dima.fedrau%40gmail.com
patch subject: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410
config: arm-randconfig-r121-20240517 (https://download.01.org/0day-ci/archive/20240517/202405170826.pUFGJfD7-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240517/202405170826.pUFGJfD7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405170826.pUFGJfD7-lkp@intel.com/
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/pwm/pwm-mc33xs2410.o: in function `mc33xs2410_pwm_apply':
>> pwm-mc33xs2410.c:(.text+0x3c0): undefined reference to `__aeabi_uldivmod'
Hello, On Wed, May 15, 2024 at 01:20:34PM +0200, Dimitri Fedrau wrote: > diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c > new file mode 100644 > index 000000000000..1904d1ee0652 > --- /dev/null > +++ b/drivers/pwm/pwm-mc33xs2410.c > @@ -0,0 +1,410 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH > + * > + * 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/delay.h> > +#include <linux/err.h> > +#include <linux/math.h> > +#include <linux/math64.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/mutex.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) > +#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)) > + > +#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) > +{ > + 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; Huh, this is complicated. Isn't that covered by regmap somehow? > +} > + > [...] > + > +static u8 mc33xs2410_pwm_get_freq(u64 period) > +{ > + u8 step, count; > + > + /* > + * Check if period is within the limits of each of the four frequency > + * ranges, 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. I'm not a native English speaker, but I find that misleading. That period is in the "possible" range is already asserted by the caller. So the switch is about "Check which step is appropriate for the given period", right? > + */ > + > + 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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1; > + > + return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | > + FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count); > +} > + > [...] > + > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) > +{ > + if (!period) > + return 0; > + > + duty_cycle *= 256; This might overflow. > + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period); round-closest is most probably wrong. Please test your driver with PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and period. > + > + /* Device is not able to generate 0% duty cycle */ > + if (!duty_cycle) > + return -ERANGE; Given that the hardware emits a low level when disabled, please disable if duty_cycle = 0 is requested. > + return duty_cycle - 1; > +} > + > [...] > +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + 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[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]); > + pwm_set_relative_duty_cycle(state, val[1] + 1, 256); pwm_set_relative_duty_cycle doesn't use the right rounding for .get_state(). > + 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)); > + > + return 0; > +} > + > [...] > +static int mc33xs2410_probe(struct spi_device *spi) > +{ > [...] > + /* Disable watchdog */ > + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); Wouldn't the watchdog functionality better be handled by a dedicated watchdog driver? Disabling it here unconditionally looks wrong. > + /* Transition to normal mode */ > + ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL, > + MC33XS2410_GLB_CTRL_MODE_MASK, > + MC33XS2410_GLB_CTRL_NORMAL_MODE); > [...] Best regards Uwe
Am Mon, Jul 29, 2024 at 11:28:56PM +0200 schrieb Uwe Kleine-König: Hi Uwe, [...] > > +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; > > Huh, this is complicated. Isn't that covered by regmap somehow? > AFAIK it isn't supported. The main reason why regmap-spi doesn't work for this device is that the device needs a CS change after transmitting 16 bits. This is not covered by regmap-spi. So I would end up implementing reg_read, regmap_write should be fine in regmap-spi. Besides that if I want to come as close as possible to an atomic configuration, which is not possible, I would have to implement some bulk read/write operations and end up with a similar implementation. I would stick to the current implementation if you agree. > > +} > > + > > [...] > > + > > +static u8 mc33xs2410_pwm_get_freq(u64 period) > > +{ > > + u8 step, count; > > + > > + /* > > + * Check if period is within the limits of each of the four frequency > > + * ranges, 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. > > I'm not a native English speaker, but I find that misleading. That > period is in the "possible" range is already asserted by the caller. So > the switch is about "Check which step is appropriate for the given > period", right? > Yes, you are right. Will fix it in the next version. > > + */ > > + > > + 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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1; > > + > > + return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | > > + FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count); > > +} > > + > > [...] > > + > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) > > +{ > > + if (!period) > > + return 0; > > + > > + duty_cycle *= 256; > > This might overflow. > How ? Max period and duty_cycle is checked by the caller and can be maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I miss anything ? > > + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period); > > round-closest is most probably wrong. Please test your driver with > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and > period. > Yes, I should probably round it down. But I tested with PWM_DEBUG enabled and it gave me the best results so far. There are still few cases where there are complaints. I try to fix it. > > + > > + /* Device is not able to generate 0% duty cycle */ > > + if (!duty_cycle) > > + return -ERANGE; > > Given that the hardware emits a low level when disabled, please disable > if duty_cycle = 0 is requested. > Ok. > > + return duty_cycle - 1; > > +} > > + > > [...] > > +static int mc33xs2410_pwm_get_state(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + 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[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]); > > + pwm_set_relative_duty_cycle(state, val[1] + 1, 256); > > pwm_set_relative_duty_cycle doesn't use the right rounding for > .get_state(). > As mentioned above, will try to fix it. > > + 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)); > > + > > + return 0; > > +} > > + > > [...] > > +static int mc33xs2410_probe(struct spi_device *spi) > > +{ > > [...] > > + /* Disable watchdog */ > > + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); > > Wouldn't the watchdog functionality better be handled by a dedicated > watchdog driver? Disabling it here unconditionally looks wrong. > Yes, would be better. I planned this after this patchset is accepted. Without disabling the watchdog, the device is not able to operate. So I would stick to it for now and come up with a patch later on. > > + /* Transition to normal mode */ > > + ret = mc33xs2410_modify_reg(spi, MC33XS2410_GLB_CTRL, > > + MC33XS2410_GLB_CTRL_MODE_MASK, > > + MC33XS2410_GLB_CTRL_NORMAL_MODE); > > [...] > > Best regards > Uwe Best regards Dimitri
Hello Dimitri, On Wed, Jul 31, 2024 at 10:46:48AM +0200, Dimitri Fedrau wrote: > Am Mon, Jul 29, 2024 at 11:28:56PM +0200 schrieb Uwe Kleine-König: > > > +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; > > > > Huh, this is complicated. Isn't that covered by regmap somehow? > > AFAIK it isn't supported. The main reason why regmap-spi doesn't work for > this device is that the device needs a CS change after transmitting 16 > bits. This is not covered by regmap-spi. So I would end up implementing > reg_read, regmap_write should be fine in regmap-spi. Besides that if I > want to come as close as possible to an atomic configuration, which is not > possible, I would have to implement some bulk read/write operations and > end up with a similar implementation. I would stick to the current > implementation if you agree. ack. > > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) > > > +{ > > > + if (!period) > > > + return 0; > > > + > > > + duty_cycle *= 256; > > > > This might overflow. > > > > How ? Max period and duty_cycle is checked by the caller and can be > maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I > miss anything ? I didn't notice the check in the caller. Please add a respective comment for the next reader who might miss that. > > > + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period); > > > > round-closest is most probably wrong. Please test your driver with > > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and > > period. > > Yes, I should probably round it down. But I tested with PWM_DEBUG enabled > and it gave me the best results so far. There are still few cases where > there are complaints. I try to fix it. I don't have the hardware so I cannot test myself. Please make sure that there are no more complaints, at least none you are aware of. PWM_DEBUG should be happy if you pick a hardware setting where period is maximal but not bigger than requested and then for that given period duty_cycle is maximal but not bigger than requested. So typically use round-down division in .apply(). In .get_state() you should return a pwm_state that makes .apply() write the exact same state as found when .get_state() was called. So typically you have to use round-up there. > > > + 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)); > > > + > > > + return 0; > > > +} > > > + > > > [...] > > > +static int mc33xs2410_probe(struct spi_device *spi) > > > +{ > > > [...] > > > + /* Disable watchdog */ > > > + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); > > > + if (ret < 0) > > > + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); > > > > Wouldn't the watchdog functionality better be handled by a dedicated > > watchdog driver? Disabling it here unconditionally looks wrong. > > Yes, would be better. I planned this after this patchset is accepted. > Without disabling the watchdog, the device is not able to operate. So I > would stick to it for now and come up with a patch later on. How long is the default timeout? Don't you need to disable the watchdog in the bootloader anyhow? If you still think the watchdog should be disabled here, please add a comment that it's conceptually wrong to do here, but needed until there is a proper watchdog driver. Should this better be a mfd driver then? Best regards Uwe
Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König: Hi Uwe, [...] > > > > +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) > > > > +{ > > > > + if (!period) > > > > + return 0; > > > > + > > > > + duty_cycle *= 256; > > > > > > This might overflow. > > > > > > > How ? Max period and duty_cycle is checked by the caller and can be > > maximum 2000000000, 2000000000 * 256 = 512000000000, fits in u64. Did I > > miss anything ? > > I didn't notice the check in the caller. Please add a respective comment > for the next reader who might miss that. > Ok. > > > > + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period); > > > > > > round-closest is most probably wrong. Please test your driver with > > > PWM_DEBUG enabled and increasing and decreasing series of duty_cycle and > > > period. > > > > Yes, I should probably round it down. But I tested with PWM_DEBUG enabled > > and it gave me the best results so far. There are still few cases where > > there are complaints. I try to fix it. > > I don't have the hardware so I cannot test myself. Please make sure that > there are no more complaints, at least none you are aware of. PWM_DEBUG > should be happy if you pick a hardware setting where period is maximal > but not bigger than requested and then for that given period duty_cycle > is maximal but not bigger than requested. So typically use round-down > division in .apply(). In .get_state() you should return a pwm_state that > makes .apply() write the exact same state as found when .get_state() was > called. So typically you have to use round-up there. > Thanks, this fixed the remaining complaints. Did it exactly as you suggested. > > > > + 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)); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > [...] > > > > +static int mc33xs2410_probe(struct spi_device *spi) > > > > +{ > > > > [...] > > > > + /* Disable watchdog */ > > > > + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); > > > > + if (ret < 0) > > > > + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); > > > > > > Wouldn't the watchdog functionality better be handled by a dedicated > > > watchdog driver? Disabling it here unconditionally looks wrong. > > > > Yes, would be better. I planned this after this patchset is accepted. > > Without disabling the watchdog, the device is not able to operate. So I > > would stick to it for now and come up with a patch later on. > > How long is the default timeout? Don't you need to disable the watchdog > in the bootloader anyhow? > Default and also maximum timeout is 256ms. My hardware is configured to let the device stay in reset until the driver puts it out of reset by setting the associated GPIO. Datasheet refers to it as "Disable mode". Outputs are turned off. Without having such reset logic, the device would need to have the watchdog disabled in the bootloader and continue in "Normal mode" or it would go into "Safe mode" while booting the kernel almost sure violating the timeout. Outputs are then controlled by the INx input logic signals. I think there is no single solution but rather depends on the use case. There are three modes which the device can be in when the driver is probed: "Disable", "Safe" and "Normal". All three are handled by this driver, assuming the watchdog should be disabled. > If you still think the watchdog should be disabled here, please add a > comment that it's conceptually wrong to do here, but needed until there > is a proper watchdog driver. > I will leave a comment, but I'm not sure if I can come up with a good solution since the maximum timeout is very low with 256ms. For interaction with userspace it must be at least 1s. Why is the handling of the watchdog conceptually wrong here, I could use devm_watchdog_register_device to register it, why can't I just disable it here ? > Should this better be a mfd driver then? > I don't thinks so, the watchdog and the outputs belong somehow together. When the device runs into an timeout it switches into "Safe" mode and the outputs are controlled by the INx input logic signals. I thought of a mfd device as a device which provides multiple functions not really belonging together. Didn't find a clear definition. What do you think of it ? Best regards Dimitri
Hello Dimitri, On Thu, Aug 01, 2024 at 04:28:02PM +0200, Dimitri Fedrau wrote: > Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König: > > > > > + 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)); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > [...] > > > > > +static int mc33xs2410_probe(struct spi_device *spi) > > > > > +{ > > > > > [...] > > > > > + /* Disable watchdog */ > > > > > + ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0); > > > > > + if (ret < 0) > > > > > + return dev_err_probe(dev, ret, "Failed to disable watchdog\n"); > > > > > > > > Wouldn't the watchdog functionality better be handled by a dedicated > > > > watchdog driver? Disabling it here unconditionally looks wrong. > > > > > > Yes, would be better. I planned this after this patchset is accepted. > > > Without disabling the watchdog, the device is not able to operate. So I > > > would stick to it for now and come up with a patch later on. > > > > How long is the default timeout? Don't you need to disable the watchdog > > in the bootloader anyhow? > > Default and also maximum timeout is 256ms. My hardware is configured to > let the device stay in reset until the driver puts it out of reset by > setting the associated GPIO. Datasheet refers to it as "Disable mode". > Outputs are turned off. > Without having such reset logic, the device would need to have the > watchdog disabled in the bootloader and continue in "Normal mode" or it > would go into "Safe mode" while booting the kernel almost sure violating > the timeout. Outputs are then controlled by the INx input logic signals. > I think there is no single solution but rather depends on the use case. > There are three modes which the device can be in when the driver is probed: > "Disable", "Safe" and "Normal". All three are handled by this driver, > assuming the watchdog should be disabled. > > [...] > > Should this better be a mfd driver then? > > > > I don't thinks so, the watchdog and the outputs belong somehow together. Ah, so the watchdog doesn't trigger a reboot of the machine, just resets the I/O lines to input? If yes, that's not what a watchdog driver is about and so this isn't a hint to create an mfd driver. Best regards Uwe
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..1904d1ee0652 --- /dev/null +++ b/drivers/pwm/pwm-mc33xs2410.c @@ -0,0 +1,410 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH + * + * 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/delay.h> +#include <linux/err.h> +#include <linux/math.h> +#include <linux/math64.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/mutex.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) +#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)) + +#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) +{ + 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 if period is within the limits of each of the four frequency + * ranges, 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(MC33XS2410_MAX_PERIOD_STEP(step), period) - 1; + + return FIELD_PREP(MC33XS2410_PWM_FREQ_STEP_MASK, step) | + FIELD_PREP(MC33XS2410_PWM_FREQ_COUNT_MASK, count); +} + +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 + * value for now by two and keep it in mind when calculating the period + * that we have doubled the frequency. + */ + 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(2 * NSEC_PER_SEC, freq); +} + +static int mc33xs2410_pwm_get_relative_duty_cycle(u64 period, u64 duty_cycle) +{ + if (!period) + return 0; + + duty_cycle *= 256; + duty_cycle = DIV_ROUND_CLOSEST_ULL(duty_cycle, period); + + /* Device is not able to generate 0% duty cycle */ + if (!duty_cycle) + return -ERANGE; + + return duty_cycle - 1; +} + +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; + u16 val[4]; + u8 mask; + int ret; + + period = min(state->period, MC33XS2410_MAX_PERIOD_STEP(0)); + if (period < MC33XS2410_MIN_PERIOD_STEP(3) + 1) + 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); + + /* duty cycle */ + duty_cycle = min(period, state->duty_cycle); + ret = mc33xs2410_pwm_get_relative_duty_cycle(period, duty_cycle); + if (ret < 0) + return ret; + + val[1] = ret; + /* 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) ? (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 = 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[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]); + pwm_set_relative_duty_cycle(state, val[1] + 1, 256); + 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)); + + 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 = to_pwm_mc33xs2410_chip(chip); + mc33xs2410->spi = spi; + chip->ops = &mc33xs2410_pwm_ops; + + ret = mc33xs2410_reset(dev); + if (ret) + return ret; + + /* Disable watchdog */ + 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_MASK, + MC33XS2410_GLB_CTRL_NORMAL_MODE); + 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 | 410 +++++++++++++++++++++++++++++++++++ 3 files changed, 423 insertions(+) create mode 100644 drivers/pwm/pwm-mc33xs2410.c