[PATCH occ hwmon v3] Add OCC hwmon driver to Kernel

Joel Stanley joel at jms.id.au
Thu Dec 10 15:02:19 AEDT 2015


Hello Adam,

On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi at msn.com> wrote:
> The default patch triggered by git pull request is hard to read.
> I resend a clean patch to the list. Please review.

Thanks! This is the preferred method for reviewing code. Even better
would be if you used git format-patch to create the email, that way we
can review your commit message as well.

Before you submit next, please run your patch through
scripts/checkpatch.pl in the kernel tree and fix all of the errors and
warnings. It will warn you about common style and syntax mistakes.

./scripts/checkpatch.pl -f drivers/hwmon/occ_i2c.c

[...]

total: 24 errors, 21 warnings, 1315 lines checked


> This version fixed issue according to Jeremy's comments.
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e13c902..7c74854 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1167,6 +1167,16 @@ config SENSORS_NCT7904
>           This driver can also be built as a module.  If so, the module
>           will be called nct7904.
>
> +config SENSORS_OCC
> +       tristate "OCC sensor BMC driver"
> +       depends on I2C
> +       help
> +         If you say yes here you get support for BMC to monitor IBM
> +         Power CPU sensors via the On-Chip-Controller (OCC).
> +
> +         This driver can aslo be built as a module. If so, the module
> +         will be called occ_i2c.
> +
>  config SENSORS_PCF8591
>         tristate "Philips PCF8591 ADC/DAC"
>         depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 9e0f3dd..d6fa923 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -123,6 +123,7 @@ obj-$(CONFIG_SENSORS_NCT6775)       += nct6775.o
>  obj-$(CONFIG_SENSORS_NCT7802)  += nct7802.o
>  obj-$(CONFIG_SENSORS_NCT7904)  += nct7904.o
>  obj-$(CONFIG_SENSORS_NTC_THERMISTOR)   += ntc_thermistor.o
> +obj-$(CONFIG_SENSORS_OCC)      += occ_i2c.o
>  obj-$(CONFIG_SENSORS_PC87360)  += pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)  += pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)  += pcf8591.o
> diff --git a/drivers/hwmon/occ_i2c.c b/drivers/hwmon/occ_i2c.c
> new file mode 100644
> index 0000000..7a5755d
> --- /dev/null
> +++ b/drivers/hwmon/occ_i2c.c
> @@ -0,0 +1,1315 @@
> +/*
> + * BMC OCC HWMON driver - read IBM Power8 OCC (On Chip Controller)
> + * sensor data via i2c.
> + *
> + * Copyright (c) 2015 IBM (Alvin Wang, Li Yi)
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +
> +
> +#define OCC_I2C_ADDR 0x50
> +#define OCC_I2C_NAME "occ-i2c"
> +
> +#define OCC_DATA_MAX   4096 /* 4KB at most */
> +/* i2c read and write occ sensors */
> +#define I2C_READ_ERROR 1
> +#define I2C_WRITE_ERROR        2
> +
> +/* To generate attn to OCC */
> +#define ATTN_DATA      0x0006B035
> +/* For BMC to read/write SRAM */
> +#define OCB_ADDRESS            0x0006B070
> +#define OCB_DATA               0x0006B075
> +#define OCB_STATUS_CONTROL_AND 0x0006B072
> +#define OCB_STATUS_CONTROL_OR  0x0006B073
> +#define OCC_COMMAND_ADDR       0xFFFF6000
> +#define OCC_RESPONSE_ADDR      0xFFFF7000

Are these addresses going to change with different versions of the OCC firmware?

> +
> +/* OCC sensor data format */
> +struct occ_sensor {
> +       uint16_t sensor_id;
> +       uint16_t value;
> +};
> +
> +struct power_sensor {
> +       uint16_t sensor_id;


> +       uint32_t update_tag;
> +       uint32_t accumulator;
> +       uint16_t value;
> +};
> +
> +struct caps_sensor {
> +       uint16_t curr_powercap;
> +       uint16_t curr_powerreading;
> +       uint16_t norm_powercap;
> +       uint16_t max_powercap;
> +       uint16_t min_powercap;
> +       uint16_t user_powerlimit;
> +};
> +
> +struct sensor_data_block {
> +       uint8_t sensor_type[4];
> +       uint8_t reserved0;
> +       uint8_t sensor_format;
> +       uint8_t sensor_length;
> +       uint8_t num_of_sensors;
> +       struct occ_sensor *sensor;
> +       struct power_sensor *power;
> +       struct caps_sensor *caps;
> +};
> +
> +struct occ_poll_header {
> +       uint8_t status;
> +       uint8_t ext_status;
> +       uint8_t occs_present;
> +       uint8_t config;
> +       uint8_t occ_state;
> +       uint8_t reserved0;
> +       uint8_t reserved1;
> +       uint8_t error_log_id;
> +       uint32_t error_log_addr_start;
> +       uint16_t error_log_length;
> +       uint8_t reserved2;
> +       uint8_t reserved3;
> +       uint8_t occ_code_level[16];
> +       uint8_t sensor_eye_catcher[6];
> +       uint8_t sensor_block_num;
> +       uint8_t sensor_data_version;
> +};
> +
> +struct occ_response {
> +       uint8_t sequence_num;
> +       uint8_t cmd_type;
> +       uint8_t rtn_status;
> +       uint16_t data_length;
> +       struct occ_poll_header header;
> +       struct sensor_data_block *blocks;
> +       uint16_t chk_sum;
> +       int temp_block_id;
> +       int freq_block_id;
> +       int power_block_id;
> +       int caps_block_id;
> +};
> +
> +/* data private to each client */
> +struct occ_drv_data {
> +       struct i2c_client       *client;
> +       struct device           *hwmon_dev;
> +       struct mutex            update_lock;
> +       bool                    valid;
> +       unsigned long           last_updated;
> +       /* Minimum timer interval for sampling In jiffies */
> +       unsigned long           update_interval;
> +       struct occ_response     occ_resp;
> +};
> +
> +enum sensor_t {
> +       freq,
> +       temp,
> +       power,
> +       caps
> +};
> +
> +static void deinit_occ_resp_buf(struct occ_response *p)
> +{
> +       int b;

Convention is to use i for indexes.

> +
> +       if (!p)
> +               return;

It's common in kernel code to omit checking for null arguments.

If you think there is a likely hood of this happening, then perhaps
WARN_ON(!b) would be better; this way we get a backtrace in the kernel
log.

> +
> +       if (!p->blocks)
> +               return;
> +
> +       for (b = 0; b < p->header.sensor_block_num; b++) {
> +               if (p->blocks[b].sensor)
> +                       kfree(p->blocks[b].sensor);
> +               if (p->blocks[b].power)
> +                       kfree(p->blocks[b].power);
> +               if (p->blocks[b].caps)
> +                       kfree(p->blocks[b].caps);
> +       }

I think Jeremy suggested in his review that you statically allocate
these instead of kmalloc and kfreeing at runtime.

> +
> +       kfree(p->blocks);
> +
> +       memset(p, 0, sizeof(*p));
> +       p->freq_block_id = -1;
> +       p->temp_block_id = -1;
> +       p->power_block_id = -1;
> +       p->caps_block_id = -1;
> +}
> +
> +static ssize_t occ_i2c_read(struct i2c_client *client, void *buf, size_t count)
> +{
> +       if (count > OCC_DATA_MAX)
> +               count = OCC_DATA_MAX;

I think we should print a message here. Perhaps even a WARN_ON? Normal
use cases should not request a too-large buffer.

> +
> +       dev_dbg(&client->dev, "i2c_read: reading %zu bytes @0x%x.\n",

Instead of 0x%x you can use %p to print pointers.

> +               count, client->addr);
> +       return i2c_master_recv(client, buf, count);
> +}
> +
> +static ssize_t occ_i2c_write(struct i2c_client *client, const void *buf,
> +                               size_t count)
> +{
> +       if (count > OCC_DATA_MAX)
> +               count = OCC_DATA_MAX;
> +
> +       dev_dbg(&client->dev, "i2c_write: writing %zu bytes @0x%x.\n",
> +               count, client->addr);
> +       return i2c_master_send(client, buf, count);
> +}
> +
> +/* read 8-byte value and put into data[offset] */
> +static int occ_getscomb(struct i2c_client *client, uint32_t address,
> +               uint8_t *data, int offset)
> +{
> +       uint32_t ret;
> +       char buf[8];
> +       int i = 0;
> +
> +       /* P8 i2c slave requires address to be shifted by 1 */
> +       address = address << 1;
> +
> +       ret = occ_i2c_write(client, &address,
> +               sizeof(address));
> +
> +       if (ret != sizeof(address))
> +               return -I2C_WRITE_ERROR;
> +
> +       ret = occ_i2c_read(client, buf, sizeof(buf));
> +       if (ret != sizeof(buf))
> +               return -I2C_READ_ERROR;
> +
> +       for (i = 0; i < 8; i++)
> +               data[offset + i] = buf[7 - i];
> +
> +       return 0;
> +}
> +
> +static int occ_putscom(struct i2c_client *client, uint32_t address,
> +               uint32_t data0, uint32_t data1)
> +{
> +       uint32_t buf[3];
> +       uint32_t ret;
> +
> +       /* P8 i2c slave requires address to be shifted by 1 */
> +       address = address << 1;
> +
> +       buf[0] = address;
> +       buf[1] = data1;
> +       buf[2] = data0;
> +
> +       ret = occ_i2c_write(client, buf, sizeof(buf));
> +       if (ret != sizeof(buf))
> +               return I2C_WRITE_ERROR;
> +
> +       return 0;
> +}
> +
> +static inline uint16_t get_occdata_length(uint8_t *d)
> +{
> +        return (d[3] << 8) | d[4];

be16_to_cpu

> +}
> +
> +static int occ_renew_sensor(struct occ_response *o, uint8_t sensor_length,
> +       uint8_t num_of_sensors, enum sensor_t t, int block)
> +{
> +       void *sensor;
> +       int ret;
> +
> +       switch (t) {
> +       case temp:
> +               sensor = o->temp_block_id == -1 ? NULL :

Perhaps put ( ) around the condition to make it easier to read.

> +               o->blocks[o->temp_block_id].sensor;

Your indentation is wrong here.
> +               break;
> +       case freq:
> +               sensor = o->freq_block_id == -1 ? NULL :
> +               o->blocks[o->freq_block_id].sensor;
> +               break;
> +       case power:
> +               sensor = o->power_block_id == -1 ? NULL :
> +               o->blocks[o->power_block_id].power;
> +               break;
> +       case caps:
> +               sensor = o->caps_block_id == -1 ? NULL :
> +               o->blocks[o->caps_block_id].caps;
> +               break;
> +       default:
> +               sensor = NULL;
> +               break;
> +       }
> +
> +       /* empty sensor block, release older sensor data */
> +       if (num_of_sensors == 0 || sensor_length == 0) {
> +               if (sensor)
> +                       kfree(sensor);
> +               return -1;
> +       }
> +
> +       switch (t) {
> +       case temp:
> +               if (!sensor || num_of_sensors !=
> +                       o->blocks[o->temp_block_id].num_of_sensors) {
> +                       if (sensor)
> +                               kfree(sensor);
> +                       o->blocks[block].sensor =
> +                               //kzalloc(sizeof(struct occ_sensor) *
> +                               //      num_of_sensors, GFP_KERNEL);
> +                               kcalloc(num_of_sensors,
> +                                       sizeof(struct occ_sensor), GFP_KERNEL);
> +                       if (!o->blocks[block].sensor) {
> +                               ret = -ENOMEM;
> +                               goto err;
> +                       }
> +               }
> +               break;
> +       case freq:
> +               if (!sensor || num_of_sensors !=
> +                       o->blocks[o->freq_block_id].num_of_sensors) {
> +                       if (sensor)
> +                               kfree(sensor);
> +                       o->blocks[block].sensor =
> +                               //kzalloc(sizeof(struct occ_sensor) *
> +                               //      num_of_sensors, GFP_KERNEL);

You still have commented out code here.

> +                               kcalloc(num_of_sensors,
> +                                       sizeof(struct occ_sensor), GFP_KERNEL);
> +                       if (!o->blocks[block].sensor) {
> +                               ret = -ENOMEM;
> +                               goto err;
> +                       }
> +               }
> +               break;
> +       case power:
> +               if (!sensor || num_of_sensors !=
> +                       o->blocks[o->power_block_id].num_of_sensors) {
> +                       if (sensor)
> +                               kfree(sensor);
> +                       o->blocks[block].power =
> +                               //kzalloc(sizeof(struct power_sensor) *
> +                               //      num_of_sensors, GFP_KERNEL);
> +                               kcalloc(num_of_sensors,
> +                               sizeof(struct power_sensor), GFP_KERNEL);

You still have commented out code here.

> +                       if (!o->blocks[block].power) {
> +                               ret = -ENOMEM;
> +                               goto err;
> +                       }
> +               }
> +               break;
> +       case caps:
> +               if (!sensor || num_of_sensors !=
> +                       o->blocks[o->caps_block_id].num_of_sensors) {
> +                       if (sensor)
> +                               kfree(sensor);
> +                       o->blocks[block].caps =
> +                               //kzalloc(sizeof(struct caps_sensor) *
> +                               //      num_of_sensors, GFP_KERNEL);
> +                               kcalloc(num_of_sensors,
> +                                       sizeof(struct caps_sensor), GFP_KERNEL);

You still have commented out code here.

> +                       if (!o->blocks[block].caps) {
> +                               ret = -ENOMEM;
> +                               goto err;
> +                       }
> +               }
> +               break;
> +       default:
> +               sensor = NULL;
> +               break;
> +       }
> +
> +       return 0;
> +err:
> +       deinit_occ_resp_buf(o);
> +       return ret;
> +}
> +
> +/* refer to OCC interface document for Poll Return Packet format */

Provide a link to this document.

> +#define RESP_HEADER_OFFSET     5
> +#define SENSOR_STR_OFFSET      37
> +#define SENSOR_BLOCK_NUM_OFFSET        43
> +#define SENSOR_BLOCK_OFFSET    45
> +static int parse_occ_response(struct i2c_client *client,
> +               uint8_t *d, struct occ_response *o)

