[PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Heiner Kallweit]

Several drivers simply return 0444 in their is_visible callback.
Add a helper in hwmon core for this use case to avoid code duplication.

There are more drivers outside drivers/hwmon which would benefit
from this helper as well.

Heiner Kallweit (9):
  hwmon: Add helper hwmon_visible_0444
  hwmon: i5500_temp: Use new helper hwmon_visible_0444
  hwmon: surface_fan: Use new helper hwmon_visible_0444
  hwmon: sl28cpld: Use new helper hwmon_visible_0444
  hwmon: gsc: Use new helper hwmon_visible_0444
  hwmon: powerz: Use new helper hwmon_visible_0444
  hwmon: raspberrypi: Use new helper hwmon_visible_0444
  hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
  hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444

 drivers/hwmon/gsc-hwmon.c           |  9 +--------
 drivers/hwmon/hwmon.c               |  7 +++++++
 drivers/hwmon/i5500_temp.c          |  8 +-------
 drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
 drivers/hwmon/nzxt-kraken2.c        |  9 +--------
 drivers/hwmon/powerz.c              |  8 +-------
 drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
 drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
 drivers/hwmon/surface_fan.c         | 10 +---------
 include/linux/hwmon.h               |  2 ++
 10 files changed, 17 insertions(+), 62 deletions(-)

-- 
2.47.0

[PATCH 1/9] hwmon: Add helper hwmon_visible_0444

[Heiner Kallweit]

Several drivers simply return 0444 in their is_visible callback.
Add a helper in hwmon core for this use case to avoid code duplication.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/hwmon.c | 7 +++++++
 include/linux/hwmon.h | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/hwmon/hwmon.c b/drivers/hwmon/hwmon.c
index 9c35c4d03..7c26e241e 100644
--- a/drivers/hwmon/hwmon.c
+++ b/drivers/hwmon/hwmon.c
@@ -108,6 +108,13 @@ static const struct attribute_group hwmon_dev_attr_group = {
 	.is_visible	= hwmon_dev_attr_is_visible,
 };
 
+umode_t hwmon_visible_0444(const void *drvdata, enum hwmon_sensor_types type,
+			   u32 attr, int channel)
+{
+	return 0444;
+}
+EXPORT_SYMBOL_GPL(hwmon_visible_0444);
+
 static const struct attribute_group *hwmon_dev_attr_groups[] = {
 	&hwmon_dev_attr_group,
 	NULL
diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 5c6a421ad..ce38051b6 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -422,6 +422,8 @@ struct hwmon_ops {
 		     u32 attr, int channel, long val);
 };
 
