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

[Guenter Roeck]

On 11/6/24 05:46, Thomas Richard wrote:

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

My thought was to use the sensor ID as channel index. In general it
would be preferable to know that, say, in0 is always the CPU voltage
and that temp1 is always the CPU Temperature.

Guenter