[patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
Mykola Kostenok
c_mykolak at mellanox.com
Wed Jul 26 21:08:38 AEST 2017
Hi, Joel.
> -----Original Message-----
> From: joel.stan at gmail.com [mailto:joel.stan at gmail.com] On Behalf Of Joel
> Stanley
> Sent: Wednesday, July 26, 2017 9:48 AM
> To: Mykola Kostenok <c_mykolak at mellanox.com>
> Cc: Guenter Roeck <linux at roeck-us.net>; Jean Delvare
> <jdelvare at suse.com>; linux-hwmon at vger.kernel.org; Jaghathiswari
> Rankappagounder Natarajan <jaghu at google.com>; Ohad Oz
> <ohado at mellanox.com>; Vadim Pasternak <vadimp at mellanox.com>;
> Patrick Venture <venture at google.com>; OpenBMC Maillist
> <openbmc at lists.ozlabs.org>; Rob Herring <robh+dt at kernel.org>
> 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 at 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 at 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.
>
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.
More information about the openbmc
mailing list