All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
@ 2015-12-08  8:29 Yi Li
  2015-12-10  4:02 ` Joel Stanley
  0 siblings, 1 reply; 5+ messages in thread
From: Yi Li @ 2015-12-08  8:29 UTC (permalink / raw)
  To: openbmc

The default patch triggered by git pull request is hard to read.
I resend a clean patch to the list. Please review.

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
+
+
+/* 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;
+
+	if (!p)
+		return;
+
+	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);
+	}
+
+	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;
+
+	dev_dbg(&client->dev, "i2c_read: reading %zu bytes @0x%x.\n",
+		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];
+}
+
+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 :
+		o->blocks[o->temp_block_id].sensor;
+		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);
+				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);
+			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);
+			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 */
+#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)
+{
+	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) {
+		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);
+		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];
+				p_sensor->update_tag =
+					(d[dnum+2] << 24) | (d[dnum+3] << 16) |
+					(d[dnum+4] << 8) | d[dnum+5];
+				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) {
+			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;
+	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);
+}
+
+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);
+}
+
+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);
+}
+
+
+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@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@cn.ibm.com>");
+MODULE_DESCRIPTION("BMC OCC hwmon driver");
+MODULE_LICENSE("GPL");

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
  2015-12-08  8:29 [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel Yi Li
@ 2015-12-10  4:02 ` Joel Stanley
  2016-01-05  8:28   ` Yi TZ Li
  0 siblings, 1 reply; 5+ messages in thread
From: Joel Stanley @ 2015-12-10  4:02 UTC (permalink / raw)
  To: Yi Li; +Cc: OpenBMC Maillist, Jeremy Kerr, shliyi

Hello Adam,

On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi@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@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@cn.ibm.com>");
> +MODULE_DESCRIPTION("BMC OCC hwmon driver");
> +MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
  2015-12-10  4:02 ` Joel Stanley
@ 2016-01-05  8:28   ` Yi TZ Li
  2016-01-05 12:59     ` Norman James
  0 siblings, 1 reply; 5+ messages in thread
From: Yi TZ Li @ 2016-01-05  8:28 UTC (permalink / raw)
  To: Joel Stanley; +Cc: Yi Li, OpenBMC Maillist, Jeremy Kerr

[-- Attachment #1: Type: text/plain, Size: 8424 bytes --]

Hi Joel,

Thanks many for your time to review the driver.

I re-submitted to patch, fixed most of the issues you pointed out, except a
few showed bellow.
Please see my reply bellow for details:

joel.stan@gmail.com wrote on 12/10/2015 12:02:19 PM:

> From: Joel Stanley <joel@jms.id.au>
> To: Yi Li <adamliyi@msn.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>, Jeremy Kerr
> <jk@ozlabs.org>, Yi TZ Li/China/IBM@IBMCN
> Date: 12/10/2015 12:03 PM
> Subject: Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
> Sent by: joel.stan@gmail.com
>
> Hello Adam,
>
> On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi@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

I run the checkpatch.pl against the new patch, here are the result:

scripts/checkpatch.pl occ_v4.patch
WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#11: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:30:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#15: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:34:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#33: FILE: arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts:35:
+                                               compatible = "ibm,occ-i2c";

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#74:
new file mode 100644

WARNING: line over 80 characters
#127: FILE: drivers/hwmon/occ_i2c.c:49:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf

WARNING: line over 80 characters
#420: FILE: drivers/hwmon/occ_i2c.c:342:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#1389: FILE: drivers/hwmon/occ_i2c.c:1311:
+       {.compatible = "ibm,occ-i2c"},

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 7 warnings, 1391 lines checked

occ_v4.patch has style problems, please review.

1) For  "DT compatible" warnings, I am not sure whether it is necessary,
since I run "scripts/checkpatch.pl -f
arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts",
it will report many warnings. I can add a description if necessary.

2) for "line over 80" warnings, it would break the link if wrap at 80
characters.

