Support for ASPEED AST2400/AST2500 PWM driver

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 <[email protected]>

---
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

Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt new file +18 -0

new file mode 100644
index 0000000..11a007c
@@ -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.
+

Documentation/devicetree/bindings/hwmon/aspeed-pwm.txt new file +17 -0

+   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.
+

Documentation/hwmon/aspeed-pwm new file +4 -0

+The device driver matches on the device tree node. The configuration values
+are read from the device tree and written to the respective registers.
+The driver provides a sysfs entry "pwm1" through which the user can
+configure the duty-cycle value (ranging from 0 to 255; 255 is 100 percent).

drivers/hwmon/Kconfig +2 -0

index d4de8d5..7f75b01 100644
@@ -341,6 +341,19 @@ config SENSORS_ASB100
          This driver can also be built as a module.  If so, the module
          will be called asb100.
 
+config SENSORS_ASPEED_PWM
+       tristate "ASPEED AST2400/AST2500 PWM driver"

drivers/hwmon/Kconfig +9 -0

+
+struct aspeed_pwm_port_data {
+       u8 pwm_port;
+       u8 pwm_enable;
+       u8 pwm_type;
+       u8 pwm_fan_ctrl;
+       void __iomem *base;
+};
+

drivers/hwmon/Kconfig +51 -0

+static const struct pwm_clock_type_params pwm_clock_type_params[] = {
+       [TYPEM] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
+       },
+       [TYPEN] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
+       },
+       [TYPEO] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL_EXT,
+       }
+};
+
+enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
+
+struct pwm_port_params {
+       u32 pwm_en;
+       u32 ctrl_reg_offset;
+       u32 pwm_type_part1;
+       u32 pwm_type_part2;
+       u32 pwm_type_mask;
+       u32 duty_ctrl_rise_point;
+       u32 duty_ctrl_fall_point;
+       u32 duty_ctrl_reg_offset;
+       u8 duty_ctrl_calc_type;
+};
+
+static const struct pwm_port_params pwm_port_params[] = {
+       [PWMA] = {
+               .pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
+               .ctrl_reg_offset = ASPEED_PTCR_CTRL,
+               .pwm_type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
+               .pwm_type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
+               .pwm_type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,

drivers/hwmon/Kconfig +18 -0

+{
+       u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);
+
+       if (val)
+               reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
+       else
+               reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
+
+       aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);
+}
+
+static void
+aspeed_set_pwm_clock_division_h(void __iomem *base, u8 pwm_clock_type,
+               u8 div_high)
+{
+       u32 reg_offset = pwm_clock_type_params[pwm_clock_type].ctrl_reg_offset;
+       u32 reg_value = aspeed_pwm_read(base, reg_offset);
+

drivers/hwmon/Kconfig +16 -0

+
+static void
+aspeed_set_pwm_clock_unit(void __iomem *base, u8 pwm_clock_type,
+               u8 unit)
+{
+       u32 reg_offset = pwm_clock_type_params[pwm_clock_type].ctrl_reg_offset;
+       u32 reg_value = aspeed_pwm_read(base, reg_offset);
+
+       reg_value &= ~pwm_clock_type_params[pwm_clock_type].unit_mask;
+       reg_value |= unit << pwm_clock_type_params[pwm_clock_type].unit_value;
+
+       aspeed_pwm_write(base, reg_value, reg_offset);
+}
+
 
+static void
+aspeed_set_pwm_fan_ctrl(struct aspeed_pwm_port_data *priv, u8 fan_ctrl)

drivers/hwmon/Kconfig +9 -0

+       } else {
+               if (dc_time_on == period)
+                       dc_time_on = 0;
+
+               aspeed_set_pwm_duty_rising(priv, 0);
+               aspeed_set_pwm_duty_falling(priv, dc_time_on);
+               aspeed_set_pwm_enable(priv, 1);
+       }
+}

drivers/hwmon/Kconfig +70 -0

