[Nouveau] [PATCH v5 2/5] nouveau_hwmon: Add nouveau_hwmon_ops structure with .is_visible/.read_string

[Karol Herbst]

2017-05-02 6:25 GMT+02:00 Martin Peres <martin.peres at free.fr>:
> On 26/04/17 19:46, Oscar Salvador wrote:
>> This patch introduces the nouveau_hwmon_ops structure, sets up
>> .is_visible and .read_string operations and adds all the functions
>> for these operations.
>> This is also a preparation for the next patches, where most of the
>> work is being done.
>> This code doesn't interacture with the old one.
>> It's just to make easier the review of all patches.
>>
>> Signed-off-by: Oscar Salvador <osalvador.vilardaga at gmail.com>
>> ---
>>  drivers/gpu/drm/nouveau/nouveau_hwmon.c | 170 ++++++++++++++++++++++++++++++++
>>  1 file changed, 170 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> index 24b40c5..e8ea8d0 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
>> @@ -764,6 +764,176 @@ static const struct hwmon_channel_info *nouveau_info[] = {
>>       &nouveau_power,
>>       NULL
>>  };
>> +
>> +static umode_t
>> +nouveau_chip_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     switch (attr) {
>> +     case hwmon_chip_update_interval:
>> +             return 0444;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_power_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +     struct nvkm_iccsense *iccsense = nvxx_iccsense(&drm->client.device);
>> +
>> +     if (!iccsense)
>> +             return 0;
>> +
>> +     switch (attr) {
>> +     case hwmon_power_input:
>> +             if (iccsense->data_valid &&
>> +                     !list_empty(&iccsense->rails))
>
> Not sure if the kernel coding style would mandate !list_empty to be
> aligned with the iccsense in the above line, but this is our coding
> style at least.
>
> Anyway, this condition has to be moved before the switch as we should
> not expose power_max/crit if power_input is not exposed.
>

well, we could expose those anyway to let the user know what the GPU
is expected to consume at most. But other than that it is quite
useless, true.

>
>> +                     return 0444;
>> +             return 0;
>> +     case hwmon_power_max:
>> +             if (iccsense->power_w_max)
>> +                     return 0444;
>> +             return 0;
>> +     case hwmon_power_crit:
>> +             if (iccsense->power_w_crit)
>> +                     return 0444;
>> +             return 0;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_temp_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +     struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +     if (therm &&
>> +             therm->attr_get &&
>> +             nvkm_therm_temp_get(therm) < 0)
>> +             return 0;
>
> You can also fold therm->attr_get on the same line as therm.
>
>> +
>> +     switch (attr) {
>> +     case hwmon_temp_input:
>> +     case hwmon_temp_max:
>> +     case hwmon_temp_max_hyst:
>> +     case hwmon_temp_crit:
>> +     case hwmon_temp_crit_hyst:
>> +     case hwmon_temp_emergency:
>> +     case hwmon_temp_emergency_hyst:
>> +             return 0444;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_pwm_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +     struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +     if (therm &&
>> +             therm->attr_get &&
>> +             therm->fan_get &&
>> +             therm->fan_get(therm) < 0)
>> +             return 0;
>
> Same, please fold as much as you can in 80 characters ;)
>
>> +
>> +     switch (attr) {
>> +     case hwmon_pwm_enable:
>> +     case hwmon_pwm_input:
>> +             return 0644;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_input_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +     struct nvkm_volt *volt = nvxx_volt(&drm->client.device);
>> +
>> +     if (!volt || nvkm_volt_get(volt) < 0)
>> +             return 0;
>> +
>> +     switch (attr) {
>> +     case hwmon_in_input:
>> +     case hwmon_in_label:
>> +     case hwmon_in_min:
>> +     case hwmon_in_max:
>> +             return 0444;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_fan_is_visible(const void *data, u32 attr, int channel)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
>> +     struct nvkm_therm *therm = nvxx_therm(&drm->client.device);
>> +
>> +     if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
>> +             return 0;
>> +
>> +     switch (attr) {
>> +     case hwmon_fan_input:
>> +             return 0444;
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static umode_t
>> +nouveau_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
>> +                                                             int channel)
>
> Could you align 'int channel' with 'const void *data'?
>
>> +{
>> +     switch (type) {
>> +     case hwmon_chip:
>> +             return nouveau_chip_is_visible(data, attr, channel);
>> +     case hwmon_temp:
>> +             return nouveau_temp_is_visible(data, attr, channel);
>> +     case hwmon_fan:
>> +             return nouveau_fan_is_visible(data, attr, channel);
>> +     case hwmon_in:
>> +             return nouveau_input_is_visible(data, attr, channel);
>> +     case hwmon_pwm:
>> +             return nouveau_pwm_is_visible(data, attr, channel);
>> +     case hwmon_power:
>> +             return nouveau_power_is_visible(data, attr, channel);
>> +     default:
>> +             return 0;
>> +     }
>> +}
>> +
>> +static const char input_label[] = "GPU core";
>> +
>> +static int
>> +nouveau_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +                                                     int channel, char **buf)
>
> Same as above.
>
>> +{
>> +     if (type == hwmon_in && attr == hwmon_in_label) {
>> +             *buf = input_label;
>> +             return 0;
>> +     }
>> +
>> +     return -EOPNOTSUPP;
>> +}
>> +
>> +static const struct hwmon_ops nouveau_hwmon_ops = {
>> +     .is_visible = nouveau_is_visible,
>> +     .read = NULL,
>> +     .read_string = nouveau_read_string,
>> +     .write = NULL,
>> +};
>> +
>> +static const struct hwmon_chip_info nouveau_chip_info = {
>> +     .ops = &nouveau_hwmon_ops,
>> +     .info = nouveau_info,
>> +};
>>  #endif
>>
>>  int
>>
>
> With the power-related conditions and all the alignments fixed, patches
> 1-2 are:
>
> Reviewed-by: Martin Peres <martin.peres at free.fr>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau