[PATCH linux v3 1/3] Documentation: dt-bindings: Document binding for ASPEED AST2400/2500 PWM support

[Joel Stanley]

Hello Jaghu,

This is the first time I've had a close look at the binding document.
Note that I'm not a device tree expert. I do have some suggestions
below that you might want to look at before we send this upstream.

On Thu, Nov 24, 2016 at 7:56 PM, Jaghathiswari Rankappagounder
Natarajan <jaghu at google.com> wrote:
> This binding provides interface for adding values related to ASPEED
> AST2400/2500 PWM support.
> The parent node indicates a PWM controller. The parent node can have
> upto 8 child nodes. Each node indicates a PWM output port.
> PWM clock types M, N and 0 are three types just to have three independent
> PWM sources. Each port can be assigned a different PWM clock type.
> Each port can have the duty-cycle set to a different value.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu at google.com>
> ---
> v2:
> - Made binding documentation update as a separate patch.
> - Included AST2500 in the description.
> - Changed format to "aspeed,ast2500-pwm".
> - Included explanation for types M ,N and O and calculation example to
> -  determine L, H, and period bits.
>
> v3:
> - Included struct clk and phandle reference to clock node.
> - Removed clock calculation description from the bindings document.
> - Added some whitespace and improved style of binding document.
>
>  .../devicetree/bindings/hwmon/aspeed-pwm.txt       | 104 +++++++++++++++++++++
>  1 file changed, 104 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt
> new file mode 100644
> index 0000000..11a007c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt
> @@ -0,0 +1,104 @@
> +ASPEED AST2400/AST2500 PWM device driver
> +
> +The ASPEED PWM controller can support 8 PWM outputs.
> +PWM clock types M, N and 0 are three types just to have three
> +independent PWM sources. Each could be assigned to the 8 PWM port
> +with it's own settings.
> +
> +Required properties:
> +- #address-cells : should be 1.
> +
> +- #size-cells : should be 1.
> +
> +- reg : address and length of the register set for the device.
> +
> +- pinctrl-names : a pinctrl state named "default" must be defined.
> +
> +- pinctrl-0 : phandle referencing pin configuration of the ASPEED PWM ports.
> +
> +- compatible : should be "aspeed,ast2400-pwm" for AST2400 or
> +  "aspeed,ast2500-pwm" for AST2500.
> +
> +- clocks : phandle reference to clocks. upto three fixed clocks providing
> +  three different input clock frequencies. each clock frequency indicates a
> +  desired PWM frequency.
> +
> +   the first PWM frequency in combination with type M PWM period value
> +    (described below) and source clock frequency will be used to calculate the
> +    type M PWM clock division high and low values.

I can't see where you describe what a "type M PWM period value" is.
Can you teach me?

> +   the second PWM frequency in combination with type N PWM period value and
> +    source clock frequency will be used to calculate the type N PWM clock
> +    division high and low values.
> +   the third PWM frequency in combination with type O PWM period value and
> +    source clock frequency will be used to calculate the type O PWM clock
> +    division high and low values.
> +
> +  Since atleast one PWM frequency or one PWM clock type(type M PWM clock type)
> +  is required, the first clock is required. the other two are optional.
> +
> +- pwm_typem_period : indicates type M PWM period. integer value in the range
> +  0 to 255.
> +
> +Optional properties:
> +- pwm_typen_period : indicates type N PWM period. integer value in the range
> +  0 to 255.
> +
> +- pwm_typeo_period : indicates type O PWM period. integer value in the range
> +  0 to 255.
> +
> +Each subnode represents a PWM output port.
> +
> +Subnode Format
> +--------------
> +Required properties:
> +- pwm_port : indicates the PWM port. values 0 through 7 indicate PWM ports
> +  A through H respectively.

Should we instead use a reg cell to indicate this?

> +
> +- pwm_enable : option to enable the PWM port indicated above. 0 is enable
> +  and 1 is disable.

It looks like you're using this as a boolean. I don't think that
upstream will accept this use of the device tree. What does "enable"
mean? Can we use the device tree convention of status =
"disabled"/"okay" to indicate if the driver should bind to this device
and enable the hardware?

> +
> +- pwm_type : indicates the clock type for the PWM port. value 0 is type M
> +  PWM clock. value 1 is type N PWM clock. value 2 is type O PWM clock.

Could we instead indicate this type in the compatible string?

> +
> +- pwm_fan_ctrl : pulse width modulation fan control. integer value
> +  in the range 0 to 255. 255 is max or 100%.

> +
> +Additional details are available in the detailed datasheet for the device
> +AST2400/AST2500.

I don't think this comment is useful, as anyone reading it will not
necessarily have access to the data sheet.

> +
> +Examples:
> +
> +clocks {
> +       #address-cells = <1>;
> +       #size-cells = <0>;
> +
> +       pwm_typem_clock: fixedclk {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               clock-frequency = <25000>;
> +       }

This is not what I meant about the clocks. See my comments in patch 2.

It does raise a separate point. Does the clock frequency value (25000)
describe a property of how the hardware is configured? If so, we might
want to keep it in the device tree, just differently.