What is d? Please use a more descriptive name.

Same with o.

> +{
> +       int b;
> +       int s;
> +       int ret;
> +       int dnum = SENSOR_BLOCK_OFFSET;
> +       struct occ_sensor *f_sensor;
> +       struct occ_sensor *t_sensor;
> +       struct power_sensor *p_sensor;
> +       struct caps_sensor *c_sensor;
> +       uint8_t sensor_block_num;
> +       uint8_t sensor_type[4];
> +       uint8_t sensor_format;
> +       uint8_t sensor_length;
> +       uint8_t num_of_sensors;
> +
> +       /* check if the data is valid */
> +       if (strncmp(&d[SENSOR_STR_OFFSET], "SENSOR", 6) != 0) {

If you have a literal string you don't need to use strncmp.

> +               dev_err(&client->dev,
> +                       "ERROR: SENSOR String in response\n");
> +               ret = -1;
> +               goto err;
> +        }
> +
> +       sensor_block_num = d[SENSOR_BLOCK_NUM_OFFSET];
> +       if (sensor_block_num == 0) {
> +               dev_err(&client->dev, "ERROR: SENSOR block num is 0\n");
> +               ret = -1;
> +               goto err;
> +       }
> +
> +       /* if sensor block has changed, re-malloc */
> +       if (sensor_block_num != o->header.sensor_block_num) {
> +               deinit_occ_resp_buf(o);
> +               //o->blocks = kzalloc(sizeof(struct sensor_data_block) *
> +               //              sensor_block_num, GFP_KERNEL);
> +               o->blocks = kcalloc(sensor_block_num,
> +                       sizeof(struct sensor_data_block), GFP_KERNEL);

You still have commented out code here.

> +               if (!o->blocks)
> +                       return -ENOMEM;
> +       }
> +
> +       memcpy(&o->header, &d[RESP_HEADER_OFFSET], sizeof(o->header));
> +       o->header.error_log_addr_start =
> +               be32_to_cpu(o->header.error_log_addr_start);
> +       o->header.error_log_length = be16_to_cpu(o->header.error_log_length);
> +
> +       dev_dbg(&client->dev, "Reading %d sensor blocks\n",
> +               o->header.sensor_block_num);
> +       for (b = 0; b < sensor_block_num; b++) {
> +               /* 8-byte sensor block head */
> +               strncpy(sensor_type, &d[dnum], 4);
> +               sensor_format = d[dnum+5];
> +               sensor_length = d[dnum+6];
> +               num_of_sensors = d[dnum+7];
> +               dnum = dnum + 8;
> +
> +               dev_dbg(&client->dev,
> +                       "sensor block[%d]: type: %s, num_of_sensors: %d\n",
> +                       b, sensor_type, num_of_sensors);
> +
> +               if (strncmp(sensor_type, "FREQ", 4) == 0) {
> +                       ret = occ_renew_sensor(o, sensor_length,
> +                               num_of_sensors, freq, b);
> +                       if (ret)
> +                               continue;
> +
> +                       o->freq_block_id = b;
> +                       for (s = 0; s < num_of_sensors; s++) {
> +                               f_sensor = &o->blocks[b].sensor[s];
> +                               f_sensor->sensor_id = (d[dnum]<<8) | d[dnum+1];
> +                               f_sensor->value = (d[dnum+2]<<8) | d[dnum+3];
> +                               dev_dbg(&client->dev,
> +                                       "sensor[%d]-[%d]: id: %u, value: %u\n",
> +                                       b, s, f_sensor->sensor_id,
> +                                       f_sensor->value);
> +                               dnum = dnum + sensor_length;
> +                       }
> +               } else if (strncmp(sensor_type, "TEMP", 4) == 0) {
> +                       ret = occ_renew_sensor(o, sensor_length,
> +                               num_of_sensors, temp, b);
> +                       if (ret)
> +                               continue;
> +
> +                       o->temp_block_id = b;
> +                       for (s = 0; s < num_of_sensors; s++) {
> +                               t_sensor = &o->blocks[b].sensor[s];
> +                               t_sensor->sensor_id = (d[dnum]<<8) | d[dnum+1];
> +                               t_sensor->value = (d[dnum+2] << 8) | d[dnum+3];
> +                               dev_dbg(&client->dev,
> +                                       "sensor[%d]-[%d]: id: %u, value: %u\n",
> +                                       b, s, t_sensor->sensor_id,
> +                                       t_sensor->value);
> +                               dnum = dnum + sensor_length;
> +                       }
> +               } else if (strncmp(sensor_type, "POWR", 4) == 0) {
> +                       ret = occ_renew_sensor(o, sensor_length,
> +                               num_of_sensors, power, b);
> +                       if (ret)
> +                               continue;
> +
> +                       o->power_block_id = b;
> +                       for (s = 0; s < num_of_sensors; s++) {
> +                               p_sensor = &o->blocks[b].power[s];
> +                               p_sensor->sensor_id = (d[dnum]<<8) | d[dnum+1];

This looks like endian swapping. Is the data in big endian? We have
be16_to_cpu for this purpose.

> +                               p_sensor->update_tag =
> +                                       (d[dnum+2] << 24) | (d[dnum+3] << 16) |
> +                                       (d[dnum+4] << 8) | d[dnum+5];

This looks like endian swapping. Is the data in big endian? We have
be32_to_cpu for this purpose.

> +                               p_sensor->accumulator =
> +                                       (d[dnum+6] << 24) | (d[dnum+7] << 16) |
> +                                       (d[dnum+8] << 8) | d[dnum+9];
> +                               p_sensor->value =
> +                                       (d[dnum+10] << 8) | d[dnum+11];
> +
> +                               dev_dbg(&client->dev,
> +                                       "sensor[%d]-[%d]: id: %u, value: %u\n",
> +                                       b, s, p_sensor->sensor_id,
> +                                       p_sensor->value);
> +
> +                               dnum = dnum + sensor_length;
> +                       }
> +               } else if (strncmp(sensor_type, "CAPS", 4) == 0) {

If you have a literal string you don't need to use strncmp.

> +                       ret = occ_renew_sensor(o, sensor_length,
> +                               num_of_sensors, caps, b);
> +                       if (ret)
> +                               continue;
> +
> +                       o->caps_block_id = b;
> +                       for (s = 0; s < num_of_sensors; s++) {
> +                               c_sensor = &o->blocks[b].caps[s];
> +                               c_sensor->curr_powercap =
> +                                       (d[dnum] << 8) | d[dnum+1];
> +                               c_sensor->curr_powerreading =
> +                                       (d[dnum+2] << 8) | d[dnum+3];
> +                               c_sensor->norm_powercap =
> +                                       (d[dnum+4] << 8) | d[dnum+5];
> +                               c_sensor->max_powercap =
> +                                       (d[dnum+6] << 8) | d[dnum+7];
> +                               c_sensor->min_powercap =
> +                                       (d[dnum+8] << 8) | d[dnum+9];
> +                               c_sensor->user_powerlimit =
> +                                       (d[dnum+10] << 8) | d[dnum+11];
> +
> +                               dnum = dnum + sensor_length;
> +                               dev_dbg(&client->dev, "CAPS sensor #%d:\n", s);
> +                               dev_dbg(&client->dev, "curr_powercap is %x\n",
> +                                       c_sensor->curr_powercap);
> +                               dev_dbg(&client->dev,
> +                                       "curr_powerreading is %x\n",
> +                                       c_sensor->curr_powerreading);
> +                               dev_dbg(&client->dev, "norm_powercap is %x\n",
> +                                       c_sensor->norm_powercap);
> +                               dev_dbg(&client->dev, "max_powercap is %x\n",
> +                                       c_sensor->max_powercap);
> +                               dev_dbg(&client->dev, "min_powercap is %x\n",
> +                                       c_sensor->min_powercap);
> +                               dev_dbg(&client->dev, "user_powerlimit is %x\n",
> +                                       c_sensor->user_powerlimit);
> +                       }
> +
> +               } else {
> +                       dev_err(&client->dev,
> +                               "ERROR: sensor type %s not supported\n",
> +                               o->blocks[b].sensor_type);
> +                       ret = -1;
> +                       goto err;
> +               }
> +
> +               strncpy(o->blocks[b].sensor_type, sensor_type, 4);
> +               o->blocks[b].sensor_format = sensor_format;
> +               o->blocks[b].sensor_length = sensor_length;
> +               o->blocks[b].num_of_sensors = num_of_sensors;
> +       }
> +
> +       return 0;
> +err:
> +       deinit_occ_resp_buf(o);
> +       return ret;
> +}
> +
> +static int occ_get_all(struct i2c_client *client, struct occ_response *occ_resp)
> +{
> +       uint8_t occ_data[OCC_DATA_MAX];
> +       uint16_t num_bytes;
> +       int b;
> +       int ret;
> +
> +       /* Init OCB */
> +       occ_putscom(client, OCB_STATUS_CONTROL_OR,  0x08000000, 0x00000000);
> +       occ_putscom(client, OCB_STATUS_CONTROL_AND, 0xFBFFFFFF, 0xFFFFFFFF);
> +
> +       /* Send poll command to OCC */
> +       occ_putscom(client, OCB_ADDRESS, OCC_COMMAND_ADDR, 0x00000000);
> +       occ_putscom(client, OCB_ADDRESS, OCC_COMMAND_ADDR, 0x00000000);
> +       occ_putscom(client, OCB_DATA, 0x00000001, 0x10001100);
> +
> +       /* Trigger ATTN */
> +       occ_putscom(client, ATTN_DATA, 0x01010000, 0x00000000);
> +
> +       /* Get response data */
> +       occ_putscom(client, OCB_ADDRESS, OCC_RESPONSE_ADDR, 0x00000000);
> +       occ_getscomb(client, OCB_DATA, occ_data, 0);
> +
> +       num_bytes = get_occdata_length(occ_data);
> +
> +       dev_dbg(&client->dev, "OCC data length: %d\n", num_bytes);
> +
> +       if (num_bytes > OCC_DATA_MAX) {
> +               dev_err(&client->dev, "ERROR: OCC data length must be < 4KB\n");
> +               return -1;
> +       }
> +
> +       if (num_bytes <= 0) {
> +               dev_err(&client->dev, "ERROR: OCC data length is zero\n");
> +               return -1;
> +       }
> +
> +       for (b = 8; b < num_bytes + 8; b = b + 8)
> +               occ_getscomb(client, OCB_DATA, occ_data, b);
> +
> +       ret = parse_occ_response(client, occ_data, occ_resp);
> +
> +       return ret;
> +}
> +
> +
> +static int occ_update_device(struct device *dev)
> +{
> +       struct occ_drv_data *data = dev_get_drvdata(dev);
> +       struct i2c_client *client = data->client;
> +       int ret = 0;
> +
> +       mutex_lock(&data->update_lock);
> +
> +       if (time_after(jiffies, data->last_updated + data->update_interval)
> +           || !data->valid) {
> +               data->valid = 1;
> +               ret = occ_get_all(client, &data->occ_resp);
> +               if (ret)
> +                       data->valid = 0;
> +               data->last_updated = jiffies;
> +       }
> +       mutex_unlock(&data->update_lock);
> +
> +       return ret;
> +}
> +
> +
> +static void* occ_get_sensor(struct device *hwmon_dev, enum sensor_t t)
> +{
> +       struct device *dev = hwmon_dev->parent;
> +       struct occ_drv_data *data = dev_get_drvdata(dev);
> +       int ret;
> +       void *sensor;
> +
> +       ret = occ_update_device(dev);
> +       if (ret != 0) {
> +               dev_err(dev, "ERROR: cannot get occ sensor data: %d\n", ret);
> +               return NULL;
> +       }
> +
> +       if (!data->occ_resp.blocks)
> +               return NULL;
> +
> +       switch (t) {
> +       case temp:
> +               sensor = data->occ_resp.temp_block_id == -1 ? NULL :
> +               data->occ_resp.blocks[data->occ_resp.temp_block_id].sensor;
> +               break;
> +       case freq:
> +               sensor = data->occ_resp.freq_block_id == -1 ? NULL :
> +               data->occ_resp.blocks[data->occ_resp.freq_block_id].sensor;
> +               break;
> +       case power:
> +               sensor = data->occ_resp.power_block_id == -1 ? NULL :
> +               data->occ_resp.blocks[data->occ_resp.power_block_id].power;
> +               break;
> +       case caps:
> +               sensor = data->occ_resp.caps_block_id == -1 ? NULL :
> +               data->occ_resp.blocks[data->occ_resp.caps_block_id].caps;
> +               break;

This switch statement is repeated in occ_renew_sensor(). You should
split it out into a function.

> +       default:
> +               sensor = NULL;
> +               break;
> +       }
> +
> +       return sensor;
> +}
> +
> +/* sysfs attributes for hwmon */
> +static ssize_t show_occ_temp_input(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct occ_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, temp);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               /* in millidegree Celsius */
> +               val = sensor[n].value * 1000;
> +
> +       return sprintf(buf, "%d\n", val);

