[PATCH linux v2 0/3] ASPEED AST2500 PWM support

[PATCH linux v2 0/3] ASPEED AST2500 PWM support

[Jaghathiswari Rankappagounder Natarajan]

Support for ASPEED AST2500 PWM driver.
The AST2500 PWM controller can support 8 PWM output ports.
There can be three different PWM sources (Type M, N and O); each PWM
source can have different settings.
Each PWM output port can have one of the three different PWM sources.
There is a sysfs file through which the user can configure the duty cycle
for the particular PWM output port.
Added devicetree binding documentation for ast2500 pwm support.
Added support in Zaius platform for 4 PWM output ports since it has
four fans.

Tested on Zaius board and observed that when duty cycle is lowered
then the fan speed is lowered and when the duty cycle is increased
then the fan speed increases.

Tested on AST2500 EVB board and observed the PWM output pulses come
correctly based on the given settings using Logic Saleae Analyzer.

Jaghathiswari Rankappagounder Natarajan (3):
  devicetree: binding documentation update for ASPEED AST2500 PWM driver
  drivers: hwmon: ASPEED AST2500 PWM driver
  devicetree : Add support in Zaius platform for 4 PWM output ports

 .../bindings/hwmon/aspeed,ast2500-pwm.txt          | 116 ++++
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts         |  43 ++
 drivers/hwmon/Kconfig                              |   5 +
 drivers/hwmon/Makefile                             |   2 +-
 drivers/hwmon/aspeed_ast2500_pwm.c                 | 662 +++++++++++++++++++++
 drivers/hwmon/aspeed_ast2500_pwm.h                 | 128 ++++
 6 files changed, 955 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
 create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.c
 create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.h

--
2.8.0.rc3.226.g39d4020

[PATCH linux v2 1/3] devicetree: binding documentation update for ASPEED AST2500 PWM driver

[Jaghathiswari Rankappagounder Natarajan]

Add binding documentation update for ASPEED AST2500 PWM driver.
The ASPEED AST2500 PWM driver supports 8 PWM output ports.
The parent node indicates a PWM controller and has options to:
provide register set information, provide pinctrl information,
enable PWM and Fan Tach clock, select clock source, provide settings for
three different PWM sources (type M, N and O).

Each sub node indicates a PWM output port and has options to:
enable PWM port, select the PWM source for the PWM port, provide
duty cycle values to be used at startup.

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.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 .../bindings/hwmon/aspeed,ast2500-pwm.txt          | 116 +++++++++++++++++++++
 1 file changed, 116 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
new file mode 100644
index 0000000..2c3f2b6
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
@@ -0,0 +1,116 @@
+ASPEED AST2500 PWM device driver
+
+The ASPEED AST2500 PWM controller can support 8 PWM outputs.
+
+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,ast2500-pwm".
+- clock_enable : option to enable PWM and fan tach clock. should be 1.
+  (D[0] in PTCR00).
+- clock_source : option for clock source selection. 0 indicates 24MHz clock
+  and 1 indicates MCLK (D[1] in PTCR00).
+- typem_pwm_clock : PWM types M, N and 0 are three types just to have three
+  independent PWM source. Each could be assigned to the 8 PWM channels
+  with it's own settings
+  This array contains 4 values.
+  The first value indicates if the values for type M PWM are valid.
+  If the first value is 1, then it indicates that the rest of the values
+  are valid. If it is 0, then invalid.
+  The second value indicates the type M PWM clock division L bit (D[3:0]
+  in PTCR04). (value 0 in D[3:0] indicates 1, value 1 in D[3:0] indicates 2,
+  value 2 in D[3:0] indicates 4, value 3 in D[3:0] indicates 6 and so on
+  with increments in multiples of two).
+  The third value indicates type M PWM clock divison H bit (D[7:4] in PTCR04).
+  (value 0 in D[7:4] indicates 1, value 1 in D[7:4] indicates 2,
+  value 2 in D[7:4] indicates 4, value 3 in D[7:4] indicates 8 and
+  so on with increments in powers of two).
+  The fourth value indicates type M PWM period bit (D[15:8] in PTCR04).
+  If type M is chosen and clock source is 24 MHz then:
+  The PWM base clock = 24Mhz / (type M clock division L bit *
+  type M clock division H bit).
+  The PWM frequency = The PWM base clock / (type M PWM period bit + 1)
+  If we plan to output 25Khz PWM frequency then we can use:
+  typem_pwm_clock = <1 5 0 95>
+  The PWM frequency = 24Mhz / (10 * 1  * (95 + 1)) = 25Khz
+- typen_pwm_clock : This array contains 4 values.
+  The first value indicates if the values for type N PWM are valid.
+  If the first value is 1, then it indicates that the rest of the values
+  are valid. If it is 0, then invalid.
+  The second value indicates the type N PWM clock division L bit (D[19:16]
+  in PTCR04).
+  The third value indicates type N PWM clock division H bit
+  (D[23:20] in PTCR04).
+  The fourth value indicates type N PWM period bit (D[31:24] in PTCR04).
+  Calculations similar as for type M.
+- typeo_pwm_clock : This array contains 4 values.
+  The first value indicates indicates if the values for type O PWM are valid.
+  If the first value is 1, then it indicates that the rest of the values
+  are valid. If it is 0, then invalid.
+  The second value indicates if the type O PWM clock division L bit
+  (D[3:0] in PTCR44).
+  The third value indicates type O PWM clock divison H bit (D[7:4] in PTCR44).
+  The fourth value indicates type O PWM period bit (D[15:8] in PTCR44).
+  Calculations similar as for type M.
+
+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.
+- pwm_enable : Option to enable the PWM port indicated above. 0 is enable
+  and 1 is disable.
+  D[8] in PTCR00 indicates enabling PWM port A,
+  D[9] in PTCR00 indicates enabling PWM port B,
+  D[10] in PTCR00 indicates enabling PWM port C,
+  D[11] in PTCR00 indicates enabling PWM port D,
+  D[8] in PTCR40 indicates enabling PWM port E,
+  D[9] in PTCR40 indicates enabling PWM port F,
+  D[10] in PTCR40 indicates enabling PWM port G,
+  D[11] in PTCR40 indicates enabling PWM port H.
+- 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.
+  D[12] in PTCR00 indicates type selection bit of PWM A port,
+  D[13] in PTCR00 indicates type selection bit of PWM B port,
+  D[14] in PTCR00 indicates type selection bit of PWM C port,
+  D[15] in PTCR00 indicates type selection bit of PWM D port,
+  D[12] in PTCR40 indicates type selection bit of PWM E port,
+  D[13] in PTCR40 indicates type selection bit of PWM F port,
+  D[14] in PTCR40 indicates type selection bit of PWM G port,
+  D[15] in PTCR40 indicates type selection bit of PWM H port.
+- pwm_duty_cycle_percent : duty cycle value.
+
+Examples:
+
+pwm: pwm-controller@1e786000 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	reg = <0x1E786000 0x78>;
+	pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default
+	 		&pinctrl_pwm2_default &pinctrl_pwm3_default>;
+	compatible = "aspeed,ast2500-pwm";
+	clock_enable = /bits/ 8 <0x01>;
+	clock_source = /bits/ 8 <0x00>;
+	typem_pwm_clock = <1 5 0 95>;
+	typen_pwm_clock = <0 0 0 0>;
+	typeo_pwm_clock = <0 0 0 0>;
+	pwm_port0 {
+		pwm_port = /bits/ 8 <0x00>;
+		pwm_enable = /bits/ 8 <0x01>;
+		pwm_type = /bits/ 8 <0x00>;
+		pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
+	};
+	pwm_port1 {
+		pwm_port = /bits/ 8 <0x01>;
+		pwm_enable = /bits/ 8 <0x01>;
+		pwm_type = /bits/ 8 <0x00>;
+		pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
+	};
+};
+
--
2.8.0.rc3.226.g39d4020

Re: [PATCH linux v2 1/3] devicetree: binding documentation update for ASPEED AST2500 PWM driver

[Joel Stanley]

On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder
Natarajan <jaghu@google.com> wrote:
> Add binding documentation update for ASPEED AST2500 PWM driver.
> The ASPEED AST2500 PWM driver supports 8 PWM output ports.
> The parent node indicates a PWM controller and has options to:
> provide register set information, provide pinctrl information,
> enable PWM and Fan Tach clock, select clock source, provide settings for
> three different PWM sources (type M, N and O).
>
> Each sub node indicates a PWM output port and has options to:
> enable PWM port, select the PWM source for the PWM port, provide
> duty cycle values to be used at startup.
>
> 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.
>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  .../bindings/hwmon/aspeed,ast2500-pwm.txt          | 116 +++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
> new file mode 100644
> index 0000000..2c3f2b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
> @@ -0,0 +1,116 @@
> +ASPEED AST2500 PWM device driver
> +
> +The ASPEED AST2500 PWM controller can support 8 PWM outputs.
> +
> +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,ast2500-pwm".
> +- clock_enable : option to enable PWM and fan tach clock. should be 1.
> +  (D[0] in PTCR00).
> +- clock_source : option for clock source selection. 0 indicates 24MHz clock
> +  and 1 indicates MCLK (D[1] in PTCR00).
> +- typem_pwm_clock : PWM types M, N and 0 are three types just to have three
> +  independent PWM source. Each could be assigned to the 8 PWM channels
> +  with it's own settings
> +  This array contains 4 values.
> +  The first value indicates if the values for type M PWM are valid.
> +  If the first value is 1, then it indicates that the rest of the values
> +  are valid. If it is 0, then invalid.
> +  The second value indicates the type M PWM clock division L bit (D[3:0]
> +  in PTCR04). (value 0 in D[3:0] indicates 1, value 1 in D[3:0] indicates 2,
> +  value 2 in D[3:0] indicates 4, value 3 in D[3:0] indicates 6 and so on
> +  with increments in multiples of two).
> +  The third value indicates type M PWM clock divison H bit (D[7:4] in PTCR04).
> +  (value 0 in D[7:4] indicates 1, value 1 in D[7:4] indicates 2,
> +  value 2 in D[7:4] indicates 4, value 3 in D[7:4] indicates 8 and
> +  so on with increments in powers of two).
> +  The fourth value indicates type M PWM period bit (D[15:8] in PTCR04).
> +  If type M is chosen and clock source is 24 MHz then:
> +  The PWM base clock = 24Mhz / (type M clock division L bit *
> +  type M clock division H bit).

We should request the APB struct clk and calculate the PWM base clock
from that. See drivers that do of_clk_get. This means the binding
would require a phandle to the clock node that we have in our device
tree.

We don't need to describe the clock calculation in the bindings document.

