[patch v1] hwmon: (aspeed-pwm-tacho) cooling device support.
Guenter Roeck
linux at roeck-us.net
Tue Jul 11 13:30:39 AEST 2017
On 07/10/2017 07:36 AM, Mykola Kostenok wrote:
>> -----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.
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 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