[RFC PATCH linux v6 5/9] hwmon: occ: Add callbacks for parsing P8 OCC datastructures
Andrew Jeffery
andrew at aj.id.au
Tue Dec 13 14:07:19 AEDT 2016
On Tue, 2016-11-29 at 17:03 -0600, eajames.ibm at gmail.com wrote:
> > From: "Edward A. James" <eajames at us.ibm.com>
>
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.
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> 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
>
> diff --git a/drivers/hwmon/occ/occ_p8.c b/drivers/hwmon/occ/occ_p8.c
> new file mode 100644
> index 0000000..0335a15
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p8.c
> @@ -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;
> +};
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.
> +
> +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]);
Both of the __be32s look like unaligned accesses. Please fix.
> + 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)
Why are we copying bus_ops rather than taking a pointer?
> +{
> + 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 <eajames at us.ibm.com>");
> +MODULE_DESCRIPTION("P8 OCC sensors");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/occ/occ_p8.h b/drivers/hwmon/occ/occ_p8.h
> new file mode 100644
> index 0000000..848d564
> --- /dev/null
> +++ b/drivers/hwmon/occ/occ_p8.h
> @@ -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__
We should probably rename the guard given I changed the name of the
file.
> +
> +#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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20161213/703f6d10/attachment-0001.sig>
More information about the openbmc
mailing list