lkml.org 
[lkml]   [2020]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 3/6] drivers: hwmon: Add the iEi WT61P803 PUZZLE HWMON driver
    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

    \
     
     \ /
      Last update: 2020-10-20 11:00    [W:3.117 / U:0.192 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site