Re: [PATCH] hwmon: OCC drivers are PowerPC-only

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Apr 09, 2019 at 10:20:22AM -0500, Eddie James wrote:
> 
> On 4/9/19 7:45 AM, Jean Delvare wrote:
> >Don't propose PowerPC-only drivers on other architectures, unless
> >build-testing.
> 
> 
> This driver does NOT only run on PowerPC; rather it runs on a BMC processor
> connected to a PowerPC processor. BMC will most likely be ARM, but shouldn't
> be restricted to that arch only.
> 
> 
> >
> >Also drop configuration symbol SENSORS_OCC which serves no purpose
> >that I can see.
> >
> >Signed-off-by: Jean Delvare <jdelvare@suse.de>
> >Cc: Eddie James <eajames@linux.ibm.com>
> >Cc: Guenter Roeck <linux@roeck-us.net>
> >---
> >SENSORS_OCC *would* serve a purpose if the common code between the
> >POWER8 driver and the POWER9 driver would go in a separate, shared
> >module, and occ-p8-hwmon and occ-p9-hwmon would only contain the
> >specific code. This would avoid packaging the same code twice in 2
> >separate modules, therefore saving some storage space for ppc
> >distributions.
> 
> 
> Well you'd never have both P8 and P9 enabled at once, so space shouldn't be
> an issue. I agree this could be cleaner but I think I was getting duplicate
> symbol errors for the compile test and so I did it this way. If this doesn't
> lead to errors in the compile test, I'm fine with this (without the change
> for PPC only though).
>

Any common code would have to be in a separate module to avoid duplicate
symbols. You'd bundle common.c and sysfs.c into one module and export
the functions called from p8/p9 specific code.

Something like

occ-common-objs := common.o sysfs.o

obj-$(CONFIG_SENSORS_OCC) += occ-common.o
obj-$(CONFIG_SENSORS_OCC_P8_I2C) += p8_i2c.o
obj-$(CONFIG_SENSORS_OCC_P9_SBE) += p9-sbe.o

Guenter

> 
> Thanks,
> 
> Eddie
> 
> >
> >As far as I can see, this would simply require exporting 2 functions
> >(occ_setup and occ_shutdown). Is there any reason why things were not
> >done that way in the first place? This would look cleaner to me.
> >
> >  drivers/hwmon/Makefile    |    2 +-
> >  drivers/hwmon/occ/Kconfig |    8 ++------
> >  2 files changed, 3 insertions(+), 7 deletions(-)
> >
> >--- linux-5.0.orig/drivers/hwmon/occ/Kconfig	2019-03-04 00:21:29.000000000 +0100
> >+++ linux-5.0/drivers/hwmon/occ/Kconfig	2019-04-09 14:08:41.316551071 +0200
> >@@ -5,7 +5,7 @@
> >  config SENSORS_OCC_P8_I2C
> >  	tristate "POWER8 OCC through I2C"
> >  	depends on I2C
> >-	select SENSORS_OCC
> >+	depends on POWERPC || COMPILE_TEST
> >  	help
> >  	 This option enables support for monitoring sensors provided by the
> >  	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
> >@@ -17,7 +17,7 @@ config SENSORS_OCC_P8_I2C
> >  config SENSORS_OCC_P9_SBE
> >  	tristate "POWER9 OCC through SBE"
> >  	depends on FSI_OCC
> >-	select SENSORS_OCC
> >+	depends on POWERPC || COMPILE_TEST
> >  	help
> >  	 This option enables support for monitoring sensors provided by the
> >  	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
> >@@ -25,7 +25,3 @@ config SENSORS_OCC_P9_SBE
> >  	 This driver can also be built as a module. If so, the module will be
> >  	 called occ-p9-hwmon.
> >-
> >-config SENSORS_OCC
> >-	bool "POWER On-Chip Controller"
> >-	depends on SENSORS_OCC_P8_I2C || SENSORS_OCC_P9_SBE
> >--- linux-5.0.orig/drivers/hwmon/Makefile	2019-03-04 00:21:29.000000000 +0100
> >+++ linux-5.0/drivers/hwmon/Makefile	2019-04-09 14:33:49.605510047 +0200
> >@@ -178,7 +178,7 @@ obj-$(CONFIG_SENSORS_WM831X)	+= wm831x-h
> >  obj-$(CONFIG_SENSORS_WM8350)	+= wm8350-hwmon.o
> >  obj-$(CONFIG_SENSORS_XGENE)	+= xgene-hwmon.o
> >-obj-$(CONFIG_SENSORS_OCC)	+= occ/
> >+obj-y				+= occ/
> >  obj-$(CONFIG_PMBUS)		+= pmbus/
> >  ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
> >
> >
>