Re: [PATCH 1/3] dt-bindings: added new pwm-sifive driver documentation

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

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

On Fri, Apr 27, 2018 at 03:59:56PM -0700, Wesley W. Terpstra wrote:
> Document new PWM device tree bindings for SiFive SoCs.
> 
> Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> ---
>  .../devicetree/bindings/pwm/pwm-sifive.txt         | 28 ++++++++++++++++++++++
>  1 file changed, 28 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..7cea20d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sifive.txt
> @@ -0,0 +1,28 @@
> +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.
> +The period also has significant restrictions on the values it can achieve,
> +which the driver rounds to the nearest achievable frequency.

How about you encode this in the driver rather than DT? We have several
drivers with similar restrictions and they usually allow the first PWM
channel to choose an arbitrary period and return an error if subsequent
channels request a period that can't be supported.

I think something similar could work with that second restriction. Why
not return an error if the exact period can't be set? Or perhaps allow
some percentage of deviation.

If the exact period is achieved in a deterministic way, it should be
possible for board designers to specify it exactly, so the consumer's
pwms property can simply take the correct period.

> +Required properties:
> +- compatible: should be "sifive,pwm0"

Why not simply "sifive,pwm"? If this is supposed to be some sort of
version number, then it is more customary to use the name of the first
SoC that integrates the IP. There are some exceptions, like for example
when the IP is third-party and is integrated in a number of different
SoCs. In such cases the IP is often properly versioned. But that doesn't
seem to be the case here.

Thierry

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