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

[Joel Stanley]

On Wed, Jul 26, 2017 at 8:54 PM, Mykola Kostenok <c_mykolak at 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.
>
> Signed-off-by: Mykola Kostenok <c_mykolak at mellanox.com>
>
> 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.
>
> v3 -> v4:
>  Pointed out by Joel Stanley:
>  - Move patch history after Signoff.
>  - Remove unnecessary type cast.
>  - Use array instead of pointers for colling_levels.
>  - Rename num_level to num_levels.
>  - Use local variable to make function easier to read.
>  - Drop unnesesary sizeof(u8).
>  - Use IS_ERR instead of PTR_ERR_OR_ZERO.

Changes look good!

> +static int aspeed_create_pwm_cooling(struct device *dev,
> +                                    struct device_node *child,
> +                                    struct aspeed_pwm_tacho_data *priv,
> +                                    u32 pwm_port, u8 num_levels)
> +{
> +       int ret;
> +       struct aspeed_cooling_device *cdev;
> +
> +       cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> +
> +       if (!cdev)
> +               return -ENOMEM;
> +
> +       cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);
> +       if (!cdev->cooling_levels)
> +               return -ENOMEM;
> +
> +       cdev->max_state = num_levels - 1;
> +       ret = of_property_read_u8_array(child, "cooling-levels",
> +                                       cdev->cooling_levels,
> +                                       num_levels);
> +       if (ret) {
> +               dev_err(dev, "Property 'cooling-levels' cannot be read.\n");
> +               return ret;
> +       }
> +       sprintf(cdev->name, "%s%d", child->name, pwm_port);

Oops, I missed this one. It looks like if child->name is too long then
this will overflow name[16]. You could dynamically allocate
cdev->name[16], or use snprintf.

Once you've fixed that I will add my reviewed-by.

Cheers,

Joel