+       if (ret)
+               return -EINVAL;
+
+       if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
+               return -EINVAL;
+
+       if (priv->pwm_fan_ctrl == fan_ctrl)
+               return count;
+
+       priv->pwm_fan_ctrl = fan_ctrl;
+       aspeed_set_pwm_fan_ctrl(priv, fan_ctrl);
+
+       return count;
+}
+
+static ssize_t
+show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
+{
+       struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);
+
+       return sprintf(buf, "%u\n", priv->pwm_fan_ctrl);
+}
+
+static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 0);
+
+static struct attribute *pwm_dev_attrs[] = {
+       &sensor_dev_attr_pwm1.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 aspeed_pwm_port_data *priv;
+
+       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+
+       hwmon = devm_hwmon_device_register_with_groups(dev, "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;
+       aspeed_set_pwm_enable(priv, val);
+
+       of_property_read_u8(child, "pwm_type", &val);
+       priv->pwm_type = val;
+       aspeed_set_pwm_type(priv, val);
+
+       of_property_read_u8(child, "pwm_fan_ctrl", &val);
+       priv->pwm_fan_ctrl = val;
+       aspeed_set_pwm_fan_ctrl(priv, val);
+
+       return 0;
+}
+

drivers/hwmon/Kconfig +42 -0

+       u32 divisor = src_frequency / pwm_frequency;
+       u32 clock_divisor = divisor / (period + 1);
+       u32 tmp_clock_divisor;
+       u8 low;
+       u8 high;
+       u32 calc_high;
+       u32 calc_low;
+
+       for (high = 0; high <= MAX_HIGH_LOW_BIT; high += 1) {
+               for (low = 0; low <= MAX_HIGH_LOW_BIT; low += 1) {
+                       calc_high = 0x1 << high;
+                       calc_low = (low == 0 ? 1 : (2 * low));
+                       tmp_clock_divisor = calc_high * calc_low;
+                       if (tmp_clock_divisor >= clock_divisor)
+                               goto set_value;
+               }
+       }
+
+       return false;
+
+set_value:
+       aspeed_set_pwm_clock_division_h(base, type, high);
+       aspeed_set_pwm_clock_division_l(base, type, low);
+       aspeed_set_pwm_clock_unit(base, type, period);
+
+       return true;
+}
+
+static int
+aspeed_pwm_probe(struct platform_device *pdev)
+{
+       u32 period;
+       struct device_node *np, *child;
+       struct resource *res;
+       void __iomem *base;
+       bool success;
+       struct clk *typem_clk, *typen_clk, *typeo_clk;
+       u32 pwm_typem_freq, pwm_typen_freq, pwm_typeo_freq;
+       int err;
+
+       np = pdev->dev.of_node;
+

drivers/hwmon/Kconfig +33 -0

+
+       err = of_property_read_u32(np, "pwm_typem_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typem_freq, period, TYPEM, base);
+               if (!success)
+                       return -EINVAL;
+       } else {
+               return -EINVAL;
+       }
+
+       err = of_property_read_u32(np, "pwm_typen_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typen_freq, period, TYPEN, base);
+               if (!success)
+                       return -EINVAL;
+       }
+
+       err = of_property_read_u32(np, "pwm_typeo_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typeo_freq, period, TYPEO, base);
+               if (!success)
+                       return -EINVAL;
+       }
+
+       for_each_child_of_node(np, child) {
+               aspeed_create_pwm_port(&pdev->dev,
+                               child, base);
+               of_node_put(child);
+       }
+       of_node_put(np);
+
+       return 0;
+}

Hello Jaghu,

This is looking better. I have some more comments below.

Review

Quoted 2 lines · Jaghathiswari Rankappagounder Natarajan

(quoted diff) +2 -0

+The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
+PWM clock types M, N and 0 are three types just to have three

independent PWM

Quoted 1 line

+sources. Each port can be assigned a different PWM clock type.

I suggest you describe the types.

And add some newlines between your paragraphs to make it easier to read.

Quoted 1 line

+The device driver matches on the device tree node. The configuration

values

Documentation/hwmon/aspeed-pwm

Quoted 3 lines

Documentation/hwmon/aspeed-pwm new file +3 -0

+are read from the device tree and written to the respective registers.
+The driver provides a sysfs entry "pwm1" through which the user can
+configure the duty-cycle value (ranging from 0 to 255; 255 is 100

percent).

drivers/hwmon/Kconfig config SENSORS_ASB100 · line 348

Quoted 13 lines

drivers/hwmon/Kconfig +5 -0

index d4de8d5..7f75b01 100644
@@ -341,6 +341,19 @@ config SENSORS_ASB100
          This driver can also be built as a module.  If so, the module
          will be called asb100.
 
