Date:   Sat, 24 Jun 2017 07:15:30 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Eddie James <eajames@...ux.vnet.ibm.com>,
        linux-kernel@...r.kernel.org
Cc:     linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        jdelvare@...e.com, mark.rutland@....com, robh+dt@...nel.org,
        gregkh@...uxfoundation.org, cbostic@...ux.vnet.ibm.com,
        jk@...abs.org, joel@....id.au, andrew@...id.au,
        "Edward A. James" <eajames@...ibm.com>
Subject: Re: [PATCH 1/7] drivers/hwmon: Add On-Chip Controller (OCC) hwmon
 driver

On 06/22/2017 03:48 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@...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@...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@...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@...ibm.com>");
> +MODULE_DESCRIPTION("BMC P9 OCC hwmon driver");
> +MODULE_LICENSE("GPL");
> 

Powered by blists - more mailing lists