+umode_t hwmon_visible_0444(const void *drvdata, enum hwmon_sensor_types type,
+			   u32 attr, int channel);
 /**
  * struct hwmon_channel_info - Channel information
  * @type:	Channel type.
-- 
2.47.0

[PATCH 2/9] hwmon: i5500_temp: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/i5500_temp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/hwmon/i5500_temp.c b/drivers/hwmon/i5500_temp.c
index 7b00b38c7..819be21d3 100644
--- a/drivers/hwmon/i5500_temp.c
+++ b/drivers/hwmon/i5500_temp.c
@@ -29,12 +29,6 @@
 #define REG_CTCTRL	0xF7
 #define REG_TSTIMER	0xF8
 
-static umode_t i5500_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
-				int channel)
-{
-	return 0444;
-}
-
 static int i5500_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
 		      long *val)
 {
@@ -84,7 +78,7 @@ static int i5500_read(struct device *dev, enum hwmon_sensor_types type, u32 attr
 }
 
 static const struct hwmon_ops i5500_ops = {
-	.is_visible = i5500_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = i5500_read,
 };
 
-- 
2.47.0

[PATCH 3/9] hwmon: surface_fan: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/surface_fan.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/hwmon/surface_fan.c b/drivers/hwmon/surface_fan.c
index de3c5a240..2f173c51d 100644
--- a/drivers/hwmon/surface_fan.c
+++ b/drivers/hwmon/surface_fan.c
@@ -18,14 +18,6 @@ SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_fan_rpm_get, __le16, {
 	.command_id      = 0x01,
 });
 
-// hwmon
-static umode_t surface_fan_hwmon_is_visible(const void *drvdata,
-					    enum hwmon_sensor_types type, u32 attr,
-					    int channel)
-{
-	return 0444;
-}
-
 static int surface_fan_hwmon_read(struct device *dev,
 				  enum hwmon_sensor_types type, u32 attr,
 				  int channel, long *val)
@@ -49,7 +41,7 @@ static const struct hwmon_channel_info *const surface_fan_info[] = {
 };
 
 static const struct hwmon_ops surface_fan_hwmon_ops = {
-	.is_visible = surface_fan_hwmon_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = surface_fan_hwmon_read,
 };
 
-- 
2.47.0

[PATCH 4/9] hwmon: sl28cpld: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/sl28cpld-hwmon.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/hwmon/sl28cpld-hwmon.c b/drivers/hwmon/sl28cpld-hwmon.c
index e020f25c9..07fff262e 100644
--- a/drivers/hwmon/sl28cpld-hwmon.c
+++ b/drivers/hwmon/sl28cpld-hwmon.c
@@ -23,13 +23,6 @@ struct sl28cpld_hwmon {
 	u32 offset;
 };
 
-static umode_t sl28cpld_hwmon_is_visible(const void *data,
-					 enum hwmon_sensor_types type,
-					 u32 attr, int channel)
-{
-	return 0444;
-}
-
 static int sl28cpld_hwmon_read(struct device *dev,
 			       enum hwmon_sensor_types type, u32 attr,
 			       int channel, long *input)
@@ -73,7 +66,7 @@ static const struct hwmon_channel_info * const sl28cpld_hwmon_info[] = {
 };
 
 static const struct hwmon_ops sl28cpld_hwmon_ops = {
-	.is_visible = sl28cpld_hwmon_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = sl28cpld_hwmon_read,
 };
 
-- 
2.47.0

Re: [PATCH 4/9] hwmon: sl28cpld: Use new helper hwmon_visible_0444

[Michael Walle]

> Use new helper hwmon_visible_0444 to simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Michael Walle <mwalle@kernel.org>

Thanks,
-michael

[PATCH 5/9] hwmon: gsc: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/gsc-hwmon.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/hwmon/gsc-hwmon.c b/drivers/hwmon/gsc-hwmon.c
index 4514f3ed9..d70b4e305 100644
--- a/drivers/hwmon/gsc-hwmon.c
+++ b/drivers/hwmon/gsc-hwmon.c
@@ -231,15 +231,8 @@ gsc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
 	return 0;
 }
 
-static umode_t
-gsc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
-		     int ch)
-{
-	return 0444;
-}
-
 static const struct hwmon_ops gsc_hwmon_ops = {
-	.is_visible = gsc_hwmon_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = gsc_hwmon_read,
 	.read_string = gsc_hwmon_read_string,
 };
-- 
2.47.0

[PATCH 6/9] hwmon: powerz: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/powerz.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/hwmon/powerz.c b/drivers/hwmon/powerz.c
index cfb635f94..fa66a6471 100644
--- a/drivers/hwmon/powerz.c
+++ b/drivers/hwmon/powerz.c
@@ -54,12 +54,6 @@ static const struct hwmon_channel_info *const powerz_info[] = {
 	NULL
 };
 
-static umode_t powerz_is_visible(const void *data, enum hwmon_sensor_types type,
-				 u32 attr, int channel)
-{
-	return 0444;
-}
-
 static int powerz_read_string(struct device *dev, enum hwmon_sensor_types type,
 			      u32 attr, int channel, const char **str)
 {
@@ -201,7 +195,7 @@ static int powerz_read(struct device *dev, enum hwmon_sensor_types type,
 }
 
 static const struct hwmon_ops powerz_hwmon_ops = {
-	.is_visible = powerz_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = powerz_read,
 	.read_string = powerz_read_string,
 };
-- 
2.47.0

[PATCH 7/9] hwmon: raspberrypi: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/raspberrypi-hwmon.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
index 65cc52e47..277ab5dac 100644
--- a/drivers/hwmon/raspberrypi-hwmon.c
+++ b/drivers/hwmon/raspberrypi-hwmon.c
@@ -81,12 +81,6 @@ static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
 	return 0;
 }
 
-static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
-			      u32 attr, int channel)
-{
-	return 0444;
-}
-
 static const struct hwmon_channel_info * const rpi_info[] = {
 	HWMON_CHANNEL_INFO(in,
 			   HWMON_I_LCRIT_ALARM),
@@ -94,7 +88,7 @@ static const struct hwmon_channel_info * const rpi_info[] = {
 };
 
 static const struct hwmon_ops rpi_hwmon_ops = {
-	.is_visible = rpi_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = rpi_read,
 };
 
-- 
2.47.0

[PATCH 8/9] hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/intel-m10-bmc-hwmon.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/hwmon/intel-m10-bmc-hwmon.c b/drivers/hwmon/intel-m10-bmc-hwmon.c
index 96397ae6f..2616c6bf9 100644
--- a/drivers/hwmon/intel-m10-bmc-hwmon.c
+++ b/drivers/hwmon/intel-m10-bmc-hwmon.c
@@ -565,13 +565,6 @@ static const struct m10bmc_hwmon_board_data n6000bmc_hwmon_bdata = {
 	.hinfo = n6000bmc_hinfo,
 };
 
-static umode_t
-m10bmc_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
-			u32 attr, int channel)
-{
-	return 0444;
-}
-
 static const struct m10bmc_sdata *
 find_sensor_data(struct m10bmc_hwmon *hw, enum hwmon_sensor_types type,
 		 int channel)
@@ -729,7 +722,7 @@ static int m10bmc_hwmon_read_string(struct device *dev,
 }
 
 static const struct hwmon_ops m10bmc_hwmon_ops = {
-	.is_visible = m10bmc_hwmon_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = m10bmc_hwmon_read,
 	.read_string = m10bmc_hwmon_read_string,
 };
-- 
2.47.0

[PATCH 9/9] hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444

[Heiner Kallweit]

Use new helper hwmon_visible_0444 to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/hwmon/nzxt-kraken2.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
index 7caf387eb..e46558e2f 100644
--- a/drivers/hwmon/nzxt-kraken2.c
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -35,13 +35,6 @@ struct kraken2_priv_data {
 	unsigned long updated; /* jiffies */
 };
 
