Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 1/2] hwmon: Add include stubs
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     jdelvare@suse.com, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220825214341.2743-1-W_Armin@gmx.de>
 <20220825214341.2743-2-W_Armin@gmx.de>
 <20220826124742.GD2897306@roeck-us.net>
From:   Armin Wolf <W_Armin@gmx.de>
Message-ID: <2127da59-c8aa-301e-4648-2abf48ea7d61@gmx.de>
Date:   Fri, 26 Aug 2022 20:07:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <20220826124742.GD2897306@roeck-us.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

Am 26.08.22 um 14:47 schrieb Guenter Roeck:

> On Thu, Aug 25, 2022 at 11:43:40PM +0200, Armin Wolf wrote:
>> If CONFIG_HWMON/CONFIG_HWMON_VID was disabled during compile
>> time, driver using the hwmon subsystem might fail to compile.
> That would be a bug in driver dependencies and is not a problem
> to be solved in the hwmon subsystem. Dummies are not provided on
> purpose so far because we _want_ driver developers to think about
> usage and for them to understand what happens if HWMON is not enabled.

My wording might have been incorrect, i meant that currently, code who uses
the hwmon subsystem must either select CONFIG_HWMON or provide #ifdef guards.

> The benefit is that it would reduce the need for conditional code
> in drivers registering with hwmon from outside the hwmon subsystem.
>
>> Provide stubs for such cases.
>>
> HWMON_VID is not user selectable, it is only needed by hwmon drivers,
> it is mandatory for the drivers needing it, those drivers _must_ select
> it, and there must be no dummies.
>
> I am not really sure about the benefits of dummies for HWMON either,
> but I am open to discussion. Either case that must be accompanied
> by matching driver patches to show its usage, to make sure that there
> is no negative impact, to show the benefits, and to get a wider audience.
>
Understandable, i will include such patches in the next revision.

>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>>   include/linux/hwmon-vid.h | 18 ++++++++++
>>   include/linux/hwmon.h     | 76 ++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 93 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/hwmon-vid.h b/include/linux/hwmon-vid.h
>> index 9409e1d207ef..329151416c47 100644
>> --- a/include/linux/hwmon-vid.h
>> +++ b/include/linux/hwmon-vid.h
>> @@ -11,9 +11,27 @@
>>   #ifndef _LINUX_HWMON_VID_H
>>   #define _LINUX_HWMON_VID_H
>>
>> +#include <linux/kconfig.h>
>> +
>> +#if IS_ENABLED(CONFIG_HWMON_VID)
>> +
>>   int vid_from_reg(int val, u8 vrm);
>>   u8 vid_which_vrm(void);
>>
>> +#else
>> +
>> +static inline int vid_from_reg(int val, u8 vrm)
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline u8 vid_which_vrm(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +#endif /* CONFIG_HWMON_VID */
>> +
>>   /* vrm is the VRM/VRD document version multiplied by 10.
>>      val is in mV to avoid floating point in the kernel.
>>      Returned value is the 4-, 5- or 6-bit VID code.
>> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
>> index 14325f93c6b2..281387ee03bc 100644
>> --- a/include/linux/hwmon.h
>> +++ b/include/linux/hwmon.h
>> @@ -13,6 +13,9 @@
>>   #define _HWMON_H_
>>
>>   #include <linux/bitops.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/kconfig.h>
>>
>>   struct device;
>>   struct attribute_group;
>> @@ -433,6 +436,8 @@ struct hwmon_chip_info {
>>   	const struct hwmon_channel_info **info;
>>   };
>>
>> +#if IS_ENABLED(CONFIG_HWMON)
> This should be IS_REACHABLE(). It doesn't help if HWMON is built as
> module and called from an in-kernel driver. Otherwise drivers using it
> would still need "depends on HWMON || HWMON=n" and it would still require
> conditional code to catch "HWMON enabled but not reachable".
>
>> +
>>   /* hwmon_device_register() is deprecated */
>>   struct device *hwmon_device_register(struct device *dev);
>>
>> @@ -467,6 +472,75 @@ int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type,
>>   char *hwmon_sanitize_name(const char *name);
>>   char *devm_hwmon_sanitize_name(struct device *dev, const char *name);
>>
>> +#else
>> +
>> +static inline struct device *hwmon_device_register(struct device *dev)
>> +{
>> +	return ERR_PTR(-ENODEV);
> -ENOTSUPP would probably be a more suitable error code.

The stubs found in other header files do return -ENODEV, so i though
that this should also be done inside hwmon.h

Armin Wolf

>> +}
>> +
>> +static inline struct device
>> +*hwmon_device_register_with_groups(struct device *dev, const char *name, void *drvdata,
>> +				   const struct attribute_group **groups)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline struct device
>> +*devm_hwmon_device_register_with_groups(struct device *dev, const char *name, void *drvdata,
>> +					const struct attribute_group **groups)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline struct device
>> +*hwmon_device_register_with_info(struct device *dev, const char *name, void *drvdata,
>> +				 const struct hwmon_chip_info *info,
>> +				 const struct attribute_group **extra_groups)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline struct device *hwmon_device_register_for_thermal(struct device *dev, const char *name,
>> +							       void *drvdata)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline struct device
>> +*devm_hwmon_device_register_with_info(struct device *dev, const char *name, void *drvdata,
>> +				      const struct hwmon_chip_info *info,
>> +				      const struct attribute_group **extra_groups)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline void hwmon_device_unregister(struct device *dev)
>> +{
>> +}
>> +
>> +static inline void devm_hwmon_device_unregister(struct device *dev)
>> +{
>> +}
>> +
>> +static inline int hwmon_notify_event(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> +				     int channel)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>> +static inline char *hwmon_sanitize_name(const char *name)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static inline char *devm_hwmon_sanitize_name(struct device *dev, const char *name)
>> +{
>> +	return ERR_PTR(-ENODEV);
>> +}
>> +
>> +#endif /* CONFIG_HWMON */
>> +
>>   /**
>>    * hwmon_is_bad_char - Is the char invalid in a hwmon name
>>    * @ch: the char to be considered
>> @@ -490,4 +564,4 @@ static inline bool hwmon_is_bad_char(const char ch)
>>   	}
>>   }
>>
>> -#endif
>> +#endif /* _HWMON_H_ */
>> --
>> 2.30.2
>>