On Wed, Feb 8, 2017 at 9:40 AM, <eajames@xxxxxxxxxxxxxxxxxx> wrote:
> From: "Edward A. James" <eajames@xxxxxxxxxx>
>
> 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 <eajames@xxxxxxxxxx>
> Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> ---
> 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
> 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.
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
