Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Thomas Richard <thomas.richard@bootlin.com>]

Hello Guenter,

Thanks for the review !!

> 
> For the first loop, this essentially repeats the cgbc_hwmon_cmd() from
> above.
> Is that how it works, i.e., that index == 0 returns both the number of
> sensors
> in the first byte of return data plus the data for the first sensor ?

Yes it is.
This is based on the Congatec implementation.
First time I read the their code, I had the same reaction than you.
But it works.

Please note that the driver provided by Congatec is the only source of
information I have (there is no technical documentation which describes
the internal behavior of this Board Controller).

I'll skip the cgbc_hwmon_cmd() for the first loop and add a comment to
not forget that it is not an error.

> 
>> +        if (ret)
>> +            return ret;
>> +
>> +        type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
>> +        id = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
>> +
>> +        if (type == CGBC_HWMON_TYPE_TEMP && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
>> +            sensor->type = hwmon_temp;
>> +            sensor->label = cgbc_hwmon_labels_temp[id];
>> +        } else if (type == CGBC_HWMON_TYPE_IN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_in)) {
>> +            /*
>> +             * The Board Controller doesn't do differences between
>> current and voltage
> 
> doesn't differentiate
> 
>> +             * sensors
>> +             */
> 
> This doesn't really explain what is happening. Please add something like
> "Get the sensor type from cgbc_hwmon_labels_in[id].label instead".
> 
>> +            sensor->type = cgbc_hwmon_labels_in[id].type;
>> +            sensor->label = cgbc_hwmon_labels_in[id].label;
>> +        } else if (type == CGBC_HWMON_TYPE_FAN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
>> +            sensor->type = hwmon_fan;
>> +            sensor->label = cgbc_hwmon_labels_fan[id];
>> +        } else {
>> +            dev_warn(dev, "Board Controller returned an unknown
>> sensor (type=%d, id=%d), ignore it",
>> +                 type, id);
>> +            continue;
>> +        }
>> +
>> +        sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
>> +        sensor->index = i;
>> +        sensor++;
>> +        hwmon->nb_sensors++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct
>> cgbc_hwmon_data *hwmon,
>> +                            enum hwmon_sensor_types type, int channel)
>> +{
>> +    struct cgbc_hwmon_sensor *sensor = NULL;
>> +    int i, cnt = 0;
>> +
>> +    for (i = 0; i < hwmon->nb_sensors; i++) {
>> +        if (hwmon->sensors[i].type == type && cnt++ == channel) {
> 
> Isn't that a bit fragile ? It assumes that the nth reported sensor of a
> given type
> reflects a specific named sensor. If that is indeed the case, why bother
> with
> all the code in cgbc_hwmon_probe_sensors() ? The index to sensor
> association
> should be well defined, and the sensor type plus the channel index
> should always
> be a constant.

I'm not sure to get your comment.

I cannot assume that the sensor list returned by the Board Controller
will be the same for all boards.
I know the MFD driver only supports one board, but I think it's better
if we can have a generic hwmon driver.

If I add some debug in cgbc_hwmon_probe_sensors() I can dump the sensor
list returned by the Board Controller:

cgbc_hwmon_probe_sensors: index=0 type=1 id=5 label=Chipset Temperature
cgbc_hwmon_probe_sensors: index=1 type=7 id=0 label=CPU Fan
cgbc_hwmon_probe_sensors: index=4 type=1 id=3 label=Board Temperature
cgbc_hwmon_probe_sensors: index=5 type=2 id=1 label=DC Runtime Voltage

It is the type and the id which give the name of the sensor.

I don't see how to do it in a different way if I cannot assume that the
list above is not the same for all boards.
If I assume that the list returned by the Board Controller is always the
same for a board (which I not even sure, if for example a fan is
plugged), I could have a static list for each different boards. But the
driver will not be generic.

If I miss something, please let me know.

> 
>> +            sensor = &hwmon->sensors[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    return sensor;
>> +}
>> +
>> +static int cgbc_hwmon_read(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr, int channel,
>> +               long *val)
>> +{
>> +    struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +    struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> +    struct cgbc_device_data *cgbc = hwmon->cgbc;
>> +    u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE];
>> +    int ret;
>> +
>> +    if (!sensor)
>> +        return -ENODEV;
> 
> How would this ever happen ? Unless I am missing something, that means
> there is a bug somewhere in the code. "No such device" is definitely
> wrong here (and elsewhere). If you don't trust your code and think
> this can happen, at least return -ENODATA.

I can remove this return -ENODEV (and also the one in read_string()).
The read() and read_string() callbacks can only be called if the sensor
is created in sysfs.
And the hwmon core creates the sysfs entry only if is_visible() does not
return an error.
The function cgbc_hwmon_find_sensor() is called in is_visible() and the
returned pointer is checked.
So if read() or read_string() is called, we know that is_visible()
didn't return an error, so cgbc_hwmon_find_sensor() didn't return an error.
So this "return -ENODEV" (in read() and read_string()) is dead code.

> 
>> +
>> +    ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
>> +    if (ret)
>> +        return ret;
>> +
>> +    *val = FIELD_PREP(CGBC_HWMON_DATA_HIGH, data[3]) |
>> FIELD_PREP(CGBC_HWMON_DATA_LOW, data[2]);
>> +
> 
> That is a pretty complex way of writing
>     *val = (data[3] << 8) | data[2];
> I'd say that is close to obfuscation, but that is your call.

I agree, it's easier to read.

> 
>> +    /* For the Board Controller 1lsb = 0.1 degree centigrade */
> 
> All other units are as expected (mV, mA, rpm) ?

Yes they are, I will mention it.

> 
>> +    if (sensor->type == hwmon_temp)
>> +        *val *= 100;
>> +
>> +    return 0;
>> +}
>> +
>> +static umode_t cgbc_hwmon_is_visible(const void *_data, enum
>> hwmon_sensor_types type, u32 attr,
>> +                     int channel)
>> +{
>> +    struct cgbc_hwmon_data *data = (struct cgbc_hwmon_data *)_data;
>> +    struct cgbc_hwmon_sensor *sensor;
>> +
>> +    sensor = cgbc_hwmon_find_sensor(data, type, channel);
>> +    if (!sensor)
>> +        return 0;
>> +
>> +    return sensor->active ? 0444 : 0;
>> +}
>> +
>> +static int cgbc_hwmon_read_string(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr,
>> +                  int channel, const char **str)
>> +{
>> +    struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +    struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> +
>> +    if (!sensor)
>> +        return -ENODEV;
>> +
>> +    *str = sensor->label;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info * const cgbc_hwmon_info[] = {
>> +    HWMON_CHANNEL_INFO(temp,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL),
>> +    HWMON_CHANNEL_INFO(in,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL),
>> +    HWMON_CHANNEL_INFO(curr,
>> +               HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT |
>> HWMON_C_LABEL,
>> +               HWMON_C_INPUT | HWMON_C_LABEL),
>> +    HWMON_CHANNEL_INFO(fan,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_ops cgbc_hwmon_ops = {
>> +    .is_visible = cgbc_hwmon_is_visible,
>> +    .read = cgbc_hwmon_read,
>> +    .read_string = cgbc_hwmon_read_string,
>> +};
>> +
>> +static const struct hwmon_chip_info cgbc_chip_info = {
>> +    .ops = &cgbc_hwmon_ops,
>> +    .info = cgbc_hwmon_info,
>> +};
>> +
>> +static int cgbc_hwmon_probe(struct platform_device *pdev)
>> +{
>> +    struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> +    struct device *dev = &pdev->dev;
>> +    struct cgbc_hwmon_data *data;
>> +    struct device *hwmon_dev;
>> +    int ret;
>> +
>> +    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    data->cgbc = cgbc;
>> +
>> +    platform_set_drvdata(pdev, data);
> 
> What is this used for ? There are no suspend/resume functions,
> so I don't see the use case.

It's useless indeed.

Regards,

Thomas