[patch v1] hwmon: (aspeed-pwm-tacho) cooling device support.

Mykola Kostenok c_mykolak at mellanox.com
Tue Jul 11 00:36:17 AEST 2017


> -----Original Message-----
> From: Guenter Roeck [mailto:linux at roeck-us.net]
> Sent: Friday, July 7, 2017 9:09 PM
> To: Mykola Kostenok <c_mykolak at mellanox.com>
> Cc: Jean Delvare <jdelvare at suse.com>; linux-hwmon at vger.kernel.org;
> Jaghathiswari Rankappagounder Natarajan <jaghu at google.com>;
> openbmc at lists.ozlabs.org; Patrick Venture <venture at google.com>; Vadim
> Pasternak <vadimp at mellanox.com>
> Subject: Re: [patch v1] hwmon: (aspeed-pwm-tacho) cooling device support.
> 
> On Thu, Jul 06, 2017 at 05:23:38PM +0300, Mykola Kostenok 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.
> >
> > Signed-off-by: Mykola Kostenok <c_mykolak at mellanox.com>
> > ---
> >  .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt |  52 ++++++--
> 
> You'll need devicetree maintainer approval for any devicetree changes.
> 
> >  drivers/hwmon/aspeed-pwm-tacho.c                   | 141
> ++++++++++++++++++++-
> >  2 files changed, 179 insertions(+), 14 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> > b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> > index cf44605..293a994 100644
> > --- a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-
> tacho.txt
> > @@ -23,9 +23,14 @@ Required properties for pwm-tacho node:
> >  - clocks : a fixed clock providing input clock frequency(PWM
> >  	   and Fan Tach clock)
> >
> > -fan subnode format:
> > +tach-channels subnode format:
> 
> I don't like the notion of renaming "fan subnode" to "tach-channels
> subnode". To me that is just a personal preference. Tomorrow I might get
> another patch changing it back or changing it to something else.
> 
> But, as mentioned above, you'll need DT maintaier approval for those
> changes in the first place.
> 
> Guenter
> 

Hi, Guenter.
Thank you for reply.

Actually we didn't rename fan subnode. We add one more level hierarchy
tach-channels and put fan subnodes inside it.
And also we add pwm-channels subnode with pwm subnodes inside 
for cooling device.

We found issue with of_node_put in this patch, so we want to send 
you patch v2.

Do we need send this changes in 2 separated patches, 
one with driver aspeed-pwm-tacho.c to you, and another 
with doc aspeed-pwm-tacho.txt to DT maintainer?

Best regards. Mykola Kostenok.

> >  ===================
> > -Under fan subnode there can upto 8 child nodes, with each child node
> > +Required properties for tach-channels node:
> > +- #address-cells : should be 1.
> > +
> > +- #size-cells : should be 0.
> > +
> > +Under tach-channels subnode there can be upto 8 child nodes, with
> > +each child node
> >  representing a fan. If there are 8 fans each fan can have one PWM
> > port and  one/two Fan tach inputs.
> >
> > @@ -39,6 +44,22 @@ Required properties for each child node:
> >  		Fan tach channel 0 and 15 indicating Fan tach channel 15.
> >  		Atleast one Fan tach input channel is required.
> >
> > +pwm-channels subnode format:
> > +Under pwm-channels subnode there can be pwm child nodes.
> > +Required properties for tach-channels node:
> > +- #address-cells : should be 1.
> > +
> > +- #size-cells : should be 0.
> > +
> > +- #cooling-cells : should be 2;
> > +
> > +pwm subnode format:
> > +- reg : should be <0x00>.
> > +
> > +- cooling-levels      : PWM duty cycle values in a range from 0 to 255
> > +			which correspond to thermal cooling states.
> > +
> > +
> >  Examples:
> >
> >  pwm_tacho_fixed_clk: fixedclk {
> > @@ -55,14 +76,25 @@ pwm_tacho: pwmtachocontroller at 1e786000 {
> >  	clocks = <&pwm_tacho_fixed_clk>;
> >  	pinctrl-names = "default";
> >  	pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default>;
> > -
> > -	fan at 0 {
> > -		reg = <0x00>;
> > -		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> > +	tach-channels {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		fan at 0 {
> > +			reg = <0x00>;
> > +			aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> > +		};
> > +		fan at 1 {
> > +			reg = <0x01>;
> > +			aspeed,fan-tach-ch = /bits/ 8 <0x01 0x02>;
> > +		};
> >  	};
> > -
> > -	fan at 1 {
> > -		reg = <0x01>;
> > -		aspeed,fan-tach-ch = /bits/ 8 <0x01 0x02>;
> > +	pwm-channels {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		#cooling-cells = <2>;
> > +		pwm at 0 {
> > +			reg = <0x00>;
> > +			cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> > +		};
> >  	};
> >  };
> > diff --git a/drivers/hwmon/aspeed-pwm-tacho.c
> > b/drivers/hwmon/aspeed-pwm-tacho.c
> > index ddfe66b..c10a37e 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];  };
> >
> > @@ -794,10 +806,111 @@ static int aspeed_create_fan(struct device *dev,
> >  	return 0;
> >  }
> >
> > +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;
> > +	int ret;
> > +
> > +	ret = of_property_read_u32(child, "reg", &pwm_port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = of_property_count_u8_elems(child, "cooling-levels");
> > +	if (ret <= 0) {
> > +		dev_err(dev, "Wrong cooling-levels data.\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	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, ret *
> > +							    sizeof(u8),
> > +							    GFP_KERNEL);
> > +	if (!priv->cdev[pwm_port]->cooling_levels)
> > +		return -ENOMEM;
> > +
> > +	priv->cdev[pwm_port]->max_state = ret - 1;
> > +	ret = of_property_read_u8_array(child, "cooling-levels",
> > +					priv->cdev[pwm_port]-
> >cooling_levels,
> > +					ret);
> > +	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_pwm_tacho_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> > -	struct device_node *np, *child;
> > +	struct device_node *np, *child, *grandchild;
> >  	struct aspeed_pwm_tacho_data *priv;
> >  	void __iomem *regs;
> >  	struct resource *res;
> > @@ -833,10 +946,30 @@ static int aspeed_pwm_tacho_probe(struct
> platform_device *pdev)
> >  	aspeed_create_type(priv);
> >
> >  	for_each_child_of_node(np, child) {
> > -		ret = aspeed_create_fan(dev, child, priv);
> > +		if (!of_node_cmp(child->name, "tach-channels")) {
> > +			for_each_child_of_node(child, grandchild) {
> > +				ret = aspeed_create_fan(dev, grandchild,
> priv);
> > +				of_node_put(grandchild);
> > +				if (ret) {
> > +					of_node_put(child);
> > +					return ret;
> > +				}
> > +			}
> > +		}
> > +
> > +		if (!of_node_cmp(child->name, "pwm-channels")) {
> > +			for_each_child_of_node(child, grandchild) {
> > +				ret = aspeed_create_pwm_cooling(dev,
> > +								grandchild,
> > +								priv);
> > +				of_node_put(grandchild);
> > +				if (ret) {
> > +					of_node_put(child);
> > +					return ret;
> > +				}
> > +			}
> > +		}
> >  		of_node_put(child);
> > -		if (ret)
> > -			return ret;
> >  	}
> >
> >  	priv->groups[0] = &pwm_dev_group;
> > --
> > 2.8.4
> >


More information about the openbmc mailing list