> +  The PWM frequency = The PWM base clock / (type M PWM period bit + 1)
> +  If we plan to output 25Khz PWM frequency then we can use:
> +  typem_pwm_clock = <1 5 0 95>
> +  The PWM frequency = 24Mhz / (10 * 1  * (95 + 1)) = 25Khz
> +- typen_pwm_clock : This array contains 4 values.
> +  The first value indicates if the values for type N PWM are valid.
> +  If the first value is 1, then it indicates that the rest of the values
> +  are valid. If it is 0, then invalid.
> +  The second value indicates the type N PWM clock division L bit (D[19:16]
> +  in PTCR04).
> +  The third value indicates type N PWM clock division H bit
> +  (D[23:20] in PTCR04).
> +  The fourth value indicates type N PWM period bit (D[31:24] in PTCR04).
> +  Calculations similar as for type M.
> +- typeo_pwm_clock : This array contains 4 values.
> +  The first value indicates indicates if the values for type O PWM are valid.
> +  If the first value is 1, then it indicates that the rest of the values
> +  are valid. If it is 0, then invalid.
> +  The second value indicates if the type O PWM clock division L bit
> +  (D[3:0] in PTCR44).
> +  The third value indicates type O PWM clock divison H bit (D[7:4] in PTCR44).
> +  The fourth value indicates type O PWM period bit (D[15:8] in PTCR44).
> +  Calculations similar as for type M.

This large block is hard to read. Try adding some whitespace :)

Spend some time comparing your bindings to others in the tree for the
style of both the documentation and the bindings themselves.

> +
> +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.
> +- pwm_enable : Option to enable the PWM port indicated above. 0 is enable
> +  and 1 is disable.
> +  D[8] in PTCR00 indicates enabling PWM port A,
> +  D[9] in PTCR00 indicates enabling PWM port B,
> +  D[10] in PTCR00 indicates enabling PWM port C,
> +  D[11] in PTCR00 indicates enabling PWM port D,
> +  D[8] in PTCR40 indicates enabling PWM port E,
> +  D[9] in PTCR40 indicates enabling PWM port F,
> +  D[10] in PTCR40 indicates enabling PWM port G,
> +  D[11] in PTCR40 indicates enabling PWM port H.

This appears to be describing the register layout, not the bindings.

> +- 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.
> +  D[12] in PTCR00 indicates type selection bit of PWM A port,
> +  D[13] in PTCR00 indicates type selection bit of PWM B port,
> +  D[14] in PTCR00 indicates type selection bit of PWM C port,
> +  D[15] in PTCR00 indicates type selection bit of PWM D port,
> +  D[12] in PTCR40 indicates type selection bit of PWM E port,
> +  D[13] in PTCR40 indicates type selection bit of PWM F port,
> +  D[14] in PTCR40 indicates type selection bit of PWM G port,
> +  D[15] in PTCR40 indicates type selection bit of PWM H port.
> +- pwm_duty_cycle_percent : duty cycle value.
> +
> +Examples:
> +
> +pwm: pwm-controller@1e786000 {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +       reg = <0x1E786000 0x78>;
> +       pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default
> +                       &pinctrl_pwm2_default &pinctrl_pwm3_default>;
> +       compatible = "aspeed,ast2500-pwm";
> +       clock_enable = /bits/ 8 <0x01>;
> +       clock_source = /bits/ 8 <0x00>;
> +       typem_pwm_clock = <1 5 0 95>;
> +       typen_pwm_clock = <0 0 0 0>;
> +       typeo_pwm_clock = <0 0 0 0>;
> +       pwm_port0 {
> +               pwm_port = /bits/ 8 <0x00>;
> +               pwm_enable = /bits/ 8 <0x01>;
> +               pwm_type = /bits/ 8 <0x00>;
> +               pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
> +       };
> +       pwm_port1 {
> +               pwm_port = /bits/ 8 <0x01>;
> +               pwm_enable = /bits/ 8 <0x01>;
> +               pwm_type = /bits/ 8 <0x00>;
> +               pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
> +       };
> +};
> +
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [PATCH linux v2 1/3] devicetree: binding documentation update for ASPEED AST2500 PWM driver

[Jaghathiswari Rankappagounder Natarajan]

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

On Wed, Nov 9, 2016 at 4:35 PM, Joel Stanley <joel@jms.id.au> wrote:

> On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder
> Natarajan <jaghu@google.com> wrote:
> > Add binding documentation update for ASPEED AST2500 PWM driver.
> > The ASPEED AST2500 PWM driver supports 8 PWM output ports.
> > The parent node indicates a PWM controller and has options to:
> > provide register set information, provide pinctrl information,
> > enable PWM and Fan Tach clock, select clock source, provide settings for
> > three different PWM sources (type M, N and O).
> >
> > Each sub node indicates a PWM output port and has options to:
> > enable PWM port, select the PWM source for the PWM port, provide
> > duty cycle values to be used at startup.
> >
> > 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.
> >
> > Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com
> >
> > ---
> >  .../bindings/hwmon/aspeed,ast2500-pwm.txt          | 116
> +++++++++++++++++++++
> >  1 file changed, 116 insertions(+)
> >  create mode 100644 Documentation/devicetree/
> bindings/hwmon/aspeed,ast2500-pwm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
> b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
> > new file mode 100644
> > index 0000000..2c3f2b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2500-pwm.txt
> > @@ -0,0 +1,116 @@
> > +ASPEED AST2500 PWM device driver
> > +
> > +The ASPEED AST2500 PWM controller can support 8 PWM outputs.
> > +
> > +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,ast2500-pwm".
> > +- clock_enable : option to enable PWM and fan tach clock. should be 1.
> > +  (D[0] in PTCR00).
> > +- clock_source : option for clock source selection. 0 indicates 24MHz
> clock
> > +  and 1 indicates MCLK (D[1] in PTCR00).
> > +- typem_pwm_clock : PWM types M, N and 0 are three types just to have
> three
> > +  independent PWM source. Each could be assigned to the 8 PWM channels
> > +  with it's own settings
> > +  This array contains 4 values.
> > +  The first value indicates if the values for type M PWM are valid.
> > +  If the first value is 1, then it indicates that the rest of the values
> > +  are valid. If it is 0, then invalid.
> > +  The second value indicates the type M PWM clock division L bit (D[3:0]
> > +  in PTCR04). (value 0 in D[3:0] indicates 1, value 1 in D[3:0]
> indicates 2,
> > +  value 2 in D[3:0] indicates 4, value 3 in D[3:0] indicates 6 and so on
> > +  with increments in multiples of two).
> > +  The third value indicates type M PWM clock divison H bit (D[7:4] in
> PTCR04).
> > +  (value 0 in D[7:4] indicates 1, value 1 in D[7:4] indicates 2,
> > +  value 2 in D[7:4] indicates 4, value 3 in D[7:4] indicates 8 and
> > +  so on with increments in powers of two).
> > +  The fourth value indicates type M PWM period bit (D[15:8] in PTCR04).
> > +  If type M is chosen and clock source is 24 MHz then:
> > +  The PWM base clock = 24Mhz / (type M clock division L bit *
> > +  type M clock division H bit).
>
> We should request the APB struct clk and calculate the PWM base clock
> from that. See drivers that do of_clk_get. This means the binding
> would require a phandle to the clock node that we have in our device
> tree.
>
> We don't need to describe the clock calculation in the bindings document.
>
> Should I indicate the clock-frequency (desired PWM base clock) in the
devicetree clock node , get that clock-frequency using clk_get_rate and
calculate type M clock division L bit and type M clock division H bit to
match that clock-frequency (having fixed clock source and type M clock
period bit values from the devicetree) ?

> +  The PWM frequency = The PWM base clock / (type M PWM period bit + 1)
> > +  If we plan to output 25Khz PWM frequency then we can use:
> > +  typem_pwm_clock = <1 5 0 95>
> > +  The PWM frequency = 24Mhz / (10 * 1  * (95 + 1)) = 25Khz
> > +- typen_pwm_clock : This array contains 4 values.
> > +  The first value indicates if the values for type N PWM are valid.
> > +  If the first value is 1, then it indicates that the rest of the values
> > +  are valid. If it is 0, then invalid.
> > +  The second value indicates the type N PWM clock division L bit
> (D[19:16]
> > +  in PTCR04).
> > +  The third value indicates type N PWM clock division H bit
> > +  (D[23:20] in PTCR04).
> > +  The fourth value indicates type N PWM period bit (D[31:24] in PTCR04).
> > +  Calculations similar as for type M.
> > +- typeo_pwm_clock : This array contains 4 values.
> > +  The first value indicates indicates if the values for type O PWM are
> valid.
> > +  If the first value is 1, then it indicates that the rest of the values
> > +  are valid. If it is 0, then invalid.
> > +  The second value indicates if the type O PWM clock division L bit
> > +  (D[3:0] in PTCR44).
> > +  The third value indicates type O PWM clock divison H bit (D[7:4] in
> PTCR44).
> > +  The fourth value indicates type O PWM period bit (D[15:8] in PTCR44).
> > +  Calculations similar as for type M.
>
> This large block is hard to read. Try adding some whitespace :)
>
> Spend some time comparing your bindings to others in the tree for the
> style of both the documentation and the bindings themselves.
>
> > +
> > +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.
> > +- pwm_enable : Option to enable the PWM port indicated above. 0 is
> enable
> > +  and 1 is disable.
> > +  D[8] in PTCR00 indicates enabling PWM port A,
> > +  D[9] in PTCR00 indicates enabling PWM port B,
> > +  D[10] in PTCR00 indicates enabling PWM port C,
> > +  D[11] in PTCR00 indicates enabling PWM port D,
> > +  D[8] in PTCR40 indicates enabling PWM port E,
> > +  D[9] in PTCR40 indicates enabling PWM port F,
> > +  D[10] in PTCR40 indicates enabling PWM port G,
> > +  D[11] in PTCR40 indicates enabling PWM port H.
>
> This appears to be describing the register layout, not the bindings.
>
> > +- 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.
> > +  D[12] in PTCR00 indicates type selection bit of PWM A port,
> > +  D[13] in PTCR00 indicates type selection bit of PWM B port,
> > +  D[14] in PTCR00 indicates type selection bit of PWM C port,
> > +  D[15] in PTCR00 indicates type selection bit of PWM D port,
> > +  D[12] in PTCR40 indicates type selection bit of PWM E port,
> > +  D[13] in PTCR40 indicates type selection bit of PWM F port,
> > +  D[14] in PTCR40 indicates type selection bit of PWM G port,
> > +  D[15] in PTCR40 indicates type selection bit of PWM H port.
> > +- pwm_duty_cycle_percent : duty cycle value.
> > +
> > +Examples:
> > +
> > +pwm: pwm-controller@1e786000 {
> > +       #address-cells = <1>;
> > +       #size-cells = <1>;
> > +       reg = <0x1E786000 0x78>;
> > +       pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default
> > +                       &pinctrl_pwm2_default &pinctrl_pwm3_default>;
> > +       compatible = "aspeed,ast2500-pwm";
> > +       clock_enable = /bits/ 8 <0x01>;
> > +       clock_source = /bits/ 8 <0x00>;
> > +       typem_pwm_clock = <1 5 0 95>;
> > +       typen_pwm_clock = <0 0 0 0>;
> > +       typeo_pwm_clock = <0 0 0 0>;
> > +       pwm_port0 {
> > +               pwm_port = /bits/ 8 <0x00>;
> > +               pwm_enable = /bits/ 8 <0x01>;
> > +               pwm_type = /bits/ 8 <0x00>;
> > +               pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
> > +       };
> > +       pwm_port1 {
> > +               pwm_port = /bits/ 8 <0x01>;
> > +               pwm_enable = /bits/ 8 <0x01>;
> > +               pwm_type = /bits/ 8 <0x00>;
> > +               pwm_duty_cycle_percent = /bits/ 8 <0x5A>;
> > +       };
> > +};
> > +
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>

