Re: [PATCH] hwmon: (core) Avoid ifdef in C source file

["Thomas Weißschuh" <linux@weissschuh.net>]

Hi Guenter,

On 2024-11-12 22:52:36-0800, Guenter Roeck wrote:
> On 11/12/24 20:39, Thomas Weißschuh wrote:
> > Using an #ifdef in a C source files to have different definitions
> > of the same symbol makes the code harder to read and understand.
> > Furthermore it makes it harder to test compilation of the different
> > branches.
> > 
> > Replace the ifdeffery with IS_ENABLED() which is just a normal
> > conditional.
> > The resulting binary is still the same as before as the compiler
> > optimizes away all the unused code and definitions.
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > This confused me a bit while looking at the implementation of
> > HWMON_C_REGISTER_TZ.
> > ---
> >   drivers/hwmon/hwmon.c | 21 ++++++---------------
> >   1 file changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
> > index 9c35c4d0369d7aad7ea61ccd25f4f63fc98b9e02..86fb674c85d3f54d475be014c3fd3dd74c815c57 100644
> > --- a/drivers/hwmon/hwmon.c
> > +++ b/drivers/hwmon/hwmon.c
> > @@ -147,11 +147,6 @@ static DEFINE_IDA(hwmon_ida);
> >   /* Thermal zone handling */
> > -/*
> > - * The complex conditional is necessary to avoid a cyclic dependency
> > - * between hwmon and thermal_sys modules.
> > - */
> > -#ifdef CONFIG_THERMAL_OF
> >   static int hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> >   {
> >   	struct hwmon_thermal_data *tdata = thermal_zone_device_priv(tz);
> > @@ -257,6 +252,9 @@ static int hwmon_thermal_register_sensors(struct device *dev)
> >   	void *drvdata = dev_get_drvdata(dev);
> >   	int i;
> > +	if (!IS_ENABLED(CONFIG_THERMAL_OF))
> > +		return 0;
> > +
> >   	for (i = 1; info[i]; i++) {
> >   		int j;
> > @@ -285,6 +283,9 @@ static void hwmon_thermal_notify(struct device *dev, int index)
> >   	struct hwmon_device *hwdev = to_hwmon_device(dev);
> >   	struct hwmon_thermal_data *tzdata;
> > +	if (!IS_ENABLED(CONFIG_THERMAL_OF))
> > +		return;
> > +
> >   	list_for_each_entry(tzdata, &hwdev->tzdata, node) {
> >   		if (tzdata->index == index) {
> >   			thermal_zone_device_update(tzdata->tzd,
> 
> There is no dummy function for thermal_zone_device_update().
> I really don't want to trust the compiler/linker to remove that code
> unless someone points me to a document explaining that it is guaranteed
> to not cause any problems.

I'm fairly sure that a declaration should be enough, and believe
to remember seeing such advise somewhere.
However there is not even a function declaration with !CONFIG_THERMAL.
So I can add an actual stub for it for v2.

What do you think?

Thomas

> > @@ -293,16 +294,6 @@ static void hwmon_thermal_notify(struct device *dev, int index)
> >   	}
> >   }
> > -#else
> > -static int hwmon_thermal_register_sensors(struct device *dev)
> > -{
> > -	return 0;
> > -}
> > -
> > -static void hwmon_thermal_notify(struct device *dev, int index) { }
> > -
> > -#endif /* IS_REACHABLE(CONFIG_THERMAL) && ... */
> > -
> >   static int hwmon_attr_base(enum hwmon_sensor_types type)
> >   {
> >   	if (type == hwmon_in || type == hwmon_intrusion)
> > 
> > ---
> > base-commit: 3022e9d00ebec31ed435ae0844e3f235dba998a9
> > change-id: 20241113-hwmon-thermal-2d2da581c276
> > 
> > Best regards,
>