Re: [PATCH v6 3/6] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Luka Kovacic <luka.kovacic@sartura.hr>
Cc: "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	linux-hwmon@vger.kernel.org,
	"Linux LED Subsystem" <linux-leds@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	"Lee Jones" <lee.jones@linaro.org>, "Pavel Machek" <pavel@ucw.cz>,
	"Dan Murphy" <dmurphy@ti.com>, "Rob Herring" <robh+dt@kernel.org>,
	"Jean Delvare" <jdelvare@suse.com>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Marek Behún" <marek.behun@nic.cz>,
	luka.perkov@sartura.hr, robert.marko@sartura.hr
Subject: Re: [PATCH v6 3/6] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
Date: Tue, 20 Oct 2020 12:00:47 +0300	[thread overview]
Message-ID: <CAHp75VdnOJKwvZUOsj3bbT8tK9uZN=NufDrdhUvS886bNUpWhA@mail.gmail.com> (raw)
In-Reply-To: <20201019221859.56680-4-luka.kovacic@sartura.hr>

On Tue, Oct 20, 2020 at 1:19 AM Luka Kovacic <luka.kovacic@sartura.hr> wrote:
>
> Add the iEi WT61P803 PUZZLE HWMON driver, that handles the fan speed
> control via PWM, reading fan speed and reading on-board temperature
> sensors.
>
> The driver registers a HWMON device and a simple thermal cooling device to
> enable in-kernel fan management.
>
> This driver depends on the iEi WT61P803 PUZZLE MFD driver.

...

> +// SPDX-License-Identifier: GPL-2.0-only
> +/* iEi WT61P803 PUZZLE MCU HWMON Driver

Shouldn't be
/*
 * IEI ...

?

...

> +/**
> + * struct iei_wt61p803_puzzle_thermal_cooling_device - Thermal cooling device instance

> + *

Please, remove all these blank lines in kernel doc descriptions.

> + * @mcu_hwmon:         MCU HWMON struct pointer
> + * @tcdev:             Thermal cooling device pointer
> + * @name:              Thermal cooling device name
> + * @pwm_channel:       PWM channel (0 or 1)
> + * @cooling_levels:    Thermal cooling device cooling levels
> + */

...

> +struct iei_wt61p803_puzzle_hwmon {
> +       struct iei_wt61p803_puzzle *mcu;
> +       unsigned char *response_buffer;
> +       bool thermal_cooling_dev_present[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM];
> +       struct iei_wt61p803_puzzle_thermal_cooling_device
> +               *cdev[IEI_WT61P803_PUZZLE_HWMON_MAX_PWM_NUM];

Isn't this constant a bit too long? Perhaps drop NUM (MAX would
suffice I think) for a starter.

> +};

...

> +       static unsigned char temp_sensor_ntc_cmd[4] = {
> +               IEI_WT61P803_PUZZLE_CMD_HEADER_START,
> +               IEI_WT61P803_PUZZLE_CMD_TEMP,
> +               IEI_WT61P803_PUZZLE_CMD_TEMP_ALL

+ comma.

> +       };

Why not to be consistent with the rest assignments, choose either
above form, or like you have done in the below functions.
Also, why 4?

> +       size_t reply_size = 0;

How is it used in all these functions?

> +       int ret;
> +
> +       ret = iei_wt61p803_puzzle_write_command(mcu_hwmon->mcu, temp_sensor_ntc_cmd,
> +                                               sizeof(temp_sensor_ntc_cmd), resp_buf,
> +                                               &reply_size);
> +
> +       if (ret)
> +               return ret;
> +
> +       /* Check the number of NTC values (should be 0x32/'2') */

> +       if (resp_buf[3] != 0x32)

Instead of comment in the parentheses, just do it here
" != '2')"

> +               return -EIO;

...

> +static int iei_wt61p803_puzzle_read(struct device *dev, enum hwmon_sensor_types type,
> +                                   u32 attr, int channel, long *val)
> +{
> +       struct iei_wt61p803_puzzle_hwmon *mcu_hwmon = dev_get_drvdata(dev->parent);
> +       int ret;
> +
> +       switch (type) {
> +       case hwmon_pwm:

> +               ret = iei_wt61p803_puzzle_read_pwm_channel(mcu_hwmon, channel, val);
> +               return ret;

  return iei_...(...);
in all such cases.

> +       case hwmon_fan:
> +               ret = iei_wt61p803_puzzle_read_fan_speed(mcu_hwmon, channel, val);
> +               return ret;
> +       case hwmon_temp:
> +               ret = iei_wt61p803_puzzle_read_temp_sensor(mcu_hwmon, channel, val);
> +               return ret;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static umode_t iei_wt61p803_puzzle_is_visible(const void *data, enum hwmon_sensor_types type,
> +                                             u32 attr, int channel)
> +{
> +       switch (type) {
> +       case hwmon_pwm:

> +               switch (attr) {
> +               case hwmon_pwm_input:
> +                       return 0644;
> +               default:
> +                       return 0;
> +               }

Isn't too long for
  if (attr == ...)
    return 0644;
  break;

...see below...

> +       case hwmon_fan:
> +               switch (attr) {
> +               case hwmon_fan_input:
> +                       return 0444;
> +               default:
> +                       return 0;
> +               }
> +       case hwmon_temp:
> +               switch (attr) {
> +               case hwmon_temp_input:
> +                       return 0444;
> +               default:
> +                       return 0;
> +               }

> +       default:
> +               return 0;

break;

> +       }

return 0;

?

> +}

...

> +       mcu_hwmon->thermal_cooling_dev_present[pwm_channel] = true;
> +

> +       num_levels = fwnode_property_read_u8_array(child, "cooling-levels", NULL, 0);

fwnode_property_count_u8()

> +       if (num_levels > 0) {

You can improve readability by reducing indentation level via
replacement to negative conditional.

> +               cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> +               if (!cdev)
> +                       return -ENOMEM;
> +
> +               cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);

For the sake of cleaness, shouldn't it be devm_kmalloc_array() ?
(Note, zeroing is not needed if you read entire array)

> +               if (!cdev->cooling_levels)
> +                       return -ENOMEM;
> +
> +               ret = fwnode_property_read_u8_array(child, "cooling-levels",
> +                                                   cdev->cooling_levels,
> +                                                   num_levels);
> +               if (ret) {
> +                       dev_err(dev, "Couldn't read property 'cooling-levels'");
> +                       return ret;
> +               }
> +
> +               snprintf(cdev->name, THERMAL_NAME_LENGTH, "iei_wt61p803_puzzle_%d", pwm_channel);
> +
> +               cdev->tcdev = devm_thermal_of_cooling_device_register(dev, NULL,
> +                               cdev->name, cdev, &iei_wt61p803_puzzle_cooling_ops);
> +               if (IS_ERR(cdev->tcdev))
> +                       return PTR_ERR(cdev->tcdev);
> +
> +               cdev->mcu_hwmon = mcu_hwmon;
> +               cdev->pwm_channel = pwm_channel;
> +
> +               mcu_hwmon->cdev[pwm_channel] = cdev;
> +       }
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko