hwmon: Add core On-Chip Controller support for POWER CPUs
RFC PATCH v6 2/9 hwmon: Add core On-Chip Controller support for POWER CPUs
From: "Edward A. James" <[email protected]>
The On-Chip Controller (OCC) exposes frequency and temperature sensor information along with power draw measurement and capping for POWER chips. The information is gathered by SCOM reads and writes over a transport of choice, and structured in a flexible fashion to accomodate differences in sensors across designs.
Add core support for querying and parsing the data structures provided by the OCC via SCOMs.
--- drivers/hwmon/Kconfig | 2 + drivers/hwmon/Makefile | 1 + drivers/hwmon/occ/Kconfig | 15 ++ drivers/hwmon/occ/Makefile | 1 + drivers/hwmon/occ/occ.c | 495 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 79 ++++++++ drivers/hwmon/occ/scom.h | 50 +++++ 7 files changed, 643 insertions(+) create mode 100644 drivers/hwmon/occ/Kconfig create mode 100644 drivers/hwmon/occ/Makefile create mode 100644 drivers/hwmon/occ/occ.c create mode 100644 drivers/hwmon/occ/occ.h create mode 100644 drivers/hwmon/occ/scom.h
drivers/hwmon/Kconfig +2 -0
@@ -1226,6 +1226,8 @@ config SENSORS_NSA320 This driver can also be built as a module. If so, the module will be called nsa320-hwmon. +source drivers/hwmon/occ/Kconfig + config SENSORS_PCF8591 tristate "Philips PCF8591 ADC/DAC" depends on I2C
drivers/hwmon/Makefile +1 -0
@@ -165,6 +165,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.o obj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o obj-$(CONFIG_PMBUS) += pmbus/ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ/ ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
drivers/hwmon/occ/Kconfig new file +15 -0
@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCC + bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC). + + Generally this is used by management controllers such as a BMC + on an OpenPower system. + + This driver can also be built as a module. If so, the module + will be called occ.
drivers/hwmon/occ/Makefile new file +1 -0
@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.o
drivers/hwmon/occ/occ.c new file +495 -0
495 lines not on this page · raw message
drivers/hwmon/occ/occ.h new file +79 -0
79 lines not on this page · raw message
drivers/hwmon/occ/scom.h new file +50 -0
50 lines not on this page · raw message
Signature · 1 line
1.9.1
Hi Edward,
I was concerned with bellow piece of code when writing P8 occ driver. Referring to occ spec:
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
Section: 1.6.2 OCC Command/Response Sequence and Appendix A: Return codes.
OCC responds a command with different return code. Currently, the occ driver regards any return code
other than 0x0 (Succeed) as error. But we may need to handle occ return code properly,
e.g: for 0xFF (command in progress), we might not regard it as an error.
Quoted 13 lines
drivers/hwmon/occ/occ.c new file +13
-0+ +static u8 occ_send_cmd (struct occ *driver,u8 seq,u8 type,u16 length, +u8 *data,u8 *resp) +{ +u32 cmd1, cmd2; +u16 checksum; +int i; + + length =cpu_to_le16 (length); + cmd1 = (seq <<24 ) | (type <<16 ) | length; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 ); +
drivers/hwmon/occ/occ.c
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+/* checksum: sum of every bytes of cmd1, cmd2 */ + checksum =0 ; +for (i =0 ; i <4 ; i++) + checksum += (cmd1 >> (i *8 )) &0xFF ; +for (i =0 ; i <4 ; i++) + checksum += (cmd2 >> (i *8 )) &0xFF ; + cmd2 |= checksum << ((2 - length) *8 ); + +/* Init OCB */ + driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
0x08000000,
Quoted 14 lines
drivers/hwmon/occ/occ.c new file +14
-0+ 0x00000000); + driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_AND , +0xFBFFFFFF ,0xFFFFFFFF ); + +/* Send command */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_DATA , cmd1, cmd2); + +/* Trigger attention */ + driver->bus_ops.putscom (driver->bus,ATTN_DATA ,0x01010000 , +0x00000000 );
drivers/hwmon/occ/occ.c line 334
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+ +/* Get response data */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.response_addr,0x00000000 ); + driver->bus_ops.getscom (driver->bus,OCB_DATA , resp,0 ); + +/* return status */ +return resp[2 ]; +} +
Thanks,
-Yi
Thanks Yi, I will add some checks for 0xFF in the next patch set. Looks like that's the only non-zero rc that isn't an error.
Quoted 66 lines · Yi TZ Li
Hi Edward,
I was concerned with bellow piece of code when writing P8 occ driver. Referring to occ spec:
https://github.com/open-power/docs/blob/master/occ/OCC_OpenPwr_FW_Interfaces.pdf
Section: 1.6.2 OCC Command/Response Sequence and Appendix A: Return codes.OCC responds a command with different return code. Currently, the occ driver regards any return code
other than0x0(Succeed) as error. But we may need to handle occ return code properly,
e.g: for0xFF(command in progress), we might not regard it as an error.Quoted 38 lines
drivers/hwmon/occ/occ.c new file +23
-0+ +static u8 occ_send_cmd (struct occ *driver,u8 seq,u8 type,u16 length, +u8 *data,u8 *resp) +{ +u32 cmd1, cmd2; +u16 checksum; +int i; + + length =cpu_to_le16 (length); + cmd1 = (seq <<24 ) | (type <<16 ) | length; +memcpy (&cmd2, data, length); + cmd2 <<= ((4 - length) *8 ); + +/* checksum: sum of every bytes of cmd1, cmd2 */ + checksum =0 ; +for (i =0 ; i <4 ; i++) + checksum += (cmd1 >> (i *8 )) &0xFF ; +for (i =0 ; i <4 ; i++) + checksum += (cmd2 >> (i *8 )) &0xFF ; + cmd2 |= checksum << ((2 - length) *8 ); + +/* Init OCB */ + driver->bus_ops.putscom(driver->bus, OCB_STATUS_CONTROL_OR,
0x08000000,drivers/hwmon/occ/occ.c new file +14
-0+ 0x00000000); + driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_AND , +0xFBFFFFFF ,0xFFFFFFFF ); + +/* Send command */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_DATA , cmd1, cmd2); + +/* Trigger attention */ + driver->bus_ops.putscom (driver->bus,ATTN_DATA ,0x01010000 , +0x00000000 );drivers/hwmon/occ/occ.c line 334
Quoted 10 lines
drivers/hwmon/occ/occ.c new file +10
-0+ +/* Get response data */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.response_addr,0x00000000 ); + driver->bus_ops.getscom (driver->bus,OCB_DATA , resp,0 ); + +/* return status */ +return resp[2 ]; +} +Thanks,
-Yi
Andrew Jeffery
NAKQuoted 5 lines · On Tue, 2016-11-29 at 17:03 -0600, [email protected] wrote:
Quoted 1 line
From: "Edward A. James" <[email protected]>
The On-Chip Controller (OCC) exposes frequency and temperature sensor information along with power draw measurement and capping for POWER chips.
I know you have simply used what I wrote, but I think we need to expand on it. Do we have more definitive documentation on the OCC that we should link to? We should be explaining all that the OCC does in a broad sense so upstream reviewers have an idea of what they are dealing with.
Quoted 1 line
The information is gathered by SCOM reads and writes
Same here: What's a "SCOM"?
Quoted 2 lines
over a
transport of choice,
What do we mean by transport? What are the choices and why pick one?
Quoted 2 lines
and structured in a flexible fashion to accomodate
differences in sensors across designs.
What sensors are we talking about? Only those listed above? How might they be different? What designs are we talking about and how do different designs influence the sensors?
Quoted 3 lines
Add core support for querying and parsing the data structures provided by the OCC via SCOMs.
Can we point to documentation for the data structures?
Quoted 136 lines
Quoted 2 lines
Signed-off-by Edward A. James <[email protected]>---
drivers/hwmon/Kconfig | 2 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/occ/Kconfig | 15 ++
drivers/hwmon/occ/Makefile | 1 +
drivers/hwmon/occ/occ.c | 495 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ.h | 79 ++++++++ drivers/hwmon/occ/scom.h | 50 +++++7 files changed, 643 insertions(+)create mode 100644
drivers/hwmon/occ/Kconfig
create mode 100644drivers/hwmon/occ/Makefile
create mode 100644drivers/hwmon/occ/occ.c
create mode 100644drivers/hwmon/occ/occ.h
create mode 100644drivers/hwmon/occ/scom.hdrivers/hwmon/Kconfig +0
-0@@ -1226,6 +1226,8 @@ config SENSORS_NSA320Quoted 2 lines
This driver can also be built as a module. If so, the module will be called nsa320-hwmon.
drivers/hwmon/Kconfig +2
-0+source drivers/hwmon/occ/Kconfig +config
SENSORS_PCF8591Quoted 2 lines
tristate "Philips PCF8591 ADC/DAC"
depends on I2Cdrivers/hwmon/Makefile +0
-0Quoted 2 lines
(quoted diff) +0
-0@@ -165,6 +165,7 @@ obj-$(CONFIG_SENSORS_WM831X) += wm831x-hwmon.oobj-$(CONFIG_SENSORS_WM8350) += wm8350-hwmon.o
Quoted 2 lines
obj-$(CONFIG_PMBUS) += pmbus/
+obj-$(CONFIG_SENSORS_PPC_OCC) += occ/
ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUGdrivers/hwmon/occ/Kconfig new file +5
-0@@ -0,0 +1,15 @@ +# +# On Chip Controller configuration +# + +menuconfig SENSORS_PPC_OCCdrivers/hwmon/occ/Kconfig line 9
Quoted 4 lines
drivers/hwmon/occ/Kconfig new file +4
-0+ bool "PPC On-Chip Controller" + help + If you say yes here you get support to monitor Power CPU + sensors via the On-Chip Controller (OCC).+
drivers/hwmon/occ/Kconfig line 12
Quoted 2 lines
drivers/hwmon/occ/Kconfig new file +2
-0+ Generally this is used by management controllers such as a BMC + on an OpenPower system.+
Quoted 2 lines
drivers/hwmon/occ/Kconfig new file +2
-0+ This driver can also be built as a module. If so, the module + will be called occ.drivers/hwmon/occ/Makefile new file +1
-0@@ -0,0 +1 @@ +obj-$(CONFIG_SENSORS_PPC_OCC) += occ.odrivers/hwmon/occ/occ.c new file +32
-0@@ -0,0 +1,495 @@ +/* +* occ.c - OCC hwmon driver +* +* This file contains the methods and data structures for the OCC hwmon driver. +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/hwmon.h> +#include <linux/hwmon-sysfs.h> +#include <linux/err.h> +#include <linux/mutex.h> +#include <linux/delay.h> +#include <linux/kernel.h> +#include <linux/device.h> + +#include "occ.h" +Quoted 1 line
+#define
OCC_DATA_MAX4096drivers/hwmon/occ/occ.c new file +2
-0+ +/* To generate attn to OCC */ Quoted 1 line
+#define
ATTN_DATA0x0006B035+/* For BMC to read/write SRAM */
drivers/hwmon/occ/occ.c line 41
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+#define OCB_ADDRESS 0x0006B070 +#define OCB_DATA 0x0006B075 +#define OCB_STATUS_CONTROL_AND 0x0006B072 +#define OCB_STATUS_CONTROL_OR 0x0006B073+
drivers/hwmon/occ/occ.c line 47
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+#define RESP_DATA_LENGTH 3 +#define RESP_HEADER_OFFSET 5 +#define SENSOR_STR_OFFSET 37 +#define SENSOR_BLOCK_NUM_OFFSET 43 +#define SENSOR_BLOCK_OFFSET 45+
Quoted 1 line
+#define
MAX_SENSOR_ATTR_LEN32drivers/hwmon/occ/occ.c new file +2
-0+ +struct occ_poll_header {Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+u8 status; +u8 ext_status; +u8 occs_present; +u8 config; +u8 occ_state; +u8 mode; +u8 ips_status; +u8 error_log_id; +u32 error_log_addr_start;
Quoted 8 lines
drivers/hwmon/occ/occ.c line 67
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +7
-0+u16 error_log_length; +u8 reserved2; +u8 reserved3; +u8 occ_code_level[16 ]; +u8 sensor_eye_catcher[6 ]; +u8 sensor_block_num; +u8 sensor_data_version;+};
It looks like we might be casting this over a byte blob we get back from the OCC. If this is the case I think we should probably declare this __attribute__((packed, aligned(4))) to be clear. The layout shouldn't contain any padding, but I think we should be explicit about it.
Even better would be to clear up any confusion here by providing some kerneldoc for the structure.
drivers/hwmon/occ/occ.c line 74
Quoted 7 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +struct occ_response {Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+u8 sequence_num; +u8 cmd_type; +u8 rtn_status;drivers/hwmon/occ/occ.c new file +1
-0+u16 data_length;
I can't see the data_length member being used anywhere. Best delete it if that is the case.
Quoted 5 lines
drivers/hwmon/occ/occ.c line 78
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+struct occ_poll_header header; +u16 chk_sum; +int sensor_block_id[MAX_OCC_SENSOR_TYPE ]; +struct sensor_data_block *blocks;+};
I'd suggest kerneldoc here too
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +struct occ {drivers/hwmon/occ/occ.c line 82
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+struct device *dev;+
Quoted 10 lines
drivers/hwmon/occ/occ.c line 92
Quoted 9 lines
drivers/hwmon/occ/occ.c new file +9
-0+void *bus; +struct occ_bus_ops bus_ops; +struct occ_ops ops; +struct occ_config config; +unsigned long update_interval; +unsigned long last_updated; +struct mutex update_lock; +struct occ_response response; +bool valid;+};
And here.
Quoted 31 lines
drivers/hwmon/occ/occ.c new file +3
-0+ +static void deinit_occ_resp_buf (struct occ_response *resp) +{Review
Quoted 1 line
(quoted diff) +1
-0+ int i;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp) + return;+
drivers/hwmon/occ/occ.c line 103
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!resp->blocks) +return ;+
drivers/hwmon/occ/occ.c line 107
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+for (i =0 ; i < resp->header.sensor_block_num; ++i) { +kfree (resp->blocks[i].sensors); + }+
drivers/hwmon/occ/occ.c line 109
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+kfree (resp->blocks);+
drivers/hwmon/occ/occ.c line 111
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+memset (resp,0 ,sizeof (struct occ_response ));+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+for (i =0 ; i <MAX_OCC_SENSOR_TYPE ; ++i) + resp->sensor_block_id[i] =-1 ;drivers/hwmon/occ/occ.c new file +3
-0+} + +static void *occ_get_sensor_by_type(struct occ_response *resp,drivers/hwmon/occ/occ.c line 118
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ enum sensor_type t)+{
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (!resp->blocks) + return NULL;+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (resp->sensor_block_id[t] ==-1 ) +return NULL ;
drivers/hwmon/occ/occ.c line 126
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +return resp->blocks[resp->sensor_block_id[t]].sensors;
+}
Quoted 13 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +static int occ_renew_sensor(struct occ *driver, u8 sensor_length,drivers/hwmon/occ/occ.c line 130
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ u8 sensor_num, enum sensor_type t, int block)+{
drivers/hwmon/occ/occ.c line 134
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+void *sensor; +int rc; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 136
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ sensor =occ_get_sensor_by_type (resp, t);+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* empty sensor block, release older sensor data */ +if (sensor_num ==0 || sensor_length ==0 ) { +kfree (sensor);
drivers/hwmon/occ/occ.c line 141
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return -1 ;
What is -1 meant to signify given you're using the standard return code (-ENOMEM) below? That is, do you want it to mean -EPERM? If not, then how are we meant to interpret the return code? Special-case -1 and hope permission is never a problem? Please resolve this.
Separately, Is freeing the structure reasonable behaviour for a function named '*renew*'? Is this some opaque way to free the sensor? It just strikes me as a bit odd. If it is reasonable can you please add a comment to justify it?
drivers/hwmon/occ/occ.c
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +2
-0+ } +Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+if (!sensor || sensor_num != + resp->blocks[resp->sensor_block_id[t]].sensor_num) {drivers/hwmon/occ/occ.c new file +1
-0+kfree (sensor);
The above might be relevant here too.
Quoted 20 lines
drivers/hwmon/occ/occ.c new file +1
-0+ resp->blocks[block].sensors =drivers/hwmon/occ/occ.c line 153
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ driver->ops.alloc_sensor (t, sensor_num); +if (!resp->blocks[block].sensors) { + rc = -ENOMEM ; + goto err; + } + }+
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;+err:
Quoted 2 lines
(quoted diff) +2
-0+ deinit_occ_resp_buf(resp); + return rc;drivers/hwmon/occ/occ.c new file +3
-0+} + +static int parse_occ_response(struct occ *driver, u8 *data,drivers/hwmon/occ/occ.c line 162
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ struct occ_response *resp)+{
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+int b; +int s; +int rc;
drivers/hwmon/occ/occ.c line 167
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int dnum =SENSOR_BLOCK_OFFSET ;
What does 'dnum' mean? Can we call this something better?
drivers/hwmon/occ/occ.c line 175
Quoted 8 lines
drivers/hwmon/occ/occ.c new file +1
-0+u8 sensor_block_num;Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+u8 sensor_type[4 ]; +u8 sensor_format; +u8 sensor_length; +u8 sensor_num; +struct device *dev = driver->dev; + void (*parse_sensor)(u8 *data, void *sensor, int sensor_type, int off,drivers/hwmon/occ/occ.c new file +1
-0+ int snum) = driver->ops.parse_sensor;
This seems unfortunate.
drivers/hwmon/occ/occ.c
Quoted 5 lines
+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* check if the data is valid */ +if (strncmp (&data[SENSOR_STR_OFFSET ],"SENSOR" ,6 ) !=0 ) { +dev_dbg (dev,"ERROR: no SENSOR String in response\n" );drivers/hwmon/occ/occ.c new file +1
-0+ rc =-1 ;
Again, we should pick a relevant error code. Please audit the rest of the patch for this issue; I'll refrain from commenting on every instance.
drivers/hwmon/occ/occ.c line 186
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +1
-0+ goto err;Review
Quoted 1 line
(quoted diff) +1
-0+ }+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ sensor_block_num = data[SENSOR_BLOCK_NUM_OFFSET ]; +if (sensor_block_num ==0 ) {drivers/hwmon/occ/occ.c new file +1
-0+dev_dbg (dev,"ERROR: SENSOR block num is 0\n" );
dev_dbg(dev, "ERROR: ...");
This seems like a contradiction. Why not dev_err()? Or drop the "ERROR:"?
Secondly, this is the number of sensor blocks in the response? Maybe we could reword this to be "Query returned no sensor blocks" for clarity.
drivers/hwmon/occ/occ.c line 194
Quoted 8 lines
drivers/hwmon/occ/occ.c new file +1
-0+ rc =-1 ;Review
Quoted 2 lines
(quoted diff) +2
-0+ goto err; + }+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* if sensor block has changed, re-malloc */ +if (sensor_block_num != resp->header.sensor_block_num) { +deinit_occ_resp_buf (resp);drivers/hwmon/occ/occ.c new file +1
-0+ resp->blocks = kcalloc(sensor_block_num,
Is there any other sanity checking that we need to apply? I guess at least we are currently bounded at 255 * sizeof(struct sensor_data_block).
drivers/hwmon/occ/occ.c line 195
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ sizeof(struct sensor_data_block),
I prefer to use the dereferenced type for sizeof() in these instances:
sizeof(*resp->blocks)
that way if the type changes we are still consistent.
Quoted 16 lines
drivers/hwmon/occ/occ.c new file +1
-0+ GFP_KERNEL);drivers/hwmon/occ/occ.c line 199
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+if (!resp->blocks) +return -ENOMEM ; + }+
drivers/hwmon/occ/occ.c line 206
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+memcpy (&resp->header, &data[RESP_HEADER_OFFSET ], + sizeof(struct occ_poll_header)); + resp->header.error_log_addr_start = +be32_to_cpu (resp->header.error_log_addr_start); + resp->header.error_log_length = +be16_to_cpu (resp->header.error_log_length);+
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+dev_dbg (dev,"Reading %d sensor blocks\n" , + resp->header.sensor_block_num); +for (b =0 ; b < sensor_block_num; b++) { +/* 8-byte sensor block head */
drivers/hwmon/occ/occ.c line 212
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+strncpy (sensor_type, &data[dnum],4 );
This makes me nervous. Can we please allocate 5 elements to sensor_type, and set the last one to '\0'?
drivers/hwmon/occ/occ.c line 216
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +1
-0+ sensor_format = data[dnum +5 ];Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ sensor_length = data[dnum +6 ]; + sensor_num = data[dnum +7 ];drivers/hwmon/occ/occ.c new file +1
-0+ dnum = dnum +8 ;
Is there anything we could do to improve the above approach? Use a packed struct definition to cast over it?
drivers/hwmon/occ/occ.c
Quoted 5 lines
+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+dev_dbg (dev,"sensor block[%d]: type: %s, sensor_num: %d\n" , b, + sensor_type, sensor_num);drivers/hwmon/occ/occ.c new file +2
-0+ +if (strncmp (sensor_type,"FREQ" ,4 ) ==0 ) {
The above suggestion would make it easier to hand off to a static inline handler for each of the processing bodies. I think that would make this function easier to read - at the moment it's just a wall of conditional text to me.
drivers/hwmon/occ/occ.c
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +1
-0+ rc = occ_renew_sensor(driver, sensor_length, sensor_num,Quoted 2 lines
(quoted diff) +2
-0+ FREQ, b); + if (rc)drivers/hwmon/occ/occ.c new file +1
-0+continue ;
Should we use one of dev_{debug,info,warn}() here? Something went wrong. If it was an allocation failure there's probably not much point processing the remaining sensors.
drivers/hwmon/occ/occ.c
Quoted 7 lines
+
Quoted 4 lines
drivers/hwmon/occ/occ.c new file +4
-0+ resp->sensor_block_id[FREQ ] = b; +for (s =0 ; s < sensor_num; s++) { +parse_sensor (data, resp->blocks[b].sensors, + FREQ, dnum, s);drivers/hwmon/occ/occ.c new file +1
-0+ dnum = dnum + sensor_length;
I'd rather we have separate counters that we add to access a sensor in a block, so we have a "global" offset for blocks and a "local" offset for sensors (inside a block). Use dnum as the global offset and have this loop increment the sensor offset, and then after the loop increment dnum to the start of the following block using the accumulated sensor offset.
The comments for the "FREQ" case are largely applicable to the remaining cases.
Quoted 67 lines
drivers/hwmon/occ/occ.c new file +1
-0+ }drivers/hwmon/occ/occ.c line 237
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+ }else if (strncmp (sensor_type,"TEMP" ,4 ) ==0 ) { + rc =occ_renew_sensor (driver, sensor_length, + sensor_num, TEMP, b); +if (rc) +continue ;+
drivers/hwmon/occ/occ.c line 249
Quoted 11 lines
drivers/hwmon/occ/occ.c new file +11
-0+ resp->sensor_block_id[TEMP ] = b; +for (s =0 ; s < sensor_num; s++) { +parse_sensor (data, resp->blocks[b].sensors, + TEMP, dnum, s); + dnum = dnum + sensor_length; + } + }else if (strncmp (sensor_type,"POWR" ,4 ) ==0 ) { + rc =occ_renew_sensor (driver, sensor_length, + sensor_num,POWER , b); +if (rc) +continue ;+
drivers/hwmon/occ/occ.c line 261
Quoted 11 lines
drivers/hwmon/occ/occ.c new file +11
-0+ resp->sensor_block_id[POWER ] = b; +for (s =0 ; s < sensor_num; s++) { +parse_sensor (data, resp->blocks[b].sensors, + POWER, dnum, s); + dnum = dnum + sensor_length; + } + }else if (strncmp (sensor_type,"CAPS" ,4 ) ==0 ) { + rc =occ_renew_sensor (driver, sensor_length, + sensor_num,CAPS , b); +if (rc) +continue ;+
drivers/hwmon/occ/occ.c line 268
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ resp->sensor_block_id[CAPS ] = b; +for (s =0 ; s < sensor_num; s++) { +parse_sensor (data, resp->blocks[b].sensors, + CAPS, dnum, s); + dnum = dnum + sensor_length; + }+
drivers/hwmon/occ/occ.c line 275
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ }else { +dev_dbg (dev,"ERROR: sensor type %s not supported\n" , + resp->blocks[b].sensor_type); + rc =-1 ; + goto err; + }+
drivers/hwmon/occ/occ.c line 281
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+strncpy (resp->blocks[b].sensor_type, sensor_type,4 ); + resp->blocks[b].sensor_format = sensor_format; + resp->blocks[b].sensor_length = sensor_length; + resp->blocks[b].sensor_num = sensor_num; + }+
Review
Quoted 1 line
(quoted diff) +1
-0+ return 0;+err:
Quoted 2 lines
(quoted diff) +2
-0+ deinit_occ_resp_buf(resp); + return rc;drivers/hwmon/occ/occ.c new file +3
-0+} + +static u8 occ_send_cmd(struct occ *driver, u8 seq, u8 type, u16 length,drivers/hwmon/occ/occ.c line 290
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ u8 *data, u8 *resp)+{
drivers/hwmon/occ/occ.c line 294
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+u32 cmd1, cmd2; +u16 checksum; +int i;+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ length =cpu_to_le16 (length); + cmd1 = (seq <<24 ) | (type <<16 ) | length;
drivers/hwmon/occ/occ.c line 298
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+memcpy (&cmd2, data, length);
NAK.
You can't use length after you've forced it to a given endianness on a CPU that can operate in both BE an LE modes.
This will work by accident, not by design.
drivers/hwmon/occ/occ.c line 306
Quoted 8 lines
drivers/hwmon/occ/occ.c new file +2
-0+ cmd2 <<= ((4 - length) *8 ); +Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* checksum: sum of every bytes of cmd1, cmd2 */ + checksum =0 ; +for (i =0 ; i <4 ; i++) + checksum += (cmd1 >> (i *8 )) &0xFF ; +for (i =0 ; i <4 ; i++)drivers/hwmon/occ/occ.c new file +1
-0+ checksum += (cmd2 >> (i *8 )) &0xFF ;
Addition is commutative:
for (i = 0; i < 4; i++) {
checksum += (cmd1 >> (i * 8)) & 0xFF;
checksum += (cmd2 >> (i * 8)) & 0xFF;
}Quoted 22 lines
drivers/hwmon/occ/occ.c new file +2
-0+ cmd2 |= checksum << ((2 - length) *8 ); +drivers/hwmon/occ/occ.c line 313
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+/* Init OCB */ + driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_OR ,0x08000000 , +0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_STATUS_CONTROL_AND , +0xFBFFFFFF ,0xFFFFFFFF );+
drivers/hwmon/occ/occ.c line 320
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+/* Send command */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.command_addr,0x00000000 ); + driver->bus_ops.putscom (driver->bus,OCB_DATA , cmd1, cmd2);+
drivers/hwmon/occ/occ.c line 324
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* Trigger attention */ + driver->bus_ops.putscom (driver->bus,ATTN_DATA ,0x01010000 , +0x00000000 );+
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+/* Get response data */ + driver->bus_ops.putscom (driver->bus,OCB_ADDRESS , + driver->config.response_addr,0x00000000 );
drivers/hwmon/occ/occ.c line 329
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ driver->bus_ops.getscom (driver->bus,OCB_DATA , resp,0 );
I'd like some documentation for all of the magic values above.
Further, getscom() and putscom() return ints, presumable a success/error code. None of the code above is doing any error handling. Please fix it.
drivers/hwmon/occ/occ.c line 332
Quoted 3 lines
+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+/* return status */ drivers/hwmon/occ/occ.c new file +1
-0+return resp[2 ];
Hmm, I guess it's handy to do this.
drivers/hwmon/occ/occ.c line 335
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+} + +static inline u16 get_occdata_length (u8 *data)
Looks like you've missed an underscore in the function name.
drivers/hwmon/occ/occ.c
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+{ +return be16_to_cpup ((const __be16 *)&data[RESP_DATA_LENGTH ]);
This looks problematic with respect to alignment. By [1] the AST2400 can handle this through an MMU trap, but by [2] this is highly discouraged in favour of the macros in the relevant unaligned.h (e.g. [3]).
[1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0360f/CDFEJCBH.html
[2] https://www.kernel.org/doc/Documentation/arm/mem_alignment
[3] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/asm-generic/unaligned.h
Quoted 17 lines
drivers/hwmon/occ/occ.c new file +4
-0+} + +static int occ_get_all (struct occ *driver) +{drivers/hwmon/occ/occ.c line 347
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+int i =0 , rc; +u8 *occ_data; +u16 num_bytes; +u8 poll_cmd_data =0x10 ; +struct device *dev = driver->dev; +struct occ_response *resp = &driver->response;+
drivers/hwmon/occ/occ.c line 351
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ occ_data =devm_kzalloc (dev,OCC_DATA_MAX ,GFP_KERNEL ); +if (!occ_data) +return -ENOMEM ;+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ rc =occ_send_cmd (driver,0 ,0 ,1 , &poll_cmd_data, occ_data); +if (rc) {
drivers/hwmon/occ/occ.c line 355
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+dev_err (dev,"ERROR: OCC Poll: 0x%x\n" , rc);
Please don't prefix these format strings with ERROR. Audit the patch for any instances of this issue.
Quoted 21 lines
drivers/hwmon/occ/occ.c new file +1
-0+ rc = -EINVAL ;Review
Quoted 2 lines
(quoted diff) +2
-0+ goto out; + }+
drivers/hwmon/occ/occ.c line 361
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ num_bytes =get_occdata_length (occ_data); +dev_dbg (dev,"OCC data length: %d\n" , num_bytes);+
drivers/hwmon/occ/occ.c line 367
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes >OCC_DATA_MAX ) { +dev_dbg (dev,"ERROR: OCC data length must be < 4KB\n" ); + rc = -EINVAL ; + goto out; + }+
drivers/hwmon/occ/occ.c line 373
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+if (num_bytes <=0 ) { +dev_dbg (dev,"ERROR: OCC data length is zero\n" ); + rc = -EINVAL ; + goto out; + }+
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+/* read remaining data */ +for (i =8 ; i < num_bytes +8 ; i +=8 )
drivers/hwmon/occ/occ.c line 377
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ driver->bus_ops.getscom (driver->bus,OCB_DATA , occ_data, i);
This interface needs work. I'd much rather we could write:
occ_data[i] = driver->bus_ops.getscom(driver->bus, OCB_DATA);
Looking at patch 4/9 I can't see any reason why we'd need such an unusual interface.
Quoted 1 line
+
Below, parse_occ_response() doesn't have the length information for occ_data, and at no point do we ever test that it's a relevant length as opposed to an insane length: That is, above you test that it's less than the maximum expected size and at least greater than zero, but num_bytes == 1 passes both of those cases and will cause parse_occ_response() to do bad things in broad daylight. Please add more sanity checking to the point where we at least validate that parse_occ_response()'s actions aren't going to access invalid memory.
Quoted 40 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =parse_occ_response (driver, occ_data, resp); + +out:Quoted 2 lines
(quoted diff) +2
-0+ devm_kfree(dev, occ_data); + return rc;drivers/hwmon/occ/occ.c new file +4
-0+} + +int occ_update_device (struct occ *driver) +{drivers/hwmon/occ/occ.c line 388
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+int rc =0 ;+
drivers/hwmon/occ/occ.c line 390
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_lock (&driver->update_lock);+
drivers/hwmon/occ/occ.c line 394
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+if (time_after (jiffies, driver->last_updated + driver->update_interval) + || !driver->valid) { + driver->valid =1 ;+
drivers/hwmon/occ/occ.c line 398
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+ rc =occ_get_all (driver); +if (rc) + driver->valid =0 ;+
Review
Quoted 2 lines
(quoted diff) +2
-0+ driver->last_updated = jiffies; + }+
drivers/hwmon/occ/occ.c line 403
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+mutex_unlock (&driver->update_lock);+
Quoted 1 line
(quoted diff) +1
-0+ return rc;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_update_device); + +void *occ_get_sensor (struct occ *driver,int sensor_type) +{Review
Quoted 1 line
(quoted diff) +1
-0+ int rc;+
Quoted 6 lines
drivers/hwmon/occ/occ.c new file +6
-0+ rc =occ_update_device (driver); +if (rc !=0 ) { +dev_dbg (driver->dev,"ERROR: cannot get occ sensor data: %d\n" , + rc); +return NULL ; + }
drivers/hwmon/occ/occ.c line 420
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ +return occ_get_sensor_by_type (&driver->response, sensor_type);
Don't we need to take update_lock before doing this to prevent concurrent updates?
Quoted 41 lines
drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor); + +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_value (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_value); + +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_sensor_id (occ, sensor_type, snum);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_sensor_id); + +int occ_get_caps_value (struct occ *occ,void *sensor,int snum,int caps_field) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ->ops.get_caps_value (sensor, snum, caps_field);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_caps_value); + +void occ_get_response_blocks (struct occ *occ,struct occ_blocks *blocks) +{Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ blocks->sensor_block_id = occ->response.sensor_block_id; + blocks->blocks = occ->response.blocks;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_get_response_blocks); + +void occ_set_update_interval (struct occ *occ,unsigned long interval) +{Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ occ->update_interval =msecs_to_jiffies (interval);drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_set_update_interval); + +int occ_set_user_powercap (struct occ *occ,u16 cap) +{drivers/hwmon/occ/occ.c line 457
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+u8 resp[8 ];+
drivers/hwmon/occ/occ.c line 459
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+ cap =cpu_to_le16 (cap);+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return occ_send_cmd (occ,0 ,0x22 ,2 , (u8 *)&cap, resp);
drivers/hwmon/occ/occ.c line 466
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_set_user_powercap); + +struct occ *occ_start (struct device *dev,void *bus,struct occ_bus_ops bus_ops, + struct occ_ops ops, struct occ_config config)
Why are we copying the structs rather than taking pointers?
Quoted 63 lines
+{
drivers/hwmon/occ/occ.c line 470
Quoted 3 lines
drivers/hwmon/occ/occ.c new file +3
-0+struct occ *driver =devm_kzalloc (dev,sizeof (struct occ ),GFP_KERNEL ); +if (!driver) +return ERR_PTR (-ENOMEM );+
drivers/hwmon/occ/occ.c line 476
Quoted 5 lines
drivers/hwmon/occ/occ.c new file +5
-0+ driver->dev = dev; + driver->bus = bus; + driver->bus_ops = bus_ops; + driver->ops = ops; + driver->config = config;+
drivers/hwmon/occ/occ.c line 479
Quoted 2 lines
drivers/hwmon/occ/occ.c new file +2
-0+ driver->update_interval =HZ ; +mutex_init (&driver->update_lock);+
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+return driver;drivers/hwmon/occ/occ.c new file +5
-0+} +EXPORT_SYMBOL (occ_start); + +int occ_stop (struct occ *occ) +{drivers/hwmon/occ/occ.c line 487
Quoted 1 line
drivers/hwmon/occ/occ.c new file +1
-0+devm_kfree (occ->dev, occ);+
Quoted 1 line
(quoted diff) +1
-0+ return 0;drivers/hwmon/occ/occ.c new file +3
-0+} +EXPORT_SYMBOL (occ_stop); +Quoted 1 line
+MODULE_AUTHOR("Eddie James <[email protected]>");
drivers/hwmon/occ/occ.c new file +2
-0+MODULE_DESCRIPTION ("OCC hwmon core driver" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ.h new file +28
-0@@ -0,0 +1,79 @@ +/* +* occ.h - hwmon OCC driver +* +* This file contains data structures and function prototypes for common access +* between different bus protocols and host systems. +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __OCC_H__ +#define __OCC_H__ + +#include "scom.h" + +struct device ; +struct occ_driver ; + +struct sensor_data_block {
Quoted 7 lines
drivers/hwmon/occ/occ.h line 34
Quoted 6 lines
drivers/hwmon/occ/occ.h new file +6
-0+u8 sensor_type[4 ]; +u8 reserved0; +u8 sensor_format; +u8 sensor_length; +u8 sensor_num; +void *sensors;+};
Do we need the reserved0 member?
Quoted 97 lines
drivers/hwmon/occ/occ.h new file +2
-0+ +enum sensor_type {Quoted 5 lines
drivers/hwmon/occ/occ.h new file +5
-0+FREQ =0 , +TEMP , +POWER , +CAPS , +MAX_OCC_SENSOR_TYPE drivers/hwmon/occ/occ.h new file +5
-0+}; + +struct occ ; + +struct occ_ops {Quoted 6 lines
drivers/hwmon/occ/occ.h new file +6
-0+void (*parse_sensor)(u8 *data,void *sensor,int sensor_type,int off, + int snum); +void *(*alloc_sensor)(int sensor_type,int num_sensors); +int (*get_sensor_value)(struct occ *driver,int sensor_type,int snum); +int (*get_sensor_id)(struct occ *driver,int sensor_type,int snum); +int (*get_caps_value)(void *sensor,int snum,int caps_field);drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_config {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+u32 command_addr; +u32 response_addr;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ_blocks {Quoted 2 lines
drivers/hwmon/occ/occ.h new file +2
-0+int *sensor_block_id; +struct sensor_data_block *blocks;drivers/hwmon/occ/occ.h new file +3
-0+}; + +struct occ *occ_start(struct device *dev, void *bus, struct occ_bus_ops bus_ops,Quoted 1 line
drivers/hwmon/occ/occ.h new file +1
-0+ const struct occ_ops ops, const struct occ_config config);drivers/hwmon/occ/occ.h new file +12
-0+int occ_stop (struct occ *occ); + +void *occ_get_sensor (struct occ *occ,int sensor_type); +int occ_get_sensor_value (struct occ *occ,int sensor_type,int snum); +int occ_get_sensor_id (struct occ *occ,int sensor_type,int snum); +int occ_get_caps_value (struct occ *occ,void *sensor,int snum,int caps_field); +void occ_get_response_blocks (struct occ *occ,struct occ_blocks *blocks); +int occ_update_device (struct occ *driver); +void occ_set_update_interval (struct occ *occ,unsigned long interval); +int occ_set_user_powercap(struct occ *occ, u16 cap); + +#endif /* __OCC_H__ */ drivers/hwmon/occ/scom.h new file +21
-0@@ -0,0 +1,50 @@ +/* +* scom.h - hwmon OCC driver +* +* This file contains data structures for scom operations to the OCC +* +* Copyright 2016 IBM Corp. +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#ifndef __SCOM_H__ +#define __SCOM_H__ +Quoted 2 lines
drivers/hwmon/occ/scom.h new file +2
-0+#define SCOM_READ_ERROR 1 +#define SCOM_WRITE_ERROR 2drivers/hwmon/occ/scom.h new file +22
-0+/* +* occ_bus_ops - represent the low-level transfer methods to communicate with +* the OCC. +* +* getscom - OCC scom read +* @bus: handle to slave device +* @address: address +* @data: where to store data read from slave; buffer size must be greater than +* or equal to offset + 8 bytes. +* @offset: offset into data pointer +* +* Returns 0 on success or one of -SCOM_READ_ERROR, -SCOM_WRITE_ERROR. +* +* putscom - OCC scom write +* @bus: handle to slave device +* @address: address +* @data0: first data byte to write +* @data1: second data byte to write +* +* Returns 0 on success or -SCOM_WRITE_ERROR on error +*/ +struct occ_bus_ops {Quoted 2 lines
drivers/hwmon/occ/scom.h new file +2
-0+int (*getscom)(void *bus,u32 address,u8 *data,size_t offset); +int (*putscom)(void *bus,u32 address,u32 data0,u32 data1);
drivers/hwmon/occ/scom.h line 50
Quoted 3 lines
drivers/hwmon/occ/scom.h new file +3
-0+}; + +#endif /* __SCOM_H__ */
Andrew
Quoted 10 lines · Andrew Jeffery
Quoted 3 lines
Quoted 2 lines
+ /* read remaining data */
+ for (i = 8; i <num_bytes+ 8; i += 8)+ driver->bus_ops.getscom(driver->bus,
OCB_DATA,occ_data, i);This interface needs work. I'd much rather we could write:
occ_data[i] =
driver->bus_ops.getscom(driver->bus, OCB_DATA);Looking at patch 4/9 I can't see any reason why we'd need such an unusual interface.
Actually, I take that back slightly. Ultimately I'd like to get away from passing i as a parameter. So instead we could continue to use the return value for errors, and pass a pointer to the memory getscom() should modify:
rc = driver->bus_ops.getscom(driver->bus, OCB_DATA, (u64 *)&occ_data[i]);
The value in (u64 *)&occ_data[i] is only valid if the call is success etc.
Andrew