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