Re: [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 :
> 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 
>> ---
>>  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[] 
>> = {
>>   _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(>client.device);
>> +
>> + if (!iccsense)
>> + return 0;
>> +
>> + switch (attr) {
>> + case hwmon_power_input:
>> + if (iccsense->data_valid &&
>> + !list_empty(>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(>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(>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(>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(>client.device);
>> +
>> + if (!therm || !therm->attr_get || nvkm_therm_fan_sense(therm) < 0)
>> + return 0;
>> +
>> + switch (attr) {
>> + 

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

[Martin Peres]

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 
> ---
>  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[] 
> = {
>   _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(>client.device);
> +
> + if (!iccsense)
> + return 0;
> +
> + switch (attr) {
> + case hwmon_power_input:
> + if (iccsense->data_valid &&
> + !list_empty(>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.


> + 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(>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(>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(>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(>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) {
>