+config SENSORS_ASPEED_PWM
+       tristate "ASPEED AST2400/AST2500 PWM driver"
+       help
+         This driver provides support for ASPEED AST2400/AST2500 PWM
+         controller and output ports. The ASPEED PWM controller can

support 8

drivers/hwmon/Kconfig config SENSORS_ASB100 · line 349

Quoted 1 line

drivers/hwmon/Kconfig +1 -0

+         PWM outputs. PWM clock types M, N and 0 are three types just

to have three

drivers/hwmon/Kconfig config SENSORS_ASB100 · line 350

Quoted 1 line

drivers/hwmon/Kconfig +1 -0

+         independent PWM sources. Each could be assigned to the 8 PWM

port

drivers/hwmon/Kconfig config SENSORS_ASB100 · line 351

Quoted 1 line

drivers/hwmon/Kconfig +1 -0

+         with its own settings. The user can configure duty cycle

through

drivers/hwmon/Kconfig config SENSORS_ASB100 · line 355

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+         the exposed hwmon sysfs entry "pwm".
+
+         This driver can also be built as a module.  If so, the module
+         will be called aspeed-pwm.

Make sure you test the code both as a module and built in to the kernel.

Quoted 9 lines

drivers/hwmon/Kconfig +9 -0

+
+struct aspeed_pwm_port_data {
+       u8 pwm_port;
+       u8 pwm_enable;
+       u8 pwm_type;
+       u8 pwm_fan_ctrl;
+       void __iomem *base;
+};
+

drivers/hwmon/Kconfig config SENSORS_ASB100

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+enum pwm_clock_type { TYPEM, TYPEN, TYPEO };
+
+struct pwm_clock_type_params {
+       u32 l_value;
+       u32 l_mask;
+       u32 h_value;
+       u32 h_mask;
+       u32 unit_value;
+       u32 unit_mask;
+       u32 ctrl_reg_offset;

ctrl_reg or even just ctrl will do.

drivers/hwmon/Kconfig

Quoted 2 lines

drivers/hwmon/Kconfig +2 -0

+};
+

I think this is an improvement on the duplicated swtich statements we had before. Do you agree?

I'm wondering if there are still ways to clean up. The mask and the value both tell us where in the register each value needs to go; perhaps we can only include one of them in the pwm_clock_type_params?

Quoted 51 lines

drivers/hwmon/Kconfig +51 -0

+static const struct pwm_clock_type_params pwm_clock_type_params[] = {
+       [TYPEM] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
+       },
+       [TYPEN] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,
+       },
+       [TYPEO] = {
+               .l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
+               .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_L_MASK,
+               .h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
+               .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_H_MASK,
+               .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
+               .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT_MASK,
+               .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL_EXT,
+       }
+};
+
+enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
+
+struct pwm_port_params {
+       u32 pwm_en;
+       u32 ctrl_reg_offset;
+       u32 pwm_type_part1;
+       u32 pwm_type_part2;
+       u32 pwm_type_mask;
+       u32 duty_ctrl_rise_point;
+       u32 duty_ctrl_fall_point;
+       u32 duty_ctrl_reg_offset;
+       u8 duty_ctrl_calc_type;
+};
+
+static const struct pwm_port_params pwm_port_params[] = {
+       [PWMA] = {
+               .pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
+               .ctrl_reg_offset = ASPEED_PTCR_CTRL,
+               .pwm_type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
+               .pwm_type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
+               .pwm_type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+               .duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
+               .duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
+               .duty_ctrl_reg_offset = ASPEED_PTCR_DUTY0_CTRL,
+               .duty_ctrl_calc_type = 0,
+       },
+
+static inline void
+aspeed_pwm_write(void __iomem *base, u32 val, u32 reg)
+{
+       writel(val, base + reg);

Is there a reason you're using writel and not iowrite32?

drivers/hwmon/Kconfig

Quoted 7 lines

drivers/hwmon/Kconfig +7 -0

+}
+
+static inline u32
+aspeed_pwm_read(void __iomem *base, u32 reg)
+{
+       u32 val = readl(base + reg);
+       return val;

You could just do return readl(base + reg).

That said, do you think the helpers for _read and _write are worth it?

drivers/hwmon/Kconfig

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+}
+
+static void
+aspeed_set_pwm_clock_enable(void __iomem *base, bool val)

Most kernel code puts the type and storage class specifiers on the same line as the function:

static void aspeed_set_pwm_clock_enable(void __iomem *base, bool val)

drivers/hwmon/Kconfig

Quoted 13 lines

drivers/hwmon/Kconfig +13 -0

+{
+       u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);
+
+       if (val)
+               reg_value |= ASPEED_PTCR_CTRL_CLK_EN;
+       else
+               reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;
+
+       aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);
+}
+
+static void
+aspeed_set_pwm_clock_source(void __iomem *base, bool val)

