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