<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Nov 27, 2016 at 10:13 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Jaghu,<br>
<br>
This is looking better. I have some more comments below.<br>
<span class="gmail-"><br>
On Thu, Nov 24, 2016 at 7:56 PM, Jaghathiswari Rankappagounder<br>
Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>> wrote:<br>
<br>
> +The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.<br>
> +PWM clock types M, N and 0 are three types just to have three independent PWM<br>
> +sources. Each port can be assigned a different PWM clock type.<br>
<br>
</span>I suggest you describe the types.<br>
<br>
And add some newlines between your paragraphs to make it easier to read.<br>
<span class="gmail-"><br>
> +The device driver matches on the device tree node. The configuration values<br>
> +are read from the device tree and written to the respective registers.<br>
> +The driver provides a sysfs entry "pwm1" through which the user can<br>
> +configure the duty-cycle value (ranging from 0 to 255; 255 is 100 percent).<br>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig<br>
> index d4de8d5..7f75b01 100644<br>
> --- a/drivers/hwmon/Kconfig<br>
> +++ b/drivers/hwmon/Kconfig<br>
> @@ -341,6 +341,19 @@ config SENSORS_ASB100<br>
> This driver can also be built as a module. If so, the module<br>
> will be called asb100.<br>
><br>
> +config SENSORS_ASPEED_PWM<br>
> + tristate "ASPEED AST2400/AST2500 PWM driver"<br>
> + help<br>
> + This driver provides support for ASPEED AST2400/AST2500 PWM<br>
> + controller and output ports. The ASPEED PWM controller can support 8<br>
> + PWM outputs. PWM clock types M, N and 0 are three types just to have three<br>
> + independent PWM sources. Each could be assigned to the 8 PWM port<br>
> + with its own settings. The user can configure duty cycle through<br>
> + the exposed hwmon sysfs entry "pwm".<br>
> +<br>
> + This driver can also be built as a module. If so, the module<br>
> + will be called aspeed-pwm.<br>
<br>
</span>Make sure you test the code both as a module and built in to the kernel.<br>
<span class="gmail-"><br>
> +<br>
> +struct aspeed_pwm_port_data {<br>
> + u8 pwm_port;<br>
> + u8 pwm_enable;<br>
> + u8 pwm_type;<br>
> + u8 pwm_fan_ctrl;<br>
> + void __iomem *base;<br>
> +};<br>
> +<br>
> +enum pwm_clock_type { TYPEM, TYPEN, TYPEO };<br>
> +<br>
> +struct pwm_clock_type_params {<br>
> + u32 l_value;<br>
> + u32 l_mask;<br>
> + u32 h_value;<br>
> + u32 h_mask;<br>
> + u32 unit_value;<br>
> + u32 unit_mask;<br>
> + u32 ctrl_reg_offset;<br>
<br>
</span>ctrl_reg or even just ctrl will do.<br>
<br>
> +};<br>
> +<br>
<br>
I think this is an improvement on the duplicated swtich statements we<br>
had before. Do you agree?<br>
<br>
I'm wondering if there are still ways to clean up. The mask and the<br>
value both tell us where in the register each value needs to go;<br>
perhaps we can only include one of them in the pwm_clock_type_params?<br>
<div><div class="gmail-h5"><br>
> +static const struct pwm_clock_type_params pwm_clock_type_params[] = {<br>
> + [TYPEM] = {<br>
> + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,<br>
> + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_L_<wbr>MASK,<br>
> + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,<br>
> + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_H_<wbr>MASK,<br>
> + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_<wbr>UNIT,<br>
> + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_<wbr>UNIT_MASK,<br>
> + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,<br>
> + },<br>
> + [TYPEN] = {<br>
> + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,<br>
> + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_L_<wbr>MASK,<br>
> + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,<br>
> + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_H_<wbr>MASK,<br>
> + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_<wbr>UNIT,<br>
> + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_<wbr>UNIT_MASK,<br>
> + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL,<br>
> + },<br>
> + [TYPEO] = {<br>
> + .l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,<br>
> + .l_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_L_<wbr>MASK,<br>
> + .h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,<br>
> + .h_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_H_<wbr>MASK,<br>
> + .unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_<wbr>UNIT,<br>
> + .unit_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_<wbr>UNIT_MASK,<br>
> + .ctrl_reg_offset = ASPEED_PTCR_CLK_CTRL_EXT,<br>
> + }<br>
> +};<br>
> +<br>
> +enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };<br>
> +<br>
> +struct pwm_port_params {<br>
> + u32 pwm_en;<br>
> + u32 ctrl_reg_offset;<br>
> + u32 pwm_type_part1;<br>
> + u32 pwm_type_part2;<br>
> + u32 pwm_type_mask;<br>
> + u32 duty_ctrl_rise_point;<br>
> + u32 duty_ctrl_fall_point;<br>
> + u32 duty_ctrl_reg_offset;<br>
> + u8 duty_ctrl_calc_type;<br>
> +};<br>
> +<br>
> +static const struct pwm_port_params pwm_port_params[] = {<br>
> + [PWMA] = {<br>
> + .pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,<br>
> + .ctrl_reg_offset = ASPEED_PTCR_CTRL,<br>
> + .pwm_type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_<wbr>TYPE_PART1,<br>
> + .pwm_type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_<wbr>TYPE_PART2,<br>
> + .pwm_type_mask = ASPEED_PTCR_CTRL_SET_PWMA_<wbr>TYPE_MASK,<br>
> + .duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,<br>
> + .duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,<br>
> + .duty_ctrl_reg_offset = ASPEED_PTCR_DUTY0_CTRL,<br>
> + .duty_ctrl_calc_type = 0,<br>
> + },<br>
> +<br>
</div></div><span class="gmail-">> +static inline void<br>
> +aspeed_pwm_write(void __iomem *base, u32 val, u32 reg)<br>
> +{<br>
> + writel(val, base + reg);<br>
<br>
</span>Is there a reason you're using writel and not iowrite32?<br>
<span class="gmail-"><br>
> +}<br>
> +<br>
> +static inline u32<br>
> +aspeed_pwm_read(void __iomem *base, u32 reg)<br>
> +{<br>
> + u32 val = readl(base + reg);<br>
> + return val;<br>
<br>
</span>You could just do return readl(base + reg).<br>
<br>
That said, do you think the helpers for _read and _write are worth it?<br>
<span class="gmail-"><br>
> +}<br>
> +<br>
> +static void<br>
> +aspeed_set_pwm_clock_enable(<wbr>void __iomem *base, bool val)<br>
<br>
</span>Most kernel code puts the type and storage class specifiers on the<br>
same line as the function:<br>
<br>
static void aspeed_set_pwm_clock_enable(<wbr>void __iomem *base, bool val)<br>
<span class="gmail-"><br>
> +{<br>
> + u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);<br>
> +<br>
> + if (val)<br>
> + reg_value |= ASPEED_PTCR_CTRL_CLK_EN;<br>
> + else<br>
> + reg_value &= ~ASPEED_PTCR_CTRL_CLK_EN;<br>
> +<br>
> + aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);<br>
> +}<br>
> +<br>
> +static void<br>
> +aspeed_set_pwm_clock_source(<wbr>void __iomem *base, bool val)<br>
<br>
</span>When I call this function I'm going to write something like this:<br>
<br>
aspeed_set_pwm_clock_source(<wbr>foo, true);<br>
<br>
It doesn't make sense to be setting a clock source to "true". I think<br>
you could call it aspeed_pwm_use_pclk() or similar, so the boolean<br>
makes sense.<br>
<div><div class="gmail-h5"><br>
> +{<br>
> + u32 reg_value = aspeed_pwm_read(base, ASPEED_PTCR_CTRL);<br>
> +<br>
> + if (val)<br>
> + reg_value |= ASPEED_PTCR_CTRL_CLK_SRC;<br>
> + else<br>
> + reg_value &= ~ASPEED_PTCR_CTRL_CLK_SRC;<br>
> +<br>
> + aspeed_pwm_write(base, reg_value, ASPEED_PTCR_CTRL);<br>
> +}<br>
> +<br>
> +static void<br>
> +aspeed_set_pwm_clock_<wbr>division_h(void __iomem *base, u8 pwm_clock_type,<br>
> + u8 div_high)<br>
> +{<br>
> + u32 reg_offset = pwm_clock_type_params[pwm_<wbr>clock_type].ctrl_reg_offset;<br>
> + u32 reg_value = aspeed_pwm_read(base, reg_offset);<br>
> +<br>
> + reg_value &= ~pwm_clock_type_params[pwm_<wbr>clock_type].h_mask;<br>
> + reg_value |= div_high << pwm_clock_type_params[pwm_<wbr>clock_type].h_value;<br>
> +<br>
> + aspeed_pwm_write(base, reg_value, reg_offset);<br>
> +}<br>
> +<br>
> +static void<br>
> +aspeed_set_pwm_clock_<wbr>division_l(void __iomem *base, u8 pwm_clock_type,<br>
> + u8 div_low)<br>
> +{<br>
<br>
</div></div>You could pass around a pointer to the clock type, instead of the index.<br>
<br>
Even better would be some kind of struct that includes both base and<br>
the clock type.<br>
<span class="gmail-"><br>
> + u32 reg_offset = pwm_clock_type_params[pwm_<wbr>clock_type].ctrl_reg_offset;<br>
> + u32 reg_value = aspeed_pwm_read(base, reg_offset);<br>
> +<br>
> + reg_value &= ~pwm_clock_type_params[pwm_<wbr>clock_type].l_mask;<br>
> + reg_value |= div_low << pwm_clock_type_params[pwm_<wbr>clock_type].l_value;<br>
> +<br>
> + aspeed_pwm_write(base, reg_value, reg_offset);<br>
> +}<br>
<br>
</span>Is there ever a case where you will set the high but not the low bit?<br>
If not you could do this:<br>
<br>
aspeed_set_pwm_clock_division(<wbr>void __iomem *base, u8 pwm_clock_type,<br>
u8 div_low, u8 div_high)<br>
{<br>
struct pwm_clock_type *pwm = &pwm_clock_type_params[pwm_<wbr>clock_type];<br>
u32 reg = aspeed_pwm_read(base, pwm->ctrl_reg_offset);<br>
<br>
reg &= ~(pwm->l_mask | pwm->h_mask);<br>
reg |= (div_low << pwm->l_value) | (div_high << pwm->h_value);<br>
<br>
aspeed_pwm_write(base, reg, pwm->ctrl_reg_offset);<br>
}<br>
<br>
I suggest attempting cleanups like this to other parts of your code.<br>
<span class="gmail-"><br>
> +<br>
> +static void<br>
> +aspeed_set_pwm_clock_unit(<wbr>void __iomem *base, u8 pwm_clock_type,<br>
> + u8 unit)<br>
> +{<br>
> + u32 reg_offset = pwm_clock_type_params[pwm_<wbr>clock_type].ctrl_reg_offset;<br>
> + u32 reg_value = aspeed_pwm_read(base, reg_offset);<br>
> +<br>
> + reg_value &= ~pwm_clock_type_params[pwm_<wbr>clock_type].unit_mask;<br>
> + reg_value |= unit << pwm_clock_type_params[pwm_<wbr>clock_type].unit_value;<br>
> +<br>
> + aspeed_pwm_write(base, reg_value, reg_offset);<br>
> +}<br>
> +<br>
<br>
> +static void<br>
</span><span class="gmail-">> +aspeed_set_pwm_fan_ctrl(<wbr>struct aspeed_pwm_port_data *priv, u8 fan_ctrl)<br>
> +{<br>
> + u16 period;<br>
> + u16 dc_time_on;<br>
> +<br>
> + period = aspeed_get_pwm_clock_unit(<wbr>priv, priv->pwm_type);<br>
> + period += 1;<br>
> + dc_time_on = (fan_ctrl * period) / PWM_MAX;<br>
> +<br>
> + if (dc_time_on == 0) {<br>
> + aspeed_set_pwm_enable(priv, 0);<br>
<br>
</span>Doesn't aspeed_set_pwm_enable take a boolean as the second parameter?<br>
<span class="gmail-"><br>
> + } else {<br>
> + if (dc_time_on == period)<br>
> + dc_time_on = 0;<br>
> +<br>
> + aspeed_set_pwm_duty_rising(<wbr>priv, 0);<br>
> + aspeed_set_pwm_duty_falling(<wbr>priv, dc_time_on);<br>
> + aspeed_set_pwm_enable(priv, 1);<br>
> + }<br>
> +}<br>
> +<br>
> +static ssize_t<br>
> +set_pwm(struct device *dev, struct device_attribute *attr, const char *buf,<br>
> + size_t count)<br>
> +{<br>
> + struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);<br>
> + u8 fan_ctrl;<br>
> + int ret;<br>
> +<br>
> + ret = kstrtou8(buf, 10, &fan_ctrl);<br>
<br>
</span>If you specify 0 as the format, then users can pass their clock rate<br>
in as hexadecimal (not that it would make much sense...) or decimal<br>
and the sysfs file will do the correct thing.</blockquote><div>The user is passing the duty cycle value here (and not the clock rate). </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> + if (ret)<br>
> + return -EINVAL;<br>
> +<br>
> + if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)<br>
> + return -EINVAL;<br>
> +<br>
> + if (priv->pwm_fan_ctrl == fan_ctrl)<br>
> + return count;<br>
> +<br>
> + priv->pwm_fan_ctrl = fan_ctrl;<br>
> + aspeed_set_pwm_fan_ctrl(priv, fan_ctrl);<br>
> +<br>
> + return count;<br>
> +}<br>
> +<br>
> +static ssize_t<br>
> +show_pwm(struct device *dev, struct device_attribute *attr, char *buf)<br>
> +{<br>
> + struct aspeed_pwm_port_data *priv = dev_get_drvdata(dev);<br>
> +<br>
> + return sprintf(buf, "%u\n", priv->pwm_fan_ctrl);<br>
> +}<br>
> +<br>
> +static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO | S_IWUSR, show_pwm, set_pwm, 0);<br>
> +<br>
> +static struct attribute *pwm_dev_attrs[] = {<br>
> + &sensor_dev_attr_pwm1.dev_<wbr>attr.attr,<br>
> + NULL,<br>
> +};<br>
> +<br>
> +ATTRIBUTE_GROUPS(pwm_dev);<br>
> +<br>
> +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 aspeed_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, "aspeed_pwm",<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>
> + aspeed_set_pwm_enable(priv, val);<br>
> +<br>
> + of_property_read_u8(child, "pwm_type", &val);<br>
> + priv->pwm_type = val;<br>
> + aspeed_set_pwm_type(priv, val);<br>
> +<br>
> + of_property_read_u8(child, "pwm_fan_ctrl", &val);<br>
> + priv->pwm_fan_ctrl = val;<br>
> + aspeed_set_pwm_fan_ctrl(priv, val);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/*<br>
> + * If the clock type is type M then :<br>
> + * The PWM frequency = 24MHz / (type M clock division L bit *<br>
> + * type M clock division H bit * (type M PWM period bit + 1))<br>
> + * Calculate type M clock division L bit and H bits given the other values<br>
> + */<br>
> +static bool<br>
> +set_clock_values(u32 pwm_frequency, u32 period, u8 type, void __iomem *base)<br>
> +{<br>
> + u32 src_frequency = 24000000;<br>
<br>
</div></div>Where does this come from? It's the pclk I think. This means your<br>
binding should do something like this:<br>
<br>
clocks = <&clk_apb>;<br>
<br>
Then in your code,<br>
<br>
pclk = devm_clk_get(&pdev->dev, NULL);<br>
pclk_rate = clk_get_rate(pclk);<br>
<br>
and then use clck_rate where you have 'src_frequency' to calculate the<br>
divisor values. A hwmon driver that does this is g762.c.</blockquote><div>For the PWM and Fan Tach controller, in the register PTCR00 (AST2500 datasheet page - 671) it is given as : </div><div><br></div><div>Clock source selection </div><div>0: from 24MHz </div><div>1: from MCLK<br></div><div><br></div><div>24MHz seems to be hardcoded. I am not sure which clock producer to use</div><div>for 24MHz and for MCLK.</div><div><br></div><div>CLKIN seems to have the option of being either 24MHz and 25 MHz.</div><div><br></div><div>I am not sure how the pclk is related here. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5">
> + u32 divisor = src_frequency / pwm_frequency;<br>
> + u32 clock_divisor = divisor / (period + 1);<br>
> + u32 tmp_clock_divisor;<br>
> + u8 low;<br>
> + u8 high;<br>
> + u32 calc_high;<br>
> + u32 calc_low;<br>
> +<br>
> + for (high = 0; high <= MAX_HIGH_LOW_BIT; high += 1) {<br>
> + for (low = 0; low <= MAX_HIGH_LOW_BIT; low += 1) {<br>
> + calc_high = 0x1 << high;<br>
> + calc_low = (low == 0 ? 1 : (2 * low));<br>
> + tmp_clock_divisor = calc_high * calc_low;<br>
> + if (tmp_clock_divisor >= clock_divisor)<br>
> + goto set_value;<br>
> + }<br>
> + }<br>
> +<br>
> + return false;<br>
> +<br>
> +set_value:<br>
> + aspeed_set_pwm_clock_division_<wbr>h(base, type, high);<br>
> + aspeed_set_pwm_clock_division_<wbr>l(base, type, low);<br>
> + aspeed_set_pwm_clock_unit(<wbr>base, type, period);<br>
> +<br>
> + return true;<br>
> +}<br>
> +<br>
> +static int<br>
> +aspeed_pwm_probe(struct platform_device *pdev)<br>
> +{<br>
> + u32 period;<br>
> + struct device_node *np, *child;<br>
> + struct resource *res;<br>
> + void __iomem *base;<br>
> + bool success;<br>
> + struct clk *typem_clk, *typen_clk, *typeo_clk;<br>
> + u32 pwm_typem_freq, pwm_typen_freq, pwm_typeo_freq;<br>
> + int err;<br>
> +<br>
> + np = pdev->dev.of_node;<br>
> +<br>
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);<br>
> + if (res == NULL)<br>
> + return -ENOENT;<br>
> +<br>
> + base = devm_ioremap(&pdev->dev, res->start, resource_size(res));<br>
> + if (!base)<br>
> + return -ENOMEM;<br>
> +<br>
> + /* Enable PWM clock */<br>
> + aspeed_set_pwm_clock_enable(<wbr>base, 1);<br>
<br>
</div></div>You're passing a '1' to a function that expects a boolean value.<br>
<span class="gmail-"><br>
> + /* Select clock source as 24MHz */<br>
> + aspeed_set_pwm_clock_source(<wbr>base, 0);<br>
<br>
</span>This would be selected by your device tree bindings. You could have it<br>
so if a clock node is populated, it uses that value, and if it's not<br>
it falls back on the 1MHz clock.<br>
<span class="gmail-"><br>
> +<br>
> + typem_clk = of_clk_get(np, 0);<br>
<br>
</span>This isn't what I meant. See the comment above about referencing the<br>
parent clock in the bindings.<br>
<span class="gmail-"><br>
> + if (!IS_ERR(typem_clk))<br>
> + pwm_typem_freq = clk_get_rate(typem_clk);<br>
> + else<br>
> + return -EINVAL;<br>
> +<br>
> + typen_clk = of_clk_get(np, 1);<br>
> + if (!IS_ERR(typen_clk))<br>
> + pwm_typen_freq = clk_get_rate(typen_clk);<br>
> +<br>
> + typeo_clk = of_clk_get(np, 2);<br>
> + if (!IS_ERR(typeo_clk))<br>
> + pwm_typeo_freq = clk_get_rate(typeo_clk);<br>
<br>
</span>This doesn't make any sense. Did you test this code?<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
> +<br>
> + err = of_property_read_u32(np, "pwm_typem_period", &period);<br>
> + if (!err) {<br>
> + success = set_clock_values(pwm_typem_<wbr>freq, period, TYPEM, base);<br>
> + if (!success)<br>
> + return -EINVAL;<br>
> + } else {<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + err = of_property_read_u32(np, "pwm_typen_period", &period);<br>
> + if (!err) {<br>
> + success = set_clock_values(pwm_typen_<wbr>freq, period, TYPEN, base);<br>
> + if (!success)<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + err = of_property_read_u32(np, "pwm_typeo_period", &period);<br>
> + if (!err) {<br>
> + success = set_clock_values(pwm_typeo_<wbr>freq, period, TYPEO, base);<br>
> + if (!success)<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + for_each_child_of_node(np, child) {<br>
> + aspeed_create_pwm_port(&pdev-><wbr>dev,<br>
> + child, base);<br>
> + of_node_put(child);<br>
> + }<br>
> + of_node_put(np);<br>
> +<br>
> + return 0;<br>
> +}<br>
</div></div></blockquote></div><br></div></div>