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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-11-13 22:51:37-0800, Guenter Roeck wrote:
> On 11/13/24 20:40, Thomas Weißschuh wrote:
> > 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@xxxxxxxxxxxxxx>
> > > > ---
> > > > 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?
> > 
> You mean an extern declaration without the actual function ?

Stub as in empty inline function:

static inline void thermal_zone_device_update(struct thermal_zone_device *,
                                             enum thermal_notify_event)
{ }

> I'd really want to see that documented. It would seem rather unusual.


[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux