| Message ID | 1498171716-26620-2-git-send-email-eajames@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On 06/22/2017 03:48 PM, Eddie James wrote: > From: "Edward A. James" <eajames@us.ibm.com> > > The OCC is a device embedded on a POWER processor that collects and > aggregates sensor data from the processor and system. The OCC can > provide the raw sensor data as well as perform thermal and power > management on the system. > > This driver provides a hwmon interface to the OCC from a service > processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs. > Communications with the POWER8 OCC are established over standard I2C > bus. The driver communicates with the POWER9 OCC through the FSI-based > OCC driver, which handles the lower-level communication details. > > This patch lays out the structure of the OCC hwmon driver. There are two > platform drivers, one each for P8 and P9 OCCs. These are probed through > the I2C tree and the FSI-based OCC driver, respectively. The patch also Where is that driver ? > defines the first common structures and methods between the two OCC > versions. > > Signed-off-by: Edward A. James <eajames@us.ibm.com> > --- > .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++ > .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++ > drivers/hwmon/Kconfig | 2 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/occ/Kconfig | 28 +++++++++ > drivers/hwmon/occ/Makefile | 11 ++++ > drivers/hwmon/occ/common.c | 43 +++++++++++++ > drivers/hwmon/occ/common.h | 41 +++++++++++++ > drivers/hwmon/occ/p8_i2c.c | 70 ++++++++++++++++++++++ > drivers/hwmon/occ/p9_sbe.c | 65 ++++++++++++++++++++ > 10 files changed, 304 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > create mode 100644 Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > create mode 100644 drivers/hwmon/occ/Kconfig > create mode 100644 drivers/hwmon/occ/Makefile > create mode 100644 drivers/hwmon/occ/common.c > create mode 100644 drivers/hwmon/occ/common.h > create mode 100644 drivers/hwmon/occ/p8_i2c.c > create mode 100644 drivers/hwmon/occ/p9_sbe.c > > diff --git a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > new file mode 100644 > index 0000000..0ecebb7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt This needs to be approved by a DT maintainer, if for nothing else for the new directory and file naming. For my part I have no idea how this relates to the "fsi" directory introduced in -next. > @@ -0,0 +1,18 @@ > +Device-tree bindings for FSI-based On-Chip Controller hwmon driver > +------------------------------------------------------------------ > + > +This node MUST be a child node of an OCC driver node. > + > +Required properties: > + - compatible = "ibm,p9-occ-hwmon"; > + > +Examples: > + > + occ@1 { > + compatible = "ibm,p9-occ"; I don't see "ibm,p9-occ" documented anywhere (including linux-next). > + reg = <1>; > + > + occ-hwmon@1 { > + compatible = "ibm,p9-occ-hwmon"; > + }; > + }; > diff --git a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > new file mode 100644 > index 0000000..8c765d0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > @@ -0,0 +1,25 @@ > +Device-tree bindings for I2C-based On-Chip Controller hwmon driver > +------------------------------------------------------------------ > + > +Required properties: > + - compatible = "ibm,p8-occ-hwmon"; > + - reg = <I2C address>; : I2C bus address > + > +Examples: > + > + i2c-bus@100 { > + #address-cells = <1>; > + #size-cells = <0>; > + clock-frequency = <100000>; > + < more properties > > + > + occ-hwmon@1 { > + compatible = "ibm,p8-occ-hwmon"; > + reg = <0x50>; > + }; > + > + occ-hwmon@2 { > + compatible = "ibm,p8-occ-hwmon"; > + reg = <0x51>; > + }; > + }; > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 5ef2814..08add7b 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1250,6 +1250,8 @@ config SENSORS_NSA320 > This driver can also be built as a module. If so, the module > will be called nsa320-hwmon. > > +source drivers/hwmon/occ/Kconfig > + > config SENSORS_PCF8591 > tristate "Philips PCF8591 ADC/DAC" > depends on I2C > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index d4641a9..0b342d0 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o > > +obj-$(CONFIG_SENSORS_OCC) += occ/ > obj-$(CONFIG_PMBUS) += pmbus/ > > ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG > diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig > new file mode 100644 > index 0000000..0501280 > --- /dev/null > +++ b/drivers/hwmon/occ/Kconfig > @@ -0,0 +1,28 @@ > +# > +# On-Chip Controller configuration > +# > + > +config SENSORS_OCC > + tristate "POWER On-Chip Controller" > + ---help--- > + This option enables support for monitoring a variety of system sensors > + provided by the On-Chip Controller (OCC) on a POWER processor. > + > + This driver can also be built as a module. If so, the module will be > + called occ-hwmon. > + > +config SENSORS_OCC_P8_I2C > + bool "POWER8 OCC through I2C" > + depends on I2C && SENSORS_OCC > + ---help--- > + This option enables support for monitoring sensors provided by the OCC > + on a POWER8 processor. Communications with the OCC are established > + through I2C bus. > + > +config SENSORS_OCC_P9_SBE > + bool "POWER9 OCC through SBE" > + depends on OCCFIFO && SENSORS_OCC OCCFIFO is not declared anywhere in the kernel, including -next. This leads me to believe that I am missing some context. As a result, I can not even compile this driver. Please provide the missing context. > + ---help--- > + This option enables support for monitoring sensors provided by the OCC > + on a POWER9 processor. Communications with the OCC are established > + through SBE engine on an FSI bus. > diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile > new file mode 100644 > index 0000000..ab5c3e9 > --- /dev/null > +++ b/drivers/hwmon/occ/Makefile > @@ -0,0 +1,11 @@ > +occ-hwmon-objs := common.o > + > +ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y) > +occ-hwmon-objs += p9_sbe.o > +endif > + > +ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y) > +occ-hwmon-objs += p8_i2c.o > +endif > + > +obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o > diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c > new file mode 100644 > index 0000000..60c808c > --- /dev/null > +++ b/drivers/hwmon/occ/common.c > @@ -0,0 +1,43 @@ > +/* > + * Copyright 2017 IBM Corp. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include "common.h" Please include local include files last, and include files needed here from here, not indirectly. > +#include <linux/hwmon.h> > + > +static int occ_poll(struct occ *occ) > +{ > + u16 checksum = occ->poll_cmd_data + 1; > + u8 cmd[8]; > + > + /* big endian */ > + cmd[0] = 0; /* sequence number */ > + cmd[1] = 0; /* cmd type */ > + cmd[2] = 0; /* data length msb */ > + cmd[3] = 1; /* data length lsb */ > + cmd[4] = occ->poll_cmd_data; /* data */ > + cmd[5] = checksum >> 8; /* checksum msb */ > + cmd[6] = checksum & 0xFF; /* checksum lsb */ > + cmd[7] = 0; > + > + return occ->send_cmd(occ, cmd); > +} > + > +int occ_setup(struct occ *occ, const char *name) > +{ > + int rc; > + > + rc = occ_poll(occ); > + if (rc < 0) { > + dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n", > + rc); > + return rc; > + } > + > + return 0; > +} > diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h > new file mode 100644 > index 0000000..dca642a > --- /dev/null > +++ b/drivers/hwmon/occ/common.h > @@ -0,0 +1,41 @@ > +/* > + * Copyright 2017 IBM Corp. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#ifndef OCC_COMMON_H > +#define OCC_COMMON_H > + > +#include <linux/hwmon-sysfs.h> > +#include <linux/sysfs.h> Those include files are not needed here. Other include files, which are needed, are missing. You can not expect the above to include everything you need for you. > + > +#define OCC_RESP_DATA_BYTES 4089 > + > +/* Same response format for all OCC versions. > + * Allocate the largest possible response. > + */ > +struct occ_response { > + u8 seq_no; > + u8 cmd_type; > + u8 return_status; > + u16 data_length_be; Why not "__be16 data_length" if that is what it is ? > + u8 data[OCC_RESP_DATA_BYTES]; > + u16 checksum_be; Same here. > +} __packed; > + > +struct occ { > + struct device *bus_dev; > + > + struct occ_response resp; > + > + u8 poll_cmd_data; /* to perform OCC poll command */ > + int (*send_cmd)(struct occ *occ, u8 *cmd); > +}; > + > +int occ_setup(struct occ *occ, const char *name); > + > +#endif /* OCC_COMMON_H */ > diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c > new file mode 100644 > index 0000000..5075146 > --- /dev/null > +++ b/drivers/hwmon/occ/p8_i2c.c > @@ -0,0 +1,70 @@ > +/* > + * Copyright 2017 IBM Corp. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include "common.h" > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/module.h> Please include files in alphabetic order, and local include file(s) last. > + > +struct p8_i2c_occ { > + struct occ occ; > + struct i2c_client *client; > +}; > + > +#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ) > + > +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd) > +{ > + return -EOPNOTSUPP; > +} > + > +static int p8_i2c_occ_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct occ *occ; > + struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev, > + sizeof(*p8_i2c_occ), > + GFP_KERNEL); I am quite sure this results in a checkpatch warning. It is also clumsy, with all those continuation lines. I would sugegst to split the variable declaration and the assignment. > + if (!p8_i2c_occ) > + return -ENOMEM; > + > + p8_i2c_occ->client = client; > + occ = &p8_i2c_occ->occ; > + occ->bus_dev = &client->dev; > + dev_set_drvdata(&client->dev, occ); > + > + occ->poll_cmd_data = 0x10; /* P8 OCC poll data */ It would be beneficial to define those commands in the include file. > + occ->send_cmd = p8_i2c_occ_send_cmd; > + > + return occ_setup(occ, "p8_occ"); > +} > + > +static const struct of_device_id p8_i2c_occ_of_match[] = { > + { .compatible = "ibm,p8-occ-hwmon" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match); > + > +static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51, I2C_CLIENT_END }; Those addresses should never be listed for auto-detection. > + > +static struct i2c_driver p8_i2c_occ_driver = { > + .class = I2C_CLASS_HWMON, FWIW, this only adds value if the driver has a detect function. > + .driver = { > + .name = "occ-hwmon", > + .of_match_table = p8_i2c_occ_of_match, > + }, > + .probe = p8_i2c_occ_probe, > + .address_list = p8_i2c_occ_addr, Same here. > +}; > + > +module_i2c_driver(p8_i2c_occ_driver); > + > +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); > +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c > new file mode 100644 > index 0000000..0cef428 > --- /dev/null > +++ b/drivers/hwmon/occ/p9_sbe.c > @@ -0,0 +1,65 @@ > +/* > + * Copyright 2017 IBM Corp. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include "common.h" > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/occ.h> > + Alphabetic order, and local include file(s) last. > +struct p9_sbe_occ { > + struct occ occ; > + struct device *sbe; > +}; > + > +#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ) > + > +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) > +{ > + return -EOPNOTSUPP; > +} > + > +static int p9_sbe_occ_probe(struct platform_device *pdev) > +{ > + struct occ *occ; > + struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev, > + sizeof(*p9_sbe_occ), > + GFP_KERNEL); Same as above. > + if (!p9_sbe_occ) > + return -ENOMEM; > + > + p9_sbe_occ->sbe = pdev->dev.parent; > + occ = &p9_sbe_occ->occ; > + occ->bus_dev = &pdev->dev; > + platform_set_drvdata(pdev, occ); > + > + occ->poll_cmd_data = 0x20; /* P9 OCC poll data */ > + occ->send_cmd = p9_sbe_occ_send_cmd; > + > + return occ_setup(occ, "p9_occ"); > +} > + > +static const struct of_device_id p9_sbe_occ_of_match[] = { > + { .compatible = "ibm,p9-occ-hwmon" }, > + { }, > +}; > + > +static struct platform_driver p9_sbe_occ_driver = { > + .driver = { > + .name = "occ-hwmon", > + .of_match_table = p9_sbe_occ_of_match, > + }, > + .probe = p9_sbe_occ_probe, > +}; > + > +module_platform_driver(p9_sbe_occ_driver); > + > +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); > +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/24/2017 09:15 AM, Guenter Roeck wrote: > On 06/22/2017 03:48 PM, Eddie James wrote: >> From: "Edward A. James" <eajames@us.ibm.com> >> >> The OCC is a device embedded on a POWER processor that collects and >> aggregates sensor data from the processor and system. The OCC can >> provide the raw sensor data as well as perform thermal and power >> management on the system. >> >> This driver provides a hwmon interface to the OCC from a service >> processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs. >> Communications with the POWER8 OCC are established over standard I2C >> bus. The driver communicates with the POWER9 OCC through the FSI-based >> OCC driver, which handles the lower-level communication details. >> >> This patch lays out the structure of the OCC hwmon driver. There are two >> platform drivers, one each for P8 and P9 OCCs. These are probed through >> the I2C tree and the FSI-based OCC driver, respectively. The patch also > > Where is that driver ? My apologies Guenter, here is the series: https://patchwork.kernel.org/patch/9802807/ https://patchwork.kernel.org/patch/9802801/ https://patchwork.kernel.org/patch/9802805/ https://patchwork.kernel.org/patch/9802803/ Do these FSI drivers need to be into linux-next before you want to look at this hwmon driver? Just a couple questions below on your comments. Thanks, Eddie > >> defines the first common structures and methods between the two OCC >> versions. >> >> Signed-off-by: Edward A. James <eajames@us.ibm.com> >> --- >> .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++ >> .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++ >> drivers/hwmon/Kconfig | 2 + >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/occ/Kconfig | 28 +++++++++ >> drivers/hwmon/occ/Makefile | 11 ++++ >> drivers/hwmon/occ/common.c | 43 +++++++++++++ >> drivers/hwmon/occ/common.h | 41 +++++++++++++ >> drivers/hwmon/occ/p8_i2c.c | 70 >> ++++++++++++++++++++++ >> drivers/hwmon/occ/p9_sbe.c | 65 >> ++++++++++++++++++++ >> 10 files changed, 304 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> create mode 100644 >> Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> create mode 100644 drivers/hwmon/occ/Kconfig >> create mode 100644 drivers/hwmon/occ/Makefile >> create mode 100644 drivers/hwmon/occ/common.c >> create mode 100644 drivers/hwmon/occ/common.h >> create mode 100644 drivers/hwmon/occ/p8_i2c.c >> create mode 100644 drivers/hwmon/occ/p9_sbe.c >> >> diff --git >> a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt >> new file mode 100644 >> index 0000000..0ecebb7 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > > This needs to be approved by a DT maintainer, if for nothing else for > the new directory and file naming. For my part I have no idea how this > relates to the "fsi" directory introduced in -next. > > >> @@ -0,0 +1,18 @@ >> +Device-tree bindings for FSI-based On-Chip Controller hwmon driver >> +------------------------------------------------------------------ >> + >> +This node MUST be a child node of an OCC driver node. >> + >> +Required properties: >> + - compatible = "ibm,p9-occ-hwmon"; >> + >> +Examples: >> + >> + occ@1 { >> + compatible = "ibm,p9-occ"; > > I don't see "ibm,p9-occ" documented anywhere (including linux-next). > >> + reg = <1>; >> + >> + occ-hwmon@1 { >> + compatible = "ibm,p9-occ-hwmon"; >> + }; >> + }; >> diff --git >> a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> new file mode 100644 >> index 0000000..8c765d0 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt >> @@ -0,0 +1,25 @@ >> +Device-tree bindings for I2C-based On-Chip Controller hwmon driver >> +------------------------------------------------------------------ >> + >> +Required properties: >> + - compatible = "ibm,p8-occ-hwmon"; >> + - reg = <I2C address>; : I2C bus address >> + >> +Examples: >> + >> + i2c-bus@100 { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + clock-frequency = <100000>; >> + < more properties > >> + >> + occ-hwmon@1 { >> + compatible = "ibm,p8-occ-hwmon"; >> + reg = <0x50>; >> + }; >> + >> + occ-hwmon@2 { >> + compatible = "ibm,p8-occ-hwmon"; >> + reg = <0x51>; >> + }; >> + }; >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 5ef2814..08add7b 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1250,6 +1250,8 @@ config SENSORS_NSA320 >> This driver can also be built as a module. If so, the module >> will be called nsa320-hwmon. >> +source drivers/hwmon/occ/Kconfig >> + >> config SENSORS_PCF8591 >> tristate "Philips PCF8591 ADC/DAC" >> depends on I2C >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index d4641a9..0b342d0 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o >> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o >> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o >> +obj-$(CONFIG_SENSORS_OCC) += occ/ >> obj-$(CONFIG_PMBUS) += pmbus/ >> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG >> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig >> new file mode 100644 >> index 0000000..0501280 >> --- /dev/null >> +++ b/drivers/hwmon/occ/Kconfig >> @@ -0,0 +1,28 @@ >> +# >> +# On-Chip Controller configuration >> +# >> + >> +config SENSORS_OCC >> + tristate "POWER On-Chip Controller" >> + ---help--- >> + This option enables support for monitoring a variety of system >> sensors >> + provided by the On-Chip Controller (OCC) on a POWER processor. >> + >> + This driver can also be built as a module. If so, the module >> will be >> + called occ-hwmon. >> + >> +config SENSORS_OCC_P8_I2C >> + bool "POWER8 OCC through I2C" >> + depends on I2C && SENSORS_OCC >> + ---help--- >> + This option enables support for monitoring sensors provided by >> the OCC >> + on a POWER8 processor. Communications with the OCC are established >> + through I2C bus. >> + >> +config SENSORS_OCC_P9_SBE >> + bool "POWER9 OCC through SBE" >> + depends on OCCFIFO && SENSORS_OCC > > OCCFIFO is not declared anywhere in the kernel, including -next. This > leads me to > believe that I am missing some context. As a result, I can not even > compile this driver. > Please provide the missing context. > >> + ---help--- >> + This option enables support for monitoring sensors provided by >> the OCC >> + on a POWER9 processor. Communications with the OCC are established >> + through SBE engine on an FSI bus. >> diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile >> new file mode 100644 >> index 0000000..ab5c3e9 >> --- /dev/null >> +++ b/drivers/hwmon/occ/Makefile >> @@ -0,0 +1,11 @@ >> +occ-hwmon-objs := common.o >> + >> +ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y) >> +occ-hwmon-objs += p9_sbe.o >> +endif >> + >> +ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y) >> +occ-hwmon-objs += p8_i2c.o >> +endif >> + >> +obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o >> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c >> new file mode 100644 >> index 0000000..60c808c >> --- /dev/null >> +++ b/drivers/hwmon/occ/common.c >> @@ -0,0 +1,43 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" > > Please include local include files last, and include files needed here > from here, not indirectly. > >> +#include <linux/hwmon.h> >> + >> +static int occ_poll(struct occ *occ) >> +{ >> + u16 checksum = occ->poll_cmd_data + 1; >> + u8 cmd[8]; >> + >> + /* big endian */ >> + cmd[0] = 0; /* sequence number */ >> + cmd[1] = 0; /* cmd type */ >> + cmd[2] = 0; /* data length msb */ >> + cmd[3] = 1; /* data length lsb */ >> + cmd[4] = occ->poll_cmd_data; /* data */ >> + cmd[5] = checksum >> 8; /* checksum msb */ >> + cmd[6] = checksum & 0xFF; /* checksum lsb */ >> + cmd[7] = 0; >> + >> + return occ->send_cmd(occ, cmd); >> +} >> + >> +int occ_setup(struct occ *occ, const char *name) >> +{ >> + int rc; >> + >> + rc = occ_poll(occ); >> + if (rc < 0) { >> + dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n", >> + rc); >> + return rc; >> + } >> + >> + return 0; >> +} >> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h >> new file mode 100644 >> index 0000000..dca642a >> --- /dev/null >> +++ b/drivers/hwmon/occ/common.h >> @@ -0,0 +1,41 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#ifndef OCC_COMMON_H >> +#define OCC_COMMON_H >> + >> +#include <linux/hwmon-sysfs.h> >> +#include <linux/sysfs.h> > > Those include files are not needed here. Other include files, > which are needed, are missing. You can not expect the above > to include everything you need for you. > >> + >> +#define OCC_RESP_DATA_BYTES 4089 >> + >> +/* Same response format for all OCC versions. >> + * Allocate the largest possible response. >> + */ >> +struct occ_response { >> + u8 seq_no; >> + u8 cmd_type; >> + u8 return_status; >> + u16 data_length_be; > > Why not "__be16 data_length" if that is what it is ? > >> + u8 data[OCC_RESP_DATA_BYTES]; >> + u16 checksum_be; > > Same here. > >> +} __packed; >> + >> +struct occ { >> + struct device *bus_dev; >> + >> + struct occ_response resp; >> + >> + u8 poll_cmd_data; /* to perform OCC poll command */ >> + int (*send_cmd)(struct occ *occ, u8 *cmd); >> +}; >> + >> +int occ_setup(struct occ *occ, const char *name); >> + >> +#endif /* OCC_COMMON_H */ >> diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c >> new file mode 100644 >> index 0000000..5075146 >> --- /dev/null >> +++ b/drivers/hwmon/occ/p8_i2c.c >> @@ -0,0 +1,70 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" >> +#include <linux/i2c.h> >> +#include <linux/init.h> >> +#include <linux/module.h> > > Please include files in alphabetic order, and local include file(s) last. > >> + >> +struct p8_i2c_occ { >> + struct occ occ; >> + struct i2c_client *client; >> +}; >> + >> +#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ) >> + >> +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static int p8_i2c_occ_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct occ *occ; >> + struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev, >> + sizeof(*p8_i2c_occ), >> + GFP_KERNEL); > > I am quite sure this results in a checkpatch warning. It is also > clumsy, with > all those continuation lines. I would sugegst to split the variable > declaration > and the assignment. > >> + if (!p8_i2c_occ) >> + return -ENOMEM; >> + >> + p8_i2c_occ->client = client; >> + occ = &p8_i2c_occ->occ; >> + occ->bus_dev = &client->dev; >> + dev_set_drvdata(&client->dev, occ); >> + >> + occ->poll_cmd_data = 0x10; /* P8 OCC poll data */ > > It would be beneficial to define those commands in the include file. I can add a #define for them, but I'm not sure it makes sense to have the P8/P9 specific command in the common include file? They don't need to be used anywhere else. > >> + occ->send_cmd = p8_i2c_occ_send_cmd; >> + >> + return occ_setup(occ, "p8_occ"); >> +} >> + >> +static const struct of_device_id p8_i2c_occ_of_match[] = { >> + { .compatible = "ibm,p8-occ-hwmon" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match); >> + >> +static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51, >> I2C_CLIENT_END }; > > Those addresses should never be listed for auto-detection. Why not? I see lots of drivers in the kernel with a list of I2C addresses to scan. > >> + >> +static struct i2c_driver p8_i2c_occ_driver = { >> + .class = I2C_CLASS_HWMON, > > FWIW, this only adds value if the driver has a detect function. > >> + .driver = { >> + .name = "occ-hwmon", >> + .of_match_table = p8_i2c_occ_of_match, >> + }, >> + .probe = p8_i2c_occ_probe, >> + .address_list = p8_i2c_occ_addr, > > Same here. > >> +}; >> + >> +module_i2c_driver(p8_i2c_occ_driver); >> + >> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); >> +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c >> new file mode 100644 >> index 0000000..0cef428 >> --- /dev/null >> +++ b/drivers/hwmon/occ/p9_sbe.c >> @@ -0,0 +1,65 @@ >> +/* >> + * Copyright 2017 IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include "common.h" >> +#include <linux/init.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/occ.h> >> + > Alphabetic order, and local include file(s) last. > >> +struct p9_sbe_occ { >> + struct occ occ; >> + struct device *sbe; >> +}; >> + >> +#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ) >> + >> +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) >> +{ >> + return -EOPNOTSUPP; >> +} >> + >> +static int p9_sbe_occ_probe(struct platform_device *pdev) >> +{ >> + struct occ *occ; >> + struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev, >> + sizeof(*p9_sbe_occ), >> + GFP_KERNEL); > > Same as above. > >> + if (!p9_sbe_occ) >> + return -ENOMEM; >> + >> + p9_sbe_occ->sbe = pdev->dev.parent; >> + occ = &p9_sbe_occ->occ; >> + occ->bus_dev = &pdev->dev; >> + platform_set_drvdata(pdev, occ); >> + >> + occ->poll_cmd_data = 0x20; /* P9 OCC poll data */ >> + occ->send_cmd = p9_sbe_occ_send_cmd; >> + >> + return occ_setup(occ, "p9_occ"); >> +} >> + >> +static const struct of_device_id p9_sbe_occ_of_match[] = { >> + { .compatible = "ibm,p9-occ-hwmon" }, >> + { }, >> +}; >> + >> +static struct platform_driver p9_sbe_occ_driver = { >> + .driver = { >> + .name = "occ-hwmon", >> + .of_match_table = p9_sbe_occ_of_match, >> + }, >> + .probe = p9_sbe_occ_probe, >> +}; >> + >> +module_platform_driver(p9_sbe_occ_driver); >> + >> +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); >> +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver"); >> +MODULE_LICENSE("GPL"); >> > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 26, 2017 at 10:06:50AM -0500, Eddie James wrote: > > > On 06/24/2017 09:15 AM, Guenter Roeck wrote: > >On 06/22/2017 03:48 PM, Eddie James wrote: > >>From: "Edward A. James" <eajames@us.ibm.com> > >> > >>The OCC is a device embedded on a POWER processor that collects and > >>aggregates sensor data from the processor and system. The OCC can > >>provide the raw sensor data as well as perform thermal and power > >>management on the system. > >> > >>This driver provides a hwmon interface to the OCC from a service > >>processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs. > >>Communications with the POWER8 OCC are established over standard I2C > >>bus. The driver communicates with the POWER9 OCC through the FSI-based > >>OCC driver, which handles the lower-level communication details. > >> > >>This patch lays out the structure of the OCC hwmon driver. There are two > >>platform drivers, one each for P8 and P9 OCCs. These are probed through > >>the I2C tree and the FSI-based OCC driver, respectively. The patch also > > > >Where is that driver ? > > My apologies Guenter, here is the series: > > https://patchwork.kernel.org/patch/9802807/ > https://patchwork.kernel.org/patch/9802801/ > https://patchwork.kernel.org/patch/9802805/ > https://patchwork.kernel.org/patch/9802803/ > > Do these FSI drivers need to be into linux-next before you want to look at > this hwmon driver? > Not necessarily in -next, but I'll need to have at least an immutable branch to be able to integrate the series. > Just a couple questions below on your comments. > > Thanks, > Eddie > > > > >>defines the first common structures and methods between the two OCC > >>versions. > >> > >>Signed-off-by: Edward A. James <eajames@us.ibm.com> > >>--- > >> .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++ > >> .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++ > >> drivers/hwmon/Kconfig | 2 + > >> drivers/hwmon/Makefile | 1 + > >> drivers/hwmon/occ/Kconfig | 28 +++++++++ > >> drivers/hwmon/occ/Makefile | 11 ++++ > >> drivers/hwmon/occ/common.c | 43 +++++++++++++ > >> drivers/hwmon/occ/common.h | 41 +++++++++++++ > >> drivers/hwmon/occ/p8_i2c.c | 70 > >>++++++++++++++++++++++ > >> drivers/hwmon/occ/p9_sbe.c | 65 > >>++++++++++++++++++++ > >> 10 files changed, 304 insertions(+) > >> create mode 100644 > >>Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > >> create mode 100644 > >>Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > >> create mode 100644 drivers/hwmon/occ/Kconfig > >> create mode 100644 drivers/hwmon/occ/Makefile > >> create mode 100644 drivers/hwmon/occ/common.c > >> create mode 100644 drivers/hwmon/occ/common.h > >> create mode 100644 drivers/hwmon/occ/p8_i2c.c > >> create mode 100644 drivers/hwmon/occ/p9_sbe.c > >> > >>diff --git a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > >>b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > >>new file mode 100644 > >>index 0000000..0ecebb7 > >>--- /dev/null > >>+++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > > > >This needs to be approved by a DT maintainer, if for nothing else for > >the new directory and file naming. For my part I have no idea how this > >relates to the "fsi" directory introduced in -next. > > > > > >>@@ -0,0 +1,18 @@ > >>+Device-tree bindings for FSI-based On-Chip Controller hwmon driver > >>+------------------------------------------------------------------ > >>+ > >>+This node MUST be a child node of an OCC driver node. > >>+ > >>+Required properties: > >>+ - compatible = "ibm,p9-occ-hwmon"; > >>+ > >>+Examples: > >>+ > >>+ occ@1 { > >>+ compatible = "ibm,p9-occ"; > > > >I don't see "ibm,p9-occ" documented anywhere (including linux-next). > > > >>+ reg = <1>; > >>+ > >>+ occ-hwmon@1 { > >>+ compatible = "ibm,p9-occ-hwmon"; > >>+ }; > >>+ }; > >>diff --git a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > >>b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > >>new file mode 100644 > >>index 0000000..8c765d0 > >>--- /dev/null > >>+++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > >>@@ -0,0 +1,25 @@ > >>+Device-tree bindings for I2C-based On-Chip Controller hwmon driver > >>+------------------------------------------------------------------ > >>+ > >>+Required properties: > >>+ - compatible = "ibm,p8-occ-hwmon"; > >>+ - reg = <I2C address>; : I2C bus address > >>+ > >>+Examples: > >>+ > >>+ i2c-bus@100 { > >>+ #address-cells = <1>; > >>+ #size-cells = <0>; > >>+ clock-frequency = <100000>; > >>+ < more properties > > >>+ > >>+ occ-hwmon@1 { > >>+ compatible = "ibm,p8-occ-hwmon"; > >>+ reg = <0x50>; > >>+ }; > >>+ > >>+ occ-hwmon@2 { > >>+ compatible = "ibm,p8-occ-hwmon"; > >>+ reg = <0x51>; > >>+ }; > >>+ }; > >>diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > >>index 5ef2814..08add7b 100644 > >>--- a/drivers/hwmon/Kconfig > >>+++ b/drivers/hwmon/Kconfig > >>@@ -1250,6 +1250,8 @@ config SENSORS_NSA320 > >> This driver can also be built as a module. If so, the module > >> will be called nsa320-hwmon. > >> +source drivers/hwmon/occ/Kconfig > >>+ > >> config SENSORS_PCF8591 > >> tristate "Philips PCF8591 ADC/DAC" > >> depends on I2C > >>diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > >>index d4641a9..0b342d0 100644 > >>--- a/drivers/hwmon/Makefile > >>+++ b/drivers/hwmon/Makefile > >>@@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o > >> obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o > >> obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o > >> +obj-$(CONFIG_SENSORS_OCC) += occ/ > >> obj-$(CONFIG_PMBUS) += pmbus/ > >> ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG > >>diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig > >>new file mode 100644 > >>index 0000000..0501280 > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/Kconfig > >>@@ -0,0 +1,28 @@ > >>+# > >>+# On-Chip Controller configuration > >>+# > >>+ > >>+config SENSORS_OCC > >>+ tristate "POWER On-Chip Controller" > >>+ ---help--- > >>+ This option enables support for monitoring a variety of system > >>sensors > >>+ provided by the On-Chip Controller (OCC) on a POWER processor. > >>+ > >>+ This driver can also be built as a module. If so, the module will > >>be > >>+ called occ-hwmon. > >>+ > >>+config SENSORS_OCC_P8_I2C > >>+ bool "POWER8 OCC through I2C" > >>+ depends on I2C && SENSORS_OCC > >>+ ---help--- > >>+ This option enables support for monitoring sensors provided by the > >>OCC > >>+ on a POWER8 processor. Communications with the OCC are established > >>+ through I2C bus. > >>+ > >>+config SENSORS_OCC_P9_SBE > >>+ bool "POWER9 OCC through SBE" > >>+ depends on OCCFIFO && SENSORS_OCC > > > >OCCFIFO is not declared anywhere in the kernel, including -next. This > >leads me to > >believe that I am missing some context. As a result, I can not even > >compile this driver. > >Please provide the missing context. > > > >>+ ---help--- > >>+ This option enables support for monitoring sensors provided by the > >>OCC > >>+ on a POWER9 processor. Communications with the OCC are established > >>+ through SBE engine on an FSI bus. > >>diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile > >>new file mode 100644 > >>index 0000000..ab5c3e9 > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/Makefile > >>@@ -0,0 +1,11 @@ > >>+occ-hwmon-objs := common.o > >>+ > >>+ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y) > >>+occ-hwmon-objs += p9_sbe.o > >>+endif > >>+ > >>+ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y) > >>+occ-hwmon-objs += p8_i2c.o > >>+endif > >>+ > >>+obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o > >>diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c > >>new file mode 100644 > >>index 0000000..60c808c > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/common.c > >>@@ -0,0 +1,43 @@ > >>+/* > >>+ * Copyright 2017 IBM Corp. > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#include "common.h" > > > >Please include local include files last, and include files needed here > >from here, not indirectly. > > > >>+#include <linux/hwmon.h> > >>+ > >>+static int occ_poll(struct occ *occ) > >>+{ > >>+ u16 checksum = occ->poll_cmd_data + 1; > >>+ u8 cmd[8]; > >>+ > >>+ /* big endian */ > >>+ cmd[0] = 0; /* sequence number */ > >>+ cmd[1] = 0; /* cmd type */ > >>+ cmd[2] = 0; /* data length msb */ > >>+ cmd[3] = 1; /* data length lsb */ > >>+ cmd[4] = occ->poll_cmd_data; /* data */ > >>+ cmd[5] = checksum >> 8; /* checksum msb */ > >>+ cmd[6] = checksum & 0xFF; /* checksum lsb */ > >>+ cmd[7] = 0; > >>+ > >>+ return occ->send_cmd(occ, cmd); > >>+} > >>+ > >>+int occ_setup(struct occ *occ, const char *name) > >>+{ > >>+ int rc; > >>+ > >>+ rc = occ_poll(occ); > >>+ if (rc < 0) { > >>+ dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n", > >>+ rc); > >>+ return rc; > >>+ } > >>+ > >>+ return 0; > >>+} > >>diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h > >>new file mode 100644 > >>index 0000000..dca642a > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/common.h > >>@@ -0,0 +1,41 @@ > >>+/* > >>+ * Copyright 2017 IBM Corp. > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#ifndef OCC_COMMON_H > >>+#define OCC_COMMON_H > >>+ > >>+#include <linux/hwmon-sysfs.h> > >>+#include <linux/sysfs.h> > > > >Those include files are not needed here. Other include files, > >which are needed, are missing. You can not expect the above > >to include everything you need for you. > > > >>+ > >>+#define OCC_RESP_DATA_BYTES 4089 > >>+ > >>+/* Same response format for all OCC versions. > >>+ * Allocate the largest possible response. > >>+ */ > >>+struct occ_response { > >>+ u8 seq_no; > >>+ u8 cmd_type; > >>+ u8 return_status; > >>+ u16 data_length_be; > > > >Why not "__be16 data_length" if that is what it is ? > > > >>+ u8 data[OCC_RESP_DATA_BYTES]; > >>+ u16 checksum_be; > > > >Same here. > > > >>+} __packed; > >>+ > >>+struct occ { > >>+ struct device *bus_dev; > >>+ > >>+ struct occ_response resp; > >>+ > >>+ u8 poll_cmd_data; /* to perform OCC poll command */ > >>+ int (*send_cmd)(struct occ *occ, u8 *cmd); > >>+}; > >>+ > >>+int occ_setup(struct occ *occ, const char *name); > >>+ > >>+#endif /* OCC_COMMON_H */ > >>diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c > >>new file mode 100644 > >>index 0000000..5075146 > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/p8_i2c.c > >>@@ -0,0 +1,70 @@ > >>+/* > >>+ * Copyright 2017 IBM Corp. > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#include "common.h" > >>+#include <linux/i2c.h> > >>+#include <linux/init.h> > >>+#include <linux/module.h> > > > >Please include files in alphabetic order, and local include file(s) last. > > > >>+ > >>+struct p8_i2c_occ { > >>+ struct occ occ; > >>+ struct i2c_client *client; > >>+}; > >>+ > >>+#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ) > >>+ > >>+static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd) > >>+{ > >>+ return -EOPNOTSUPP; > >>+} > >>+ > >>+static int p8_i2c_occ_probe(struct i2c_client *client, > >>+ const struct i2c_device_id *id) > >>+{ > >>+ struct occ *occ; > >>+ struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev, > >>+ sizeof(*p8_i2c_occ), > >>+ GFP_KERNEL); > > > >I am quite sure this results in a checkpatch warning. It is also clumsy, > >with > >all those continuation lines. I would sugegst to split the variable > >declaration > >and the assignment. > > > >>+ if (!p8_i2c_occ) > >>+ return -ENOMEM; > >>+ > >>+ p8_i2c_occ->client = client; > >>+ occ = &p8_i2c_occ->occ; > >>+ occ->bus_dev = &client->dev; > >>+ dev_set_drvdata(&client->dev, occ); > >>+ > >>+ occ->poll_cmd_data = 0x10; /* P8 OCC poll data */ > > > >It would be beneficial to define those commands in the include file. > > I can add a #define for them, but I'm not sure it makes sense to have the > P8/P9 specific command in the common include file? They don't need to be > used anywhere else. > In the source file is ok as well. > > > >>+ occ->send_cmd = p8_i2c_occ_send_cmd; > >>+ > >>+ return occ_setup(occ, "p8_occ"); > >>+} > >>+ > >>+static const struct of_device_id p8_i2c_occ_of_match[] = { > >>+ { .compatible = "ibm,p8-occ-hwmon" }, > >>+ {} > >>+}; > >>+MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match); > >>+ > >>+static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51, > >>I2C_CLIENT_END }; > > > >Those addresses should never be listed for auto-detection. > > Why not? I see lots of drivers in the kernel with a list of I2C addresses to > scan. > Sure, but not in the EEPROM address range (0x50..0x57), and not without detect function. A detect function on an EEPROM is pretty much worthless; one would only have to write the right (or wrong) sequence of values into the EEPROM to cause lots of false positive detections. > > > >>+ > >>+static struct i2c_driver p8_i2c_occ_driver = { > >>+ .class = I2C_CLASS_HWMON, > > > >FWIW, this only adds value if the driver has a detect function. > > > >>+ .driver = { > >>+ .name = "occ-hwmon", > >>+ .of_match_table = p8_i2c_occ_of_match, > >>+ }, > >>+ .probe = p8_i2c_occ_probe, > >>+ .address_list = p8_i2c_occ_addr, > > > >Same here. > > > >>+}; > >>+ > >>+module_i2c_driver(p8_i2c_occ_driver); > >>+ > >>+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); > >>+MODULE_DESCRIPTION("BMC P8 OCC hwmon driver"); > >>+MODULE_LICENSE("GPL"); > >>diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c > >>new file mode 100644 > >>index 0000000..0cef428 > >>--- /dev/null > >>+++ b/drivers/hwmon/occ/p9_sbe.c > >>@@ -0,0 +1,65 @@ > >>+/* > >>+ * Copyright 2017 IBM Corp. > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#include "common.h" > >>+#include <linux/init.h> > >>+#include <linux/module.h> > >>+#include <linux/platform_device.h> > >>+#include <linux/occ.h> > >>+ > >Alphabetic order, and local include file(s) last. > > > >>+struct p9_sbe_occ { > >>+ struct occ occ; > >>+ struct device *sbe; > >>+}; > >>+ > >>+#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ) > >>+ > >>+static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) > >>+{ > >>+ return -EOPNOTSUPP; > >>+} > >>+ > >>+static int p9_sbe_occ_probe(struct platform_device *pdev) > >>+{ > >>+ struct occ *occ; > >>+ struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev, > >>+ sizeof(*p9_sbe_occ), > >>+ GFP_KERNEL); > > > >Same as above. > > > >>+ if (!p9_sbe_occ) > >>+ return -ENOMEM; > >>+ > >>+ p9_sbe_occ->sbe = pdev->dev.parent; > >>+ occ = &p9_sbe_occ->occ; > >>+ occ->bus_dev = &pdev->dev; > >>+ platform_set_drvdata(pdev, occ); > >>+ > >>+ occ->poll_cmd_data = 0x20; /* P9 OCC poll data */ > >>+ occ->send_cmd = p9_sbe_occ_send_cmd; > >>+ > >>+ return occ_setup(occ, "p9_occ"); > >>+} > >>+ > >>+static const struct of_device_id p9_sbe_occ_of_match[] = { > >>+ { .compatible = "ibm,p9-occ-hwmon" }, > >>+ { }, > >>+}; > >>+ > >>+static struct platform_driver p9_sbe_occ_driver = { > >>+ .driver = { > >>+ .name = "occ-hwmon", > >>+ .of_match_table = p9_sbe_occ_of_match, > >>+ }, > >>+ .probe = p9_sbe_occ_probe, > >>+}; > >>+ > >>+module_platform_driver(p9_sbe_occ_driver); > >>+ > >>+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); > >>+MODULE_DESCRIPTION("BMC P9 OCC hwmon driver"); > >>+MODULE_LICENSE("GPL"); > >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 22, 2017 at 05:48:30PM -0500, Eddie James wrote: > From: "Edward A. James" <eajames@us.ibm.com> > > The OCC is a device embedded on a POWER processor that collects and > aggregates sensor data from the processor and system. The OCC can > provide the raw sensor data as well as perform thermal and power > management on the system. > > This driver provides a hwmon interface to the OCC from a service > processor (e.g. a BMC). The driver supports both POWER8 and POWER9 OCCs. > Communications with the POWER8 OCC are established over standard I2C > bus. The driver communicates with the POWER9 OCC through the FSI-based > OCC driver, which handles the lower-level communication details. > > This patch lays out the structure of the OCC hwmon driver. There are two > platform drivers, one each for P8 and P9 OCCs. These are probed through > the I2C tree and the FSI-based OCC driver, respectively. The patch also > defines the first common structures and methods between the two OCC > versions. > > Signed-off-by: Edward A. James <eajames@us.ibm.com> > --- > .../devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt | 18 ++++++ > .../devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt | 25 ++++++++ > drivers/hwmon/Kconfig | 2 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/occ/Kconfig | 28 +++++++++ > drivers/hwmon/occ/Makefile | 11 ++++ > drivers/hwmon/occ/common.c | 43 +++++++++++++ > drivers/hwmon/occ/common.h | 41 +++++++++++++ > drivers/hwmon/occ/p8_i2c.c | 70 ++++++++++++++++++++++ > drivers/hwmon/occ/p9_sbe.c | 65 ++++++++++++++++++++ > 10 files changed, 304 insertions(+) > create mode 100644 Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > create mode 100644 Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt > create mode 100644 drivers/hwmon/occ/Kconfig > create mode 100644 drivers/hwmon/occ/Makefile > create mode 100644 drivers/hwmon/occ/common.c > create mode 100644 drivers/hwmon/occ/common.h > create mode 100644 drivers/hwmon/occ/p8_i2c.c > create mode 100644 drivers/hwmon/occ/p9_sbe.c > > diff --git a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > new file mode 100644 > index 0000000..0ecebb7 > --- /dev/null > +++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt > @@ -0,0 +1,18 @@ > +Device-tree bindings for FSI-based On-Chip Controller hwmon driver > +------------------------------------------------------------------ > + > +This node MUST be a child node of an OCC driver node. Bindings describe h/w, not drivers. And hwmon is a Linuxism. > + > +Required properties: > + - compatible = "ibm,p9-occ-hwmon"; > + > +Examples: > + > + occ@1 { > + compatible = "ibm,p9-occ"; > + reg = <1>; > + > + occ-hwmon@1 { > + compatible = "ibm,p9-occ-hwmon"; See my comment in the other version I just reviewed... > + }; > + }; -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt new file mode 100644 index 0000000..0ecebb7 --- /dev/null +++ b/Documentation/devicetree/bindings/fsi/ibm,p9-occ-hwmon.txt @@ -0,0 +1,18 @@ +Device-tree bindings for FSI-based On-Chip Controller hwmon driver +------------------------------------------------------------------ + +This node MUST be a child node of an OCC driver node. + +Required properties: + - compatible = "ibm,p9-occ-hwmon"; + +Examples: + + occ@1 { + compatible = "ibm,p9-occ"; + reg = <1>; + + occ-hwmon@1 { + compatible = "ibm,p9-occ-hwmon"; + }; + }; diff --git a/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt new file mode 100644 index 0000000..8c765d0 --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/ibm,p8-occ-hwmon.txt @@ -0,0 +1,25 @@ +Device-tree bindings for I2C-based On-Chip Controller hwmon driver +------------------------------------------------------------------ + +Required properties: + - compatible = "ibm,p8-occ-hwmon"; + - reg = <I2C address>; : I2C bus address + +Examples: + + i2c-bus@100 { + #address-cells = <1>; + #size-cells = <0>; + clock-frequency = <100000>; + < more properties > + + occ-hwmon@1 { + compatible = "ibm,p8-occ-hwmon"; + reg = <0x50>; + }; + + occ-hwmon@2 { + compatible = "ibm,p8-occ-hwmon"; + reg = <0x51>; + }; + }; diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 5ef2814..08add7b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1250,6 +1250,8 @@ config SENSORS_NSA320 This driver can also be built as a module. If so, the module will be called nsa320-hwmon. +source drivers/hwmon/occ/Kconfig + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index d4641a9..0b342d0 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -170,6 +170,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o +obj-$(CONFIG_SENSORS_OCC) += occ/ obj-$(CONFIG_PMBUS) += pmbus/ ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig new file mode 100644 index 0000000..0501280 --- /dev/null +++ b/drivers/hwmon/occ/Kconfig @@ -0,0 +1,28 @@ +# +# On-Chip Controller configuration +# + +config SENSORS_OCC + tristate "POWER On-Chip Controller" + ---help--- + This option enables support for monitoring a variety of system sensors + provided by the On-Chip Controller (OCC) on a POWER processor. + + This driver can also be built as a module. If so, the module will be + called occ-hwmon. + +config SENSORS_OCC_P8_I2C + bool "POWER8 OCC through I2C" + depends on I2C && SENSORS_OCC + ---help--- + This option enables support for monitoring sensors provided by the OCC + on a POWER8 processor. Communications with the OCC are established + through I2C bus. + +config SENSORS_OCC_P9_SBE + bool "POWER9 OCC through SBE" + depends on OCCFIFO && SENSORS_OCC + ---help--- + This option enables support for monitoring sensors provided by the OCC + on a POWER9 processor. Communications with the OCC are established + through SBE engine on an FSI bus. diff --git a/drivers/hwmon/occ/Makefile b/drivers/hwmon/occ/Makefile new file mode 100644 index 0000000..ab5c3e9 --- /dev/null +++ b/drivers/hwmon/occ/Makefile @@ -0,0 +1,11 @@ +occ-hwmon-objs := common.o + +ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y) +occ-hwmon-objs += p9_sbe.o +endif + +ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y) +occ-hwmon-objs += p8_i2c.o +endif + +obj-$(CONFIG_SENSORS_OCC) += occ-hwmon.o diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c new file mode 100644 index 0000000..60c808c --- /dev/null +++ b/drivers/hwmon/occ/common.c @@ -0,0 +1,43 @@ +/* + * Copyright 2017 IBM Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include "common.h" +#include <linux/hwmon.h> + +static int occ_poll(struct occ *occ) +{ + u16 checksum = occ->poll_cmd_data + 1; + u8 cmd[8]; + + /* big endian */ + cmd[0] = 0; /* sequence number */ + cmd[1] = 0; /* cmd type */ + cmd[2] = 0; /* data length msb */ + cmd[3] = 1; /* data length lsb */ + cmd[4] = occ->poll_cmd_data; /* data */ + cmd[5] = checksum >> 8; /* checksum msb */ + cmd[6] = checksum & 0xFF; /* checksum lsb */ + cmd[7] = 0; + + return occ->send_cmd(occ, cmd); +} + +int occ_setup(struct occ *occ, const char *name) +{ + int rc; + + rc = occ_poll(occ); + if (rc < 0) { + dev_err(occ->bus_dev, "failed to get OCC poll response: %d\n", + rc); + return rc; + } + + return 0; +} diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h new file mode 100644 index 0000000..dca642a --- /dev/null +++ b/drivers/hwmon/occ/common.h @@ -0,0 +1,41 @@ +/* + * Copyright 2017 IBM Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef OCC_COMMON_H +#define OCC_COMMON_H + +#include <linux/hwmon-sysfs.h> +#include <linux/sysfs.h> + +#define OCC_RESP_DATA_BYTES 4089 + +/* Same response format for all OCC versions. + * Allocate the largest possible response. + */ +struct occ_response { + u8 seq_no; + u8 cmd_type; + u8 return_status; + u16 data_length_be; + u8 data[OCC_RESP_DATA_BYTES]; + u16 checksum_be; +} __packed; + +struct occ { + struct device *bus_dev; + + struct occ_response resp; + + u8 poll_cmd_data; /* to perform OCC poll command */ + int (*send_cmd)(struct occ *occ, u8 *cmd); +}; + +int occ_setup(struct occ *occ, const char *name); + +#endif /* OCC_COMMON_H */ diff --git a/drivers/hwmon/occ/p8_i2c.c b/drivers/hwmon/occ/p8_i2c.c new file mode 100644 index 0000000..5075146 --- /dev/null +++ b/drivers/hwmon/occ/p8_i2c.c @@ -0,0 +1,70 @@ +/* + * Copyright 2017 IBM Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include "common.h" +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/module.h> + +struct p8_i2c_occ { + struct occ occ; + struct i2c_client *client; +}; + +#define to_p8_i2c_occ(x) container_of((x), struct p8_i2c_occ, occ) + +static int p8_i2c_occ_send_cmd(struct occ *occ, u8 *cmd) +{ + return -EOPNOTSUPP; +} + +static int p8_i2c_occ_probe(struct i2c_client *client, + const struct i2c_device_id *id) +{ + struct occ *occ; + struct p8_i2c_occ *p8_i2c_occ = devm_kzalloc(&client->dev, + sizeof(*p8_i2c_occ), + GFP_KERNEL); + if (!p8_i2c_occ) + return -ENOMEM; + + p8_i2c_occ->client = client; + occ = &p8_i2c_occ->occ; + occ->bus_dev = &client->dev; + dev_set_drvdata(&client->dev, occ); + + occ->poll_cmd_data = 0x10; /* P8 OCC poll data */ + occ->send_cmd = p8_i2c_occ_send_cmd; + + return occ_setup(occ, "p8_occ"); +} + +static const struct of_device_id p8_i2c_occ_of_match[] = { + { .compatible = "ibm,p8-occ-hwmon" }, + {} +}; +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match); + +static const unsigned short p8_i2c_occ_addr[] = { 0x50, 0x51, I2C_CLIENT_END }; + +static struct i2c_driver p8_i2c_occ_driver = { + .class = I2C_CLASS_HWMON, + .driver = { + .name = "occ-hwmon", + .of_match_table = p8_i2c_occ_of_match, + }, + .probe = p8_i2c_occ_probe, + .address_list = p8_i2c_occ_addr, +}; + +module_i2c_driver(p8_i2c_occ_driver); + +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); +MODULE_DESCRIPTION("BMC P8 OCC hwmon driver"); +MODULE_LICENSE("GPL"); diff --git a/drivers/hwmon/occ/p9_sbe.c b/drivers/hwmon/occ/p9_sbe.c new file mode 100644 index 0000000..0cef428 --- /dev/null +++ b/drivers/hwmon/occ/p9_sbe.c @@ -0,0 +1,65 @@ +/* + * Copyright 2017 IBM Corp. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include "common.h" +#include <linux/init.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/occ.h> + +struct p9_sbe_occ { + struct occ occ; + struct device *sbe; +}; + +#define to_p9_sbe_occ(x) container_of((x), struct p9_sbe_occ, occ) + +static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd) +{ + return -EOPNOTSUPP; +} + +static int p9_sbe_occ_probe(struct platform_device *pdev) +{ + struct occ *occ; + struct p9_sbe_occ *p9_sbe_occ = devm_kzalloc(&pdev->dev, + sizeof(*p9_sbe_occ), + GFP_KERNEL); + if (!p9_sbe_occ) + return -ENOMEM; + + p9_sbe_occ->sbe = pdev->dev.parent; + occ = &p9_sbe_occ->occ; + occ->bus_dev = &pdev->dev; + platform_set_drvdata(pdev, occ); + + occ->poll_cmd_data = 0x20; /* P9 OCC poll data */ + occ->send_cmd = p9_sbe_occ_send_cmd; + + return occ_setup(occ, "p9_occ"); +} + +static const struct of_device_id p9_sbe_occ_of_match[] = { + { .compatible = "ibm,p9-occ-hwmon" }, + { }, +}; + +static struct platform_driver p9_sbe_occ_driver = { + .driver = { + .name = "occ-hwmon", + .of_match_table = p9_sbe_occ_of_match, + }, + .probe = p9_sbe_occ_probe, +}; + +module_platform_driver(p9_sbe_occ_driver); + +MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>"); +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver"); +MODULE_LICENSE("GPL");