[-- Attachment #2: Type: text/html, Size: 10675 bytes --]

[PATCH linux v2 2/3] drivers: hwmon: ASPEED AST2500 PWM driver

[Jaghathiswari Rankappagounder Natarajan]

The ASPEED AST2500 PWM controller can support upto 8 PWM output ports.
There are three different PWM sources(types M, N and O)
and each PWM output port can be assigned a particular PWM source.
There is a sysfs file through which the user can control
the duty cycle of a particular PWM port. The duty cycle can range from 0 to
100 percent.

v2:
- Merged the drivers for PWM controller and PWM device as one PWM driver.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 drivers/hwmon/Kconfig              |   5 +
 drivers/hwmon/Makefile             |   2 +-
 drivers/hwmon/aspeed_ast2500_pwm.c | 662 +++++++++++++++++++++++++++++++++++++
 drivers/hwmon/aspeed_ast2500_pwm.h | 128 +++++++
 4 files changed, 796 insertions(+), 1 deletion(-)
 create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.c
 create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 3b34ba9..7edd94e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1800,6 +1800,11 @@ config SENSORS_ULTRA45
 	  This driver provides support for the Ultra45 workstation environmental
 	  sensors.

+config ASPEED_AST2500_PWM
+	tristate "ASPEED AST2500 PWM driver"
+	help
+	  This driver provides support for ASPEED AST2500 PWM output ports.
+
 if ACPI

 comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c0f3201..23529c2 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -163,7 +163,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)	+= w83l785ts.o
 obj-$(CONFIG_SENSORS_W83L786NG)	+= w83l786ng.o
 obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-hwmon.o
 obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
-
+obj-$(CONFIG_ASPEED_AST2500_PWM)	+= aspeed_ast2500_pwm.o
 obj-$(CONFIG_PMBUS)		+= pmbus/

 ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
