| Message ID | 3aecbfc5-e11b-4425-9c6b-88dac2d32945@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | hwmon: Add and use helper hwmon_visible_0444 | expand |
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
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 >
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 > > > >
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
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
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