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

[Luka Kovacic <luka.kovacic@sartura.hr>]

Hi Andy,

On Tue, Oct 20, 2020 at 10:59 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> 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.

Okay, I'll drop NUM.

>
> > +};
>
> ...
>
> > +       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.

Assignments, where the array content will not be modified with custom
values are done as above.
Although I could change these to the other form, if that makes it clearer.

> Also, why 4?

1 additional character is always required, as this array is passed by reference
to the iei_wt61p803_puzzle_write_command() function, which requires it to
store a calculated checksum of the array content.

This is done to avoid unnecessary copying of the array inside the MFD driver.

The checksum is a part of the command, so the driver and the MCU can check
the integrity of the sent data.

>
> > +       size_t reply_size = 0;
>
> How is it used in all these functions?

I will add an additional check for the size of the received reply, as
it should be fixed.

>
> > +       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)

I agree, this can be converted to devm_kmalloc_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

I'll fix the issues you have mentioned above in the next patchset.

Kind regards,
Luka