diff --git a/drivers/hwmon/aspeed_ast2500_pwm.c b/drivers/hwmon/aspeed_ast2500_pwm.c
new file mode 100644
index 0000000..fda49d7
--- /dev/null
+++ b/drivers/hwmon/aspeed_ast2500_pwm.c
@@ -0,0 +1,662 @@
+/*
+ * Aspeed AST2500 PWM device driver
+ * * Copyright (c) 2016 Google, Inc
+ *
+ * * This program is free software; you can redistribute it and/or modify
+ * * it under the terms of the GNU General Public License version 2 as
+ * * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/platform_device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/of_device.h>
+#include <linux/io.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/sysfs.h>
+
+#include "aspeed_pwm.h"
+
+struct ast_pwm_port_data {
+	u8 pwm_port;
+	u8 pwm_enable;
+	u8 pwm_type;
+	u8 pwm_duty_cycle;
+	void __iomem *base;
+};
+
+struct ast_pwm_controller_data {
+	void __iomem *base;
+};
+
+static inline void
+ast_pwm_write(void __iomem *base, u32 val, u32 reg)
+{
+	writel(val, base + reg);
+}
+
+static inline u32
+ast_pwm_read(void __iomem *base, u32 reg)
+{
+	u32 val = readl(base + reg);
+	return val;
+}
+
+static void
+ast_set_pwm_clock_enable(struct ast_pwm_controller_data *priv, u8 val)
+{
+	if (val) {
+		ast_pwm_write(priv->base,
+			ast_pwm_read(priv->base, AST_PTCR_CTRL) |
+			AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
+	} else {
+		ast_pwm_write(priv->base,
+			ast_pwm_read(priv->base, AST_PTCR_CTRL) &
+			~AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
+	}
+}
+
+static void
+ast_set_pwm_clock_source(struct ast_pwm_controller_data *priv, u8 val)
+{
+	if (val) {
+		ast_pwm_write(priv->base,
+			ast_pwm_read(priv->base, AST_PTCR_CTRL) |
+			AST_PTCR_CTRL_CLK_SRC, AST_PTCR_CTRL);
+	} else {
+		ast_pwm_write(priv->base,
+			ast_pwm_read(priv->base, AST_PTCR_CTRL) &
+			~AST_PTCR_CTRL_CLK_SRC, AST_PTCR_CTRL);
+	}
+}
+
+static void
+ast_set_pwm_clock_division_h(struct ast_pwm_controller_data *priv,
+		u8 pwm_type, u8 div_high)
+{
+	switch (pwm_type) {
+	case PWM_TYPE_M:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEM_H_MASK) |
+			(div_high << AST_PTCR_CLK_CTRL_TYPEM_H),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_N:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEN_H_MASK) |
+			(div_high << AST_PTCR_CLK_CTRL_TYPEN_H),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_O:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_EXT_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEO_H_MASK) |
+			(div_high << AST_PTCR_CLK_CTRL_TYPEO_H),
+			AST_PTCR_CLK_EXT_CTRL);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_clock_division_l(struct ast_pwm_controller_data *priv,
+		u8 pwm_type, u8 div_low)
+{
+	switch (pwm_type) {
+	case PWM_TYPE_M:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEM_L_MASK) |
+			(div_low << AST_PTCR_CLK_CTRL_TYPEM_L),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_N:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEN_L_MASK) |
+			(div_low << AST_PTCR_CLK_CTRL_TYPEN_L),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_O:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_EXT_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEO_L_MASK) |
+			(div_low << AST_PTCR_CLK_CTRL_TYPEO_L),
+			AST_PTCR_CLK_EXT_CTRL);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_clock_unit(struct ast_pwm_controller_data *priv, u8 pwm_type,
+		u8 unit)
+{
+	switch (pwm_type) {
+	case PWM_TYPE_M:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEM_UNIT_MASK) |
+			(unit << AST_PTCR_CLK_CTRL_TYPEM_UNIT),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_N:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEN_UNIT_MASK) |
+			(unit << AST_PTCR_CLK_CTRL_TYPEN_UNIT),
+			AST_PTCR_CLK_CTRL);
+		break;
+	case PWM_TYPE_O:
+		ast_pwm_write(priv->base,
+			(ast_pwm_read(priv->base, AST_PTCR_CLK_EXT_CTRL) &
+			~AST_PTCR_CLK_CTRL_TYPEO_UNIT_MASK) |
+			(unit << AST_PTCR_CLK_CTRL_TYPEO_UNIT),
+			AST_PTCR_CLK_EXT_CTRL);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_enable(struct ast_pwm_port_data *priv, u8 enable)
+{
+	u8 pwm_ch = priv->pwm_port;
+
+	switch (pwm_ch) {
+	case PWMA:
+		if (enable)
+			ast_pwm_write(priv->base,
+				ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) |
+				AST_PTCR_CTRL_PWMA_EN,
+				AST_PTCR_CTRL);
+		else
+			ast_pwm_write(priv->base,
+				ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) &
+				~AST_PTCR_CTRL_PWMA_EN,
+				AST_PTCR_CTRL);
+		break;
+	case PWMB:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) |
+				AST_PTCR_CTRL_PWMB_EN),
+				AST_PTCR_CTRL);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) &
+				~AST_PTCR_CTRL_PWMB_EN),
+				AST_PTCR_CTRL);
+		break;
+	case PWMC:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) |
+				AST_PTCR_CTRL_PWMC_EN),
+				AST_PTCR_CTRL);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) &
+				~AST_PTCR_CTRL_PWMC_EN),
+				AST_PTCR_CTRL);
+		break;
+	case PWMD:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) |
+				AST_PTCR_CTRL_PWMD_EN),
+				AST_PTCR_CTRL);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL) &
+				~AST_PTCR_CTRL_PWMD_EN),
+				AST_PTCR_CTRL);
+		break;
+	case PWME:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) |
+				AST_PTCR_CTRL_PWME_EN),
+				AST_PTCR_CTRL_EXT);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) &
+				~AST_PTCR_CTRL_PWME_EN),
+				AST_PTCR_CTRL_EXT);
+		break;
+	case PWMF:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) |
+				AST_PTCR_CTRL_PWMF_EN),
+				AST_PTCR_CTRL_EXT);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) &
+				~AST_PTCR_CTRL_PWMF_EN),
+				AST_PTCR_CTRL_EXT);
+		break;
+	case PWMG:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) |
+				AST_PTCR_CTRL_PWMG_EN),
+				AST_PTCR_CTRL_EXT);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) &
+				~AST_PTCR_CTRL_PWMG_EN),
+				AST_PTCR_CTRL_EXT);
+		break;
+	case PWMH:
+		if (enable)
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) |
+				AST_PTCR_CTRL_PWMH_EN),
+				AST_PTCR_CTRL_EXT);
+		else
+			ast_pwm_write(priv->base,
+				(ast_pwm_read(priv->base,
+				AST_PTCR_CTRL_EXT) &
+				~AST_PTCR_CTRL_PWMH_EN),
+				AST_PTCR_CTRL_EXT);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_type(struct ast_pwm_port_data *priv, u8 type)
+{
+	u32 tmp1, tmp2;
+	u8 pwm_ch = priv->pwm_port;
+
+	tmp1 = ast_pwm_read(priv->base, AST_PTCR_CTRL);
+	tmp2 = ast_pwm_read(priv->base, AST_PTCR_CTRL_EXT);
+
+	switch (pwm_ch) {
+	case PWMA:
+		tmp1 &= ~AST_PTCR_CTRL_SET_PWMA_TYPE_MASK;
+		tmp1 |= AST_PTCR_CTRL_SET_PWMA_TYPE(type);
+		ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
+
+		break;
+	case PWMB:
+		tmp1 &= ~AST_PTCR_CTRL_SET_PWMB_TYPE_MASK;
+		tmp1 |= AST_PTCR_CTRL_SET_PWMB_TYPE(type);
+		ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
+		break;
+	case PWMC:
+		tmp1 &= ~AST_PTCR_CTRL_SET_PWMC_TYPE_MASK;
+		tmp1 |= AST_PTCR_CTRL_SET_PWMC_TYPE(type);
+		ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
+		break;
+	case PWMD:
+		tmp1 &= ~AST_PTCR_CTRL_SET_PWMD_TYPE_MASK;
+		tmp1 |= AST_PTCR_CTRL_SET_PWMD_TYPE(type);
+		ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
+		break;
+	case PWME:
+		tmp2 &= ~AST_PTCR_CTRL_SET_PWME_TYPE_MASK;
+		tmp2 |= AST_PTCR_CTRL_SET_PWME_TYPE(type);
+		ast_pwm_write(priv->base, tmp2, AST_PTCR_CTRL_EXT);
+		break;
+	case PWMF:
+		tmp2 &= ~AST_PTCR_CTRL_SET_PWMF_TYPE_MASK;
+		tmp2 |= AST_PTCR_CTRL_SET_PWMF_TYPE(type);
+		ast_pwm_write(priv->base, tmp2, AST_PTCR_CTRL_EXT);
+		break;
+	case PWMG:
+		tmp2 &= ~AST_PTCR_CTRL_SET_PWMG_TYPE_MASK;
+		tmp2 |= AST_PTCR_CTRL_SET_PWMG_TYPE(type);
+		ast_pwm_write(priv->base, tmp2, AST_PTCR_CTRL_EXT);
+		break;
+	case PWMH:
+		tmp2 &= ~AST_PTCR_CTRL_SET_PWMH_TYPE_MASK;
+		tmp2 |= AST_PTCR_CTRL_SET_PWMH_TYPE(type);
+		ast_pwm_write(priv->base, tmp2, AST_PTCR_CTRL_EXT);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_duty_rising(struct ast_pwm_port_data *priv, u8 rising)
+{
+	u32 tmp = 0;
+	u8 pwm_ch = priv->pwm_port;
+
+	switch (pwm_ch) {
+	case PWMA:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY0_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_RISE_POINT_MASK;
+		tmp |= rising;
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY0_CTRL);
+		break;
+	case PWMB:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY0_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_RISE_POINT_MASK;
+		tmp |= (rising << DUTY_CTRL_PWM2_RISE_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY0_CTRL);
+		break;
+	case PWMC:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY1_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_RISE_POINT_MASK;
+		tmp |= rising;
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY1_CTRL);
+		break;
+	case PWMD:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY1_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_RISE_POINT_MASK;
+		tmp |= (rising << DUTY_CTRL_PWM2_RISE_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY1_CTRL);
+		break;
+	case PWME:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY2_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_RISE_POINT_MASK;
+		tmp |= rising;
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY2_CTRL);
+		break;
+	case PWMF:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY2_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_RISE_POINT_MASK;
+		tmp |= (rising << DUTY_CTRL_PWM2_RISE_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY2_CTRL);
+		break;
+	case PWMG:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY3_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_RISE_POINT_MASK;
+		tmp |= rising;
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY3_CTRL);
+		break;
+	case PWMH:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY3_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_RISE_POINT_MASK;
+		tmp |= (rising << DUTY_CTRL_PWM2_RISE_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY3_CTRL);
+		break;
+	}
+}
+
+static void
+ast_set_pwm_duty_falling(struct ast_pwm_port_data *priv, u8 falling)
+{
+	u32 tmp = 0;
+	u8 pwm_ch = priv->pwm_port;
+
+	switch (pwm_ch) {
+	case PWMA:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY0_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM1_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY0_CTRL);
+		break;
+	case PWMB:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY0_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM2_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY0_CTRL);
+		break;
+	case PWMC:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY1_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM1_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY1_CTRL);
+		break;
+	case PWMD:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY1_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM2_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY1_CTRL);
+		break;
+	case PWME:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY2_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM1_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY2_CTRL);
+		break;
+	case PWMF:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY2_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM2_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY2_CTRL);
+		break;
+	case PWMG:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY3_CTRL);
+		tmp &= ~DUTY_CTRL_PWM1_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM1_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY3_CTRL);
+		break;
+	case PWMH:
+		tmp = ast_pwm_read(priv->base, AST_PTCR_DUTY3_CTRL);
+		tmp &= ~DUTY_CTRL_PWM2_FALL_POINT_MASK;
+		tmp |= (falling << DUTY_CTRL_PWM2_FALL_POINT);
+		ast_pwm_write(priv->base, tmp, AST_PTCR_DUTY3_CTRL);
+		break;
+	}
+}
+
+static u8
+ast_get_pwm_clock_unit(struct ast_pwm_port_data *priv, u8 pwm_type)
+{
+	u8 tmp = 0;
+
+	switch (pwm_type) {
+	case PWM_TYPE_M:
+		tmp = (ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			AST_PTCR_CLK_CTRL_TYPEM_UNIT_MASK) >>
+			AST_PTCR_CLK_CTRL_TYPEM_UNIT;
+		break;
+	case PWM_TYPE_N:
+		tmp = (ast_pwm_read(priv->base, AST_PTCR_CLK_CTRL) &
+			AST_PTCR_CLK_CTRL_TYPEN_UNIT_MASK) >>
+			AST_PTCR_CLK_CTRL_TYPEN_UNIT;
+		break;
+	case PWM_TYPE_O:
+		tmp = (ast_pwm_read(priv->base, AST_PTCR_CLK_EXT_CTRL) &
+			AST_PTCR_CLK_CTRL_TYPEO_UNIT_MASK) >>
+			AST_PTCR_CLK_CTRL_TYPEO_UNIT;
+		break;
+	}
+	return tmp;
+}
+
+static void
+ast_set_pwm_duty_cycle(struct ast_pwm_port_data *priv, u8 duty_cycle)
+{
+	u8 period;
+	u8 dc_time_on;
+
+	period = ast_get_pwm_clock_unit(priv, priv->pwm_type);
+	period += 1;
+
+	dc_time_on = (duty_cycle * period) / 100;
+	if (dc_time_on == 0) {
+		ast_set_pwm_enable(priv, 0);
+	} else {
+		if (dc_time_on == period) {
+			dc_time_on = 0;
+		}
+		ast_set_pwm_duty_rising(priv, 0);
+		ast_set_pwm_duty_falling(priv, dc_time_on);
+		ast_set_pwm_enable(priv, 1);
+	}
+}
+
+static ssize_t
+set_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
+		size_t count)
+{
+	struct ast_pwm_port_data *priv = dev_get_drvdata(dev);
+	u8 duty_cycle;
+	int ret;
+
+	ret = kstrtou8(buf, 10, &duty_cycle);
+	if (ret)
+		return -EINVAL;
+
+	if (duty_cycle < 0 || duty_cycle > 100)
+		return -EINVAL;
+
+	priv->pwm_duty_cycle = duty_cycle;
+	ast_set_pwm_duty_cycle(priv, duty_cycle);
+	return count;
+}
+
+static ssize_t
+show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct ast_pwm_port_data *priv = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", priv->pwm_duty_cycle);
+}
+
+static SENSOR_DEVICE_ATTR(pwm, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 0);
+
+static struct attribute *pwm_dev_attrs[] = {
+	&sensor_dev_attr_pwm.dev_attr.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(pwm_dev);
+
+static int
+aspeed_create_pwm_port(struct device *dev, struct device_node *child,
+		void __iomem *base)
+{
+	struct device *hwmon;
+	u8 val;
+	struct ast_pwm_port_data *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	hwmon = devm_hwmon_device_register_with_groups(dev, "pwm_port",
+		priv, pwm_dev_groups);
+	if (IS_ERR(hwmon)) {
+		dev_err(dev, "Failed to register hwmon device\n");
+		return PTR_ERR(hwmon);
+	}
+	priv->base = base;
+
+	of_property_read_u8(child, "pwm_port", &val);
+	priv->pwm_port = val;
+
+	of_property_read_u8(child, "pwm_enable", &val);
+	priv->pwm_enable = val;
+	ast_set_pwm_enable(priv, val);
+
+	of_property_read_u8(child, "pwm_type", &val);
+	priv->pwm_type = val;
+	ast_set_pwm_type(priv, val);
+
+	of_property_read_u8(child, "pwm_duty_cycle_percent", &val);
+	priv->pwm_duty_cycle = val;
+	ast_set_pwm_duty_cycle(priv, val);
+	return 0;
+}
+
+static int
+aspeed_ast2500_pwm_probe(struct platform_device *pdev)
+{
+	u32 buf[4];
+	struct device_node *np, *child;
+	struct resource *res;
+	u8 val;
+	int err;
+	struct ast_pwm_controller_data *priv;
+
+	np = pdev->dev.of_node;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct ast_pwm_controller_data),
+			GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL) {
+		return -ENOENT;
+	}
+
+	priv->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	if (!priv->base)
+		return -ENOMEM;
+
+	of_property_read_u8(np, "clock_enable", &val);
+	ast_set_pwm_clock_enable(priv, val);
+	of_property_read_u8(np, "clock_source", &val);
+	ast_set_pwm_clock_source(priv, val);
+
+	of_property_read_u32_array(np, "typem_pwm_clock", buf, 4);
+	if (buf[0] == 1) {
+		ast_set_pwm_clock_division_l(priv, PWM_TYPE_M, buf[1]);
+		ast_set_pwm_clock_division_h(priv, PWM_TYPE_M, buf[2]);
+		ast_set_pwm_clock_unit(priv, PWM_TYPE_M, buf[3]);
+	}
+
+	of_property_read_u32_array(np, "typen_pwm_clock", buf, 4);
+	if (buf[0] == 1) {
+		ast_set_pwm_clock_division_l(priv, PWM_TYPE_N, buf[1]);
+		ast_set_pwm_clock_division_h(priv, PWM_TYPE_N, buf[2]);
+		ast_set_pwm_clock_unit(priv, PWM_TYPE_N, buf[3]);
+	}
+
+	of_property_read_u32_array(np, "typeo_pwm_clock", buf, 4);
+	if (buf[0] == 1) {
+		ast_set_pwm_clock_division_l(priv, PWM_TYPE_O, buf[1]);
+		ast_set_pwm_clock_division_h(priv, PWM_TYPE_O, buf[2]);
+		ast_set_pwm_clock_unit(priv, PWM_TYPE_O, buf[3]);
+	}
+
+	for_each_child_of_node(np, child) {
+		aspeed_create_pwm_port(&pdev->dev,
+				child, priv->base);
+		of_node_put(child);
+	}
+	of_node_put(np);
+	return 0;
+}
+
+static int
+aspeed_ast2500_pwm_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static const struct of_device_id of_pwm_match_table[] = {
+	{ .compatible = "aspeed,ast2500-pwm", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_match_table);
+
+static struct platform_driver aspeed_ast2500_pwm_driver = {
+	.probe		= aspeed_ast2500_pwm_probe,
+	.remove		= aspeed_ast2500_pwm_remove,
+	.driver		= {
+		.name	= "aspeed_ast2500_pwm",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_pwm_match_table,
+	},
+};
+
+module_platform_driver(aspeed_ast2500_pwm_driver);
+
+MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
+MODULE_DESCRIPTION("ASPEED AST2500 PWM device driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/aspeed_ast2500_pwm.h b/drivers/hwmon/aspeed_ast2500_pwm.h
new file mode 100644
index 0000000..1986fd2
--- /dev/null
+++ b/drivers/hwmon/aspeed_ast2500_pwm.h
@@ -0,0 +1,128 @@
+/*
+ * ASPEED AST2500 PWM header file
+ * * Copyright (c) 2016 Google, Inc
+ *
+ * * This program is free software; you can redistribute it and/or modify
+ * * it under the terms of the GNU General Public License version 2 as
+ * * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __ASPEED_PWM_H
+#define __ASPEED_PWM_H __FILE__
+
+/* AST PWM & FAN Register Definition */
+#define AST_PTCR_CTRL			0x00
+#define AST_PTCR_CLK_CTRL		0x04
+#define AST_PTCR_DUTY0_CTRL		0x08
+#define AST_PTCR_DUTY1_CTRL		0x0c
+#define AST_PTCR_CTRL_EXT		0x40
+#define AST_PTCR_CLK_EXT_CTRL		0x44
+#define AST_PTCR_DUTY2_CTRL		0x48
+#define AST_PTCR_DUTY3_CTRL		0x4c
+
+/* COMMON Definition */
+#define PWM_TYPE_M	0x0
+#define PWM_TYPE_N	0x1
+#define PWM_TYPE_O	0x2
+
+#define PWMA	0x0
+#define PWMB	0x1
+#define PWMC	0x2
+#define PWMD	0x3
+#define PWME	0x4
+#define PWMF	0x5
+#define PWMG	0x6
+#define PWMH	0x7
+
+#define DUTY_CTRL_PWM2_FALL_POINT			(24)
+#define DUTY_CTRL_PWM2_FALL_POINT_MASK			(0xff<<24)
+#define DUTY_CTRL_PWM2_RISE_POINT			(16)
+#define DUTY_CTRL_PWM2_RISE_POINT_MASK			(0xff<<16)
+#define DUTY_CTRL_PWM1_FALL_POINT			(8)
+#define DUTY_CTRL_PWM1_FALL_POINT_MASK			(0xff<<8)
+#define DUTY_CTRL_PWM1_RISE_POINT			(0)
+#define DUTY_CTRL_PWM1_RISE_POINT_MASK			(0xff)
+
+/* AST_PTCR_CTRL : 0x00 - General Control Register */
+#define AST_PTCR_CTRL_SET_PWMD_TYPE(x)		((x & 0x1) << 15 | \
+						(x & 0x2) << 6)
+#define AST_PTCR_CTRL_SET_PWMD_TYPE_MASK	((0x1 << 7) | (0x1 << 15))
+
+#define AST_PTCR_CTRL_SET_PWMC_TYPE(x)		((x & 0x1) << 14 | \
+						(x & 0x2) << 5)
+#define AST_PTCR_CTRL_SET_PWMC_TYPE_MASK	((0x1 << 6) | (0x1 << 14))
+
+#define AST_PTCR_CTRL_SET_PWMB_TYPE(x)		((x & 0x1) << 13 | \
+						(x & 0x2) << 4)
+#define AST_PTCR_CTRL_SET_PWMB_TYPE_MASK	((0x1 << 5) | (0x1 << 13))
+
+#define AST_PTCR_CTRL_SET_PWMA_TYPE(x)		((x & 0x1) << 12 | \
+						(x & 0x2) << 3)
+#define AST_PTCR_CTRL_SET_PWMA_TYPE_MASK	((0x1 << 4) | (0x1 << 12))
+
+#define	AST_PTCR_CTRL_PWMD			(11)
+#define	AST_PTCR_CTRL_PWMD_EN		(0x1 << 11)
+#define	AST_PTCR_CTRL_PWMC			(10)
+#define	AST_PTCR_CTRL_PWMC_EN		(0x1 << 10)
+#define	AST_PTCR_CTRL_PWMB			(9)
+#define	AST_PTCR_CTRL_PWMB_EN		(0x1 << 9)
+#define	AST_PTCR_CTRL_PWMA			(8)
+#define	AST_PTCR_CTRL_PWMA_EN		(0x1 << 8)
+
+/*0:24Mhz, 1:MCLK */
+#define	AST_PTCR_CTRL_CLK_SRC		0x2
+#define	AST_PTCR_CTRL_CLK_EN		0x1
+
+/* AST_PTCR_CLK_CTRL : 0x04 - Clock Control Register */
+/* TYPE N */
+#define AST_PTCR_CLK_CTRL_TYPEN_UNIT			(24)
+#define AST_PTCR_CLK_CTRL_TYPEN_UNIT_MASK		(0xff << 24)
+#define AST_PTCR_CLK_CTRL_TYPEN_H			(20)
+#define AST_PTCR_CLK_CTRL_TYPEN_H_MASK			(0xf << 20)
+#define AST_PTCR_CLK_CTRL_TYPEN_L			(16)
+#define AST_PTCR_CLK_CTRL_TYPEN_L_MASK			(0xf << 16)
+/* TYPE M */
+#define AST_PTCR_CLK_CTRL_TYPEM_UNIT			(8)
+#define AST_PTCR_CLK_CTRL_TYPEM_UNIT_MASK		(0xff << 8)
+#define AST_PTCR_CLK_CTRL_TYPEM_H			(4)
+#define AST_PTCR_CLK_CTRL_TYPEM_H_MASK			(0xf << 4)
+#define AST_PTCR_CLK_CTRL_TYPEM_L			(0)
+#define AST_PTCR_CLK_CTRL_TYPEM_L_MASK			(0xf)
+
+/* AST_PTCR_CTRL_EXT : 0x40 - General Control Extension #1 Register */
+#define AST_PTCR_CTRL_SET_PWMH_TYPE(x)		((x & 0x1) << 15 | \
+						(x & 0x2) << 6)
+#define AST_PTCR_CTRL_SET_PWMH_TYPE_MASK	((0x1 << 7) | (0x1 << 15))
+
+#define AST_PTCR_CTRL_SET_PWMG_TYPE(x)		((x & 0x1) << 14 | \
+						(x & 0x2) << 5)
+#define AST_PTCR_CTRL_SET_PWMG_TYPE_MASK	((0x1 << 6) | (0x1 << 14))
+
+#define AST_PTCR_CTRL_SET_PWMF_TYPE(x)		((x & 0x1) << 13 | \
+						(x & 0x2) << 4)
+#define AST_PTCR_CTRL_SET_PWMF_TYPE_MASK	((0x1 << 5) | (0x1 << 13))
+
+#define AST_PTCR_CTRL_SET_PWME_TYPE(x)		((x & 0x1) << 12 | \
+						(x & 0x2) << 3)
+#define AST_PTCR_CTRL_SET_PWME_TYPE_MASK	((0x1 << 4) | (0x1 << 12))
+
+#define	AST_PTCR_CTRL_PWMH		(11)
+#define	AST_PTCR_CTRL_PWMH_EN		(0x1 << 11)
+#define	AST_PTCR_CTRL_PWMG		(10)
+#define	AST_PTCR_CTRL_PWMG_EN		(0x1 << 10)
+#define	AST_PTCR_CTRL_PWMF		(9)
+#define	AST_PTCR_CTRL_PWMF_EN		(0x1 << 9)
+#define	AST_PTCR_CTRL_PWME		(8)
+#define	AST_PTCR_CTRL_PWME_EN		(0x1 << 8)
+
+/* AST_PTCR_CLK_EXT_CTRL : 0x44 - Clock Control Extension #1 Register */
+/* TYPE O */
+#define AST_PTCR_CLK_CTRL_TYPEO_UNIT		(8)
+#define AST_PTCR_CLK_CTRL_TYPEO_UNIT_MASK	(0xff << 8)
+#define AST_PTCR_CLK_CTRL_TYPEO_H		(4)
+#define AST_PTCR_CLK_CTRL_TYPEO_H_MASK		(0xf << 4)
+#define AST_PTCR_CLK_CTRL_TYPEO_L		(0)
+#define AST_PTCR_CLK_CTRL_TYPEO_L_MASK		(0xf)
+
+#endif /* __ASPEED_PWM_H */
--
2.8.0.rc3.226.g39d4020

Re: [PATCH linux v2 2/3] drivers: hwmon: ASPEED AST2500 PWM driver

[Joel Stanley]

Hello Jaghu,

On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder
Natarajan <jaghu@google.com> wrote:
> The ASPEED AST2500 PWM controller can support upto 8 PWM output ports.

I noticed that the ast2500 and ast2400 share the same PWM IP, so we
can use the same driver for both. You should reflect this in the
naming of the Kconfig, filename, commit message and the code itself.

> There are three different PWM sources(types M, N and O)
> and each PWM output port can be assigned a particular PWM source.
> There is a sysfs file through which the user can control
> the duty cycle of a particular PWM port. The duty cycle can range from 0 to
> 100 percent.

Can you explain why this is a hwmon driver and not a pwm driver?

>
> v2:
> - Merged the drivers for PWM controller and PWM device as one PWM driver.

Thanks for adding the changelog. This should go below the --, two
lines lower. We do this to ensure the changelog is not part of the
committed patch.

>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  drivers/hwmon/Kconfig              |   5 +
>  drivers/hwmon/Makefile             |   2 +-
>  drivers/hwmon/aspeed_ast2500_pwm.c | 662 +++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/aspeed_ast2500_pwm.h | 128 +++++++
>  4 files changed, 796 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.c
>  create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.h
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 3b34ba9..7edd94e 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1800,6 +1800,11 @@ config SENSORS_ULTRA45
>           This driver provides support for the Ultra45 workstation environmental
>           sensors.
>
> +config ASPEED_AST2500_PWM
> +       tristate "ASPEED AST2500 PWM driver"
> +       help
> +         This driver provides support for ASPEED AST2500 PWM output ports.
> +
>  if ACPI
>
>  comment "ACPI drivers"
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c0f3201..23529c2 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -163,7 +163,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)     += w83l785ts.o
>  obj-$(CONFIG_SENSORS_W83L786NG)        += w83l786ng.o
>  obj-$(CONFIG_SENSORS_WM831X)   += wm831x-hwmon.o
>  obj-$(CONFIG_SENSORS_WM8350)   += wm8350-hwmon.o
> -
> +obj-$(CONFIG_ASPEED_AST2500_PWM)       += aspeed_ast2500_pwm.o
>  obj-$(CONFIG_PMBUS)            += pmbus/
>
>  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> diff --git a/drivers/hwmon/aspeed_ast2500_pwm.c b/drivers/hwmon/aspeed_ast2500_pwm.c
> new file mode 100644
> index 0000000..fda49d7
> --- /dev/null
> +++ b/drivers/hwmon/aspeed_ast2500_pwm.c
> @@ -0,0 +1,662 @@
> +/*
> + * Aspeed AST2500 PWM device driver

most of the time we don't add the name at the top of hte file.


> + * * Copyright (c) 2016 Google, Inc
> + *
> + * * This program is free software; you can redistribute it and/or modify
> + * * it under the terms of the GNU General Public License version 2 as
> + * * published by the Free Software Foundation.

Lose the second row of stars.

The new drivers we contribute are v2 or later. This is your choice,
but if there's no issue adding the or later it would be nice for all
of the Aspeed code to be consistent.
> + *
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/io.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/sysfs.h>
> +
> +#include "aspeed_pwm.h"
> +
> +struct ast_pwm_port_data {

For better or worse, we've settled on "aspeed" as the prefix for all
of the drivers. Please use that instead of ast.

> +       u8 pwm_port;
> +       u8 pwm_enable;
> +       u8 pwm_type;
> +       u8 pwm_duty_cycle;
> +       void __iomem *base;
> +};
> +
> +struct ast_pwm_controller_data {
> +       void __iomem *base;
> +};
> +
> +static inline void
> +ast_pwm_write(void __iomem *base, u32 val, u32 reg)
> +{
> +       writel(val, base + reg);
> +}
> +
> +static inline u32
> +ast_pwm_read(void __iomem *base, u32 reg)
> +{
> +       u32 val = readl(base + reg);
> +       return val;
> +}
> +
> +static void
> +ast_set_pwm_clock_enable(struct ast_pwm_controller_data *priv, u8 val)

val is a boolean.

> +{
> +       if (val) {
> +               ast_pwm_write(priv->base,
> +                       ast_pwm_read(priv->base, AST_PTCR_CTRL) |
> +                       AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
> +       } else {
> +               ast_pwm_write(priv->base,
> +                       ast_pwm_read(priv->base, AST_PTCR_CTRL) &
> +                       ~AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
> +       }
> +}

> +static void
> +ast_set_pwm_enable(struct ast_pwm_port_data *priv, u8 enable)
> +{
> +       u8 pwm_ch = priv->pwm_port;
> +

The switch statements where you read-modify-write are hard to read. I
suggest reading the register value into a variable, modify it in your
switch statement, and write it at the end. This makes it easier to
read what operations you're performing on the bits.

u32 reg = ast_pwm_read(priv->base, AST_PTCR_CTRL);

switch (pwm_ch) {
case PWMA:
     port = AST_PTCR_CTRL_PWMA_EN;
     break;
case PWMB:
     port = AST_PTCR_CTRL_PWMB_EN;
     break;
...
}

if (enable)
   reg |= port;
else
   reg &= ~port;

ast_pwm_write(priv->base, reg, AST_PTCR_CTRL);

I think this structure will make this more readable. You could store
the AST_PTCR_CTRL_PWMx_EN value as priv->pwm_port, and lose the switch
statement all together.

This suggestion wont work for the other switch statements obviously.
Can you think of a representation that would allow you to look up the
values without requiring a switch statement? Take a look at some of
the other pwm drivers for inspiration.

> +       switch (pwm_ch) {
> +       case PWMA:
> +               if (enable)
> +                       ast_pwm_write(priv->base,
> +                               ast_pwm_read(priv->base,
> +                               AST_PTCR_CTRL) |
> +                               AST_PTCR_CTRL_PWMA_EN,
> +                               AST_PTCR_CTRL);

> +static void
> +ast_set_pwm_type(struct ast_pwm_port_data *priv, u8 type)
> +{
> +       u32 tmp1, tmp2;
> +       u8 pwm_ch = priv->pwm_port;
> +
> +       tmp1 = ast_pwm_read(priv->base, AST_PTCR_CTRL);
> +       tmp2 = ast_pwm_read(priv->base, AST_PTCR_CTRL_EXT);

I think we can do better than tmp1 and tmp2. perhaps ctrl and ctrl_ext?

> +
> +       switch (pwm_ch) {

As with pwm_type, pwm_ch could be an enum. That way you get warnings
if you omit an option in your switch statement.

> +       case PWMA:
> +               tmp1 &= ~AST_PTCR_CTRL_SET_PWMA_TYPE_MASK;
> +               tmp1 |= AST_PTCR_CTRL_SET_PWMA_TYPE(type);
> +               ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
> +
> +               break;

> +static int
> +aspeed_create_pwm_port(struct device *dev, struct device_node *child,
> +               void __iomem *base)
> +{
> +       struct device *hwmon;
> +       u8 val;
> +       struct ast_pwm_port_data *priv;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       hwmon = devm_hwmon_device_register_with_groups(dev, "pwm_port",

"aspeed_pwm"?

> +               priv, pwm_dev_groups);
> +       if (IS_ERR(hwmon)) {
> +               dev_err(dev, "Failed to register hwmon device\n");
> +               return PTR_ERR(hwmon);
> +       }
> +       priv->base = base;
> +
> +       of_property_read_u8(child, "pwm_port", &val);
> +       priv->pwm_port = val;
> +
> +       of_property_read_u8(child, "pwm_enable", &val);
> +       priv->pwm_enable = val;
> +       ast_set_pwm_enable(priv, val);
> +
> +       of_property_read_u8(child, "pwm_type", &val);
> +       priv->pwm_type = val;
> +       ast_set_pwm_type(priv, val);
> +
> +       of_property_read_u8(child, "pwm_duty_cycle_percent", &val);
> +       priv->pwm_duty_cycle = val;
> +       ast_set_pwm_duty_cycle(priv, val);
> +       return 0;
> +}
> +
> +static int
> +aspeed_ast2500_pwm_probe(struct platform_device *pdev)
> +{
> +       u32 buf[4];
> +       struct device_node *np, *child;
> +       struct resource *res;
> +       u8 val;
> +       int err;
> +       struct ast_pwm_controller_data *priv;
> +
> +       np = pdev->dev.of_node;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(struct ast_pwm_controller_data),
> +                       GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;


Can you explain why you chose to allocate structure here? You don't
seem to use it outside of the probe function which suggests it's
unnecessary.

You can just get the base address, map it in (ioremap) to configure
the state and then discard the mapping in the probe.

> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (res == NULL) {
> +               return -ENOENT;
> +       }
> +
> +       priv->base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +       if (!priv->base)
> +               return -ENOMEM;
> +
> +       of_property_read_u8(np, "clock_enable", &val);
> +       ast_set_pwm_clock_enable(priv, val);
> +       of_property_read_u8(np, "clock_source", &val);
> +       ast_set_pwm_clock_source(priv, val);

This needs reworking. I will comment on the bindings which will lead
to changes in this code.

> +
> +       of_property_read_u32_array(np, "typem_pwm_clock", buf, 4);
> +       if (buf[0] == 1) {
> +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_M, buf[1]);
> +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_M, buf[2]);
> +               ast_set_pwm_clock_unit(priv, PWM_TYPE_M, buf[3]);
> +       }
> +
> +       of_property_read_u32_array(np, "typen_pwm_clock", buf, 4);
> +       if (buf[0] == 1) {
> +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_N, buf[1]);
> +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_N, buf[2]);
> +               ast_set_pwm_clock_unit(priv, PWM_TYPE_N, buf[3]);
> +       }
> +
> +       of_property_read_u32_array(np, "typeo_pwm_clock", buf, 4);
> +       if (buf[0] == 1) {
> +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_O, buf[1]);
> +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_O, buf[2]);
> +               ast_set_pwm_clock_unit(priv, PWM_TYPE_O, buf[3]);
> +       }
> +
> +       for_each_child_of_node(np, child) {
> +               aspeed_create_pwm_port(&pdev->dev,
> +                               child, priv->base);
> +               of_node_put(child);
> +       }
> +       of_node_put(np);
> +       return 0;
> +}
> +
> +static int
> +aspeed_ast2500_pwm_remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {
> +       { .compatible = "aspeed,ast2500-pwm", },

As discussed above, this should contain the ast2400 string as well.

> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_ast2500_pwm_driver = {
> +       .probe          = aspeed_ast2500_pwm_probe,
> +       .remove         = aspeed_ast2500_pwm_remove,

Your remove does nothing, you can omit it.

> +       .driver         = {
> +               .name   = "aspeed_ast2500_pwm",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = of_pwm_match_table,
> +       },
> +};
> +
> +module_platform_driver(aspeed_ast2500_pwm_driver);
> +
> +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>");
> +MODULE_DESCRIPTION("ASPEED AST2500 PWM device driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/aspeed_ast2500_pwm.h b/drivers/hwmon/aspeed_ast2500_pwm.h
> new file mode 100644
> index 0000000..1986fd2
> --- /dev/null
> +++ b/drivers/hwmon/aspeed_ast2500_pwm.h

Headers in the kernel are generally only used when we need to share
details between parts of the kernel. Given there isn't a lot here, I
think these definitions should be in the c file.


> +#define DUTY_CTRL_PWM2_FALL_POINT                      (24)
> +#define DUTY_CTRL_PWM2_FALL_POINT_MASK                 (0xff<<24)
> +#define DUTY_CTRL_PWM2_RISE_POINT                      (16)
> +#define DUTY_CTRL_PWM2_RISE_POINT_MASK                 (0xff<<16)
> +#define DUTY_CTRL_PWM1_FALL_POINT                      (8)
> +#define DUTY_CTRL_PWM1_FALL_POINT_MASK                 (0xff<<8)

Try using GENMASK. For this it would be GENMASK(15, 8).

Do you really need the mask definitions?

> +#define DUTY_CTRL_PWM1_RISE_POINT                      (0)
> +#define DUTY_CTRL_PWM1_RISE_POINT_MASK                 (0xff)
> +
> +/* AST_PTCR_CTRL : 0x00 - General Control Register */
> +#define AST_PTCR_CTRL_SET_PWMD_TYPE(x)         ((x & 0x1) << 15 | \
> +                                               (x & 0x2) << 6)
> +#define AST_PTCR_CTRL_SET_PWMD_TYPE_MASK       ((0x1 << 7) | (0x1 << 15))

