| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250320185118.GA352940@legfed1>
Date: Thu, 20 Mar 2025 19:51:18 +0100
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>, linux-pwm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/2] pwm: add support for NXPs high-side switch
MC33XS2410
Hi Uwe,
Am Wed, Mar 19, 2025 at 04:37:43PM +0100 schrieb Uwe Kleine-König:
[...]
> > +#define MC33XS2410_PWM_CTRL1 0x05
> > +/* x in { 1 ... 4 } */
>
> s/x/chan/
>
Will fix it.
> > +#define MC33XS2410_PWM_CTRL1_POL_INV(chan) BIT((chan) + 1)
> > +
> > +#define MC33XS2410_PWM_CTRL3 0x07
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_CTRL3_EN(chan) BIT(4 + (chan) - 1)
> > +
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_FREQ(chan) (0x08 + (chan) - 1)
> > +#define MC33XS2410_PWM_FREQ_STEP GENMASK(7, 6)
> > +#define MC33XS2410_PWM_FREQ_COUNT GENMASK(5, 0)
> > +
> > +/* x in { 1 ... 4 } */
> > +#define MC33XS2410_PWM_DC(chan) (0x0c + (chan) - 1)
> > +
> > +#define MC33XS2410_WDT 0x14
> > +
> > +#define MC33XS2410_PWM_MIN_PERIOD 488282
> > +/* x in { 0 ... 3 } */
>
> s/x/step/
>
Will fix it.
[...]
> > +static u8 mc33xs2410_pwm_get_freq(u64 period)
> > +{
> > + u8 step, count;
> > +
> > + /*
> > + * Check which step is appropriate for the given period, 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.
>
> This is hard to understand as the argument is presented in several
> steps:
> algo starts with high step values (that's not in the comment,
> you have to take that from the switch statement)
> high step represents high freq = low period
> high freq yield better resolution
> better resolution is what is favoured.
>
> After investing some time to reunderstand the rational here
> I suggest:
>
> /*
> * Check which step [0 .. 3] is appropriate for the given
> * period. The period ranges for the different step values
> * overlap. Prefer big step values as these allow more
> * finegrained duty cycle selection.
> */
>
I think this is way better then before. But did you mean:
Prefer big step values as these allow more finegrained "periods" then
"duty cylces" ?
[...]
> > +
> > + /*
> > + * steps:
> > + * - 0 = 0.5Hz
> > + * - 1 = 2Hz
> > + * - 2 = 8Hz
> > + * - 3 = 32Hz
> > + * frequency = (code + 1) x steps.
> > + *
> > + * To avoid losing precision in case steps value is zero, scale the
> > + * steps value for now by two and keep it in mind when calculating the
> > + * period that the frequency had been doubled.
> > + */
> > + doubled_steps = 1 << (FIELD_GET(MC33XS2410_PWM_FREQ_STEP, reg) * 2);
> > + code = FIELD_GET(MC33XS2410_PWM_FREQ_COUNT, reg);
> > + freq = (code + 1) * doubled_steps;
>
> doubled_freq?
>
Will change to doubled_freq.
> > + /* Convert frequency to period, considering the doubled frequency. */
> > + return DIV_ROUND_UP(2 * NSEC_PER_SEC, freq);
> > +}
> > +
> > +static int mc33xs2410_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct spi_device *spi = pwmchip_get_drvdata(chip);
> > + u8 reg[4] = {
> > + MC33XS2410_PWM_FREQ(pwm->hwpwm + 1),
> > + MC33XS2410_PWM_DC(pwm->hwpwm + 1),
> > + MC33XS2410_PWM_CTRL1,
> > + MC33XS2410_PWM_CTRL3
> > + };
> > + u64 period, duty_cycle;
> > + int ret, rel_dc;
> > + u16 rd_val[2];
> > + u8 wr_val[4];
> > + u8 mask;
> > +
> > + period = min(state->period, MC33XS2410_PWM_MAX_PERIOD(0));
> > + if (period < MC33XS2410_PWM_MIN_PERIOD)
> > + return -EINVAL;
> > +
> > + ret = mc33xs2410_read_regs(spi, ®[2], MC33XS2410_FRAME_IN_DATA_RD, rd_val, 2);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* frequency */
> > + wr_val[0] = mc33xs2410_pwm_get_freq(period);
> > + /* Continue calculations with the possibly truncated period */
> > + period = mc33xs2410_pwm_get_period(wr_val[0]);
> > +
> > + /* duty cycle */
> > + duty_cycle = min(period, state->duty_cycle);
> > + rel_dc = div64_u64(duty_cycle * 256, period) - 1;
> > + if (rel_dc < 0)
> > + wr_val[1] = 0;
> > + else
> > + wr_val[1] = rel_dc;
> > +
> > + /* polarity */
> > + mask = MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm + 1);
> > + wr_val[2] = (state->polarity == PWM_POLARITY_INVERSED) ?
> > + (rd_val[0] | mask) : (rd_val[0] & ~mask);
> > +
> > + /*
> > + * As the hardware cannot generate a 0% relative duty cycle but emits a
> > + * constant low signal when disabled, also disable in the duty_cycle = 0
> > + * case.
> > + */
> > + mask = MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm + 1);
> > + wr_val[3] = (state->enabled && rel_dc >= 0) ? (rd_val[1] | mask) :
> > + (rd_val[1] & ~mask);
>
> This is wrong for inversed polarity I think.
Thanks for finding this. Yes, it is for the case when duty_cycle=0 and
polarity=PWM_POLARITY_INVERSED. The device should then output a constant
high signal. This could be done by:
enabled=1
duty_cycle=100%
polarity=normal
Just tested it, it works. What do you think ?
>
> > + return mc33xs2410_write_regs(spi, reg, wr_val, 4);
> > +}
> > +
> > [..]
> > +static int mc33xs2410_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct pwm_chip *chip;
> > + int ret;
> > +
> > + chip = devm_pwmchip_alloc(dev, 4, 0);
> > + if (IS_ERR(chip))
> > + return PTR_ERR(chip);
> > +
> > + spi->bits_per_word = 16;
> > + spi->mode |= SPI_CS_WORD;
> > + ret = spi_setup(spi);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pwmchip_set_drvdata(chip, spi);
> > + chip->ops = &mc33xs2410_pwm_ops;
> > + ret = mc33xs2410_reset(dev);
>
> What does this reset? Does it change the output signal if the bootloader
> already setup the hardware? The answer to that has to go into a comment.
> (And if it interupts the output, better skip the reset part.)
>
If reset of the device is asserted on probe it deasserts the reset and
makes the device available. On the other hand if it is already setup by
the bootloader the reset will clear all registers to default values which
means that the output signal will change. The reset gpio is optional, so
the user can decide wether to use it or not. Both usecases are
supported. I will add an comment, but will keep the reset handling.
Best regards,
Dimitri Fedrau
Powered by blists - more mailing lists