Hi, Joel. > -----Original Message----- > From: joel.stan@xxxxxxxxx [mailto:joel.stan@xxxxxxxxx] On Behalf Of Joel > Stanley > Sent: Wednesday, July 26, 2017 9:48 AM > To: Mykola Kostenok <c_mykolak@xxxxxxxxxxxx> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; Jean Delvare > <jdelvare@xxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx; Jaghathiswari > Rankappagounder Natarajan <jaghu@xxxxxxxxxx>; Ohad Oz > <ohado@xxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; > Patrick Venture <venture@xxxxxxxxxx>; OpenBMC Maillist > <openbmc@xxxxxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx> > Subject: Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support. > > Hi Mykola, > > On Tue, Jul 25, 2017 at 11:47 PM, Mykola Kostenok > <c_mykolak@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > --- > > 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. > Ack. > > > 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; > Ack. > > + > > + 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] > > Ack. > > + 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, > Ack. > 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? > Ack. > > +{ > > + 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; > > Local variable it ok. But it's not pwm_port *port, it will be aspeed_cooling_device *cdev. > > + 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. > Ack. > > + 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); > Ack. > > + > > + 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 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. After this patch is accepted, we'd like to add CONFIG_OF_DYNAMIC (which requires adding of CONFIG_OF_UNITTEST) to aspeed_g5_defconfig. We need CONFIG_OF_DYNAMIC for hotplug operations. Best regards. Mykola Kostenok. ��.n��������+%������w��{.n�����{��&��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