Hello, [dropping Alexandru Ardelean from Cc as their address bounces] On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote: > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote: > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote: > > > + ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG, > > > + ADP5585_OSC_EN, ADP5585_OSC_EN); > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > > The last four lines are equivalent to > > > > return ret; > > I prefer the existing code but can also change it. Well, I see the upside of your approach. If this was my only concern I wouldn't refuse to apply the patch. > > > + regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG, > > > + ADP5585_OSC_EN, 0); > > > +} > > > + > > > +static int pwm_adp5585_apply(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip); > > > + u32 on, off; > > > + int ret; > > > + > > > + if (!state->enabled) { > > > + guard(mutex)(&adp5585_pwm->lock); > > > + > > > + return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG, > > > + ADP5585_PWM_EN, 0); > > > + } > > > + > > > + if (state->period < ADP5585_PWM_MIN_PERIOD_NS || > > > + state->period > ADP5585_PWM_MAX_PERIOD_NS) > > > + return -EINVAL; > > > > Make this: > > > > if (state->period < ADP5585_PWM_MIN_PERIOD_NS) > > return -EINVAL; > > > > period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period) > > duty_cycle = min(period, state->period); > > I haven't been able to find documentation about the expected behaviour. > What's the rationale for returning an error if the period is too low, > but silently clamping it if it's too high ? Well, it's only implicitly documented in the implementation of PWM_DEBUG. The reasoning is a combination of the following thoughts: - Requiring exact matches is hard to work with, so some deviation between request and configured value should be allowed. - Rounding in both directions has strange and surprising effects. The corner cases (for all affected parties (=consumer, lowlevel driver and pwm core)) are easier if you only round in one direction. One ugly corner case in your suggested patch is: ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks. If the consumer requests period=64000.2 clock ticks, you configure for 64000. If the consumer requests period=65535.2 clock ticks you return -EINVAL. Another strange corner case is: Consider a hardware that can implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then only values >502 ns. If you configure for 501 ns, you'd get 500.3 ns. get_state() would tell you it's running at 500 ns. If you then configure 500 ns you won't get 500.3 ns any more. - If you want to allow 66535.2 clock ticks (and return 65535), what should be the maximal value that should yield 65535? Each cut-off value is arbitrary, so using \infty looks reasonable (to me at least). - Rounding down is easier than rounding up, because that's what C's / does. (Well, this is admittedly a bit arbitrary, because if you round down in .apply() you have to round up in .get_state().) > > round-closest is wrong. Testing with PWM_DEBUG should point that out. > > The right algorithm is: > > > > on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) > > off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on > > > > > > > + if (state->polarity == PWM_POLARITY_INVERSED) > > > + swap(on, off); > > > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim > > you can. > > OK, but what's the rationale ? This is also an area where I couldn't > find documentation. I don't have a good rationale here. IMHO this inverted polarity stuff is only a convenience for consumers because the start of the period isn't visible from the output wave form (apart from (maybe) the moment where you change the configuration) and so .period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL isn't distinguishable from .period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED . But it's a historic assumption of the pwm core that there is a relevant difference between the two polarities and I want at least a consistent behaviour among the lowlevel drivers. BTW, this convenience is the reason I'm not yet clear how I want to implemement a duty_offset. > > > + ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip); > > > + if (ret) { > > > + mutex_destroy(&adp5585_pwm->lock); > > > + return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n"); > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void adp5585_pwm_remove(struct platform_device *pdev) > > > +{ > > > + struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev); > > > + > > > + mutex_destroy(&adp5585_pwm->lock); > > > > Huh, this is a bad idea. The mutex is gone while the pwmchip is still > > registered. AFAIK calling mutex_destroy() is optional, and > > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of > > .probe(). > > mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config > option is selected, it gets more useful. I would prefer moving away from > the devm_* registration, and unregister the pwm_chip in .remove() > manually, before destroying the mutex. In that case I'd prefer a devm_mutex_init()?! Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature