Re: [PATCH linux v7 6/6] hwmon: occ: Add callbacks for parsing P9 OCC datastructures
From: Joel Stanley <joel@jms.id.au>
Date: 2017-02-10 05:33:38
Also in:
linux-hwmon, lkml
On Wed, Feb 8, 2017 at 9:40 AM, [off-list ref] wrote:
From: "Edward A. James" <redacted> Add functions to parse the data structures that are specific to the OCC on the POWER9 processor. These are the sensor data structures, including temperature, frequency, power, and "caps." Signed-off-by: Edward A. James <redacted> Signed-off-by: Andrew Jeffery <redacted> --- Documentation/hwmon/occ | 3 + drivers/hwmon/occ/occ_p9.c | 309 +++++++++++++++++++++++++++++++++++++++++++++ drivers/hwmon/occ/occ_p9.h | 30 +++++ 3 files changed, 342 insertions(+) create mode 100644 drivers/hwmon/occ/occ_p9.c create mode 100644 drivers/hwmon/occ/occ_p9.h
quoted hunk ↗ jump to hunk
diff --git a/drivers/hwmon/occ/occ_p9.c b/drivers/hwmon/occ/occ_p9.c new file mode 100644 index 0000000..9c1283c --- /dev/null +++ b/drivers/hwmon/occ/occ_p9.c@@ -0,0 +1,309 @@ +/* + * occ_p9.c - OCC hwmon driver + * + * This file contains the Power9-specific methods and data structures for + * the OCC hwmon driver. + * + * Copyright 2016 IBM Corp.
It's 2017.
+ * + * 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.
We generally just include the first paragraph. Same goes for all of the files.
+
+static const u32 p9_sensor_hwmon_configs[MAX_OCC_SENSOR_TYPE] = {
+ HWMON_I_INPUT | HWMON_I_LABEL, /* freq: value | label */
+ /* temp: value | label | fru_type */
+ HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_TYPE,
+ /* power: value | label | accum[0] | accum[1] | update_tag |
+ * (function_id | (apss_channel << 8))
+ */
+ HWMON_P_INPUT | HWMON_P_LABEL | HWMON_P_AVERAGE_MIN |
+ HWMON_P_AVERAGE_MAX | HWMON_P_AVERAGE_INTERVAL |
+ HWMON_P_RESET_HISTORY,
+ /* caps: curr | max | min | norm | user | source */
+ HWMON_P_CAP | HWMON_P_CAP_MAX | HWMON_P_CAP_MIN | HWMON_P_MAX |
+ HWMON_P_ALARM | HWMON_P_CAP_ALARM,
I find this really hard to read. Perhaps something like this:
#define FREQ_CONFIG (HWMON_I_INPUT | HWMON_I_LABEL)
#deifne TEMP_CONFIG (HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_TYPE)
#define POWER_CONFIG ( HWMON_P_INPUT | HWMON_P_LABEL |
HWMON_P_AVERAGE_MIN | \
HWMON_P_AVERAGE_MAX |
HWMON_P_AVERAGE_INTERVAL | \
HWMON_P_RESET_HISTORY)
etc. Do the same in the p8 driver.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hwmon/occ/occ_p9.h b/drivers/hwmon/occ/occ_p9.h new file mode 100644 index 0000000..18ca16a --- /dev/null +++ b/drivers/hwmon/occ/occ_p9.h
+ +#ifndef __OCC_P9_H__ +#define __OCC_P9_H__ + +#include "scom.h" + +struct device;
Include the header for struct device instead. Did you consider the one header file for all of your shared functions? I don't think there's much value in having a whole heap of small ones.
+ +const u32 *p9_get_sensor_hwmon_configs(void); +struct occ *p9_occ_start(struct device *dev, void *bus, + struct occ_bus_ops *bus_ops); + +#endif /* __OCC_P9_H__ */ -- 1.8.3.1