-static umode_t kraken2_is_visible(const void *data,
-				  enum hwmon_sensor_types type,
-				  u32 attr, int channel)
-{
-	return 0444;
-}
-
 static int kraken2_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *val)
 {
@@ -81,7 +74,7 @@ static int kraken2_read_string(struct device *dev, enum hwmon_sensor_types type,
 }
 
 static const struct hwmon_ops kraken2_hwmon_ops = {
-	.is_visible = kraken2_is_visible,
+	.is_visible = hwmon_visible_0444,
 	.read = kraken2_read,
 	.read_string = kraken2_read_string,
 };
-- 
2.47.0

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Guenter Roeck]

On 10/9/24 13:02, Heiner Kallweit wrote:
> Several drivers simply return 0444 in their is_visible callback.
> Add a helper in hwmon core for this use case to avoid code duplication.
> 
> There are more drivers outside drivers/hwmon which would benefit
> from this helper as well.
> 
> Heiner Kallweit (9):
>    hwmon: Add helper hwmon_visible_0444
>    hwmon: i5500_temp: Use new helper hwmon_visible_0444
>    hwmon: surface_fan: Use new helper hwmon_visible_0444
>    hwmon: sl28cpld: Use new helper hwmon_visible_0444
>    hwmon: gsc: Use new helper hwmon_visible_0444
>    hwmon: powerz: Use new helper hwmon_visible_0444
>    hwmon: raspberrypi: Use new helper hwmon_visible_0444
>    hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
>    hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
> 
>   drivers/hwmon/gsc-hwmon.c           |  9 +--------
>   drivers/hwmon/hwmon.c               |  7 +++++++
>   drivers/hwmon/i5500_temp.c          |  8 +-------
>   drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
>   drivers/hwmon/nzxt-kraken2.c        |  9 +--------
>   drivers/hwmon/powerz.c              |  8 +-------
>   drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
>   drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
>   drivers/hwmon/surface_fan.c         | 10 +---------
>   include/linux/hwmon.h               |  2 ++
>   10 files changed, 17 insertions(+), 62 deletions(-)
> 

