[PATCH linux 3/7] hwmon: occ: Add I2C transport implementation for SCOM operations

[Andrew Jeffery]

Hi Eddie,

Some minor comments below.

On Fri, 2016-12-16 at 14:35 -0600, eajames.ibm at gmail.com wrote:
> > From: "Edward A. James" <eajames at us.ibm.com>
> 
> Add functions to send SCOM operations over I2C bus. The BMC can
> communicate with the Power8 host processor over I2C, but needs to use SCOM
> operations in order to access the OCC register space.
> 
> > Signed-off-by: Edward A. James <eajames at us.ibm.com>
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> ---
>  drivers/hwmon/occ/occ_scom_i2c.c | 67 ++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/occ/occ_scom_i2c.h | 26 ++++++++++++++++
>  2 files changed, 93 insertions(+)
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
>  create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
> 
> diff --git a/drivers/hwmon/occ/occ_scom_i2c.c b/drivers/hwmon/occ/occ_scom_i2c.c
> new file mode 100644
> index 0000000..70e0f51
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_scom_i2c.c
> @@ -0,0 +1,67 @@
> +/*
> + * occ_scom_i2c.c - hwmon OCC driver
> + *
> + * This file contains the functions for performing SCOM operations over I2C bus
> + * to access the OCC.
> + *
> + * Copyright 2016 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +#include "scom.h"
> +#include "occ_scom_i2c.h"
> +
> +int occ_i2c_getscom(void *bus, u32 address, u8 *data)
> +{
> > +	ssize_t rc;
> > +	u64 buf;
> > +	struct i2c_client *client = bus;
> +
> +	rc = i2c_master_send(client, (const char *)&address, sizeof(u32));

From the documentation: "Returns negative errno, or else the number of
bytes read"

> +	if (rc != sizeof(u32))
> +		return -EIO;

So what we should probably be doing here is:

    if (rc >= 0 && rc != sizeof(u32))
        return -EIO;
    else
        return rc;

> +
> > +	rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
> > +	if (rc != sizeof(u64))
> +		return -EIO;

As above.

> +
> +	*((u64 *)data) = be64_to_cpu(buf);

Please change the getscom prototypes to take a u64 *. That way we can
get compiler warnings if caller code is broken.

> +
> > +	return 0;
> +}
> +EXPORT_SYMBOL(occ_i2c_getscom);
> +
> +int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1)
> +{
> > +	u32 buf[3];
> > +	ssize_t rc;
> > +	struct i2c_client *client = bus;
> +
> > +	buf[0] = address;
> > +	buf[1] = data1;
> > +	buf[2] = data0;
> +
> > +	rc = i2c_master_send(client, (const char *)buf, sizeof(u32) * 3);
> > +	if (rc != sizeof(u32) * 3)
> +		return -EIO;

See comment on error handling above.

> +
> > +	return 0;
> +}
> +EXPORT_SYMBOL(occ_i2c_putscom);
> +
> > +MODULE_AUTHOR("Eddie James <eajames at us.ibm.com>");
> +MODULE_DESCRIPTION("I2C OCC SCOM transport");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_scom_i2c.h b/drivers/hwmon/occ/occ_scom_i2c.h
> new file mode 100644
> index 0000000..204eb4f
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_scom_i2c.h
> @@ -0,0 +1,26 @@
> +/*
> + * occ_scom_i2c.h - hwmon OCC driver
> + *
> + * This file contains function protoypes for peforming SCOM operations over I2C
> + * bus to access the OCC.
> + *
> + * Copyright 2016 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __OCC_SCOM_I2C_H__
> +#define __OCC_SCOM_I2C_H__
> +
> +int occ_i2c_getscom(void *bus, u32 address, u8 *data);

so "u64 *data" here, and in scom.h.

> +int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
> +
> +#endif /* __OCC_SCOM_I2C_H__ */

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161220/3b76a640/attachment.sig>