[RFC PATCH linux 0/9] On-Chip Controller (OCC) hwmon driver
Eddie James
eajames.ibm at gmail.com
Wed Nov 30 09:58:59 AEDT 2016
Hi Andrew,
Thanks a lot for putting together this new patchset! On the whole, it
looks very good! The split between sysfs, occ, and hardware transport
layers is solid. I'm pretty satisfied with your modified structuring,
although I had a couple of comments and questions. I'll send those out
in the applicable patches.
I also tested this on palmetto and barreleye systems and fixed a
couple of bugs. I went ahead and put them in a new patchset, which I'm
about to send out. For reference, here's my only diffs though:
Signed-off-by: Edward A. James <eajames at us.ibm.com>
---
drivers/hwmon/occ/occ.c | 2 ++
drivers/hwmon/occ/occ_sysfs.c | 15 +++++++--------
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/occ/occ.c b/drivers/hwmon/occ/occ.c
index 18936c8..d2fd81e 100644
--- a/drivers/hwmon/occ/occ.c
+++ b/drivers/hwmon/occ/occ.c
@@ -469,6 +470,7 @@ struct occ *occ_start(struct device *dev, void
*bus, struct occ_bus_ops bus_ops,
if (!driver)
return ERR_PTR(-ENOMEM);
+ driver->dev = dev;
driver->bus = bus;
driver->bus_ops = bus_ops;
driver->ops = ops;
diff --git a/drivers/hwmon/occ/occ_sysfs.c b/drivers/hwmon/occ/occ_sysfs.c
index ef45bd0..aab4fbe 100644
--- a/drivers/hwmon/occ/occ_sysfs.c
+++ b/drivers/hwmon/occ/occ_sysfs.c
@@ -63,9 +63,9 @@ static ssize_t show_input(struct device *dev,
struct sensor_attr_data *sdata = container_of(attr,
struct sensor_attr_data,
dev_attr);
- struct occ *driver = dev_get_drvdata(dev);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
- val = occ_get_sensor_value(driver, sdata->type, sdata->hwmon_index - 1);
+ val = occ_get_sensor_value(driver->occ, sdata->type,
sdata->hwmon_index - 1);
if (sdata->type == TEMP)
val *= 1000; /* in millidegree Celsius */
@@ -79,9 +79,9 @@ static ssize_t show_label(struct device *dev,
struct sensor_attr_data *sdata = container_of(attr,
struct sensor_attr_data,
dev_attr);
- struct occ *driver = dev_get_drvdata(dev);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
- val = occ_get_sensor_id(driver, sdata->type,
+ val = occ_get_sensor_id(driver->occ, sdata->type,
sdata->hwmon_index - 1);
return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
@@ -95,15 +95,15 @@ static ssize_t show_caps(struct device *dev,
struct sensor_attr_data *sdata = container_of(attr,
struct sensor_attr_data,
dev_attr);
- struct occ *driver = dev_get_drvdata(dev);
+ struct occ_sysfs *driver = dev_get_drvdata(dev);
- sensor = occ_get_sensor(driver, CAPS);
+ sensor = occ_get_sensor(driver->occ, CAPS);
if (!sensor) {
val = -1;
return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
}
- val = occ_get_caps_value(driver, sensor, sdata->hwmon_index - 1,
+ val = occ_get_caps_value(driver->occ, sensor, sdata->hwmon_index - 1,
sdata->attr_id);
return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
@@ -467,7 +467,6 @@ struct occ_sysfs *occ_sysfs_start(struct device
*dev, struct occ *occ,
if (!hwmon)
return ERR_PTR(-ENOMEM);
- hwmon->dev = dev;
hwmon->occ = occ;
hwmon->num_caps_fields = config->num_caps_fields;
hwmon->caps_names = config->caps_names;
--
1.9.1
On Thu, Nov 10, 2016 at 10:12 PM, Andrew Jeffery <andrew at aj.id.au> wrote:
> Hi Eddie,
>
>> This patchset provides a new OCC hwmon driver. There are a number
>> of issues with the existing driver. Firstly, i2c access was embedded
>> throughout the driver. Secondly, there is no way to easily differentiate
>> versions of the OCC.
>>
>> This patchset addresses the first issue by abstracting the bus transfer
>> protocol into a modular structure. In this way, any low level transfer
>> method may be easily implemented.
>>
>> The second issue is addressed by separating the "version specific" code
>> for the OCC and the common hwmon code. This task is not yet complete, but
>> the general structure is in place. Ultimately, different OCC versions
>> could be probed up using the device tree.
>
> I agree with all of this. However, I thought some of the layering could still
> be improved, so rather than provide a review through prose I figured it might
> be better for both of us if I mangled the patches directly.
>
> I've attacked two things. The first is I found the series hard to review due to
> the way the patches were split, as the patches went from trivial to huge (and
> complex) in a fairly short step:
>
>>
>> Edward A. James (5):
>> Revert "hwmon: Add Power8 OCC hwmon driver"
>> drivers: Add hwmon occ driver framework
>> drivers: hwmon OCC scom bus operations
>> drivers: Add hwmon OCC version specific functions
>> drivers: OCC hwmon driver and sysfs
>
> So I have reworked them into what is now 8 patches (see below) and tried to
> tell a bit more of a story with them (hopefully I succeeded!) Second, I tried
> my hand at reworking the layering as mentioned above, such that hardware
> interaction is split from sysfs, and that the SCOM transport and sensor
> configuration is decoupled from the data parsing. You largely had all of these
> in-place already, I just cut and pasted code around a bit.
>
> Please review the patches and provide feedback! I'd like to hear arguments for
> or against both of our approaches. Note though that I've only tested that these
> patches compile, I absolutely have not done any further testing.
>
> Joel: Any chance you could chime in with an architectural review?
>
> When the dust has settled on the bigger issues we can start to look at the
> finer details of the APIs (there are some bugs, oddities and complexities that
> I would like to address before they are applied to openbmc/linux).
>
> Cheers,
>
> Andrew
>
> Andrew Jeffery (8):
> hwmon: Add core On-Chip Controller support for POWER CPUs
> hwmon: occ: Add sysfs interface
> hwmon: occ: Add I2C SCOM transport implementation
> hwmon: occ: Add callbacks for parsing P8 OCC datastructures
> hwmon: occ: Add hwmon implementation for the P8 OCC
> arm: aspeed: dt: Fix OCC compatible strings
> hwmon: occ: Add callbacks for parsing P9 OCC datastructures
> wip: hwmon: occ: Add sketch of the hwmon implementation for the P9 OCC
>
> Edward A. James (1):
> Revert "hwmon: Add Power8 OCC hwmon driver"
>
> .../devicetree/bindings/i2c/i2c-ibm-occ.txt | 13 -
> arch/arm/boot/dts/aspeed-bmc-opp-barreleye.dts | 4 +-
> arch/arm/boot/dts/aspeed-bmc-opp-firestone.dts | 4 +-
> arch/arm/boot/dts/aspeed-bmc-opp-garrison.dts | 4 +-
> arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts | 2 +-
> drivers/hwmon/Kconfig | 13 +-
> drivers/hwmon/Makefile | 2 +-
> drivers/hwmon/occ/Kconfig | 38 +
> drivers/hwmon/occ/Makefile | 3 +
> drivers/hwmon/occ/occ.c | 494 ++++++++
> drivers/hwmon/occ/occ.h | 79 ++
> drivers/hwmon/occ/occ_p8.c | 219 ++++
> drivers/hwmon/occ/occ_p8.h | 30 +
> drivers/hwmon/occ/occ_p8_i2c.c | 133 +++
> drivers/hwmon/occ/occ_p9.c | 261 ++++
> drivers/hwmon/occ/occ_p9.h | 30 +
> drivers/hwmon/occ/occ_p9_fsi.c | 93 ++
> drivers/hwmon/occ/occ_scom_i2c.c | 68 ++
> drivers/hwmon/occ/occ_scom_i2c.h | 20 +
> drivers/hwmon/occ/occ_sysfs.c | 498 ++++++++
> drivers/hwmon/occ/occ_sysfs.h | 51 +
> drivers/hwmon/occ/scom.h | 50 +
> drivers/hwmon/power8_occ_i2c.c | 1254 --------------------
> 23 files changed, 2076 insertions(+), 1287 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/i2c/i2c-ibm-occ.txt
> 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/occ_p8.c
> create mode 100644 drivers/hwmon/occ/occ_p8.h
> create mode 100644 drivers/hwmon/occ/occ_p8_i2c.c
> create mode 100644 drivers/hwmon/occ/occ_p9.c
> create mode 100644 drivers/hwmon/occ/occ_p9.h
> create mode 100644 drivers/hwmon/occ/occ_p9_fsi.c
> create mode 100644 drivers/hwmon/occ/occ_scom_i2c.c
> create mode 100644 drivers/hwmon/occ/occ_scom_i2c.h
> create mode 100644 drivers/hwmon/occ/occ_sysfs.c
> create mode 100644 drivers/hwmon/occ/occ_sysfs.h
> create mode 100644 drivers/hwmon/occ/scom.h
> delete mode 100644 drivers/hwmon/power8_occ_i2c.c
>
> --
> 2.9.3
>
More information about the openbmc
mailing list