I really don't want to add such hwmon-specific but at the same time
generic helpers. If such a helper is made available in the core kernel,
I'll be happy to accept patches using it, but otherwise please refrain
from submitting such patch series.

Thanks,
Guenter

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Heiner Kallweit]

On 10.10.2024 00:19, Guenter Roeck wrote:
> On 10/9/24 13:02, Heiner Kallweit wrote:
>> Several drivers simply return 0444 in their is_visible callback.
>> Add a helper in hwmon core for this use case to avoid code duplication.
>>
>> There are more drivers outside drivers/hwmon which would benefit
>> from this helper as well.
>>
>> Heiner Kallweit (9):
>>    hwmon: Add helper hwmon_visible_0444
>>    hwmon: i5500_temp: Use new helper hwmon_visible_0444
>>    hwmon: surface_fan: Use new helper hwmon_visible_0444
>>    hwmon: sl28cpld: Use new helper hwmon_visible_0444
>>    hwmon: gsc: Use new helper hwmon_visible_0444
>>    hwmon: powerz: Use new helper hwmon_visible_0444
>>    hwmon: raspberrypi: Use new helper hwmon_visible_0444
>>    hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
>>    hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
>>
>>   drivers/hwmon/gsc-hwmon.c           |  9 +--------
>>   drivers/hwmon/hwmon.c               |  7 +++++++
>>   drivers/hwmon/i5500_temp.c          |  8 +-------
>>   drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
>>   drivers/hwmon/nzxt-kraken2.c        |  9 +--------
>>   drivers/hwmon/powerz.c              |  8 +-------
>>   drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
>>   drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
>>   drivers/hwmon/surface_fan.c         | 10 +---------
>>   include/linux/hwmon.h               |  2 ++
>>   10 files changed, 17 insertions(+), 62 deletions(-)
>>
> 
> I really don't want to add such hwmon-specific but at the same time
> generic helpers. If such a helper is made available in the core kernel,
> I'll be happy to accept patches using it, but otherwise please refrain
> from submitting such patch series.
> 
What would you consider a suited place, drivers/base or fs/sysfs or lib or .. ?
For enum hwmon_sensor_types we have to include linux/hwmon.h. None of these
places has any hwmon code, and I would expect concerns if generic core code
includes subsystem headers.

> Thanks,
> Guenter
> 

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Jonathan Cameron]

On Thu, 10 Oct 2024 07:44:31 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 10.10.2024 00:19, Guenter Roeck wrote:
> > On 10/9/24 13:02, Heiner Kallweit wrote:  
> >> Several drivers simply return 0444 in their is_visible callback.
> >> Add a helper in hwmon core for this use case to avoid code duplication.
> >>
> >> There are more drivers outside drivers/hwmon which would benefit
> >> from this helper as well.
> >>
> >> Heiner Kallweit (9):
> >>    hwmon: Add helper hwmon_visible_0444
> >>    hwmon: i5500_temp: Use new helper hwmon_visible_0444
> >>    hwmon: surface_fan: Use new helper hwmon_visible_0444
> >>    hwmon: sl28cpld: Use new helper hwmon_visible_0444
> >>    hwmon: gsc: Use new helper hwmon_visible_0444
> >>    hwmon: powerz: Use new helper hwmon_visible_0444
> >>    hwmon: raspberrypi: Use new helper hwmon_visible_0444
> >>    hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
> >>    hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
> >>
> >>   drivers/hwmon/gsc-hwmon.c           |  9 +--------
> >>   drivers/hwmon/hwmon.c               |  7 +++++++
> >>   drivers/hwmon/i5500_temp.c          |  8 +-------
> >>   drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
> >>   drivers/hwmon/nzxt-kraken2.c        |  9 +--------
> >>   drivers/hwmon/powerz.c              |  8 +-------
> >>   drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
> >>   drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
> >>   drivers/hwmon/surface_fan.c         | 10 +---------
> >>   include/linux/hwmon.h               |  2 ++
> >>   10 files changed, 17 insertions(+), 62 deletions(-)
> >>  
> > 
> > I really don't want to add such hwmon-specific but at the same time
> > generic helpers. If such a helper is made available in the core kernel,
> > I'll be happy to accept patches using it, but otherwise please refrain
> > from submitting such patch series.
> >   
> What would you consider a suited place, drivers/base or fs/sysfs or lib or .. ?
> For enum hwmon_sensor_types we have to include linux/hwmon.h. None of these
> places has any hwmon code, and I would expect concerns if generic core code
> includes subsystem headers.

