Date:   Mon, 26 Jun 2017 10:06:50 -0500
From:   Eddie James <eajames@...ux.vnet.ibm.com>
To:     Guenter Roeck <linux@...ck-us.net>, 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/24/2017 09:15 AM, Guenter Roeck wrote:
> 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 ?

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@...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@...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