| Message ID | 20170706142338.32732-1-c_mykolak@mellanox.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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@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 > =================== > -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Guenter Roeck [mailto:linux@roeck-us.net] > Sent: Friday, July 7, 2017 9:09 PM > To: Mykola Kostenok <c_mykolak@mellanox.com> > Cc: Jean Delvare <jdelvare@suse.com>; linux-hwmon@vger.kernel.org; > Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>; > openbmc@lists.ozlabs.org; Patrick Venture <venture@google.com>; Vadim > Pasternak <vadimp@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@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@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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/10/2017 07:36 AM, Mykola Kostenok wrote: >> -----Original Message----- >> From: Guenter Roeck [mailto:linux@roeck-us.net] >> Sent: Friday, July 7, 2017 9:09 PM >> To: Mykola Kostenok <c_mykolak@mellanox.com> >> Cc: Jean Delvare <jdelvare@suse.com>; linux-hwmon@vger.kernel.org; >> Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>; >> openbmc@lists.ozlabs.org; Patrick Venture <venture@google.com>; Vadim >> Pasternak <vadimp@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@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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: =================== -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;
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@mellanox.com> --- .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 52 ++++++-- drivers/hwmon/aspeed-pwm-tacho.c | 141 ++++++++++++++++++++- 2 files changed, 179 insertions(+), 14 deletions(-)