Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

[Thierry Reding]

On Fri, Dec 11, 2020 at 11:34:54AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote:
> > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> > > > Like I said, that's not what I was saying. I was merely saying that if
> > > > there aren't any use-cases that current users rely on that would be
> > > > broken by using this simpler implementation, then I'm okay with it, even
> > > > if it's less flexible than a more complicated implementation. It should
> > > > be possible to determine what the current users are by inspecting device
> > > > trees present in the kernel. Anything outside the kernel isn't something
> > > > we need to consider, as usual.
> > > 
> > > If "users in mainline" is the criteria that's a word.
> > 
> > I didn't say "users in mainline", I said "use-cases". What I don't want
> > to happen is for this change under discussion to break any existing use-
> > cases of any existing users in the kernel. I said that we can determine
> > what the existing users are by looking at which device trees use the
> > compatible strings that the driver matches on.
> > 
> > > So you agree we remove the following drivers?:
> > > 
> > >  - pwm-hibvt.c
> > >    Last driver specific change in Feb 2019, no mainline user
> > >  - pwm-sprd.c
> > >    Last driver specific change in Aug 2019, no mainline user
> > 
> > No, that's an extrapolation of what I was saying above. Drivers with no
> > apparent users are a separate topic, so don't conflate it with the issue
> > at hand.
> 
> I cannot follow (and I think that's the problem between us and why those
> conflicts happen between us). For me it's a logic consequence of
> "Anything outside the kernel isn't something we need to consider, as
> usual." that drivers that are untouched for some period and have no
> mainline users can/should go away. (Is "extrapolation" as strong as
> "implication", or has it subjective interpretation added? My dictionary
> isn't accurate enough for that question.) But it seems there is
> something with my logic or you not saying exactly what you actually
> mean. (Did I miss any option? If yes it might be covered by a problem in
> my logic.)

To me this is not as black and white as it seems to be for you. First I
wasn't talking about unused drivers, but about use-cases that are not
represented in mainline. Secondly I didn't say anything about removing
support for use-cases that weren't in use upstream. All I said was that
I didn't want any changes to regress existing use-cases.

"Guessing" how that statement reflects on my thoughts about unused
drivers is extrapolation. Your logic implication could've been correct,
but in this case it wasn't because I consider a driver that was
upstreamed to be part of the kernel, and people invested a fair amount
of work to get it to that point, so I'm in no hurry to get rid of them.
Instead, I prefer to give people the benefit of the doubt and assume
that they had planned to get users upstream and for some reason just
haven't gotten around to it.

On the other hand, almost 18-24 months without activity is quite long. A
compromise that works well for me is to keep drivers, even unused ones,
as long as they're not getting in the way.

> Having said that, even in the question at hand (i.e. what is the better
> compromise for mapping the inter-channel hardware limitations to
> software policy in the pac9685 driver) the idea "let's inspecting device
> trees present in the kernel" doesn't work, because for this driver there
> are none, too. (It might be used by a mainline machine via ACPI, but
> this is even less possible to consider for our judgements than a device
> tree with such a device and no user but the sysfs PWM interface.)

Perhaps Clemens and Sven can shed some light into how this driver is
being used. There clearly seem to be people interested in this driver,
so why are there no consumers of this upstream. What's keeping people
from upstreaming device trees that make use of this?

> > While it's certainly unfortunate that these don't seem to be used, I see
> > no reason why we should remove them. They don't create much of a
> > maintenance burden, so I'm fine with keeping them in the hopes that
> > users may still show up at some point.
> 
> The problem I have with them is that I expect your voice of dissent when
> I find the time to improve the rounding behaviour of these drivers.
> My ultimate goal is to make the PWM framework a system where consumers
> can rely on a consistent behaviour of the API and a way to actually
> order what they need and get it. I'm not entirely sure we agree that
> we're not there yet.

I don't think we entirely agree that the framework is currently
inconsistent. As you and others have shown in various threads, one way
of rounding isn't necessarily always the best way. Some consumers expect
a certain accuracy and don't care whether we over- or undershoot. Some
controllers may give more accurate results when rounding up than when
rounding down and for others rounding to nearest is the best option. A
lot of consumers don't care all that much about rounding as long as the
duty-cycle/period ratio is reasonably close.

So there are a lot of variables in all of this and I don't think we can
fix the rules so that they work for everyone. That's why I think we need
to stick with the status quo that does work for pretty much everyone so
far by being not overly strict and then extend that if we have to (i.e.
if we come across a use-case where the current status quo doesn't work).

> > > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder
> > > if it makes sense to check the machine.dts files if some of the PMWs are
> > > completely unused. Do you consider status = "okay" a use that we have to
> > > retain even if the node has no phandle?
> > 
> > A PWM controller may be in use via sysfs even if it has no phandle.
> 
> Yeah, I expected you will say that. (And I agree.)

sysfs on the PWM side does complicate things a little bit, but on the
other hand it's not very different from other consumers. For any change
at the API level (such as adding rounding modes, if we have to) we just
need to make sure this can be represented using the sysfs interface as
well.

And by the way, as an additional argument against removing seemingly
unused drivers: it's technically possible to instantiate a PCA9685 from
userspace using sysfs, and there might be people relying on this to get
their job done. Again, there are many shades of grey here and lots of
unknowns, and extrapolation doesn't work very well under those
circumstances.

Thierry

Attachment: signature.asc
Description: PGP signature