On Wed, Feb 06, 2019 at 05:16:34PM +0100, Thierry Reding wrote: > On Wed, Feb 06, 2019 at 04:38:54PM +0100, Uwe Kleine-König wrote: > > On Wed, Feb 06, 2019 at 01:40:55PM +0100, Thierry Reding wrote: > > > On Wed, Feb 06, 2019 at 12:07:30PM +0100, Uwe Kleine-König wrote: > > > > On Wed, Feb 06, 2019 at 04:18:47PM +0530, Yash Shah wrote: > > > > > On Wed, Jan 30, 2019 at 1:44 PM Uwe Kleine-König > > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > On Tue, Jan 29, 2019 at 05:13:18PM +0530, Yash Shah wrote: > > > > > > > DT documentation for PWM controller added. > > > > > > > > > > > > > > Signed-off-by: Wesley W. Terpstra <wesley@xxxxxxxxxx> > > > > > > > [Atish: Compatible string update] > > > > > > > Signed-off-by: Atish Patra <atish.patra@xxxxxxx> > > > > > > > Signed-off-by: Yash Shah <yash.shah@xxxxxxxxxx> > > > > > > > --- > > > > > > > .../devicetree/bindings/pwm/pwm-sifive.txt | 33 ++++++++++++++++++++++ > > > > > > > 1 file changed, 33 insertions(+) > > > > > > > create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sifive.txt b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > > > > > new file mode 100644 > > > > > > > index 0000000..8dcb40d > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt > > > > > > > @@ -0,0 +1,33 @@ > > > > > > > +SiFive PWM controller > > > > > > > + > > > > > > > +Unlike most other PWM controllers, the SiFive PWM controller currently only > > > > > > > +supports one period for all channels in the PWM. This is set globally in DTS. > > > > > > > > > > > > You can simply drop this if the first user can set this using the usual > > > > > > interface. Don't you like this suggestion that I already made a few > > > > > > times now? > > > > > > > > > > > > Did you consider to make the driver support only a single output with a > > > > > > more flexible period setting? > > > > > > > > > > We cannot consider supporting only single output since we have boards that > > > > > use the additional PWM channels to control individual LED brightness > > > > > of a tri-color LED. > > > > > If we go down to one channel, then we can't control the brightness of > > > > > the individual LEDs. > > > > > It will break the use case. > > > > > > > > OK. > > > > > > > > > I am considering the below approach, let me know if it's fine by you. > > > > > > > > > > - Drop the global period property and allow the only first user to change period > > > > > using the usual interface. > > > > > - A note in the binding that all PWMs need to run at the same > > > > > period. If the driver already refuses to apply incompatible periods, > > > > > the users are going to notice that they've got the DT wrong. > > > > > - In driver code, count the users using the .request and .free callbacks. > > > > > Based on this, allow changes to period iff the user count is one. > > > > > > > > Not sure you need to point this limitation in the binding doc. Other > > > > than that this is fine. > > > > > > I think it's useful to point this out in the binding documentation since > > > it's something that device tree writers will want to know. > > > > OK, we're talking about an FPGA implementation but I still think if the > > dt-author is the first to know about this limitation this is too late. > > The hardware designer must know about that, and they don't look into the > > bindind documentation. The binding documentation should IMHO only > > describe the binding and functional limitiations of the device are out > > of scope. > > > > The binding for the Freescale network interface doesn't describe that it > > only supports 480 MBit/s in its GHz mode, and that's good. The binding > > for i2c devices doesn't describe that a reboot in the middle of a > > transfer might block the bus. > > > > The target for the binding documentation is to document how a given > > device is described in a device tree. It's not there to duplicate > > information that belongs in the reference manual. > > But the device tree bindings for PWM devices describe where and how to > describe the period for each PWM that you reference. That's entirely > different from the examples that you give above. Ah right, I had not on my radar that the binding also gives a suggestion for the period. Then yes, I agree that pointing out that all pwms belonging to the same provider should have matching periods is a good idea. Is using this "typical" approach to specify the period in a pwm-list a bad idea for this device type for this reason? On the other hand diverting from the typical approach has its own cost. Not sure ... Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
