hwmon: occ: Add callbacks for parsing P8 OCC datastructures
RFC PATCH v6 5/9 hwmon: occ: Add callbacks for parsing P8 OCC datastructures
From: "Edward A. James" <[email protected]>
--- drivers/hwmon/occ/occ_p8.c | 219 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p8.h | 30 +++++++ 2 files changed, 249 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p8.c create mode 100644 drivers/hwmon/occ/occ_p8.h
drivers/hwmon/occ/occ_p8.c new file +219 -0
@@ -0,0 +1,219 @@ +/* +* p8.c - OCC hwmon driver +* +* This file contains the Power8-specific 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/err.h> +#include <linux/of.h> +#include <linux/kernel.h> +#include <linux/device.h> +#include <linux/platform_device.h> + +#include "occ.h" +#include "occ_p8.h" + +/* P8 OCC sensor data format */ +struct occ_sensor_p8 { +u16 sensor_id; +u16 value; +}; + +struct power_sensor_p8 { +u16 sensor_id; +u32 update_tag; +u32 accumulator; +u16 value; +}; + +struct caps_sensor_p8 { +u16 curr_powercap; +u16 curr_powerreading; +u16 norm_powercap; +u16 max_powercap; +u16 min_powercap; +u16 user_powerlimit; +}; + +void p8_parse_sensor (u8 *data,void *sensor,int sensor_type,int off, +int snum) +{ +switch (sensor_type) { +case FREQ : +case TEMP : + { +struct occ_sensor_p8 *os = + &(((struct occ_sensor_p8 *)sensor)[snum]); + os->sensor_id = +be16_to_cpup ((const __be16 *)&data[off]); + os->value = +be16_to_cpup ((const __be16 *)&data[off +2 ]); + } +break ; +case POWER : + { +struct power_sensor_p8 *ps = + &(((struct power_sensor_p8 *)sensor)[snum]); + ps->sensor_id = +be16_to_cpup ((const __be16 *)&data[off]); + ps->update_tag = +be32_to_cpup ((const __be32 *)&data[off +2 ]); + ps->accumulator = +be32_to_cpup ((const __be32 *)&data[off +6 ]); + ps->value = +be16_to_cpup ((const __be16 *)&data[off +10 ]); + } +break ; +case CAPS : + { +struct caps_sensor_p8 *cs = + &(((struct caps_sensor_p8 *)sensor)[snum]); + cs->curr_powercap = +be16_to_cpup ((const __be16 *)&data[off]); + cs->curr_powerreading = +be16_to_cpup ((const __be16 *)&data[off +2 ]); + cs->norm_powercap = +be16_to_cpup ((const __be16 *)&data[off +4 ]); + cs->max_powercap = +be16_to_cpup ((const __be16 *)&data[off +6 ]); + cs->min_powercap = +be16_to_cpup ((const __be16 *)&data[off +8 ]); + cs->user_powerlimit = +be16_to_cpup ((const __be16 *)&data[off +10 ]); + } +break ; + }; +} + +void *p8_alloc_sensor (int sensor_type,int num_sensors) +{ +switch (sensor_type) { +case FREQ : +case TEMP : +return kcalloc (num_sensors, +sizeof (struct occ_sensor_p8 ), +GFP_KERNEL ); +case POWER : +return kcalloc (num_sensors, +sizeof (struct power_sensor_p8 ), +GFP_KERNEL ); +case CAPS : +return kcalloc (num_sensors, +sizeof (struct caps_sensor_p8 ), +GFP_KERNEL ); +default : +return NULL ; + } +} + +int p8_get_sensor_value (struct occ *driver,int sensor_type,int snum) +{ +void *sensor; + +if (sensor_type ==CAPS ) +return -1 ; + + sensor =occ_get_sensor (driver, sensor_type); +if (!sensor) +return -1 ; + +switch (sensor_type) { +case FREQ : +case TEMP : +return ((struct occ_sensor_p8 *)sensor)[snum].value; +case POWER : +return ((struct power_sensor_p8 *)sensor)[snum].value; +default : +return -1 ; + } +} + +int p8_get_sensor_id (struct occ *driver,int sensor_type,int snum) +{ +void *sensor; +int i = snum; + +if (sensor_type ==CAPS ) +return -1 ; + + sensor =occ_get_sensor (driver, sensor_type); +if (!sensor) +return -1 ; + +switch (sensor_type) { +case FREQ : +case TEMP : +return ((struct occ_sensor_p8 *)sensor)[i].sensor_id; +case POWER : +return ((struct power_sensor_p8 *)sensor)[i].sensor_id; +default : +return -1 ; + } +} + +int p8_get_caps_value (void *sensor,int snum,int caps_field) +{ +struct caps_sensor_p8 *caps_sensor = sensor; + +switch (caps_field) { +case 0 : +return caps_sensor[snum].curr_powercap; +case 1 : +return caps_sensor[snum].curr_powerreading; +case 2 : +return caps_sensor[snum].norm_powercap; +case 3 : +return caps_sensor[snum].max_powercap; +case 4 : +return caps_sensor[snum].min_powercap; +case 5 : +return caps_sensor[snum].user_powerlimit; +default : +return -1 ; + } +} + +static const struct occ_ops p8_ops = { + .parse_sensor = p8_parse_sensor, + .alloc_sensor = p8_alloc_sensor, + .get_sensor_value = p8_get_sensor_value, + .get_sensor_id = p8_get_sensor_id, + .get_caps_value = p8_get_caps_value, +}; + +static const struct occ_config p8_config = { + .command_addr =0xFFFF6000 , + .response_addr =0xFFFF7000 , +}; + +struct occ *occ_p8_start (struct device *dev,void *bus,struct occ_bus_ops bus_ops) +{ +return occ_start (dev, bus, bus_ops, p8_ops, p8_config); +} +EXPORT_SYMBOL (occ_p8_start); + +int occ_p8_stop (struct occ *occ) +{ +return occ_stop (occ); +} +EXPORT_SYMBOL (occ_p8_stop); + +MODULE_AUTHOR ("Eddie James <[email protected]>" ); +MODULE_DESCRIPTION ("P8 OCC sensors" ); +MODULE_LICENSE ("GPL" );
drivers/hwmon/occ/occ_p8.h new file +30 -0
@@ -0,0 +1,30 @@ +/* +* p8.h - OCC hwmon driver +* +* This file contains Power8-specific function prototypes +* +* 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 __P8_H__ +#define __P8_H__ + +#include "scom.h" + +struct device ; + +struct occ *occ_p8_start (struct device *dev,void *bus, +struct occ_bus_ops bus_ops); +int occ_p8_stop (struct occ *occ); + +#endif /* __P8_H__ */
Signature · 1 line
1.9.1
Quoted 2 lines · On Tue, 2016-11-29 at 17:03 -0600, [email protected] wrote:
Quoted 1 line
From: "Edward A. James" <[email protected]>
Again this is probably driven by my patches, but we need to describe what we are doing here. Pointers to documentation and any relevant sections inside would be handy to upstream reviewers.
Quoted 40 lines
Signed-off-by Edward A. James <[email protected]>---
drivers/hwmon/occ/occ_p8.c | 219 +++++++++++++++++++++++++++++++++++++++++++++drivers/hwmon/occ/occ_p8.h| 30 +++++++2 files changed, 249 insertions(+)create mode 100644
drivers/hwmon/occ/occ_p8.c
create mode 100644drivers/hwmon/occ/occ_p8.hdrivers/hwmon/occ/occ_p8.c new file +26
-0@@ -0,0 +1,219 @@ +/* +* p8.c - OCC hwmon driver +* +* This file contains the Power8-specific 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/err.h> +#include <linux/of.h> +#include <linux/kernel.h> +#include <linux/device.h>
drivers/hwmon/occ/occ_p8.c line 36
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+#include <linux/platform_device.h> + +#include "occ.h" +#include "occ_p8.h" + +/* P8 OCC sensor data format */ +struct occ_sensor_p8 { + u16 sensor_id; + u16 value; +};
A bit of a nit with consistency:
We should pick one approach to naming our types and symbols. Here the struct has "p8" as a suffix, but some of the functions use "p8" as a prefix, and yet others use "p8" somewhere in the middle of the name.
My 2c is to use p8_occ as a prefix, so:
struct p8_occ_sensor {...};
void *p8_occ_alloc_sensor(...) {...};
int p8_occ_start(...) {...};
What colour do you pick for the bikeshed? Whatever you pick, please fix all the types and symbols.
Quoted 32 lines
drivers/hwmon/occ/occ_p8.c new file +32
-0+ +struct power_sensor_p8 { + u16 sensor_id; + u32 update_tag; + u32 accumulator; + u16 value; +}; + +struct caps_sensor_p8 { + u16 curr_powercap; + u16 curr_powerreading; + u16 norm_powercap; + u16 max_powercap; + u16 min_powercap; + u16 user_powerlimit; +}; + +void p8_parse_sensor (u8 *data,void *sensor,int sensor_type,int off, + int snum) +{ + switch (sensor_type) { + case FREQ: + case TEMP: + { + struct occ_sensor_p8 *os = + &(((struct occ_sensor_p8 *)sensor)[snum]); + os->sensor_id = + be16_to_cpup((const __be16 *)&data[off]); + os->value = + be16_to_cpup((const __be16 *)&data[off + 2]); + } + break;
drivers/hwmon/occ/occ_p8.c line 78
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+ case POWER: + { + struct power_sensor_p8 *ps = + &(((struct power_sensor_p8 *)sensor)[snum]); + ps->sensor_id = + be16_to_cpup((const __be16 *)&data[off]); + ps->update_tag = + be32_to_cpup((const __be32 *)&data[off + 2]); + ps->accumulator = + be32_to_cpup((const __be32 *)&data[off + 6]);
Both of the __be32s look like unaligned accesses. Please fix.
Quoted 117 lines
drivers/hwmon/occ/occ_p8.c new file +117
-0+ ps->value = + be16_to_cpup((const __be16 *)&data[off + 10]); + } + break; + case CAPS: + { + struct caps_sensor_p8 *cs = + &(((struct caps_sensor_p8 *)sensor)[snum]); + cs->curr_powercap = + be16_to_cpup((const __be16 *)&data[off]); + cs->curr_powerreading = + be16_to_cpup((const __be16 *)&data[off + 2]); + cs->norm_powercap = + be16_to_cpup((const __be16 *)&data[off + 4]); + cs->max_powercap = + be16_to_cpup((const __be16 *)&data[off + 6]); + cs->min_powercap = + be16_to_cpup((const __be16 *)&data[off + 8]); + cs->user_powerlimit = + be16_to_cpup((const __be16 *)&data[off + 10]); + } + break; + }; +} + +void *p8_alloc_sensor (int sensor_type,int num_sensors) +{ + switch (sensor_type) { + case FREQ: + case TEMP: + return kcalloc(num_sensors, + sizeof(struct occ_sensor_p8), + GFP_KERNEL); + case POWER: + return kcalloc(num_sensors, + sizeof(struct power_sensor_p8), + GFP_KERNEL); + case CAPS: + return kcalloc(num_sensors, + sizeof(struct caps_sensor_p8), + GFP_KERNEL); + default: + return NULL; + } +} + +int p8_get_sensor_value (struct occ *driver,int sensor_type,int snum) +{ + void *sensor; + + if (sensor_type == CAPS) + return -1; + + sensor = occ_get_sensor(driver, sensor_type); + if (!sensor) + return -1; + + switch (sensor_type) { + case FREQ: + case TEMP: + return ((struct occ_sensor_p8 *)sensor)[snum].value; + case POWER: + return ((struct power_sensor_p8 *)sensor)[snum].value; + default: + return -1; + } +} + +int p8_get_sensor_id (struct occ *driver,int sensor_type,int snum) +{ + void *sensor; + int i = snum; + + if (sensor_type == CAPS) + return -1; + + sensor = occ_get_sensor(driver, sensor_type); + if (!sensor) + return -1; + + switch (sensor_type) { + case FREQ: + case TEMP: + return ((struct occ_sensor_p8 *)sensor)[i].sensor_id; + case POWER: + return ((struct power_sensor_p8 *)sensor)[i].sensor_id; + default: + return -1; + } +} + +int p8_get_caps_value (void *sensor,int snum,int caps_field) +{ + struct caps_sensor_p8 *caps_sensor = sensor; + + switch (caps_field) { + case 0: + return caps_sensor[snum].curr_powercap; + case 1: + return caps_sensor[snum].curr_powerreading; + case 2: + return caps_sensor[snum].norm_powercap; + case 3: + return caps_sensor[snum].max_powercap; + case 4: + return caps_sensor[snum].min_powercap; + case 5: + return caps_sensor[snum].user_powerlimit; + default: + return -1; + } +} + +static const struct occ_ops p8_ops = { + .parse_sensor = p8_parse_sensor, + .alloc_sensor = p8_alloc_sensor, + .get_sensor_value = p8_get_sensor_value,
drivers/hwmon/occ/occ_p8.c line 205
Quoted 10 lines
drivers/hwmon/occ/occ_p8.c new file +10
-0+ .get_sensor_id = p8_get_sensor_id, + .get_caps_value = p8_get_caps_value, +}; + +static const struct occ_config p8_config = { + .command_addr = 0xFFFF6000, + .response_addr = 0xFFFF7000, +}; + +struct occ *occ_p8_start (struct device *dev,void *bus,struct occ_bus_ops bus_ops)
Why are we copying bus_ops rather than taking a pointer?
Quoted 30 lines
drivers/hwmon/occ/occ_p8.c new file +11
-0+{ + return occ_start(dev, bus, bus_ops, p8_ops, p8_config); +} +EXPORT_SYMBOL (occ_p8_start); + +int occ_p8_stop (struct occ *occ) +{ + return occ_stop(occ); +} +EXPORT_SYMBOL (occ_p8_stop); +Quoted 1 line
+MODULE_AUTHOR("Eddie James <[email protected]>");
drivers/hwmon/occ/occ_p8.c new file +2
-0+MODULE_DESCRIPTION ("P8 OCC sensors" ); +MODULE_LICENSE ("GPL" );drivers/hwmon/occ/occ_p8.h new file +10
-0@@ -0,0 +1,30 @@ +/* +* p8.h - OCC hwmon driver +* +* This file contains Power8-specific function prototypes +* +* 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
drivers/hwmon/occ/occ_p8.h line 20
Quoted 10 lines
drivers/hwmon/occ/occ_p8.h new file +10
-0+* (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 __P8_H__ +#define __P8_H__
We should probably rename the guard given I changed the name of the file.
drivers/hwmon/occ/occ_p8.h line 30
Quoted 10 lines
drivers/hwmon/occ/occ_p8.h new file +10
-0+ +#include "scom.h" + +struct device ; + +struct occ *occ_p8_start (struct device *dev,void *bus, + struct occ_bus_ops bus_ops); +int occ_p8_stop (struct occ *occ); + +#endif /* __P8_H__ */
Cheers,
Andrew