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

[Luka Kovacic]

Hello Andy,

On Fri, Oct 23, 2020 at 11:47 PM Luka Kovacic <luka.kovacic@xxxxxxxxxx> wrote:
>
> Hi Andy,
>
> On Tue, Oct 20, 2020 at 10:59 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> >
> > On Tue, Oct 20, 2020 at 1:19 AM Luka Kovacic <luka.kovacic@xxxxxxxxxx> 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.

I sent out a new patchset that fixes all of the mentioned points,
except this one.
I'd like to keep the assignments, which aren't changed later in the
code assigned,
when the variable is defined.

Kind regards,
Luka

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