I don't see this as particularly generic. 

For normal attribute handling with fixed permissions you'd just not provide
an is_visible function and hardcode the permissions in the attribute.

Maybe there are other users that need an is_visible callback?

Jonathan
> 
> > Thanks,
> > Guenter
> >   
> 
> 

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Guenter Roeck]

On 10/9/24 22:44, Heiner Kallweit wrote:
> On 10.10.2024 00:19, Guenter Roeck wrote:
>> On 10/9/24 13:02, Heiner Kallweit wrote:
>>> Several drivers simply return 0444 in their is_visible callback.
>>> Add a helper in hwmon core for this use case to avoid code duplication.
>>>
>>> There are more drivers outside drivers/hwmon which would benefit
>>> from this helper as well.
>>>
>>> Heiner Kallweit (9):
>>>     hwmon: Add helper hwmon_visible_0444
>>>     hwmon: i5500_temp: Use new helper hwmon_visible_0444
>>>     hwmon: surface_fan: Use new helper hwmon_visible_0444
>>>     hwmon: sl28cpld: Use new helper hwmon_visible_0444
>>>     hwmon: gsc: Use new helper hwmon_visible_0444
>>>     hwmon: powerz: Use new helper hwmon_visible_0444
>>>     hwmon: raspberrypi: Use new helper hwmon_visible_0444
>>>     hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
>>>     hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
>>>
>>>    drivers/hwmon/gsc-hwmon.c           |  9 +--------
>>>    drivers/hwmon/hwmon.c               |  7 +++++++
>>>    drivers/hwmon/i5500_temp.c          |  8 +-------
>>>    drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
>>>    drivers/hwmon/nzxt-kraken2.c        |  9 +--------
>>>    drivers/hwmon/powerz.c              |  8 +-------
>>>    drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
>>>    drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
>>>    drivers/hwmon/surface_fan.c         | 10 +---------
>>>    include/linux/hwmon.h               |  2 ++
>>>    10 files changed, 17 insertions(+), 62 deletions(-)
>>>
>>
>> I really don't want to add such hwmon-specific but at the same time
>> generic helpers. If such a helper is made available in the core kernel,
>> I'll be happy to accept patches using it, but otherwise please refrain
>> from submitting such patch series.
>>
> What would you consider a suited place, drivers/base or fs/sysfs or lib or .. ?
> For enum hwmon_sensor_types we have to include linux/hwmon.h. None of these
> places has any hwmon code, and I would expect concerns if generic core code
> includes subsystem headers.
> 

"There are more drivers outside drivers/hwmon which would benefit
from this helper as well" very clearly suggests that the function
is not hwmon specific. Yet, obviously it is if it requires a hwmon
include file.

