| Message ID | 20170725141727.13105-1-c_mykolak@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Mykola, On Tue, Jul 25, 2017 at 11:47 PM, Mykola Kostenok <c_mykolak@mellanox.com> wrote: > Add support in aspeed-pwm-tacho driver for cooling device creation. > This cooling device could be bound to a thermal zone > for the thermal control. Device will appear in /sys/class/thermal > folder as cooling_deviceX. Then it could be bound to particular > thermal zones. Allow specification of the cooling levels > vector - PWM duty cycle values in a range from 0 to 255 > which correspond to thermal cooling states. > > v1 -> v2: > - Remove device tree binding file from the patch. > - Move of_node_put out of cycle because of_get_next_child > already do of_put_node on previous child. > > v2 -> v3: > Pointed out by Rob Herring: > - Put cooling-levels under fan subnodes. Looking better! I have a few comments below. Some are nits (small issues that aren't a big deal), but I chose to point them out so you learn for next time. If you have any questions then please ask. > > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> > --- Convention is to stick the changes between versions (the v2-> v3 bits) down here, so that it doesn't end up in the final commit message. > drivers/hwmon/aspeed-pwm-tacho.c | 118 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 116 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c > index ddfe66b..ae8dfee 100644 > --- a/drivers/hwmon/aspeed-pwm-tacho.c > +++ b/drivers/hwmon/aspeed-pwm-tacho.c > @@ -20,6 +20,7 @@ > #include <linux/platform_device.h> > #include <linux/sysfs.h> > #include <linux/regmap.h> > +#include <linux/thermal.h> > > /* ASPEED PWM & FAN Tach Register Definition */ > #define ASPEED_PTCR_CTRL 0x00 > @@ -166,6 +167,16 @@ > /* How long we sleep in us while waiting for an RPM result. */ > #define ASPEED_RPM_STATUS_SLEEP_USEC 500 > > +struct aspeed_cooling_device { > + char name[16]; > + struct aspeed_pwm_tacho_data *priv; > + struct thermal_cooling_device *tcdev; > + int pwm_port; > + u8 *cooling_levels; > + u8 max_state; > + u8 cur_state; > +}; > + > struct aspeed_pwm_tacho_data { > struct regmap *regmap; > unsigned long clk_freq; > @@ -180,6 +191,7 @@ struct aspeed_pwm_tacho_data { > u8 pwm_port_type[8]; > u8 pwm_port_fan_ctrl[8]; > u8 fan_tach_ch_source[16]; > + struct aspeed_cooling_device *cdev[8]; > const struct attribute_group *groups[3]; > }; > > @@ -765,6 +777,97 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv, > } > } > > +static int > +aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev, > + unsigned long *state) > +{ > + struct aspeed_cooling_device *cdev = > + (struct aspeed_cooling_device *)tcdev->devdata; > + > + *state = cdev->max_state; > + > + return 0; > +} > + > +static int > +aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev, > + unsigned long *state) > +{ > + struct aspeed_cooling_device *cdev = > + (struct aspeed_cooling_device *)tcdev->devdata; > + > + *state = cdev->cur_state; > + > + return 0; > +} > + > +static int > +aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev, > + unsigned long state) > +{ > + struct aspeed_cooling_device *cdev = > + (struct aspeed_cooling_device *)tcdev->devdata; You can drop the cast, as devdata is a void *: struct aspeed_cooling_device *cdev = tcdev->devdata; > + > + if (state > cdev->max_state) > + return -EINVAL; > + > + cdev->cur_state = state; > + cdev->priv->pwm_port_fan_ctrl[cdev->pwm_port] = *(cdev->cooling_levels > + + cdev->cur_state); That pointer maths looks scary :) How about this instead? cdev->priv->pwm_port_fan_ctrl[cdev->pwm_port] = cdev->cooling_levels[cdev->cur_state] > + aspeed_set_pwm_port_fan_ctrl(cdev->priv, cdev->pwm_port, > + *(cdev->cooling_levels + > + cdev->cur_state)); Same here: aspeed_set_pwm_port_fan_ctrl(cdev->priv, cdev->pwm_port, cdev->cooling_levels[cdev->cur_state]); > + > + return 0; > +} > + > +static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = { > + .get_max_state = aspeed_pwm_cz_get_max_state, > + .get_cur_state = aspeed_pwm_cz_get_cur_state, > + .set_cur_state = aspeed_pwm_cz_set_cur_state, > +}; > + > +static int aspeed_create_pwm_cooling(struct device *dev, > + struct device_node *child, > + struct aspeed_pwm_tacho_data *priv, > + u32 pwm_port, u8 num_level) Can we call this num_levels instead of num_level? > +{ > + int ret; > + > + priv->cdev[pwm_port] = devm_kzalloc(dev, sizeof(*priv->cdev[pwm_port]), > + GFP_KERNEL); This function would be easier to read if we used a local variable: struct pwm_port *port; port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL); Then at the bottom of the function, do priv->cdev[pwm_port] = port; > + if (!priv->cdev[pwm_port]) > + return -ENOMEM; > + > + priv->cdev[pwm_port]->cooling_levels = devm_kzalloc(dev, num_level * The sizeof(8) isn't useful, you could drop it. > + sizeof(u8), > + GFP_KERNEL); > + if (!priv->cdev[pwm_port]->cooling_levels) > + return -ENOMEM; > + > + priv->cdev[pwm_port]->max_state = num_level - 1; > + ret = of_property_read_u8_array(child, "cooling-levels", > + priv->cdev[pwm_port]->cooling_levels, > + num_level); > + if (ret) { > + dev_err(dev, "Property 'cooling-levels' cannot be read.\n"); > + return ret; > + } > + sprintf(priv->cdev[pwm_port]->name, "%s%d", child->name, pwm_port); > + > + priv->cdev[pwm_port]->tcdev = thermal_of_cooling_device_register(child, > + priv->cdev[pwm_port]->name, > + priv->cdev[pwm_port], > + &aspeed_pwm_cool_ops); > + if (PTR_ERR_OR_ZERO(priv->cdev[pwm_port]->tcdev)) > + return PTR_ERR(priv->cdev[pwm_port]->tcdev); I think you meant this: if (IS_ERR(priv->cdev[pwm_port]->tcdev)) return PTR_ERR(priv->cdev[pwm_port]->tcdev); > + > + priv->cdev[pwm_port]->priv = priv; > + priv->cdev[pwm_port]->pwm_port = pwm_port; > + > + return 0; > +} > + > static int aspeed_create_fan(struct device *dev, > struct device_node *child, > struct aspeed_pwm_tacho_data *priv) > @@ -778,6 +881,15 @@ static int aspeed_create_fan(struct device *dev, > return ret; > aspeed_create_pwm_port(priv, (u8)pwm_port); > > + ret = of_property_count_u8_elems(child, "cooling-levels"); > + > + if (ret > 0) { > + ret = aspeed_create_pwm_cooling(dev, child, priv, pwm_port, > + ret); > + if (ret) > + return ret; > + } > + > count = of_property_count_u8_elems(child, "aspeed,fan-tach-ch"); > if (count < 1) > return -EINVAL; > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev) > > for_each_child_of_node(np, child) { > ret = aspeed_create_fan(dev, child, priv); > - of_node_put(child); > - if (ret) > + if (ret) { > + of_node_put(child); > return ret; > + } > } > + of_node_put(child); I'm not sure about these. Cheers, Joel -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mykola, I know you've sent out subsequent versions, but I wanted to address one of your arguments here: On Wed, 2017-07-26 at 11:08 +0000, Mykola Kostenok wrote: > > > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct > > > platform_device *pdev) > > > > > > for_each_child_of_node(np, child) { > > > ret = aspeed_create_fan(dev, child, priv); > > > - of_node_put(child); > > > - if (ret) > > > + if (ret) { > > > + of_node_put(child); > > > return ret; > > > + } > > > } > > > + of_node_put(child); > > > > I'm not sure about these. > > > > Cheers, > > > > Joel > > If CONFIG_OF_UNITTEST is set, system initialization fails on this of_node_put. > I checked and found that for_each_child_of_node is macro witch use __of_get_next_child > in cycle. __of_get_next_child do of_node_put previous child but not last. > > static struct device_node *__of_get_next_child(const struct device_node *node, > struct device_node *prev) > { > struct device_node *next; > > if (!node) > return NULL; > > next = prev ? prev->sibling : node->child; > for (; next; next = next->sibling) > if (of_node_get(next)) > break; > of_node_put(prev); > return next; > } > #define __for_each_child_of_node(parent, child) \ > for (child = __of_get_next_child(parent, NULL); child != NULL; \ > child = __of_get_next_child(parent, child)) > > So inside cycle we should not use of_node_put on each child. We must use it only on last child in cycle. I was just looking at this myself for a different driver, and I don't think this argument is right. The natural terminating condition of the loop is child == NULL. child == NULL occurs if we have zero-or-more- children; the child is always initialised as part of the loop header and would be NULL if there are no children. If we have more than one child, the reference to the last valid child is passed to of_node_put() in __of_get_next_child() in order to return the terminating NULL. Given __of_get_next_child() is passed the last node and the result is NULL, the call to of_node_put() after the loop is always invoked on NULL, which performs an early return. As such I think it is unnecessary. Cheers, Andrew
On Thu, 2017-07-27 at 08:36 +0000, Mykola Kostenok wrote: > > -----Original Message----- > > > > From: Andrew Jeffery [mailto:andrew@aj.id.au] > > Sent: Thursday, July 27, 2017 9:01 AM > > > > To: Mykola Kostenok <c_mykolak@mellanox.com>; Joel Stanley > > > > <joel@jms.id.au> > > > > Cc: linux-hwmon@vger.kernel.org; Jaghathiswari Rankappagounder > > > > > > Natarajan <jaghu@google.com>; Jean Delvare <jdelvare@suse.com>; Ohad > > > > > > Oz <ohado@mellanox.com>; Vadim Pasternak <vadimp@mellanox.com>; > > > > Patrick Venture <venture@google.com>; OpenBMC Maillist > > > > > > <openbmc@lists.ozlabs.org>; Rob Herring <robh+dt@kernel.org>; Guenter > > > > Roeck <linux@roeck-us.net> > > Subject: Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support. > > > > Hi Mykola, > > > > I know you've sent out subsequent versions, but I wanted to address one of > > your arguments here: > > > > On Wed, 2017-07-26 at 11:08 +0000, Mykola Kostenok wrote: > > > > > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct > > > > > platform_device *pdev) > > > > > > > > > > for_each_child_of_node(np, child) { > > > > > ret = aspeed_create_fan(dev, child, priv); > > > > > - of_node_put(child); > > > > > - if (ret) > > > > > + if (ret) { > > > > > + of_node_put(child); > > > > > return ret; > > > > > + } > > > > > } > > > > > + of_node_put(child); > > > > > > > > I'm not sure about these. > > > > > > > > Cheers, > > > > > > > > Joel > > > > > > If CONFIG_OF_UNITTEST is set, system initialization fails on this > > > > of_node_put. > > > I checked and found that for_each_child_of_node is macro witch use > > > __of_get_next_child > > > in cycle. __of_get_next_child do of_node_put previous child but not last. > > > > > > static struct device_node *__of_get_next_child(const struct > > > device_node *node, > > > struct device_node > > > *prev) { > > > struct device_node *next; > > > > > > if (!node) > > > return NULL; > > > > > > next = prev ? prev->sibling : node->child; > > > for (; next; next = next->sibling) > > > if (of_node_get(next)) > > > break; > > > of_node_put(prev); > > > return next; > > > } > > > #define __for_each_child_of_node(parent, child) \ > > > for (child = __of_get_next_child(parent, NULL); child != NULL; > > > \ > > > child = __of_get_next_child(parent, child)) > > > > > > So inside cycle we should not use of_node_put on each child. We must use > > > > it only on last child in cycle. > > > > I was just looking at this myself for a different driver, and I don't think this > > argument is right. The natural terminating condition of the loop is child == > > NULL. child == NULL occurs if we have zero-or-more- children; the child is > > always initialised as part of the loop header and would be NULL if there are > > no children. If we have more than one child, the reference to the last valid > > child is passed to of_node_put() in __of_get_next_child() in order to return > > the terminating NULL. Given > > __of_get_next_child() is passed the last node and the result is NULL, the call > > to of_node_put() after the loop is always invoked on NULL, which performs > > an early return. > > > > As such I think it is unnecessary. > > > > Cheers, > > > > Andrew > > Ok, I agree that we don’t need of_node_put after loop. > We must do of_node_put only in case of error. Yep, this is the right approach as far as I can see. Thanks, Andrew > > So I will send next patch v6 with this: > for_each_child_of_node(np, child) { > ret = aspeed_create_fan(dev, child, priv); > - of_node_put(child); > - if (ret) > + if (ret) { > + of_node_put(child); > return ret; > + } > } > > Without it and with CONFIG_OF_UNITTEST we see this: > [ 3.000000] [<80010080>] (unwind_backtrace) from [<8000d934>] (show_stack+0x20/0x24) > [ 3.000000] [<8000d934>] (show_stack) from [<801dbf8c>] (dump_stack+0x20/0x28) > [ 3.000000] [<801dbf8c>] (dump_stack) from [<8030ad14>] (of_node_release+0x98/0xa0) > [ 3.000000] [<8030ad14>] (of_node_release) from [<801de0ec>] (kobject_put+0xf8/0x1ec) > [ 3.000000] [<801de0ec>] (kobject_put) from [<8030a340>] (of_node_put+0x24/0x28) > [ 3.000000] [<8030a340>] (of_node_put) from [<80305fe4>] (__of_get_next_child+0x58/0x70) > [ 3.000000] [<80305fe4>] (__of_get_next_child) from [<8030668c>] (of_get_next_child+0x20/0x28) > [ 3.000000] [<8030668c>] (of_get_next_child) from [<802e39ac>] (aspeed_pwm_tacho_probe+0x490/0x574) > [ 3.000000] [<802e39ac>] (aspeed_pwm_tacho_probe) from [<80244090>] (platform_drv_probe+0x60/0xc0) > [ 3.000000] [<80244090>] (platform_drv_probe) from [<80242408>] (driver_probe_device+0x280/0x44c) > [ 3.000000] [<80242408>] (driver_probe_device) from [<802426c4>] (__driver_attach+0xf0/0x104) > [ 3.000000] [<802426c4>] (__driver_attach) from [<802403d8>] (bus_for_each_dev+0x7c/0xb0) > [ 3.000000] [<802403d8>] (bus_for_each_dev) from [<8024286c>] (driver_attach+0x28/0x30) > [ 3.000000] [<8024286c>] (driver_attach) from [<80240e38>] (bus_add_driver+0x14c/0x268) > [ 3.000000] [<80240e38>] (bus_add_driver) from [<802432f8>] (driver_register+0x88/0x104) > [ 3.000000] [<802432f8>] (driver_register) from [<80244cd0>] (__platform_driver_register+0x40/0x54) > [ 3.000000] [<80244cd0>] (__platform_driver_register) from [<805bfc64>] (aspeed_pwm_tacho_driver_init+0x18/0x20) > [ 3.000000] [<805bfc64>] (aspeed_pwm_tacho_driver_init) from [<8059de70>] (do_one_initcall+0xac/0x168) > [ 3.000000] [<8059de70>] (do_one_initcall) from [<8059e040>] (kernel_init_freeable+0x114/0x1cc) > [ 3.000000] [<8059e040>] (kernel_init_freeable) from [<804072a0>] (kernel_init+0x18/0x104) > [ 3.000000] [<804072a0>] (kernel_init) from [<8000a5e8>] (ret_from_fork+0x14/0x2c) > [ 3.200000] OF: ERROR: Bad of_node_put() on /ahb/apb/pwm-tacho-controller@1e786000/fan@1 > And kernel panic at the end. > > Best regards. Mykola Kostenok.
diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c index ddfe66b..ae8dfee 100644 --- a/drivers/hwmon/aspeed-pwm-tacho.c +++ b/drivers/hwmon/aspeed-pwm-tacho.c @@ -20,6 +20,7 @@ #include <linux/platform_device.h> #include <linux/sysfs.h> #include <linux/regmap.h> +#include <linux/thermal.h> /* ASPEED PWM & FAN Tach Register Definition */ #define ASPEED_PTCR_CTRL 0x00 @@ -166,6 +167,16 @@ /* How long we sleep in us while waiting for an RPM result. */ #define ASPEED_RPM_STATUS_SLEEP_USEC 500 +struct aspeed_cooling_device { + char name[16]; + struct aspeed_pwm_tacho_data *priv; + struct thermal_cooling_device *tcdev; + int pwm_port; + u8 *cooling_levels; + u8 max_state; + u8 cur_state; +}; + struct aspeed_pwm_tacho_data { struct regmap *regmap; unsigned long clk_freq; @@ -180,6 +191,7 @@ struct aspeed_pwm_tacho_data { u8 pwm_port_type[8]; u8 pwm_port_fan_ctrl[8]; u8 fan_tach_ch_source[16]; + struct aspeed_cooling_device *cdev[8]; const struct attribute_group *groups[3]; }; @@ -765,6 +777,97 @@ static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv, } } +static int +aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev, + unsigned long *state) +{ + struct aspeed_cooling_device *cdev = + (struct aspeed_cooling_device *)tcdev->devdata; + + *state = cdev->max_state; + + return 0; +} + +static int +aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev, + unsigned long *state) +{ + struct aspeed_cooling_device *cdev = + (struct aspeed_cooling_device *)tcdev->devdata; + + *state = cdev->cur_state; + + return 0; +} + +static int +aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev, + unsigned long state) +{ + struct aspeed_cooling_device *cdev = + (struct aspeed_cooling_device *)tcdev->devdata; + + if (state > cdev->max_state) + return -EINVAL; + + cdev->cur_state = state; + cdev->priv->pwm_port_fan_ctrl[cdev->pwm_port] = *(cdev->cooling_levels + + cdev->cur_state); + aspeed_set_pwm_port_fan_ctrl(cdev->priv, cdev->pwm_port, + *(cdev->cooling_levels + + cdev->cur_state)); + + return 0; +} + +static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = { + .get_max_state = aspeed_pwm_cz_get_max_state, + .get_cur_state = aspeed_pwm_cz_get_cur_state, + .set_cur_state = aspeed_pwm_cz_set_cur_state, +}; + +static int aspeed_create_pwm_cooling(struct device *dev, + struct device_node *child, + struct aspeed_pwm_tacho_data *priv, + u32 pwm_port, u8 num_level) +{ + int ret; + + priv->cdev[pwm_port] = devm_kzalloc(dev, sizeof(*priv->cdev[pwm_port]), + GFP_KERNEL); + if (!priv->cdev[pwm_port]) + return -ENOMEM; + + priv->cdev[pwm_port]->cooling_levels = devm_kzalloc(dev, num_level * + sizeof(u8), + GFP_KERNEL); + if (!priv->cdev[pwm_port]->cooling_levels) + return -ENOMEM; + + priv->cdev[pwm_port]->max_state = num_level - 1; + ret = of_property_read_u8_array(child, "cooling-levels", + priv->cdev[pwm_port]->cooling_levels, + num_level); + if (ret) { + dev_err(dev, "Property 'cooling-levels' cannot be read.\n"); + return ret; + } + sprintf(priv->cdev[pwm_port]->name, "%s%d", child->name, pwm_port); + + priv->cdev[pwm_port]->tcdev = thermal_of_cooling_device_register(child, + priv->cdev[pwm_port]->name, + priv->cdev[pwm_port], + &aspeed_pwm_cool_ops); + if (PTR_ERR_OR_ZERO(priv->cdev[pwm_port]->tcdev)) + return PTR_ERR(priv->cdev[pwm_port]->tcdev); + + priv->cdev[pwm_port]->priv = priv; + priv->cdev[pwm_port]->pwm_port = pwm_port; + + return 0; +} + static int aspeed_create_fan(struct device *dev, struct device_node *child, struct aspeed_pwm_tacho_data *priv) @@ -778,6 +881,15 @@ static int aspeed_create_fan(struct device *dev, return ret; aspeed_create_pwm_port(priv, (u8)pwm_port); + ret = of_property_count_u8_elems(child, "cooling-levels"); + + if (ret > 0) { + ret = aspeed_create_pwm_cooling(dev, child, priv, pwm_port, + ret); + if (ret) + return ret; + } + count = of_property_count_u8_elems(child, "aspeed,fan-tach-ch"); if (count < 1) return -EINVAL; @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct platform_device *pdev) for_each_child_of_node(np, child) { ret = aspeed_create_fan(dev, child, priv); - of_node_put(child); - if (ret) + if (ret) { + of_node_put(child); return ret; + } } + of_node_put(child); priv->groups[0] = &pwm_dev_group; priv->groups[1] = &fan_dev_group;
Add support in aspeed-pwm-tacho driver for cooling device creation. This cooling device could be bound to a thermal zone for the thermal control. Device will appear in /sys/class/thermal folder as cooling_deviceX. Then it could be bound to particular thermal zones. Allow specification of the cooling levels vector - PWM duty cycle values in a range from 0 to 255 which correspond to thermal cooling states. v1 -> v2: - Remove device tree binding file from the patch. - Move of_node_put out of cycle because of_get_next_child already do of_put_node on previous child. v2 -> v3: Pointed out by Rob Herring: - Put cooling-levels under fan subnodes. Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com> --- drivers/hwmon/aspeed-pwm-tacho.c | 118 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 116 insertions(+), 2 deletions(-)