[RFC PATCH linux 4/9] hwmon: occ: Add I2C SCOM transport implementation

[Andrew Jeffery]

On Tue, 2016-11-29 at 16:59 -0600, Eddie James wrote:
> Hi Andrew,
> 
> Just a general comment on this one. I'm not sure it's worth having an
> extra layer just to isolate these functions from the
> p8_i2c_getscom/putscom. Those just do the address shift for P8. But
> there's no situation where i2c is used that's not on the P8. So this
> seems like unnecessary complexity to me. I'm not sure, just a thought.

My perspective is that baking P8 knowledge into the I2C SCOM operations
 makes the code surprising. Maybe renaming the functions is enough,
something like p8_occ_getscom(). To me it doesn't seem like much cost
in complexity to separate the transport from implementation-specific
quirks with the addressing, and gaining a general approach in the
process.

Maybe Joel can weigh in here?

Andrew

> 
> Thanks,
> Eddie
> 
> > On Thu, Nov 10, 2016 at 10:12 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> > The BMC queries the OCC on POWER8 chips with SCOMs over I2C.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  drivers/hwmon/occ/occ_scom_i2c.c | 68 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/hwmon/occ/occ_scom_i2c.h | 20 ++++++++++++
> >  2 files changed, 88 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 000000000000..b1a060331c4a
> > --- /dev/null
> > +++ b/drivers/hwmon/occ/occ_scom_i2c.c
> > @@ -0,0 +1,68 @@
> > +/*
> > + * occ_i2c.c - hwmon OCC driver
> > + *
> > + * This file contains the i2c layer for accessing the OCC over i2c bus.
> > + *
> > + * 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"
> > +
> > +#define OCC_I2C_NAME   "occ-p8-i2c"
> > +
> > +int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset)
> > +{
> > +       ssize_t rc;
> > +       u64 buf;
> > +       struct i2c_client *client = bus;
> > +
> > +       rc = i2c_master_send(client, (const char *)&address, sizeof(u32));
> > +       if (rc != sizeof(u32))
> > +               return -SCOM_WRITE_ERROR;
> > +
> > +       rc = i2c_master_recv(client, (char *)&buf, sizeof(u64));
> > +       if (rc != sizeof(u64))
> > +               return -SCOM_READ_ERROR;
> > +
> > +       *((u64 *)&data[offset]) = be64_to_cpu(buf);
> > +
> > +       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 -SCOM_WRITE_ERROR;
> > +
> > +       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 000000000000..92225f4cd584
> > --- /dev/null
> > +++ b/drivers/hwmon/occ/occ_scom_i2c.h
> > @@ -0,0 +1,20 @@
> > +/*
> > + * occ_i2c.c - hwmon OCC driver
> > + *
> > + * This file contains the i2c layer for accessing the OCC over i2c bus.
> > + *
> > + * 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.
> > + */
> > +
> > +int occ_i2c_getscom(void *bus, u32 address, u8 *data, size_t offset);
> > +int occ_i2c_putscom(void *bus, u32 address, u32 data0, u32 data1);
> > --
> > 2.9.3
> > 
-------------- 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/20161130/f6cd14d1/attachment-0001.sig>