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

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

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, &reg, &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