You can use BIT(7) | BIT(15).

> +
> +#define AST_PTCR_CTRL_SET_PWMC_TYPE(x)         ((x & 0x1) << 14 | \
> +                                               (x & 0x2) << 5)

> +#define        AST_PTCR_CTRL_PWMD                      (11)
> +#define        AST_PTCR_CTRL_PWMD_EN           (0x1 << 11)
> +#define        AST_PTCR_CTRL_PWMC                      (10)
> +#define        AST_PTCR_CTRL_PWMC_EN           (0x1 << 10)
> +#define        AST_PTCR_CTRL_PWMB                      (9)
> +#define        AST_PTCR_CTRL_PWMB_EN           (0x1 << 9)
> +#define        AST_PTCR_CTRL_PWMA                      (8)
> +#define        AST_PTCR_CTRL_PWMA_EN           (0x1 << 8)

Consider only having the bit defintiions or the _EN. One can be
derived from the other.

You don't need to guard the integers with ( ).

> +
> +/*0:24Mhz, 1:MCLK */
> +#define        AST_PTCR_CTRL_CLK_SRC           0x2
> +#define        AST_PTCR_CTRL_CLK_EN            0x1

Re: [PATCH linux v2 2/3] drivers: hwmon: ASPEED AST2500 PWM driver

