Re: [PATCH v9 2/2] pwm: sifive: Add a driver for SiFive SoC PWM

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]

On Tue, Mar 12, 2019 at 02:17:12PM +0100, Uwe Kleine-König wrote:
> On Tue, Mar 12, 2019 at 01:12:18PM +0100, Thierry Reding wrote:
> > On Tue, Mar 12, 2019 at 10:17:39AM +0100, Uwe Kleine-König wrote:
[...]
> > > There are a few other things that could be improved, but I think they
> > > could be addressed later. For some of these I don't even know what to
> > > suggest, for some Thierry might not agree it is worth fixing:
> > > 
> > >  - rounding
> > >    how to round? When should a request declined, when is rounding ok?
> > >    There is still "if (state->period != pwm->approx_period) return -EBUSY"
> > >    in this driver. This is better than before, but if state-period ==
> > >    pwm->approx_period + 1 the result (if accepted) might be the same as
> > >    without the +1 and so returning -EBUSY for one case and accepting the
> > >    other is strange.
> > 
> > Perhaps a good idea would be to reject a configuration only after we've
> > determined that it is incompatible? If we're really going to end up with
> > the same configuration within a given margin of period or duty cycle and
> > we can't do much about it, there's little point in rejecting such
> > configurations.
> 
> It seems we agree here. Is this important enough to delay taking this
> driver further? Currently the driver rejects too broad so if it annoys
> someone this can still be fixed later and there is only little harm
> (assuming correct error handling in the consumers).

I don't think it has to be a blocker. As you said, we'd be giving users
more flexibility, not restricting them, so it should be fine to do later
on.

> > >  - don't call PWM API functions designed for consumers (here: pwm_get_state)
> > 
> > Agreed. The driver can just access pwm_device.state directly.
> 
> I wouldn't do this either. IMHO the driver should only look into its
> hardware registers instead of using framework interna (or consumer API
> calls).

The current hardware state is already the software representation of
what the hardware has programmed. I think it's fair for drivers to make
use of that in order to avoid having to read back from hardware.
Especially if reading back from hardware might require switching on the
module to access registers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]