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

[Uwe Kleine-König]

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

Attachment: signature.asc
Description: PGP signature