[PATCH 0/2] hwmon: Add include stubs

[PATCH 0/2] hwmon: Add include stubs

[Armin Wolf]

Currently, hwmon.h and hwmon-vid.h provide no stub definitions
in case hwmon/hwmon-vid support was disabled. This forces drivers
using those functions to either select CONFIG_HWMON or to use
lots of #ifdef to avoid compilation errors in such a case.
One example is the ath10k driver, but radeon and amdgpu
would also profit from being able to no longer having to select
CONFIG_HWMON.

The first patch adds include stubs to hwmon.h and hwmon-vid.h
so that drivers can omit such workarounds.

The second patch fixes a minor issue in hwmon.h.

Both patches where tested with CONFIG_HWMON set to y, m and n,
and the resulting kernel was able to boot successfully.

Armin Wolf (2):
  hwmon: Add include stubs
  hwmon: Use struct definitions from header files

 include/linux/hwmon-vid.h | 18 +++++++++
 include/linux/hwmon.h     | 81 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 95 insertions(+), 4 deletions(-)

--
2.30.2

[PATCH 1/2] hwmon: Add include stubs

[Armin Wolf]

If CONFIG_HWMON/CONFIG_HWMON_VID was disabled during compile
time, driver using the hwmon subsystem might fail to compile.
Provide stubs for such cases.

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)
+
 /* 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);
+}
+
+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

Re: [PATCH 1/2] hwmon: Add include stubs

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

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.

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

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

Re: [PATCH 1/2] hwmon: Add include stubs

[Armin Wolf]

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

[PATCH 2/2] hwmon: Use struct definitions from header files

[Armin Wolf]

The structs attribute_group and device are provided
by linux/sysfs.h and linux/device.h.
Use those definitions.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
 include/linux/hwmon.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 281387ee03bc..e8acc35af12d 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -13,12 +13,11 @@
 #define _HWMON_H_

 #include <linux/bitops.h>
+#include <linux/device.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/kconfig.h>
-
-struct device;
-struct attribute_group;
+#include <linux/sysfs.h>

 enum hwmon_sensor_types {
 	hwmon_chip,
--
2.30.2

Re: [PATCH 2/2] hwmon: Use struct definitions from header files

[Guenter Roeck]

On Thu, Aug 25, 2022 at 11:43:41PM +0200, Armin Wolf wrote:
> The structs attribute_group and device are provided
> by linux/sysfs.h and linux/device.h.
> Use those definitions.
> 

No. The limited definitions are on purpose, meaning the details
are not needed in this header file and that drivers using the
structures must include the necessary files directly. That is what
struct declarations without details are to be used for in C, and
there is neither a desire nor need to change that code.

Guenter

> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  include/linux/hwmon.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 281387ee03bc..e8acc35af12d 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -13,12 +13,11 @@
>  #define _HWMON_H_
> 
>  #include <linux/bitops.h>
> +#include <linux/device.h>
>  #include <linux/err.h>
>  #include <linux/errno.h>
>  #include <linux/kconfig.h>
> -
> -struct device;
> -struct attribute_group;
> +#include <linux/sysfs.h>
> 
>  enum hwmon_sensor_types {
>  	hwmon_chip,
> --
> 2.30.2
>