[Jaghathiswari Rankappagounder Natarajan]

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

On Tue, Nov 8, 2016 at 11:15 PM, Joel Stanley <joel@jms.id.au> wrote:

> Hello Jaghu,
>
> On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder
> Natarajan <jaghu@google.com> wrote:
> > The ASPEED AST2500 PWM controller can support upto 8 PWM output ports.
>
> I noticed that the ast2500 and ast2400 share the same PWM IP, so we
> can use the same driver for both. You should reflect this in the
> naming of the Kconfig, filename, commit message and the code itself.
>
> > There are three different PWM sources(types M, N and O)
> > and each PWM output port can be assigned a particular PWM source.
> > There is a sysfs file through which the user can control
> > the duty cycle of a particular PWM port. The duty cycle can range from 0
> to
> > 100 percent.
>
> Can you explain why this is a hwmon driver and not a pwm driver?

The fan tacho controller shares some common registers with PWM
controller; the fan tacho controller needs to be a hwmon driver; so if PWM
controller driver is a hwmon driver, then there is flexibility to add fan
tacho controller functionality to the same PWM driver and make it as common
driver for PWM and fan tacho controller.
The duty_cycle value available in pwm  sysfs interface needs to be
configured in nano seconds and the duty_cycle value available in hwmon
sysfs interface can be configured with a integer value (ranging from 0 to
255; with 255 being 100 percent). So thought that hwmon sysfs interface is
easier.


>
> >
> > v2:
> > - Merged the drivers for PWM controller and PWM device as one PWM driver.
>
> Thanks for adding the changelog. This should go below the --, two
> lines lower. We do this to ensure the changelog is not part of the
> committed patch.
>
> >
> > Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com
> >
> > ---
> >  drivers/hwmon/Kconfig              |   5 +
> >  drivers/hwmon/Makefile             |   2 +-
> >  drivers/hwmon/aspeed_ast2500_pwm.c | 662 ++++++++++++++++++++++++++++++
> +++++++
> >  drivers/hwmon/aspeed_ast2500_pwm.h | 128 +++++++
> >  4 files changed, 796 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.c
> >  create mode 100644 drivers/hwmon/aspeed_ast2500_pwm.h
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 3b34ba9..7edd94e 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1800,6 +1800,11 @@ config SENSORS_ULTRA45
> >           This driver provides support for the Ultra45 workstation
> environmental
> >           sensors.
> >
> > +config ASPEED_AST2500_PWM
> > +       tristate "ASPEED AST2500 PWM driver"
> > +       help
> > +         This driver provides support for ASPEED AST2500 PWM output
> ports.
> > +
> >  if ACPI
> >
> >  comment "ACPI drivers"
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index c0f3201..23529c2 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -163,7 +163,7 @@ obj-$(CONFIG_SENSORS_W83L785TS)     += w83l785ts.o
> >  obj-$(CONFIG_SENSORS_W83L786NG)        += w83l786ng.o
> >  obj-$(CONFIG_SENSORS_WM831X)   += wm831x-hwmon.o
> >  obj-$(CONFIG_SENSORS_WM8350)   += wm8350-hwmon.o
> > -
> > +obj-$(CONFIG_ASPEED_AST2500_PWM)       += aspeed_ast2500_pwm.o
> >  obj-$(CONFIG_PMBUS)            += pmbus/
> >
> >  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> > diff --git a/drivers/hwmon/aspeed_ast2500_pwm.c b/drivers/hwmon/aspeed_
> ast2500_pwm.c
> > new file mode 100644
> > index 0000000..fda49d7
> > --- /dev/null
> > +++ b/drivers/hwmon/aspeed_ast2500_pwm.c
> > @@ -0,0 +1,662 @@
> > +/*
> > + * Aspeed AST2500 PWM device driver
>
> most of the time we don't add the name at the top of hte file.
>
>
> > + * * Copyright (c) 2016 Google, Inc
> > + *
> > + * * This program is free software; you can redistribute it and/or
> modify
> > + * * it under the terms of the GNU General Public License version 2 as
> > + * * published by the Free Software Foundation.
>
> Lose the second row of stars.
>
> The new drivers we contribute are v2 or later. This is your choice,
> but if there's no issue adding the or later it would be nice for all
> of the Aspeed code to be consistent.
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_device.h>
> > +#include <linux/io.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/sysfs.h>
> > +
> > +#include "aspeed_pwm.h"
> > +
> > +struct ast_pwm_port_data {
>
> For better or worse, we've settled on "aspeed" as the prefix for all
> of the drivers. Please use that instead of ast.
>
> > +       u8 pwm_port;
> > +       u8 pwm_enable;
> > +       u8 pwm_type;
> > +       u8 pwm_duty_cycle;
> > +       void __iomem *base;
> > +};
> > +
> > +struct ast_pwm_controller_data {
> > +       void __iomem *base;
> > +};
> > +
> > +static inline void
> > +ast_pwm_write(void __iomem *base, u32 val, u32 reg)
> > +{
> > +       writel(val, base + reg);
> > +}
> > +
> > +static inline u32
> > +ast_pwm_read(void __iomem *base, u32 reg)
> > +{
> > +       u32 val = readl(base + reg);
> > +       return val;
> > +}
> > +
> > +static void
> > +ast_set_pwm_clock_enable(struct ast_pwm_controller_data *priv, u8 val)
>
> val is a boolean.
>
> > +{
> > +       if (val) {
> > +               ast_pwm_write(priv->base,
> > +                       ast_pwm_read(priv->base, AST_PTCR_CTRL) |
> > +                       AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
> > +       } else {
> > +               ast_pwm_write(priv->base,
> > +                       ast_pwm_read(priv->base, AST_PTCR_CTRL) &
> > +                       ~AST_PTCR_CTRL_CLK_EN, AST_PTCR_CTRL);
> > +       }
> > +}
>
> > +static void
> > +ast_set_pwm_enable(struct ast_pwm_port_data *priv, u8 enable)
> > +{
> > +       u8 pwm_ch = priv->pwm_port;
> > +
>
> The switch statements where you read-modify-write are hard to read. I
> suggest reading the register value into a variable, modify it in your
> switch statement, and write it at the end. This makes it easier to
> read what operations you're performing on the bits.
>
> u32 reg = ast_pwm_read(priv->base, AST_PTCR_CTRL);
>
> switch (pwm_ch) {
> case PWMA:
>      port = AST_PTCR_CTRL_PWMA_EN;
>      break;
> case PWMB:
>      port = AST_PTCR_CTRL_PWMB_EN;
>      break;
> ...
> }
>
> if (enable)
>    reg |= port;
> else
>    reg &= ~port;
>
> ast_pwm_write(priv->base, reg, AST_PTCR_CTRL);
>
> I think this structure will make this more readable. You could store
> the AST_PTCR_CTRL_PWMx_EN value as priv->pwm_port, and lose the switch
> statement all together.
>
> This suggestion wont work for the other switch statements obviously.
> Can you think of a representation that would allow you to look up the
> values without requiring a switch statement? Take a look at some of
> the other pwm drivers for inspiration.
>
> > +       switch (pwm_ch) {
> > +       case PWMA:
> > +               if (enable)
> > +                       ast_pwm_write(priv->base,
> > +                               ast_pwm_read(priv->base,
> > +                               AST_PTCR_CTRL) |
> > +                               AST_PTCR_CTRL_PWMA_EN,
> > +                               AST_PTCR_CTRL);
>
> > +static void
> > +ast_set_pwm_type(struct ast_pwm_port_data *priv, u8 type)
> > +{
> > +       u32 tmp1, tmp2;
> > +       u8 pwm_ch = priv->pwm_port;
> > +
> > +       tmp1 = ast_pwm_read(priv->base, AST_PTCR_CTRL);
> > +       tmp2 = ast_pwm_read(priv->base, AST_PTCR_CTRL_EXT);
>
> I think we can do better than tmp1 and tmp2. perhaps ctrl and ctrl_ext?
>
> > +
> > +       switch (pwm_ch) {
>
> As with pwm_type, pwm_ch could be an enum. That way you get warnings
> if you omit an option in your switch statement.
>
> > +       case PWMA:
> > +               tmp1 &= ~AST_PTCR_CTRL_SET_PWMA_TYPE_MASK;
> > +               tmp1 |= AST_PTCR_CTRL_SET_PWMA_TYPE(type);
> > +               ast_pwm_write(priv->base, tmp1, AST_PTCR_CTRL);
> > +
> > +               break;
>
> > +static int
> > +aspeed_create_pwm_port(struct device *dev, struct device_node *child,
> > +               void __iomem *base)
> > +{
> > +       struct device *hwmon;
> > +       u8 val;
> > +       struct ast_pwm_port_data *priv;
> > +
> > +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       hwmon = devm_hwmon_device_register_with_groups(dev, "pwm_port",
>
> "aspeed_pwm"?
>
> > +               priv, pwm_dev_groups);
> > +       if (IS_ERR(hwmon)) {
> > +               dev_err(dev, "Failed to register hwmon device\n");
> > +               return PTR_ERR(hwmon);
> > +       }
> > +       priv->base = base;
> > +
> > +       of_property_read_u8(child, "pwm_port", &val);
> > +       priv->pwm_port = val;
> > +
> > +       of_property_read_u8(child, "pwm_enable", &val);
> > +       priv->pwm_enable = val;
> > +       ast_set_pwm_enable(priv, val);
> > +
> > +       of_property_read_u8(child, "pwm_type", &val);
> > +       priv->pwm_type = val;
> > +       ast_set_pwm_type(priv, val);
> > +
> > +       of_property_read_u8(child, "pwm_duty_cycle_percent", &val);
> > +       priv->pwm_duty_cycle = val;
> > +       ast_set_pwm_duty_cycle(priv, val);
> > +       return 0;
> > +}
> > +
> > +static int
> > +aspeed_ast2500_pwm_probe(struct platform_device *pdev)
> > +{
> > +       u32 buf[4];
> > +       struct device_node *np, *child;
> > +       struct resource *res;
> > +       u8 val;
> > +       int err;
> > +       struct ast_pwm_controller_data *priv;
> > +
> > +       np = pdev->dev.of_node;
> > +
> > +       priv = devm_kzalloc(&pdev->dev, sizeof(struct
> ast_pwm_controller_data),
> > +                       GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
>
>
> Can you explain why you chose to allocate structure here? You don't
> seem to use it outside of the probe function which suggests it's
> unnecessary.
>
> You can just get the base address, map it in (ioremap) to configure
> the state and then discard the mapping in the probe.
>
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (res == NULL) {
> > +               return -ENOENT;
> > +       }
> > +
> > +       priv->base = devm_ioremap(&pdev->dev, res->start,
> resource_size(res));
> > +       if (!priv->base)
> > +               return -ENOMEM;
> > +
> > +       of_property_read_u8(np, "clock_enable", &val);
> > +       ast_set_pwm_clock_enable(priv, val);
> > +       of_property_read_u8(np, "clock_source", &val);
> > +       ast_set_pwm_clock_source(priv, val);
>
> This needs reworking. I will comment on the bindings which will lead
> to changes in this code.
>
> > +
> > +       of_property_read_u32_array(np, "typem_pwm_clock", buf, 4);
> > +       if (buf[0] == 1) {
> > +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_M, buf[1]);
> > +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_M, buf[2]);
> > +               ast_set_pwm_clock_unit(priv, PWM_TYPE_M, buf[3]);
> > +       }
> > +
> > +       of_property_read_u32_array(np, "typen_pwm_clock", buf, 4);
> > +       if (buf[0] == 1) {
> > +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_N, buf[1]);
> > +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_N, buf[2]);
> > +               ast_set_pwm_clock_unit(priv, PWM_TYPE_N, buf[3]);
> > +       }
> > +
> > +       of_property_read_u32_array(np, "typeo_pwm_clock", buf, 4);
> > +       if (buf[0] == 1) {
> > +               ast_set_pwm_clock_division_l(priv, PWM_TYPE_O, buf[1]);
> > +               ast_set_pwm_clock_division_h(priv, PWM_TYPE_O, buf[2]);
> > +               ast_set_pwm_clock_unit(priv, PWM_TYPE_O, buf[3]);
> > +       }
> > +
> > +       for_each_child_of_node(np, child) {
> > +               aspeed_create_pwm_port(&pdev->dev,
> > +                               child, priv->base);
> > +               of_node_put(child);
> > +       }
> > +       of_node_put(np);
> > +       return 0;
> > +}
> > +
> > +static int
> > +aspeed_ast2500_pwm_remove(struct platform_device *pdev)
> > +{
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id of_pwm_match_table[] = {
> > +       { .compatible = "aspeed,ast2500-pwm", },
>
> As discussed above, this should contain the ast2400 string as well.
>
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> > +
> > +static struct platform_driver aspeed_ast2500_pwm_driver = {
> > +       .probe          = aspeed_ast2500_pwm_probe,
> > +       .remove         = aspeed_ast2500_pwm_remove,
>
> Your remove does nothing, you can omit it.
>
> > +       .driver         = {
> > +               .name   = "aspeed_ast2500_pwm",
> > +               .owner  = THIS_MODULE,
> > +               .of_match_table = of_pwm_match_table,
> > +       },
> > +};
> > +
> > +module_platform_driver(aspeed_ast2500_pwm_driver);
> > +
> > +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <
> jaghu@google.com>");
> > +MODULE_DESCRIPTION("ASPEED AST2500 PWM device driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/hwmon/aspeed_ast2500_pwm.h b/drivers/hwmon/aspeed_
> ast2500_pwm.h
> > new file mode 100644
> > index 0000000..1986fd2
> > --- /dev/null
> > +++ b/drivers/hwmon/aspeed_ast2500_pwm.h
>
> Headers in the kernel are generally only used when we need to share
> details between parts of the kernel. Given there isn't a lot here, I
> think these definitions should be in the c file.
>
>
> > +#define DUTY_CTRL_PWM2_FALL_POINT                      (24)
> > +#define DUTY_CTRL_PWM2_FALL_POINT_MASK                 (0xff<<24)
> > +#define DUTY_CTRL_PWM2_RISE_POINT                      (16)
> > +#define DUTY_CTRL_PWM2_RISE_POINT_MASK                 (0xff<<16)
> > +#define DUTY_CTRL_PWM1_FALL_POINT                      (8)
> > +#define DUTY_CTRL_PWM1_FALL_POINT_MASK                 (0xff<<8)
>
> Try using GENMASK. For this it would be GENMASK(15, 8).
>
> Do you really need the mask definitions?
>
> > +#define DUTY_CTRL_PWM1_RISE_POINT                      (0)
> > +#define DUTY_CTRL_PWM1_RISE_POINT_MASK                 (0xff)
> > +
> > +/* AST_PTCR_CTRL : 0x00 - General Control Register */
> > +#define AST_PTCR_CTRL_SET_PWMD_TYPE(x)         ((x & 0x1) << 15 | \
> > +                                               (x & 0x2) << 6)
> > +#define AST_PTCR_CTRL_SET_PWMD_TYPE_MASK       ((0x1 << 7) | (0x1 <<
> 15))
>
> You can use BIT(7) | BIT(15).
>
> > +
> > +#define AST_PTCR_CTRL_SET_PWMC_TYPE(x)         ((x & 0x1) << 14 | \
> > +                                               (x & 0x2) << 5)
>
> > +#define        AST_PTCR_CTRL_PWMD                      (11)
> > +#define        AST_PTCR_CTRL_PWMD_EN           (0x1 << 11)
> > +#define        AST_PTCR_CTRL_PWMC                      (10)
> > +#define        AST_PTCR_CTRL_PWMC_EN           (0x1 << 10)
> > +#define        AST_PTCR_CTRL_PWMB                      (9)
> > +#define        AST_PTCR_CTRL_PWMB_EN           (0x1 << 9)
> > +#define        AST_PTCR_CTRL_PWMA                      (8)
> > +#define        AST_PTCR_CTRL_PWMA_EN           (0x1 << 8)
>
> Consider only having the bit defintiions or the _EN. One can be
> derived from the other.
>
> You don't need to guard the integers with ( ).
>
> > +
> > +/*0:24Mhz, 1:MCLK */
> > +#define        AST_PTCR_CTRL_CLK_SRC           0x2
> > +#define        AST_PTCR_CTRL_CLK_EN            0x1
>

