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:
>
> [...]
> > > +
> > > +#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.
OK, fine for me.
> > 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 ?
yepp.
> > 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 ?
I guess so, otherwise you don't get consistent ranges.
> > 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) ?
Consistency is trump.
> > > +#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)
> >
> > 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.
ack.
> > > +{
> > > + 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.
> > 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.
> > > + /* 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?
> > > [...]
> > > +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.
ok.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
