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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 07/10/2017 07:36 AM, Mykola Kostenok wrote:
-----Original Message-----
From: Guenter Roeck [mailto:linux@xxxxxxxxxxxx]
Sent: Friday, July 7, 2017 9:09 PM
To: Mykola Kostenok <c_mykolak@xxxxxxxxxxxx>
Cc: Jean Delvare <jdelvare@xxxxxxxx>; linux-hwmon@xxxxxxxxxxxxxxx;
Jaghathiswari Rankappagounder Natarajan <jaghu@xxxxxxxxxx>;
openbmc@xxxxxxxxxxxxxxxx; Patrick Venture <venture@xxxxxxxxxx>; Vadim
Pasternak <vadimp@xxxxxxxxxxxx>
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@xxxxxxxxxxxx>
---
  .../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.

Hmm. The above very much looks like a rename to me.

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?

Yes.

Guenter

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@1e786000 {
  	clocks = <&pwm_tacho_fixed_clk>;
  	pinctrl-names = "default";
  	pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default>;
-
-	fan@0 {
-		reg = <0x00>;
-		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
+	tach-channels {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		fan@0 {
+			reg = <0x00>;
+			aspeed,fan-tach-ch = /bits/ 8 <0x00>;
+		};
+		fan@1 {
+			reg = <0x01>;
+			aspeed,fan-tach-ch = /bits/ 8 <0x01 0x02>;
+		};
  	};
-
-	fan@1 {
-		reg = <0x01>;
-		aspeed,fan-tach-ch = /bits/ 8 <0x01 0x02>;
+	pwm-channels {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#cooling-cells = <2>;
+		pwm@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



--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux