Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410

[Dimitri Fedrau <dima.fedrau@gmail.com>]

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