Date:   Fri, 26 Aug 2022 05:47:42 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Armin Wolf <W_Armin@....de>
Cc:     jdelvare@...e.com, linux-hwmon@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] hwmon: Add include stubs

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

Powered by blists - more mailing lists