> > +
> > +/* 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?
>

Actually I did not find the document for most of the addresses. I copied
them from Alvin's OCC code.
If anyone know the source (possibly from OCC firmware developers), please
help.
The only OCC document I can find is:
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
It defines OCC_COMMAND_ADDR and OCC_RESPONSE_ADDR only.

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

The deinit_occ_resp_buf() function is supposed to be called in multiple
situation.
E.g, error handling code in a function. In those case, pointer might be
NULL.
So WARN_ON() is not expected in that case.


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

The OCC sensor type and sensor number will change depending on whether OCC
is master or slave.
Also there is possible of bad cores and the number of sensors may change.
So using dynamically allocated sensor block is nature I think.
It is possible that the driver allocates a block of memory and manage
itself.
But looks there is no great benefit.

> > +{
> > +       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.
>

"d[SENSOR_STR_OFFSET]" does not have '\0' to terminate a string. So have to
use strncmp.


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

"sensor_type" has no '\0' to terminate it. strncmp() is necessary.


> > +               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?

Do you mean using snprintf()? After reading the man page:
"Because  sprintf()  and vsprintf() assume an arbitrarily long string,
callers must be careful not to overflow the actual space; this
       is often impossible to assure.  Note that the length of the strings
produced is locale-dependent  and  difficult  to  predict.   Use
       snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf
(3))."

snprintf() seems more safe, but we can find a lot of "sprintf()" in sysfs
show* functions in drivers/hwmon/. Looks to me not a big issue.
Anyway, I will change to snprintf() to be safer:
I use the same code as driver/hwmon/lm95241.c

"        return snprintf(buf, PAGE_SIZE - 1,
                        data->config & to_sensor_dev_attr(attr)->index ?
                        "127000\n" : "255000\n");
"

Thanks,
-adam



[-- Attachment #2: Type: text/html, Size: 13230 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
  2016-01-05  8:28   ` Yi TZ Li
@ 2016-01-05 12:59     ` Norman James
  0 siblings, 0 replies; 5+ messages in thread
From: Norman James @ 2016-01-05 12:59 UTC (permalink / raw)
  To: Yi TZ Li; +Cc: Joel Stanley, OpenBMC Maillist


[-- Attachment #1.1: Type: text/plain, Size: 9569 bytes --]


The OCC register definitions are set by the hardware and can be found in
the P8 SCOM register documentation.   They won't ever change for P8 and P8
+.


> > +
> > +/* 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

Regards,
Norman James
IBM - POWER Systems Architect
Phone: 1-512-286-6807 (T/L: 363-6807)
Internet: njames@us.ibm.com




From:	"Yi TZ Li" <shliyi@cn.ibm.com>
To:	Joel Stanley <joel@jms.id.au>
Cc:	OpenBMC Maillist <openbmc@lists.ozlabs.org>
Date:	01/05/2016 02:30 AM
Subject:	Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
Sent by:	"openbmc" <openbmc-bounces+njames=us.ibm.com@lists.ozlabs.org>



Hi Joel,

Thanks many for your time to review the driver.

I re-submitted to patch, fixed most of the issues you pointed out, except a
few showed bellow.
Please see my reply bellow for details:

joel.stan@gmail.com wrote on 12/10/2015 12:02:19 PM:

> From: Joel Stanley <joel@jms.id.au>
> To: Yi Li <adamliyi@msn.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>, Jeremy Kerr
> <jk@ozlabs.org>, Yi TZ Li/China/IBM@IBMCN
> Date: 12/10/2015 12:03 PM
> Subject: Re: [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel
> Sent by: joel.stan@gmail.com
>
> Hello Adam,
>
> On Tue, Dec 8, 2015 at 6:59 PM, Yi Li <adamliyi@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

I run the checkpatch.pl against the new patch, here are the result:

scripts/checkpatch.pl occ_v4.patch
WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#11: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:30:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#15: FILE: arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts:34:
+                                               compatible = "ibm,occ-i2c";

WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#33: FILE: arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts:35:
+                                               compatible = "ibm,occ-i2c";

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#74:
new file mode 100644

WARNING: line over 80 characters
#127: FILE: drivers/hwmon/occ_i2c.c:49:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf


WARNING: line over 80 characters
#420: FILE: drivers/hwmon/occ_i2c.c:342:
+ *
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf


WARNING: DT compatible string "ibm,occ-i2c" appears un-documented --
check ./Documentation/devicetree/bindings/
#1389: FILE: drivers/hwmon/occ_i2c.c:1311:
+       {.compatible = "ibm,occ-i2c"},

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 7 warnings, 1391 lines checked

occ_v4.patch has style problems, please review.

1) For  "DT compatible" warnings, I am not sure whether it is necessary,
since I run "scripts/checkpatch.pl -f
arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts",
it will report many warnings. I can add a description if necessary.

2) for "line over 80" warnings, it would break the link if wrap at 80
characters.

> > +
> > +/* 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?
>

Actually I did not find the document for most of the addresses. I copied
them from Alvin's OCC code.
If anyone know the source (possibly from OCC firmware developers), please
help.
The only OCC document I can find is:
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf

It defines OCC_COMMAND_ADDR and OCC_RESPONSE_ADDR only.

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

The deinit_occ_resp_buf() function is supposed to be called in multiple
situation.
E.g, error handling code in a function. In those case, pointer might be
NULL.
So WARN_ON() is not expected in that case.


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

The OCC sensor type and sensor number will change depending on whether OCC
is master or slave.
Also there is possible of bad cores and the number of sensors may change.
So using dynamically allocated sensor block is nature I think.
It is possible that the driver allocates a block of memory and manage
itself.
But looks there is no great benefit.

> > +{
> > +       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.
>

"d[SENSOR_STR_OFFSET]" does not have '\0' to terminate a string. So have to
use strncmp.


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

"sensor_type" has no '\0' to terminate it. strncmp() is necessary.


> > +               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?

Do you mean using snprintf()? After reading the man page:
"Because  sprintf()  and vsprintf() assume an arbitrarily long string,
callers must be careful not to overflow the actual space; this
       is often impossible to assure.  Note that the length of the strings
produced is locale-dependent  and  difficult  to  predict.   Use
       snprintf() and vsnprintf() instead (or asprintf(3) and vasprintf
(3))."

snprintf() seems more safe, but we can find a lot of "sprintf()" in sysfs
show* functions in drivers/hwmon/. Looks to me not a big issue.
Anyway, I will change to snprintf() to be safer:
I use the same code as driver/hwmon/lm95241.c

"        return snprintf(buf, PAGE_SIZE - 1,
                        data->config & to_sensor_dev_attr(attr)->index ?
                        "127000\n" : "255000\n");
"

Thanks,
-adam


_______________________________________________
openbmc mailing list
openbmc@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/openbmc


[-- Attachment #1.2: Type: text/html, Size: 16676 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH occ hwmon v3] Add OCC HWMON driver to kernel
@ 2015-12-08  8:21 Yi Li
  0 siblings, 0 replies; 5+ messages in thread
From: Yi Li @ 2015-12-08  8:21 UTC (permalink / raw)
  To: openbmc

The default patch triggered by git pull request is hard to read.
I resend a clean patch to the list. Please review.

This patch fixed many issue from Jeremy's V2 review.

Thanks,
-Yi

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
+
+
+/* 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;
+
+	if (!p)
+		return;
+
+	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);
+	}
+
+	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;
+
+	dev_dbg(&client->dev, "i2c_read: reading %zu bytes @0x%x.\n",
+		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];
+}
+
+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 :
+		o->blocks[o->temp_block_id].sensor;
+		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);
+				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);
+			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);
+			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 */
+#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)
+{
+	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) {
+		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);
+		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];
+				p_sensor->update_tag =
+					(d[dnum+2] << 24) | (d[dnum+3] << 16) |
+					(d[dnum+4] << 8) | d[dnum+5];
+				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) {
+			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;
+	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);
+}
+
+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);
+}
+
+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);
+}
+
+
+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@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@cn.ibm.com>");
+MODULE_DESCRIPTION("BMC OCC hwmon driver");
+MODULE_LICENSE("GPL");

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-01-05 14:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-08  8:29 [PATCH occ hwmon v3] Add OCC hwmon driver to Kernel Yi Li
2015-12-10  4:02 ` Joel Stanley
2016-01-05  8:28   ` Yi TZ Li
2016-01-05 12:59     ` Norman James
  -- strict thread matches above, loose matches on Subject: below --
2015-12-08  8:21 [PATCH occ hwmon v3] Add OCC HWMON driver to kernel Yi Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.