When I call this function I'm going to write something like this:

aspeed_set_pwm_clock_source(foo, true);

It doesn't make sense to be setting a clock source to "true". I think you could call it aspeed_pwm_use_pclk() or similar, so the boolean makes sense.

Quoted 6 lines

drivers/hwmon/Kconfig +6 -0

+{
+       u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);
+
+       if (val)
+               reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;
+       else

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+               reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;
+
+       aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);
+}
+
+static void
+aspeed_set_pwm_clock_division_h(void __iomem *base, u8 pwm_clock_type,
+               u8 div_high)
+{
+       u32 reg_offset = pwm_clock_type_params[pwm_

clock_type].ctrl_reg_offset;

drivers/hwmon/Kconfig

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+       u32 reg_value = aspeed_pwm_read(base, reg_offset);
+
+       reg_value &= ~pwm_clock_type_params[pwm_clock_type].h_mask;
+       reg_value |= div_high << pwm_clock_type_params[pwm_

clock_type].h_value;

drivers/hwmon/Kconfig

Quoted 8 lines

drivers/hwmon/Kconfig +8 -0

+
+       aspeed_pwm_write(base, reg_value, reg_offset);
+}
+
+static void
+aspeed_set_pwm_clock_division_l(void __iomem *base, u8 pwm_clock_type,
+               u8 div_low)
+{

You could pass around a pointer to the clock type, instead of the index.

Even better would be some kind of struct that includes both base and the clock type.

Quoted 1 line

+ u32 reg_offset = pwm_clock_type_params[pwm_

clock_type].ctrl_reg_offset;

drivers/hwmon/Kconfig

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+       u32 reg_value = aspeed_pwm_read(base, reg_offset);
+
+       reg_value &= ~pwm_clock_type_params[pwm_clock_type].l_mask;
+       reg_value |= div_low << pwm_clock_type_params[pwm_

clock_type].l_value;

drivers/hwmon/Kconfig

Quoted 3 lines

drivers/hwmon/Kconfig +3 -0

+
+       aspeed_pwm_write(base, reg_value, reg_offset);
+}

Is there ever a case where you will set the high but not the low bit? If not you could do this:

aspeed_set_pwm_clock_division(void __iomem *base, u8 pwm_clock_type, u8 div_low, u8 div_high)
{
struct pwm_clock_type *pwm = &pwm_clock_type_params[pwm_

clock_type];
       u32 reg = aspeed_pwm_read(base, pwm->ctrl_reg_offset);

       reg &= ~(pwm->l_mask | pwm->h_mask);
       reg |= (div_low << pwm->l_value) | (div_high << pwm->h_value);

       aspeed_pwm_write(base, reg, pwm->ctrl_reg_offset);
}

I suggest attempting cleanups like this to other parts of your code.

drivers/hwmon/Kconfig

Quoted 6 lines

drivers/hwmon/Kconfig +6 -0

+
+static void
+aspeed_set_pwm_clock_unit(void __iomem *base, u8 pwm_clock_type,
+               u8 unit)
+{
+       u32 reg_offset = pwm_clock_type_params[pwm_

clock_type].ctrl_reg_offset;

drivers/hwmon/Kconfig

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+       u32 reg_value = aspeed_pwm_read(base, reg_offset);
+
+       reg_value &= ~pwm_clock_type_params[pwm_clock_type].unit_mask;
+       reg_value |= unit << pwm_clock_type_params[pwm_

clock_type].unit_value;

Quoted 7 lines

drivers/hwmon/Kconfig +6 -0

+
+       aspeed_pwm_write(base, reg_value, reg_offset);
+}
+
 
