Re: [PATCH linux v7 5/6] hwmon: occ: Add hwmon implementation for the P8 OCC

[Joel Stanley <joel@jms.id.au>]

On Wed, Feb 8, 2017 at 9:40 AM,  <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Add code to tie the hwmon sysfs code and the POWER8 OCC code together, as
> well as probe the entire driver from the I2C bus. I2C is the communication
> method between the BMC and the P8 OCC.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/hwmon/occ.txt |  13 +++
>  drivers/hwmon/occ/Kconfig                       |  14 ++++
>  drivers/hwmon/occ/Makefile                      |   1 +
>  drivers/hwmon/occ/p8_occ_i2c.c                  | 104 ++++++++++++++++++++++++

For consistency, how about we call this occ_p8_i2c.c? The other files
have the machine name second.


>  4 files changed, 132 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/occ.txt
>  create mode 100644 drivers/hwmon/occ/p8_occ_i2c.c
>

> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> index cdb64a7..3a5188f 100644
> --- a/drivers/hwmon/occ/Kconfig
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -13,3 +13,17 @@ menuconfig SENSORS_PPC_OCC
>
>           This driver can also be built as a module. If so, the module
>           will be called occ.
> +
> +if SENSORS_PPC_OCC
> +
> +config SENSORS_PPC_OCC_P8_I2C
> +       tristate "POWER8 OCC hwmon support"
> +       depends on I2C
> +       help
> +        Provide a hwmon sysfs interface for the POWER8 On-Chip Controller,
> +        exposing temperature, frequency and power measurements.
> +
> +        This driver can also be built as a module. If so, the module will be
> +        called p8-occ-i2c.

Mention that this driver is for the BMC (or just "service processor"),
and is not useful in the Power8 kernel.

I'm trying to think of a better prefix than PPC. PPC means much more
than Power8 and Power9, which is what we mean. It's also confusing as
we don't run this on any PowerPC machine - it's for an ARM chip.

> +
> +int p8_i2c_getscom(void *bus, u32 address, u64 *data)
> +{
> +       /* P8 i2c slave requires address to be shifted by 1 */
> +       address = address << 1;

I think these are scom addresses? Please indicate that so we don't get
them confused with i2c.

Or are they scom operations?

> +
> +       return occ_i2c_getscom(bus, address, data);
> +}
> +
> +int p8_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)

The functions can be static.

> +{
> +       /* P8 i2c slave requires address to be shifted by 1 */
> +       address = address << 1;
> +
> +       return occ_i2c_putscom(bus, address, data0, data1);
> +}