Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon driver

Guenter Roeck
2017-06-24T14:15:35Z

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");
>