+static void
+aspeed_set_pwm_fan_ctrl(struct aspeed_pwm_port_data *priv, u8 fan_ctrl)

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+{
+       u16 period;
+       u16 dc_time_on;
+
+       period = aspeed_get_pwm_clock_unit(priv, priv->pwm_type);
+       period += 1;
+       dc_time_on = (fan_ctrl * period) / PWM_MAX;
+
+       if (dc_time_on == 0) {
+               aspeed_set_pwm_enable(priv, 0);

Doesn't aspeed_set_pwm_enable take a boolean as the second parameter?

drivers/hwmon/Kconfig

Quoted 12 lines

drivers/hwmon/Kconfig +12 -0

+       } else {
+               if (dc_time_on == period)
+                       dc_time_on = 0;
+
+               aspeed_set_pwm_duty_rising(priv, 0);
+               aspeed_set_pwm_duty_falling(priv, dc_time_on);
+               aspeed_set_pwm_enable(priv, 1);
+       }
+}
+
+static ssize_t
+set_pwm(struct device *dev, struct device_attribute *attr, const char

*buf,

drivers/hwmon/Kconfig

Quoted 7 lines

drivers/hwmon/Kconfig +7 -0

+               size_t count)
+{
+       struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);
+       u8 fan_ctrl;
+       int ret;
+
+       ret = kstrtou8(buf, 10, &fan_ctrl);

If you specify 0 as the format, then users can pass their clock rate in as hexadecimal (not that it would make much sense...) or decimal and the sysfs file will do the correct thing.

Quoted 14 lines

drivers/hwmon/Kconfig +14 -0

+       if (ret)
+               return -EINVAL;
+
+       if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
+               return -EINVAL;
+
+       if (priv->pwm_fan_ctrl == fan_ctrl)
+               return count;
+
+       priv->pwm_fan_ctrl = fan_ctrl;
+       aspeed_set_pwm_fan_ctrl(priv, fan_ctrl);
+
+       return count;
+}

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+
+static ssize_t
+show_pwm(struct device *dev, struct device_attribute *attr, char *buf)
+{
+       struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);
+
+       return sprintf(buf, "%u\n", priv->pwm_fan_ctrl);
+}
+
+static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm,

0);

Quoted 11 lines

drivers/hwmon/Kconfig +11 -0

+
+static struct attribute *pwm_dev_attrs[] = {
+       &sensor_dev_attr_pwm1.dev_attr.attr,
+       NULL,
+};
+
+ATTRIBUTE_GROUPS(pwm_dev);
+
+static int
+aspeed_create_pwm_port(struct device *dev, struct device_node *child,
+               void __iomem *base)

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+{
+       struct device *hwmon;
+       u8 val;
+       struct aspeed_pwm_port_data *priv;
+
+       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+       if (!priv)
+               return -ENOMEM;
+
+       hwmon = devm_hwmon_device_register_with_groups(dev,

"aspeed_pwm",

Quoted 1 line

+ priv,

pwm_dev_groups);

Quoted 19 lines

drivers/hwmon/Kconfig +19 -0

+       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;
+       aspeed_set_pwm_enable(priv, val);
+
+       of_property_read_u8(child, "pwm_type", &val);
+       priv->pwm_type = val;
+       aspeed_set_pwm_type(priv, val);
+
+       of_property_read_u8(child, "pwm_fan_ctrl", &val);
+       priv->pwm_fan_ctrl = val;

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+       aspeed_set_pwm_fan_ctrl(priv, val);
+
+       return 0;
+}
+
+/*
+ * If the clock type is type M then :
+ * The PWM frequency = 24MHz / (type M clock division L bit *
+ * type M clock division H bit * (type M PWM period bit + 1))
+ * Calculate type M clock division L bit and H bits given the other

values

drivers/hwmon/Kconfig

Quoted 3 lines

+ */

drivers/hwmon/Kconfig +2 -0

+static bool
+set_clock_values(u32 pwm_frequency, u32 period, u8 type, void __iomem

*base)

drivers/hwmon/Kconfig

Quoted 2 lines

drivers/hwmon/Kconfig +2 -0