[-- Attachment #2: Type: text/html, Size: 20615 bytes --]

Re: [PATCH linux v2 2/3] drivers: hwmon: ASPEED AST2500 PWM driver

[Joel Stanley]

On Thu, Nov 24, 2016 at 7:56 PM, Jaghathiswari Rankappagounder
Natarajan <jaghu@google.com> wrote:
>
>
> On Tue, Nov 8, 2016 at 11:15 PM, Joel Stanley <joel@jms.id.au> wrote:
>>
>> Hello Jaghu,
>>
>> On Wed, Nov 9, 2016 at 12:41 PM, Jaghathiswari Rankappagounder
>> Natarajan <jaghu@google.com> wrote:
>> > The ASPEED AST2500 PWM controller can support upto 8 PWM output ports.
>>
>> I noticed that the ast2500 and ast2400 share the same PWM IP, so we
>> can use the same driver for both. You should reflect this in the
>> naming of the Kconfig, filename, commit message and the code itself.
>>
>> > There are three different PWM sources(types M, N and O)
>> > and each PWM output port can be assigned a particular PWM source.
>> > There is a sysfs file through which the user can control
>> > the duty cycle of a particular PWM port. The duty cycle can range from 0
>> > to
>> > 100 percent.
>>
>> Can you explain why this is a hwmon driver and not a pwm driver?
>
> The fan tacho controller shares some common registers with PWM
> controller; the fan tacho controller needs to be a hwmon driver; so if PWM
> controller driver is a hwmon driver, then there is flexibility to add fan
> tacho controller functionality to the same PWM driver and make it as common
> driver for PWM and fan tacho controller.
> The duty_cycle value available in pwm  sysfs interface needs to be
> configured in nano seconds and the duty_cycle value available in hwmon sysfs
> interface can be configured with a integer value (ranging from 0 to 255;
> with 255 being 100 percent). So thought that hwmon sysfs interface is
> easier.

Okay. Make sure you include this information in your cover letter and
the commit message for the driver.

[PATCH linux v2 3/3] devicetree : Add support in Zaius platform for 4 PWM output ports

[Jaghathiswari Rankappagounder Natarajan]

Zaius has four fans. Add support for four PWM output ports in Zaius.

v2:
- make the pwmX entries children of the pwm_controller entry.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
index 4c4754b..5ba8fed 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-zaius.dts
@@ -58,6 +58,49 @@
 			};
 		};
 	};
+
+	pwm: pwm-controller@1e786000 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x1E786000 0x78>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default
+				&pinctrl_pwm2_default &pinctrl_pwm3_default>;
+		compatible = "aspeed,ast2500-pwm";
+		clock_enable = /bits/ 8 <0x01>;
+		clock_source = /bits/ 8 <0x00>;
+		typem_pwm_clock = <1 5 0 95>;
+		typen_pwm_clock = <0 0 0 0>;
+		typeo_pwm_clock = <0 0 0 0>;
+
+		pwm_port0 {
+			pwm_port = /bits/ 8 <0x00>;
+			pwm_enable = /bits/ 8 <0x01>;
+			pwm_type = /bits/ 8 <0x00>;
+			pwm_duty_cycle_percent = /bits/ 8 <0x64>;
+		};
+
+		pwm_port1 {
+			pwm_port = /bits/ 8 <0x01>;
+			pwm_enable = /bits/ 8 <0x01>;
+			pwm_type = /bits/ 8 <0x00>;
+			pwm_duty_cycle_percent = /bits/ 8 <0x64>;
+		};
+
+		pwm_port2 {
+			pwm_port = /bits/ 8 <0x02>;
+			pwm_enable = /bits/ 8 <0x01>;
+			pwm_type = /bits/ 8 <0x00>;
+			pwm_duty_cycle_percent = /bits/ 8 <0x64>;
+		};
+
+		pwm_port3 {
+			pwm_port = /bits/ 8 <0x03>;
+			pwm_enable = /bits/ 8 <0x01>;
+			pwm_type = /bits/ 8 <0x00>;
+			pwm_duty_cycle_percent = /bits/ 8 <0x64>;
+		};
+	};
 };

 &uart5 {
--
2.8.0.rc3.226.g39d4020