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
