Hello Dimitri, On Thu, Mar 20, 2025 at 07:51:18PM +0100, Dimitri Fedrau wrote: > Am Wed, Mar 19, 2025 at 04:37:43PM +0100 schrieb Uwe Kleine-König: funny denglisch. :-) > > 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" ? It's both. I wrote without considering period because (in my bubble) most consumers tend to care more for finegrained duty cycle than exact period. Feel free to add "period and" to my proposal. (Assuming you agree that the result is correct then.) > > > + /* > > > + * 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 ? Sounds like the right thing to do. Probably needs a comment as this is an asymmetry between the two polarities. > > > + 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. Conceptually it's wrong to make the distinction between "A reset at probe time is desired" and "Please no reset at probe" via a modification in the device tree as this is supposed to be an application agnostic hardware description. I have no nice and constructive suggestion here. I think I'd go for only deasserting reset in the Linux driver and otherwise rely on the bootloader to pass the device in a usable state. With a sane hardware design not doing anything is a valid possibility for the bootloader then. Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