If this is hwmon specific, I'll want to see a different solution.
Instead of calling an exported function, hwmon drivers should set the
is_visible callback to a defined value, such as HWMON_VISIBLE_0444.
This could be an ERR_PTR() which is converted by the hwmon core.
This lets us add, for example, HWMON_VISIBLE_0644, and it avoids the
notion that the function could be called by non-hwmon drivers
(and the notion that it is a function in the first place). Instead
of calling the callback directly, the hwmon core would then have a
helper function which evaluates the pointer and either returns
a constant or calls the callback.

Guenter

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Heiner Kallweit]

On 10.10.2024 16:31, Guenter Roeck wrote:
> On 10/9/24 22:44, Heiner Kallweit wrote:
>> On 10.10.2024 00:19, Guenter Roeck wrote:
>>> On 10/9/24 13:02, Heiner Kallweit wrote:
>>>> Several drivers simply return 0444 in their is_visible callback.
>>>> Add a helper in hwmon core for this use case to avoid code duplication.
>>>>
>>>> There are more drivers outside drivers/hwmon which would benefit
>>>> from this helper as well.
>>>>
>>>> Heiner Kallweit (9):
>>>>     hwmon: Add helper hwmon_visible_0444
>>>>     hwmon: i5500_temp: Use new helper hwmon_visible_0444
>>>>     hwmon: surface_fan: Use new helper hwmon_visible_0444
>>>>     hwmon: sl28cpld: Use new helper hwmon_visible_0444
>>>>     hwmon: gsc: Use new helper hwmon_visible_0444
>>>>     hwmon: powerz: Use new helper hwmon_visible_0444
>>>>     hwmon: raspberrypi: Use new helper hwmon_visible_0444
>>>>     hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
>>>>     hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
>>>>
>>>>    drivers/hwmon/gsc-hwmon.c           |  9 +--------
>>>>    drivers/hwmon/hwmon.c               |  7 +++++++
>>>>    drivers/hwmon/i5500_temp.c          |  8 +-------
>>>>    drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
>>>>    drivers/hwmon/nzxt-kraken2.c        |  9 +--------
>>>>    drivers/hwmon/powerz.c              |  8 +-------
>>>>    drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
>>>>    drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
>>>>    drivers/hwmon/surface_fan.c         | 10 +---------
>>>>    include/linux/hwmon.h               |  2 ++
>>>>    10 files changed, 17 insertions(+), 62 deletions(-)
>>>>
>>>
>>> I really don't want to add such hwmon-specific but at the same time
>>> generic helpers. If such a helper is made available in the core kernel,
>>> I'll be happy to accept patches using it, but otherwise please refrain
>>> from submitting such patch series.
>>>
>> What would you consider a suited place, drivers/base or fs/sysfs or lib or .. ?
>> For enum hwmon_sensor_types we have to include linux/hwmon.h. None of these
>> places has any hwmon code, and I would expect concerns if generic core code
>> includes subsystem headers.
>>
> 
> "There are more drivers outside drivers/hwmon which would benefit
> from this helper as well" very clearly suggests that the function
> is not hwmon specific. Yet, obviously it is if it requires a hwmon
> include file.
> 
What I was referring to is device drivers in other subsystems which
expose e.g. thermal sensor data via hwmon API.

> If this is hwmon specific, I'll want to see a different solution.
> Instead of calling an exported function, hwmon drivers should set the
> is_visible callback to a defined value, such as HWMON_VISIBLE_0444.
> This could be an ERR_PTR() which is converted by the hwmon core.
> This lets us add, for example, HWMON_VISIBLE_0644, and it avoids the
> notion that the function could be called by non-hwmon drivers
> (and the notion that it is a function in the first place). Instead
> of calling the callback directly, the hwmon core would then have a
> helper function which evaluates the pointer and either returns
> a constant or calls the callback.
> 
What goes in the same direction, but may be better/cleaner:
We could add an umode_t parameter to struct hwmon_ops, and if it's
non-zero then hwmon core interprets it as static visibility instead
of calling is_visible().

> Guenter
> 
Heiner

Re: [PATCH 0/9] hwmon: Add and use helper hwmon_visible_0444

[Heiner Kallweit]

