Hello Uwe,
Am Thu, Oct 24, 2024 at 11:19:16PM +0200 schrieb Uwe Kleine-König:
> Hello Dimitri,
>
> On Wed, Oct 23, 2024 at 02:52:21PM +0200, Dimitri Fedrau wrote:
> > Am Tue, Oct 22, 2024 at 09:54:50AM +0200 schrieb Uwe Kleine-König:
> > > > +{
> > > > + 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.
>
> Seems to be subjective.
>
I will get rid of it. Due to your proposal below, to use SPI_CS_WORD, the
code to read/write from/to the device can be simplified by using a single
transaction.
> > > 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.
>
> There is SPI_CS_WORD for this usecase.
>
Thanks, didn't know about it. Helps a lot to simplify the code to
read/write from/to device. Will switch to 16 bits per word and use
SPI_CS_WORD.
> > > > + /* 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.
>
> What breaks if you drop the check for state->enabled?
>
The device is unable to generate a 0% duty cycle, to support this you
proposed in an earlier review to disable the output. Without checking if
the output is disabled, the mc33xs2410_pwm_get_state function returns the
wrong duty cycle for a previously setted 0% duty cycle. A "0" value in the
MC33XS2410_PWM_DC register means that the relative duty cylce is 1/256. As
a result there are complaints if PWM_DEBUG is enabled.
Best regards,
Dimitri