How do you know "%d\n" will fit into buf?

> +}
> +
> +static ssize_t show_occ_temp_label(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct occ_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, temp);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               val = sensor[n].sensor_id;
> +
> +       return sprintf(buf, "%d\n", val);

As above.

> +}
> +
> +static ssize_t show_occ_power_label(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct power_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, power);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               val = sensor[n].sensor_id;
> +
> +       return sprintf(buf, "%d\n", val);

As above.

> +}
> +
> +
> +static ssize_t show_occ_power_input(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct power_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, power);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               val = sensor[n].value;
> +
> +       return sprintf(buf, "%d\n", val);
> +}
> +
> +
> +static ssize_t show_occ_freq_label(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct occ_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, freq);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               val = sensor[n].sensor_id;
> +
> +       return sprintf(buf, "%d\n", val);
> +}
> +
> +
> +static ssize_t show_occ_freq_input(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +       int n = attr->index;
> +       struct occ_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, freq);
> +       if (!sensor)
> +               val = -1;
> +       else
> +               val = sensor[n].value;
> +
> +       return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t show_occ_caps(struct device *hwmon_dev,
> +               struct device_attribute *da, char *buf)
> +{
> +       struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(da);
> +       int nr = attr->nr;
> +       int n = attr->index;
> +       struct caps_sensor *sensor;
> +       int val;
> +
> +       sensor = occ_get_sensor(hwmon_dev, caps);
> +       if (!sensor) {
> +               val = -1;
> +               return sprintf(buf, "%d\n", val);
> +       }
> +
> +       switch (nr) {
> +       case 0:
> +               val = sensor[n].curr_powercap;
> +               break;
> +       case 1:
> +               val = sensor[n].curr_powerreading;
> +               break;
> +       case 2:
> +               val = sensor[n].norm_powercap;
> +               break;
> +       case 3:
> +               val = sensor[n].max_powercap;
> +               break;
> +       case 4:
> +               val = sensor[n].min_powercap;
> +               break;
> +       case 5:
> +               val = sensor[n].user_powerlimit;
> +               break;
> +       default:
> +               val = -1;
> +       }
> +
> +       return sprintf(buf, "%d\n", val);
> +}
> +
> +static struct sensor_device_attribute temp_input[] = {
> +       SENSOR_ATTR(temp1_input, S_IRUGO, show_occ_temp_input, NULL, 0),
> +       SENSOR_ATTR(temp2_input, S_IRUGO, show_occ_temp_input, NULL, 1),
> +       SENSOR_ATTR(temp3_input, S_IRUGO, show_occ_temp_input, NULL, 2),
> +       SENSOR_ATTR(temp4_input, S_IRUGO, show_occ_temp_input, NULL, 3),
> +       SENSOR_ATTR(temp5_input, S_IRUGO, show_occ_temp_input, NULL, 4),
> +       SENSOR_ATTR(temp6_input, S_IRUGO, show_occ_temp_input, NULL, 5),
> +       SENSOR_ATTR(temp7_input, S_IRUGO, show_occ_temp_input, NULL, 6),
> +       SENSOR_ATTR(temp8_input, S_IRUGO, show_occ_temp_input, NULL, 7),
> +       SENSOR_ATTR(temp9_input, S_IRUGO, show_occ_temp_input, NULL, 8),
> +       SENSOR_ATTR(temp10_input, S_IRUGO, show_occ_temp_input, NULL, 9),
> +       SENSOR_ATTR(temp11_input, S_IRUGO, show_occ_temp_input, NULL, 10),
> +       SENSOR_ATTR(temp12_input, S_IRUGO, show_occ_temp_input, NULL, 11),
> +       SENSOR_ATTR(temp13_input, S_IRUGO, show_occ_temp_input, NULL, 12),
> +       SENSOR_ATTR(temp14_input, S_IRUGO, show_occ_temp_input, NULL, 13),
> +       SENSOR_ATTR(temp15_input, S_IRUGO, show_occ_temp_input, NULL, 14),
> +       SENSOR_ATTR(temp16_input, S_IRUGO, show_occ_temp_input, NULL, 15),
> +       SENSOR_ATTR(temp17_input, S_IRUGO, show_occ_temp_input, NULL, 16),
> +       SENSOR_ATTR(temp18_input, S_IRUGO, show_occ_temp_input, NULL, 17),
> +       SENSOR_ATTR(temp19_input, S_IRUGO, show_occ_temp_input, NULL, 18),
> +       SENSOR_ATTR(temp20_input, S_IRUGO, show_occ_temp_input, NULL, 19),
> +       SENSOR_ATTR(temp21_input, S_IRUGO, show_occ_temp_input, NULL, 20),
> +       SENSOR_ATTR(temp22_input, S_IRUGO, show_occ_temp_input, NULL, 21),
> +};
> +
> +static struct sensor_device_attribute temp_label[] = {
> +       SENSOR_ATTR(temp1_label, S_IRUGO, show_occ_temp_label, NULL, 0),
> +       SENSOR_ATTR(temp2_label, S_IRUGO, show_occ_temp_label, NULL, 1),
> +       SENSOR_ATTR(temp3_label, S_IRUGO, show_occ_temp_label, NULL, 2),
> +       SENSOR_ATTR(temp4_label, S_IRUGO, show_occ_temp_label, NULL, 3),
> +       SENSOR_ATTR(temp5_label, S_IRUGO, show_occ_temp_label, NULL, 4),
> +       SENSOR_ATTR(temp6_label, S_IRUGO, show_occ_temp_label, NULL, 5),
> +       SENSOR_ATTR(temp7_label, S_IRUGO, show_occ_temp_label, NULL, 6),
> +       SENSOR_ATTR(temp8_label, S_IRUGO, show_occ_temp_label, NULL, 7),
> +       SENSOR_ATTR(temp9_label, S_IRUGO, show_occ_temp_label, NULL, 8),
> +       SENSOR_ATTR(temp10_label, S_IRUGO, show_occ_temp_label, NULL, 9),
> +       SENSOR_ATTR(temp11_label, S_IRUGO, show_occ_temp_label, NULL, 10),
> +       SENSOR_ATTR(temp12_label, S_IRUGO, show_occ_temp_label, NULL, 11),
> +       SENSOR_ATTR(temp13_label, S_IRUGO, show_occ_temp_label, NULL, 12),
> +       SENSOR_ATTR(temp14_label, S_IRUGO, show_occ_temp_label, NULL, 13),
> +       SENSOR_ATTR(temp15_label, S_IRUGO, show_occ_temp_label, NULL, 14),
> +       SENSOR_ATTR(temp16_label, S_IRUGO, show_occ_temp_label, NULL, 15),
> +       SENSOR_ATTR(temp17_label, S_IRUGO, show_occ_temp_label, NULL, 16),
> +       SENSOR_ATTR(temp18_label, S_IRUGO, show_occ_temp_label, NULL, 17),
> +       SENSOR_ATTR(temp19_label, S_IRUGO, show_occ_temp_label, NULL, 18),
> +       SENSOR_ATTR(temp20_label, S_IRUGO, show_occ_temp_label, NULL, 19),
> +       SENSOR_ATTR(temp21_label, S_IRUGO, show_occ_temp_label, NULL, 20),
> +       SENSOR_ATTR(temp22_label, S_IRUGO, show_occ_temp_label, NULL, 21),
> +
> +};
> +
> +#define TEMP_UNIT_ATTRS(X)                      \
> +{      &temp_input[X].dev_attr.attr,           \
> +       &temp_label[X].dev_attr.attr,          \
> +       NULL                                    \
> +}
> +
> +/* 10-core CPU, occ has 22 temp sensors, more socket, more sensors */
> +static struct attribute *occ_temp_attr[][3] = {
> +       TEMP_UNIT_ATTRS(0),
> +       TEMP_UNIT_ATTRS(1),
> +       TEMP_UNIT_ATTRS(2),
> +       TEMP_UNIT_ATTRS(3),
> +       TEMP_UNIT_ATTRS(4),
> +       TEMP_UNIT_ATTRS(5),
> +       TEMP_UNIT_ATTRS(6),
> +       TEMP_UNIT_ATTRS(7),
> +       TEMP_UNIT_ATTRS(8),
> +       TEMP_UNIT_ATTRS(9),
> +       TEMP_UNIT_ATTRS(10),
> +       TEMP_UNIT_ATTRS(11),
> +       TEMP_UNIT_ATTRS(12),
> +       TEMP_UNIT_ATTRS(13),
> +       TEMP_UNIT_ATTRS(14),
> +       TEMP_UNIT_ATTRS(15),
> +       TEMP_UNIT_ATTRS(16),
> +       TEMP_UNIT_ATTRS(17),
> +       TEMP_UNIT_ATTRS(18),
> +       TEMP_UNIT_ATTRS(19),
> +       TEMP_UNIT_ATTRS(20),
> +       TEMP_UNIT_ATTRS(21),
> +};
> +
> +static const struct attribute_group occ_temp_attr_group[] = {
> +       { .attrs = occ_temp_attr[0] },
> +       { .attrs = occ_temp_attr[1] },
> +       { .attrs = occ_temp_attr[2] },
> +       { .attrs = occ_temp_attr[3] },
> +       { .attrs = occ_temp_attr[4] },
> +       { .attrs = occ_temp_attr[5] },
> +       { .attrs = occ_temp_attr[6] },
> +       { .attrs = occ_temp_attr[7] },
> +       { .attrs = occ_temp_attr[8] },
> +       { .attrs = occ_temp_attr[9] },
> +       { .attrs = occ_temp_attr[10] },
> +       { .attrs = occ_temp_attr[11] },
> +       { .attrs = occ_temp_attr[12] },
> +       { .attrs = occ_temp_attr[13] },
> +       { .attrs = occ_temp_attr[14] },
> +       { .attrs = occ_temp_attr[15] },
> +       { .attrs = occ_temp_attr[16] },
> +       { .attrs = occ_temp_attr[17] },
> +       { .attrs = occ_temp_attr[18] },
> +       { .attrs = occ_temp_attr[19] },
> +       { .attrs = occ_temp_attr[20] },
> +       { .attrs = occ_temp_attr[21] },
> +};
> +
> +
> +static struct sensor_device_attribute freq_input[] = {
> +       SENSOR_ATTR(freq1_input, S_IRUGO, show_occ_freq_input, NULL, 0),
> +       SENSOR_ATTR(freq2_input, S_IRUGO, show_occ_freq_input, NULL, 1),
> +       SENSOR_ATTR(freq3_input, S_IRUGO, show_occ_freq_input, NULL, 2),
> +       SENSOR_ATTR(freq4_input, S_IRUGO, show_occ_freq_input, NULL, 3),
> +       SENSOR_ATTR(freq5_input, S_IRUGO, show_occ_freq_input, NULL, 4),
> +       SENSOR_ATTR(freq6_input, S_IRUGO, show_occ_freq_input, NULL, 5),
> +       SENSOR_ATTR(freq7_input, S_IRUGO, show_occ_freq_input, NULL, 6),
> +       SENSOR_ATTR(freq8_input, S_IRUGO, show_occ_freq_input, NULL, 7),
> +       SENSOR_ATTR(freq9_input, S_IRUGO, show_occ_freq_input, NULL, 8),
> +       SENSOR_ATTR(freq10_input, S_IRUGO, show_occ_freq_input, NULL, 9),
> +};
> +
> +static struct sensor_device_attribute freq_label[] = {
> +       SENSOR_ATTR(freq1_label, S_IRUGO, show_occ_freq_label, NULL, 0),
> +       SENSOR_ATTR(freq2_label, S_IRUGO, show_occ_freq_label, NULL, 1),
> +       SENSOR_ATTR(freq3_label, S_IRUGO, show_occ_freq_label, NULL, 2),
> +       SENSOR_ATTR(freq4_label, S_IRUGO, show_occ_freq_label, NULL, 3),
> +       SENSOR_ATTR(freq5_label, S_IRUGO, show_occ_freq_label, NULL, 4),
> +       SENSOR_ATTR(freq6_label, S_IRUGO, show_occ_freq_label, NULL, 5),
> +       SENSOR_ATTR(freq7_label, S_IRUGO, show_occ_freq_label, NULL, 6),
> +       SENSOR_ATTR(freq8_label, S_IRUGO, show_occ_freq_label, NULL, 7),
> +       SENSOR_ATTR(freq9_label, S_IRUGO, show_occ_freq_label, NULL, 8),
> +       SENSOR_ATTR(freq10_label, S_IRUGO, show_occ_freq_label, NULL, 9),
> +
> +};
> +
> +#define FREQ_UNIT_ATTRS(X)                      \
> +{      &freq_input[X].dev_attr.attr,           \
> +       &freq_label[X].dev_attr.attr,          \
> +       NULL                                    \
> +}
> +
> +/* 10-core CPU, occ has 22 freq sensors, more socket, more sensors */
> +static struct attribute *occ_freq_attr[][3] = {
> +       FREQ_UNIT_ATTRS(0),
> +       FREQ_UNIT_ATTRS(1),
> +       FREQ_UNIT_ATTRS(2),
> +       FREQ_UNIT_ATTRS(3),
> +       FREQ_UNIT_ATTRS(4),
> +       FREQ_UNIT_ATTRS(5),
> +       FREQ_UNIT_ATTRS(6),
> +       FREQ_UNIT_ATTRS(7),
> +       FREQ_UNIT_ATTRS(8),
> +       FREQ_UNIT_ATTRS(9),
> +};
> +
> +static const struct attribute_group occ_freq_attr_group[] = {
> +       { .attrs = occ_freq_attr[0] },
> +       { .attrs = occ_freq_attr[1] },
> +       { .attrs = occ_freq_attr[2] },
> +       { .attrs = occ_freq_attr[3] },
> +       { .attrs = occ_freq_attr[4] },
> +       { .attrs = occ_freq_attr[5] },
> +       { .attrs = occ_freq_attr[6] },
> +       { .attrs = occ_freq_attr[7] },
> +       { .attrs = occ_freq_attr[8] },
> +       { .attrs = occ_freq_attr[9] },
> +};
> +
> +static struct sensor_device_attribute_2 caps_curr_powercap[] = {
> +       SENSOR_ATTR_2(caps_curr_powercap, S_IRUGO, show_occ_caps, NULL, 0, 0),
> +};
> +static struct sensor_device_attribute_2 caps_curr_powerreading[] = {
> +       SENSOR_ATTR_2(caps_curr_powerreading, S_IRUGO,
> +               show_occ_caps, NULL, 1, 0),
> +};
> +static struct sensor_device_attribute_2 caps_norm_powercap[] = {
> +       SENSOR_ATTR_2(caps_norm_powercap, S_IRUGO, show_occ_caps,
> +               NULL, 2, 0),
> +};
> +static struct sensor_device_attribute_2 caps_max_powercap[] = {
> +       SENSOR_ATTR_2(caps_max_powercap, S_IRUGO, show_occ_caps, NULL, 3, 0),
> +};
> +static struct sensor_device_attribute_2 caps_min_powercap[] = {
> +       SENSOR_ATTR_2(caps_min_powercap, S_IRUGO, show_occ_caps, NULL, 4, 0),
> +};
> +static struct sensor_device_attribute_2 caps_user_powerlimit[] = {
> +       SENSOR_ATTR_2(caps_user_powerlimit, S_IRUGO, show_occ_caps, NULL, 5, 0),
> +};
> +#define CAPS_UNIT_ATTRS(X)                      \
> +{      &caps_curr_powercap[X].dev_attr.attr,           \
> +       &caps_curr_powerreading[X].dev_attr.attr,           \
> +       &caps_norm_powercap[X].dev_attr.attr,           \
> +       &caps_max_powercap[X].dev_attr.attr,           \
> +       &caps_min_powercap[X].dev_attr.attr,           \
> +       &caps_user_powerlimit[X].dev_attr.attr,           \
> +       NULL                                    \
> +}
> +
> +/* 10-core CPU, occ has 1 caps sensors */
> +static struct attribute *occ_caps_attr[][7] = {
> +       CAPS_UNIT_ATTRS(0),
> +};
> +static const struct attribute_group occ_caps_attr_group[] = {
> +       { .attrs = occ_caps_attr[0] },
> +};
> +
> +static struct sensor_device_attribute power_input[] = {
> +       SENSOR_ATTR(power1_input, S_IRUGO, show_occ_power_input, NULL, 0),
> +       SENSOR_ATTR(power2_input, S_IRUGO, show_occ_power_input, NULL, 1),
> +       SENSOR_ATTR(power3_input, S_IRUGO, show_occ_power_input, NULL, 2),
> +       SENSOR_ATTR(power4_input, S_IRUGO, show_occ_power_input, NULL, 3),
> +       SENSOR_ATTR(power5_input, S_IRUGO, show_occ_power_input, NULL, 4),
> +       SENSOR_ATTR(power6_input, S_IRUGO, show_occ_power_input, NULL, 5),
> +       SENSOR_ATTR(power7_input, S_IRUGO, show_occ_power_input, NULL, 6),
> +       SENSOR_ATTR(power8_input, S_IRUGO, show_occ_power_input, NULL, 7),
> +       SENSOR_ATTR(power9_input, S_IRUGO, show_occ_power_input, NULL, 8),
> +       SENSOR_ATTR(power10_input, S_IRUGO, show_occ_power_input, NULL, 9),
> +       SENSOR_ATTR(power11_input, S_IRUGO, show_occ_power_input, NULL, 10),
> +};
> +
> +static struct sensor_device_attribute power_label[] = {
> +       SENSOR_ATTR(power1_label, S_IRUGO, show_occ_power_label, NULL, 0),
> +       SENSOR_ATTR(power2_label, S_IRUGO, show_occ_power_label, NULL, 1),
> +       SENSOR_ATTR(power3_label, S_IRUGO, show_occ_power_label, NULL, 2),
> +       SENSOR_ATTR(power4_label, S_IRUGO, show_occ_power_label, NULL, 3),
> +       SENSOR_ATTR(power5_label, S_IRUGO, show_occ_power_label, NULL, 4),
> +       SENSOR_ATTR(power6_label, S_IRUGO, show_occ_power_label, NULL, 5),
> +       SENSOR_ATTR(power7_label, S_IRUGO, show_occ_power_label, NULL, 6),
> +       SENSOR_ATTR(power8_label, S_IRUGO, show_occ_power_label, NULL, 7),
> +       SENSOR_ATTR(power9_label, S_IRUGO, show_occ_power_label, NULL, 8),
> +       SENSOR_ATTR(power10_label, S_IRUGO, show_occ_power_label, NULL, 9),
> +       SENSOR_ATTR(power11_label, S_IRUGO, show_occ_power_label, NULL, 10),
> +};
> +
> +#define POWER_UNIT_ATTRS(X)                      \
> +{      &power_input[X].dev_attr.attr,           \
> +       &power_label[X].dev_attr.attr,          \
> +       NULL                                    \
> +}
> +
> +/* 10-core CPU, occ has 11 power sensors, more socket, more sensors */
> +static struct attribute *occ_power_attr[][3] = {
> +       POWER_UNIT_ATTRS(0),
> +       POWER_UNIT_ATTRS(1),
> +       POWER_UNIT_ATTRS(2),
> +       POWER_UNIT_ATTRS(3),
> +       POWER_UNIT_ATTRS(4),
> +       POWER_UNIT_ATTRS(5),
> +       POWER_UNIT_ATTRS(6),
> +       POWER_UNIT_ATTRS(7),
> +       POWER_UNIT_ATTRS(8),
> +       POWER_UNIT_ATTRS(9),
> +       POWER_UNIT_ATTRS(10),
> +};
> +
> +static const struct attribute_group occ_power_attr_group[] = {
> +       { .attrs = occ_power_attr[0] },
> +       { .attrs = occ_power_attr[1] },
> +       { .attrs = occ_power_attr[2] },
> +       { .attrs = occ_power_attr[3] },
> +       { .attrs = occ_power_attr[4] },
> +       { .attrs = occ_power_attr[5] },
> +       { .attrs = occ_power_attr[6] },
> +       { .attrs = occ_power_attr[7] },
> +       { .attrs = occ_power_attr[8] },
> +       { .attrs = occ_power_attr[9] },
> +       { .attrs = occ_power_attr[10] },
> +};
> +
> +static ssize_t show_update_interval(struct device *hwmon_dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct device *dev = hwmon_dev->parent;
> +       struct occ_drv_data *data = dev_get_drvdata(dev);
> +
> +        return sprintf(buf, "%u\n", jiffies_to_msecs(data->update_interval));
> +}
> +
> +static ssize_t set_update_interval(struct device *hwmon_dev,
> +                               struct device_attribute *attr,
> +                               const char *buf, size_t count)
> +{
> +       struct device *dev = hwmon_dev->parent;
> +       struct occ_drv_data *data = dev_get_drvdata(dev);
> +        unsigned long val;
> +        int err;
> +
> +        err = kstrtoul(buf, 10, &val);
> +        if (err)
> +                return err;
> +
> +       data->update_interval = msecs_to_jiffies(val);
> +        return count;
> +}
> +static DEVICE_ATTR(update_interval, S_IWUSR | S_IRUGO,
> +               show_update_interval, set_update_interval);
> +
> +static ssize_t show_name(struct device *hwmon_dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +        return sprintf(buf, "%s\n", OCC_I2C_NAME);
> +}
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
> +
> +static void occ_remove_sysfs_files(struct device *dev)
> +{
> +       int i = 0;
> +
> +       device_remove_file(dev, &dev_attr_update_interval);
> +       device_remove_file(dev, &dev_attr_name);
> +
> +       for (i = 0; i < ARRAY_SIZE(occ_temp_attr_group); i++)
> +               sysfs_remove_group(&dev->kobj, &occ_temp_attr_group[i]);
> +
> +       for (i = 0; i < ARRAY_SIZE(occ_freq_attr_group); i++)
> +               sysfs_remove_group(&dev->kobj, &occ_freq_attr_group[i]);
> +
> +       for (i = 0; i < ARRAY_SIZE(occ_power_attr_group); i++)
> +               sysfs_remove_group(&dev->kobj, &occ_power_attr_group[i]);
> +
> +       for (i = 0; i < ARRAY_SIZE(occ_caps_attr_group); i++)
> +               sysfs_remove_group(&dev->kobj, &occ_caps_attr_group[i]);
> +}
> +
> +
> +static int occ_create_sysfs_attribute(struct device *dev)
> +{
> +       /* The sensor number varies for different
> +        * platform depending on core number. We'd better
> +        * create them dynamically
> +        */
> +       struct occ_drv_data *drv_data = dev_get_drvdata(dev);
> +       int i = 0;
> +       int num_of_sensors = 0;
> +       int ret = 0;
> +       struct occ_response *rsp = NULL;
> +
> +       /* get sensor number from occ. */
> +       rsp = &drv_data->occ_resp;
> +
> +       rsp->freq_block_id = -1;
> +       rsp->temp_block_id = -1;
> +       rsp->power_block_id = -1;
> +       rsp->caps_block_id = -1;
> +
> +       ret = occ_update_device(dev);
> +       if (ret != 0) {
> +               dev_err(dev, "ERROR: cannot get occ sensor data: %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (!rsp->blocks)
> +               return -1;
> +
> +       ret = device_create_file(drv_data->hwmon_dev,
> +                       &dev_attr_name);
> +       if (ret)
> +               goto error;
> +
> +       ret = device_create_file(drv_data->hwmon_dev,
> +                       &dev_attr_update_interval);
> +       if (ret)
> +               goto error;
> +
> +       /* temp sensors */
> +       if (rsp->temp_block_id >= 0) {
> +               num_of_sensors =
> +                       rsp->blocks[rsp->temp_block_id].num_of_sensors;
> +               for (i = 0; i < num_of_sensors; i++) {
> +                       ret = sysfs_create_group(&drv_data->hwmon_dev->kobj,
> +                               &occ_temp_attr_group[i]);
> +                       if (ret) {
> +                               dev_err(dev, "error create temp sysfs entry\n");
> +                               goto error;
> +                       }
> +               }
> +       }
> +
> +       /* freq sensors */
> +       if (rsp->freq_block_id >= 0) {
> +               num_of_sensors =
> +                       rsp->blocks[rsp->freq_block_id].num_of_sensors;
> +               for (i = 0; i < num_of_sensors; i++) {
> +                       ret = sysfs_create_group(&drv_data->hwmon_dev->kobj,
> +                               &occ_freq_attr_group[i]);
> +                       if (ret) {
> +                               dev_err(dev, "error create freq sysfs entry\n");
> +                               goto error;
> +                       }
> +               }
> +       }
> +
> +       /* power sensors */
> +       if (rsp->power_block_id >= 0) {
> +               num_of_sensors =
> +                       rsp->blocks[rsp->power_block_id].num_of_sensors;
> +               for (i = 0; i < num_of_sensors; i++) {
> +                       ret = sysfs_create_group(&drv_data->hwmon_dev->kobj,
> +                               &occ_power_attr_group[i]);
> +                       if (ret) {
> +                               dev_err(dev, "error create power sysfs entry\n");
> +                               goto error;
> +                       }
> +               }
> +       }
> +
> +       /* caps sensors */
> +       if (rsp->caps_block_id >= 0) {
> +               num_of_sensors =
> +                       rsp->blocks[rsp->caps_block_id].num_of_sensors;
> +               for (i = 0; i < num_of_sensors; i++) {
> +                       ret = sysfs_create_group(&drv_data->hwmon_dev->kobj,
> +                               &occ_caps_attr_group[i]);
> +                       if (ret) {
> +                               dev_err(dev, "error create caps sysfs entry\n");
> +                               goto error;
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +error:
> +       occ_remove_sysfs_files(drv_data->hwmon_dev);
> +       return ret;
> +}
> +
> +/* device probe and removal */
> +
> +
> +enum occ_type {
> +       occ_id,
> +};
> +
> +static int occ_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +       struct device *dev = &client->dev;
> +       struct occ_drv_data *data;
> +       int ret = 0;
> +
> +       data = devm_kzalloc(dev, sizeof(struct occ_drv_data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       data->client = client;
> +       i2c_set_clientdata(client, data);
> +       mutex_init(&data->update_lock);
> +       data->update_interval = HZ;
> +
> +       /* configure the driver */
> +       dev_dbg(dev, "occ register hwmon @0x%x\n", client->addr);
> +
> +       /* create sysfs attributes based on sensor number read from OCC */
> +       data->hwmon_dev = hwmon_device_register(dev);
> +       if (IS_ERR(data->hwmon_dev))
> +               return PTR_ERR(data->hwmon_dev);
> +
> +       ret = occ_create_sysfs_attribute(dev);
> +       if (ret) {
> +               hwmon_device_unregister(data->hwmon_dev);
> +               return ret;
> +       }
> +
> +       data->hwmon_dev->parent = dev;
> +
> +       dev_dbg(dev, "%s: sensor '%s'\n",
> +                dev_name(data->hwmon_dev), client->name);
> +
> +       dev_info(dev, "occ i2c driver ready: i2c addr at 0x%x\n", client->addr);
> +
> +       return 0;
> +}
> +
> +static int occ_remove(struct i2c_client *client)
> +{
> +       struct occ_drv_data *data = i2c_get_clientdata(client);
> +
> +       /* free allocated sensor memory */
> +       deinit_occ_resp_buf(&data->occ_resp);
> +
> +       occ_remove_sysfs_files(data->hwmon_dev);
> +       hwmon_device_unregister(data->hwmon_dev);
> +       return 0;
> +}
> +
> +/* used by old-style board info. */
> +static const struct i2c_device_id occ_ids[] = {
> +        { OCC_I2C_NAME, occ_id, },
> +        { /* LIST END */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, occ_ids);
> +
> +/* use by device table */
> +static const struct of_device_id i2c_occ_of_match[] = {
> +       {.compatible = "ibm,occ-i2c"},
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_occ_of_match);
> +
> +/* i2c-core uses i2c-detect() to detect device in bellow address list.
> + *  If exists, address will be assigned to client.
> + * It is also possible to read address from device table.
> + */
> +static const unsigned short normal_i2c[] = {0x50, 0x51, I2C_CLIENT_END };
> +
> +static struct i2c_driver occ_driver = {
> +       .class          = I2C_CLASS_HWMON,
> +       .driver = {
> +               .name   = OCC_I2C_NAME,
> +               .pm     = NULL,
> +               .of_match_table = i2c_occ_of_match,
> +       },
> +       .probe          = occ_probe,
> +       .remove         = occ_remove,
> +        .id_table       = occ_ids,
> +       .address_list   = normal_i2c,
> +};
> +
> +module_i2c_driver(occ_driver);
> +
> +MODULE_AUTHOR("Li Yi <shliyi at cn.ibm.com>");
> +MODULE_DESCRIPTION("BMC OCC hwmon driver");
> +MODULE_LICENSE("GPL");


More information about the openbmc mailing list