On 10.10.2024 17:14, Heiner Kallweit wrote:
> On 10.10.2024 16:31, Guenter Roeck wrote:
>> On 10/9/24 22:44, Heiner Kallweit wrote:
>>> On 10.10.2024 00:19, Guenter Roeck wrote:
>>>> On 10/9/24 13:02, Heiner Kallweit wrote:
>>>>> Several drivers simply return 0444 in their is_visible callback.
>>>>> Add a helper in hwmon core for this use case to avoid code duplication.
>>>>>
>>>>> There are more drivers outside drivers/hwmon which would benefit
>>>>> from this helper as well.
>>>>>
>>>>> Heiner Kallweit (9):
>>>>>     hwmon: Add helper hwmon_visible_0444
>>>>>     hwmon: i5500_temp: Use new helper hwmon_visible_0444
>>>>>     hwmon: surface_fan: Use new helper hwmon_visible_0444
>>>>>     hwmon: sl28cpld: Use new helper hwmon_visible_0444
>>>>>     hwmon: gsc: Use new helper hwmon_visible_0444
>>>>>     hwmon: powerz: Use new helper hwmon_visible_0444
>>>>>     hwmon: raspberrypi: Use new helper hwmon_visible_0444
>>>>>     hwmon: intel-m10-bmc: Use new helper hwmon_visible_0444
>>>>>     hwmon: nzxt-kraken2: Use new helper hwmon_visible_0444
>>>>>
>>>>>    drivers/hwmon/gsc-hwmon.c           |  9 +--------
>>>>>    drivers/hwmon/hwmon.c               |  7 +++++++
>>>>>    drivers/hwmon/i5500_temp.c          |  8 +-------
>>>>>    drivers/hwmon/intel-m10-bmc-hwmon.c |  9 +--------
>>>>>    drivers/hwmon/nzxt-kraken2.c        |  9 +--------
>>>>>    drivers/hwmon/powerz.c              |  8 +-------
>>>>>    drivers/hwmon/raspberrypi-hwmon.c   |  8 +-------
>>>>>    drivers/hwmon/sl28cpld-hwmon.c      |  9 +--------
>>>>>    drivers/hwmon/surface_fan.c         | 10 +---------
>>>>>    include/linux/hwmon.h               |  2 ++
>>>>>    10 files changed, 17 insertions(+), 62 deletions(-)
>>>>>
>>>>
>>>> I really don't want to add such hwmon-specific but at the same time
>>>> generic helpers. If such a helper is made available in the core kernel,
>>>> I'll be happy to accept patches using it, but otherwise please refrain
>>>> from submitting such patch series.
>>>>
>>> What would you consider a suited place, drivers/base or fs/sysfs or lib or .. ?
>>> For enum hwmon_sensor_types we have to include linux/hwmon.h. None of these
>>> places has any hwmon code, and I would expect concerns if generic core code
>>> includes subsystem headers.
>>>
>>
>> "There are more drivers outside drivers/hwmon which would benefit
>> from this helper as well" very clearly suggests that the function
>> is not hwmon specific. Yet, obviously it is if it requires a hwmon
>> include file.
>>
> What I was referring to is device drivers in other subsystems which
> expose e.g. thermal sensor data via hwmon API.
> 
>> If this is hwmon specific, I'll want to see a different solution.
>> Instead of calling an exported function, hwmon drivers should set the
>> is_visible callback to a defined value, such as HWMON_VISIBLE_0444.
>> This could be an ERR_PTR() which is converted by the hwmon core.
>> This lets us add, for example, HWMON_VISIBLE_0644, and it avoids the
>> notion that the function could be called by non-hwmon drivers
>> (and the notion that it is a function in the first place). Instead
>> of calling the callback directly, the hwmon core would then have a
>> helper function which evaluates the pointer and either returns
>> a constant or calls the callback.
>>
> What goes in the same direction, but may be better/cleaner:
> We could add an umode_t parameter to struct hwmon_ops, and if it's
> non-zero then hwmon core interprets it as static visibility instead
> of calling is_visible().
> 
I just sent a patch with this approach to have a basis for discussion.

>> Guenter
>>
> Heiner