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