All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
@ 2017-07-25 14:17 Mykola Kostenok
  2017-07-26  6:48   ` Joel Stanley
  0 siblings, 1 reply; 9+ messages in thread
From: Mykola Kostenok @ 2017-07-25 14:17 UTC (permalink / raw)
  To: Guenter Roeck, Jean Delvare, linux-hwmon
  Cc: Mykola Kostenok, Rob Herring,
	Jaghathiswari Rankappagounder Natarajan, openbmc,
	Patrick Venture, Vadim Pasternak, Ohad Oz

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(-)

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;
-- 
2.8.4


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
  2017-07-25 14:17 [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support Mykola Kostenok
@ 2017-07-26  6:48   ` Joel Stanley
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2017-07-26  6:48 UTC (permalink / raw)
  To: Mykola Kostenok
  Cc: Guenter Roeck, Jean Delvare, linux-hwmon,
	Jaghathiswari Rankappagounder Natarajan, Ohad Oz,
	Vadim Pasternak, Patrick Venture, OpenBMC Maillist, Rob Herring

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
@ 2017-07-26  6:48   ` Joel Stanley
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2017-07-26  6:48 UTC (permalink / raw)
  To: Mykola Kostenok
  Cc: Guenter Roeck, Jean Delvare, linux-hwmon,
	Jaghathiswari Rankappagounder Natarajan, Ohad Oz,
	Vadim Pasternak, Patrick Venture, OpenBMC Maillist, Rob Herring

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
  2017-07-26  6:48   ` Joel Stanley
  (?)
@ 2017-07-26 11:08   ` Mykola Kostenok
  2017-07-27  6:01       ` Andrew Jeffery
  -1 siblings, 1 reply; 9+ messages in thread
From: Mykola Kostenok @ 2017-07-26 11:08 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Guenter Roeck, Jean Delvare, linux-hwmon,
	Jaghathiswari Rankappagounder Natarajan, Ohad Oz,
	Vadim Pasternak, Patrick Venture, OpenBMC Maillist, Rob Herring

Hi, Joel.

> -----Original Message-----
> From: joel.stan@gmail.com [mailto:joel.stan@gmail.com] On Behalf Of Joel
> Stanley
> Sent: Wednesday, July 26, 2017 9:48 AM
> To: Mykola Kostenok <c_mykolak@mellanox.com>
> Cc: Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> <jdelvare@suse.com>; linux-hwmon@vger.kernel.org; Jaghathiswari
> Rankappagounder Natarajan <jaghu@google.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>
> 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@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.
> 

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
  2017-07-26 11:08   ` Mykola Kostenok
@ 2017-07-27  6:01       ` Andrew Jeffery
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2017-07-27  6:01 UTC (permalink / raw)
  To: Mykola Kostenok, Joel Stanley
  Cc: linux-hwmon, Jaghathiswari Rankappagounder Natarajan,
	Jean Delvare, Ohad Oz, Vadim Pasternak, Patrick Venture,
	OpenBMC Maillist, Rob Herring, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
@ 2017-07-27  6:01       ` Andrew Jeffery
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2017-07-27  6:01 UTC (permalink / raw)
  To: Mykola Kostenok, Joel Stanley
  Cc: linux-hwmon, Jaghathiswari Rankappagounder Natarajan,
	Jean Delvare, Ohad Oz, Vadim Pasternak, Patrick Venture,
	OpenBMC Maillist, Rob Herring, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
  2017-07-27  6:01       ` Andrew Jeffery
  (?)
@ 2017-07-27  8:36       ` Mykola Kostenok
  2017-07-27  8:49           ` Andrew Jeffery
  -1 siblings, 1 reply; 9+ messages in thread
From: Mykola Kostenok @ 2017-07-27  8:36 UTC (permalink / raw)
  To: Andrew Jeffery, Joel Stanley
  Cc: linux-hwmon, Jaghathiswari Rankappagounder Natarajan,
	Jean Delvare, Ohad Oz, Vadim Pasternak, Patrick Venture,
	OpenBMC Maillist, Rob Herring, Guenter Roeck

> -----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.

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
  2017-07-27  8:36       ` Mykola Kostenok
@ 2017-07-27  8:49           ` Andrew Jeffery
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2017-07-27  8:49 UTC (permalink / raw)
  To: Mykola Kostenok, Joel Stanley
  Cc: linux-hwmon, Jaghathiswari Rankappagounder Natarajan,
	Jean Delvare, Ohad Oz, Vadim Pasternak, Patrick Venture,
	OpenBMC Maillist, Rob Herring, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 7057 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
@ 2017-07-27  8:49           ` Andrew Jeffery
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jeffery @ 2017-07-27  8:49 UTC (permalink / raw)
  To: Mykola Kostenok, Joel Stanley
  Cc: linux-hwmon, Jaghathiswari Rankappagounder Natarajan,
	Jean Delvare, Ohad Oz, Vadim Pasternak, Patrick Venture,
	OpenBMC Maillist, Rob Herring, Guenter Roeck

[-- Attachment #1: Type: text/plain, Size: 7057 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2017-07-27  8:49 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-25 14:17 [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support Mykola Kostenok
2017-07-26  6:48 ` Joel Stanley
2017-07-26  6:48   ` Joel Stanley
2017-07-26 11:08   ` Mykola Kostenok
2017-07-27  6:01     ` Andrew Jeffery
2017-07-27  6:01       ` Andrew Jeffery
2017-07-27  8:36       ` Mykola Kostenok
2017-07-27  8:49         ` Andrew Jeffery
2017-07-27  8:49           ` Andrew Jeffery

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.