| Message ID | 20220204180106.154000-1-sean.anderson@seco.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v13,1/2] dt-bindings: pwm: Add Xilinx AXI Timer | expand |
Hello, just a few minor things left for me to criticise: On Fri, Feb 04, 2022 at 01:01:06PM -0500, Sean Anderson wrote: > [...] > + regmap_read(priv->map, TCSR1, &tcsr1); > + state->period = xilinx_timer_get_period(priv, tlr0, tcsr0); > + state->duty_cycle = xilinx_timer_get_period(priv, tlr1, tcsr1); > + state->enabled = xilinx_timer_pwm_enabled(tcsr0, tcsr1); > + state->polarity = PWM_POLARITY_NORMAL; > + > + /* > + * 100% duty cycle results in constant low output. This may be slightly > + * wrong if rate >= 1GHz, so fix this if you have such hardware :) > + */ I'd drop "slightly" here. If the bug happens (e.g. tlr0 = 999999999, tlr1 = 999999998, clkrate = 1000000001 Hz) then you diagnose a nearly 100% relative duty cycle as 0%. Also s/>= 1GHz/> 1 GHz/. > [...] > + if (one_timer) > + return dev_err_probe(dev, -EINVAL, > + "Two timers required for PWM mode\n"); > + > + One empty line here would be enough. > diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h > new file mode 100644 > index 000000000000..1f7757b84a5e > --- /dev/null > +++ b/include/clocksource/timer-xilinx.h > @@ -0,0 +1,91 @@ > [...] > +/** > + * xilinx_timer_common_init() - Perform common initialization for Xilinx > + * AXI timer drivers. > + * @priv: The timer's private data > + * @np: The devicetree node for the timer > + * @one_timer: Set to %1 if there is only one timer > + * > + * This performs common initialization, such as detecting endianness, > + * and parsing devicetree properties. @priv->regs must be initialized > + * before calling this function. This function initializes @priv->read, > + * @priv->write, and @priv->width. > + * > + * Return: 0, or negative errno > + */ > +int xilinx_timer_common_init(struct device_node *np, > + struct xilinx_timer_priv *priv, > + u32 *one_timer); This function is gone, so the prototype should go away, too. Best regards Uwe
diff --git a/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml new file mode 100644 index 000000000000..dd168d41d2e0 --- /dev/null +++ b/Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml @@ -0,0 +1,92 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/timer/xlnx,xps-timer.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Xilinx LogiCORE IP AXI Timer Device Tree Binding + +maintainers: + - Sean Anderson <sean.anderson@seco.com> + +properties: + compatible: + contains: + const: xlnx,xps-timer-1.00.a + + clocks: + maxItems: 1 + + clock-names: + const: s_axi_aclk + + interrupts: + maxItems: 1 + + reg: + maxItems: 1 + + '#pwm-cells': true + + xlnx,count-width: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [8, 16, 32] + default: 32 + description: + The width of the counter(s), in bits. + + xlnx,one-timer-only: + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [ 0, 1 ] + description: + Whether only one timer is present in this block. + +required: + - compatible + - reg + - xlnx,one-timer-only + +allOf: + - if: + required: + - '#pwm-cells' + then: + allOf: + - required: + - clocks + - properties: + xlnx,one-timer-only: + const: 0 + else: + required: + - interrupts + - if: + required: + - clocks + then: + required: + - clock-names + +additionalProperties: false + +examples: + - | + timer@800e0000 { + clock-names = "s_axi_aclk"; + clocks = <&zynqmp_clk 71>; + compatible = "xlnx,xps-timer-1.00.a"; + reg = <0x800e0000 0x10000>; + interrupts = <0 39 2>; + xlnx,count-width = <16>; + xlnx,one-timer-only = <0x0>; + }; + + timer@800f0000 { + #pwm-cells = <0>; + clock-names = "s_axi_aclk"; + clocks = <&zynqmp_clk 71>; + compatible = "xlnx,xps-timer-1.00.a"; + reg = <0x800e0000 0x10000>; + xlnx,count-width = <32>; + xlnx,one-timer-only = <0x0>; + };