+{
+       u32 src_frequency = 24000000;

Where does this come from? It's the pclk I think. This means your binding should do something like this:

  clocks = <&clk_apb>;

Then in your code,

pclk = devm_clk_get(&pdev->dev, NULL);
pclk_rate = clk_get_rate(pclk);

and then use clck_rate where you have 'src_frequency' to calculate the divisor values. A hwmon driver that does this is g762.c.

Quoted 42 lines

drivers/hwmon/Kconfig +42 -0

+       u32 divisor = src_frequency / pwm_frequency;
+       u32 clock_divisor = divisor / (period + 1);
+       u32 tmp_clock_divisor;
+       u8 low;
+       u8 high;
+       u32 calc_high;
+       u32 calc_low;
+
+       for (high = 0; high <= MAX_HIGH_LOW_BIT; high += 1) {
+               for (low = 0; low <= MAX_HIGH_LOW_BIT; low += 1) {
+                       calc_high = 0x1 << high;
+                       calc_low = (low == 0 ? 1 : (2 * low));
+                       tmp_clock_divisor = calc_high * calc_low;
+                       if (tmp_clock_divisor >= clock_divisor)
+                               goto set_value;
+               }
+       }
+
+       return false;
+
+set_value:
+       aspeed_set_pwm_clock_division_h(base, type, high);
+       aspeed_set_pwm_clock_division_l(base, type, low);
+       aspeed_set_pwm_clock_unit(base, type, period);
+
+       return true;
+}
+
+static int
+aspeed_pwm_probe(struct platform_device *pdev)
+{
+       u32 period;
+       struct device_node *np, *child;
+       struct resource *res;
+       void __iomem *base;
+       bool success;
+       struct clk *typem_clk, *typen_clk, *typeo_clk;
+       u32 pwm_typem_freq, pwm_typen_freq, pwm_typeo_freq;
+       int err;
+
+       np = pdev->dev.of_node;
+

drivers/hwmon/Kconfig

Quoted 10 lines

drivers/hwmon/Kconfig +10 -0

+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (res == NULL)
+               return -ENOENT;
+
+       base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+       if (!base)
+               return -ENOMEM;
+
+       /* Enable PWM clock */
+       aspeed_set_pwm_clock_enable(base, 1);

You're passing a '1' to a function that expects a boolean value.

drivers/hwmon/Kconfig

Quoted 2 lines

drivers/hwmon/Kconfig +2 -0

+       /* Select clock source as 24MHz */
+       aspeed_set_pwm_clock_source(base, 0);

This would be selected by your device tree bindings. You could have it so if a clock node is populated, it uses that value, and if it's not it falls back on the 1MHz clock.

drivers/hwmon/Kconfig

Quoted 2 lines

drivers/hwmon/Kconfig +2 -0

+
+       typem_clk = of_clk_get(np, 0);

This isn't what I meant. See the comment above about referencing the parent clock in the bindings.

drivers/hwmon/Kconfig

Quoted 12 lines

drivers/hwmon/Kconfig +12 -0

+       if (!IS_ERR(typem_clk))
+               pwm_typem_freq = clk_get_rate(typem_clk);
+       else
+               return -EINVAL;
+
+       typen_clk = of_clk_get(np, 1);
+       if (!IS_ERR(typen_clk))
+               pwm_typen_freq = clk_get_rate(typen_clk);
+
+       typeo_clk = of_clk_get(np, 2);
+       if (!IS_ERR(typeo_clk))
+               pwm_typeo_freq = clk_get_rate(typeo_clk);

This doesn't make any sense. Did you test this code?

drivers/hwmon/Kconfig

Quoted 4 lines

drivers/hwmon/Kconfig +4 -0

+
+       err = of_property_read_u32(np, "pwm_typem_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typem_freq, period,

TYPEM, base);

drivers/hwmon/Kconfig

Quoted 9 lines

drivers/hwmon/Kconfig +9 -0

+               if (!success)
+                       return -EINVAL;
+       } else {
+               return -EINVAL;
+       }
+
+       err = of_property_read_u32(np, "pwm_typen_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typen_freq, period,

TYPEN, base);

drivers/hwmon/Kconfig

Quoted 7 lines

drivers/hwmon/Kconfig +7 -0

+               if (!success)
+                       return -EINVAL;
+       }
+
+       err = of_property_read_u32(np, "pwm_typeo_period", &period);
+       if (!err) {
+               success = set_clock_values(pwm_typeo_freq, period,

TYPEO, base);

Quoted 13 lines

drivers/hwmon/Kconfig +13 -0

+               if (!success)
+                       return -EINVAL;
+       }
+
+       for_each_child_of_node(np, child) {
+               aspeed_create_pwm_port(&pdev->dev,
+                               child, base);
+               of_node_put(child);
+       }
+       of_node_put(np);
+
+       return 0;
+}