[PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver
Andrew Jeffery
andrew at aj.id.au
Mon Aug 7 15:41:30 AEST 2017
On Fri, 2017-08-04 at 10:03 -0500, Eddie James wrote:
>
> On 08/03/2017 07:34 PM, Andrew Jeffery wrote:
> > On Wed, 2017-08-02 at 15:25 -0500, Eddie James wrote:
> > > > From: "Edward A. James" <eajames at us.ibm.com>
> > >
> > > Add the driver to monitor power supplies with hwmon over pmbus.
> > >
> > > > Signed-off-by: Edward A. James <eajames at us.ibm.com>
> > >
> > > ---
> > > drivers/hwmon/pmbus/Kconfig | 10 +++
> > > drivers/hwmon/pmbus/Makefile | 1 +
> > > drivers/hwmon/pmbus/ibmps.c | 164 +++++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 175 insertions(+)
> > > create mode 100644 drivers/hwmon/pmbus/ibmps.c
> > >
> > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > > index 04f6baa..079a087 100644
> > > --- a/drivers/hwmon/pmbus/Kconfig
> > > +++ b/drivers/hwmon/pmbus/Kconfig
> > > @@ -37,6 +37,16 @@ config SENSORS_ADM1275
> > > > > > > > This driver can also be built as a module. If so, the module will
> > > > be called adm1275.
> > >
> > >
> > > +config SENSORS_IBMPS
> > > > > > > > + tristate "IBM Power Supply"
> > > > > > > > + default n
> > > > > > > > + help
> > > > > > > > + If you say yes here you get hardware monitoring support for IBM
> > > > + power supply.
> > >
> > > +
> > > > > > > > + This driver can also be built as a module. If so, the module will
> > > > + be called ibmps.
> > >
> > > +
> > > config SENSORS_IR35221
> > > > > > > > tristate "Infineon IR35221"
> > > > default n
> > >
> > > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > > index 663801c..76d77e0 100644
> > > --- a/drivers/hwmon/pmbus/Makefile
> > > +++ b/drivers/hwmon/pmbus/Makefile
> > > @@ -5,6 +5,7 @@
> > > > > > > > obj-$(CONFIG_PMBUS) += pmbus_core.o
> > > > > > > > obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
> > > > > > > > obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
> > > > > > > > +obj-$(CONFIG_SENSORS_IBMPS) += ibmps.o
> > > > > > > > obj-$(CONFIG_SENSORS_IR35221) += ir35221.o
> > > > > > > > obj-$(CONFIG_SENSORS_LM25066) += lm25066.o
> > > > obj-$(CONFIG_SENSORS_LTC2978) += ltc2978.o
> > >
> > > diff --git a/drivers/hwmon/pmbus/ibmps.c b/drivers/hwmon/pmbus/ibmps.c
> > > new file mode 100644
> > > index 0000000..1928dd9
> > > --- /dev/null
> > > +++ b/drivers/hwmon/pmbus/ibmps.c
> > > @@ -0,0 +1,164 @@
> > > +/*
> > > + * Copyright 2017 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.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/jiffies.h>
> > > +#include <linux/module.h>
> > > +
> > > +#include "pmbus.h"
> > > +
> >
> > I think a comment is needed here to point out these bits are for
> > STATUS_MFR_SPECIFIC.
> >
> > > > > > +#define IBMPS_MFR_FAN_FAULT BIT(0)
> > > > > > > > +#define IBMPS_MFR_THERMAL_FAULT BIT(1)
> > > > > > > > +#define IBMPS_MFR_OV_FAULT BIT(2)
> > > > > > > > +#define IBMPS_MFR_UV_FAULT BIT(3)
> > > > > > > > +#define IBMPS_MFR_PS_KILL BIT(4)
> > > > > > > > +#define IBMPS_MFR_OC_FAULT BIT(5)
> > > > > > > > +#define IBMPS_MFR_VAUX_FAULT BIT(6)
> > > > +#define IBMPS_MFR_CURRENT_SHARE_WARNING BIT(7)
> > >
> > > +
> > > +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg);
> > > +
> >
> > This makes me curious.
> >
> > > +static int ibmps_read_byte_data(struct i2c_client *client, int page, int reg)
> > > +{
> > > > + int rc, mfr;
> > >
> > > +
> > > > > > > > + switch (reg) {
> > > > > > > > + case PMBUS_STATUS_BYTE:
> > > > + case PMBUS_STATUS_WORD:
> > >
> > > + rc = ibmps_read_word_data(client, page, PMBUS_STATUS_WORD);
> >
> > Feels wrong to return "junk" in the result for a PMBUS_STATUS_BYTE
> > query.
>
> Good point, I'll mask the byte one.
>
> >
> > > > > > + break;
> > > > > > > > + case PMBUS_STATUS_VOUT:
> > > > > > > > + case PMBUS_STATUS_IOUT:
> > > > > > > > + case PMBUS_STATUS_TEMPERATURE:
> > > > > > > > + case PMBUS_STATUS_FAN_12:
> > > > > > > > + rc = pmbus_read_byte_data(client, page, reg);
> > > > > > > > + if (rc < 0)
> > > > + return rc;
> > >
> > > +
> > > > > > > > + mfr = pmbus_read_byte_data(client, page,
> > > > > > > > + PMBUS_STATUS_MFR_SPECIFIC);
> > > > > > > > + if (mfr < 0)
> > > > + return rc;
> > >
> > > +
> > > > > > > > + if (reg == PMBUS_STATUS_FAN_12) {
> > > > > > > > + if (mfr & IBMPS_MFR_FAN_FAULT)
> > > > > > > > + rc |= PB_FAN_FAN1_FAULT;
> > > > > > > > + } else if (reg == PMBUS_STATUS_TEMPERATURE) {
> > > > > > > > + if (mfr & IBMPS_MFR_THERMAL_FAULT)
> > > > > > > > + rc |= PB_TEMP_OT_FAULT;
> > > > > > > > + } else if (reg == PMBUS_STATUS_VOUT) {
> > > > > > > > + if (mfr & (IBMPS_MFR_OV_FAULT | IBMPS_MFR_VAUX_FAULT))
> > > > > > > > + rc |= PB_VOLTAGE_OV_FAULT;
> > > > > > > > + if (mfr & IBMPS_MFR_UV_FAULT)
> > > > > > > > + rc |= PB_VOLTAGE_UV_FAULT;
> > > > > > > > + } else if (reg == PMBUS_STATUS_IOUT) {
> > > > > > > > + if (mfr & IBMPS_MFR_OC_FAULT)
> > > > > > > > + rc |= PB_IOUT_OC_FAULT;
> > > > + if (mfr & IBMPS_MFR_CURRENT_SHARE_WARNING)
> > >
> > > > > > + rc |= PB_CURRENT_SHARE_FAULT;
> > > + }
> >
> > I wonder why it was done this way and not simply via the spec'ed bits?
> > Maybe some don't fit, but it seems there's a pretty direct mapping for
> > the most part.
> >
> > Do the spec'ed status bits ever get set? As in, is there ever some
> > ambiguity about how the bits got set?
>
> Yes I believe some are set by the "normal" status bits, but I'm not sure
> the coverage is complete, so we definitely need to add the MFR register
> bits as well.
Okay, I'll have a look at the datasheet.
>
> >
> > > > > > > > + break;
> > > > > > > > + default:
> > > > > > > > + if (reg >= PMBUS_VIRT_BASE)
> > > > + return -ENXIO;
> > >
> > > +
> > > + rc = pmbus_read_byte_data(client, page, reg);
> >
> > The alternative here is -ENODATA; not sure which is better though.
> >
> > > > > > + break;
> > > > + }
> > >
> > > +
> > > > + return rc;
> > >
> > > +}
> > > +
> > > +static int ibmps_read_word_data(struct i2c_client *client, int page, int reg)
> > > +{
> > > > + int rc, mfr;
> > >
> > > +
> > > > > > > > + switch (reg) {
> > > > > > > > + case PMBUS_STATUS_BYTE:
> > > > > > > > + case PMBUS_STATUS_WORD:
> > > > > > > > + rc = pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
> > > > > > > > + if (rc < 0)
> > > > + return rc;
> > >
> > > +
> > > > > > > > + mfr = pmbus_read_byte_data(client, page,
> > > > > > > > + PMBUS_STATUS_MFR_SPECIFIC);
> > > > > > > > + if (mfr < 0)
> > > > + return rc;
> > >
> > > +
> > > > > > > > + if (mfr & IBMPS_MFR_PS_KILL)
> > > > + rc |= PB_STATUS_OFF;
> > >
> > > +
> > > > + if (mfr)
> > >
> > > + rc |= PB_STATUS_WORD_MFR;
> >
> > This isn't set automatically by the device?
>
> I'll have to check on that.
>
> >
> > > > > > + break;
> > > > > > > > + default:
> > > > > > > > + if (reg >= PMBUS_VIRT_BASE)
> > > > + return -ENXIO;
> > >
> > > +
> > > > > > > > + rc = pmbus_read_word_data(client, page, reg);
> > > > + if (rc < 0)
> > >
> > > + rc = ibmps_read_byte_data(client, page, reg);
> >
> > Is this really the way we should go about accesses?
>
> The power supply seems to mix up byte and word accesses. For example,
> the actual data regs are words, but the status_* are bytes (except
> status_word, obviously). This was basically the only way I could get
> everything working.
I don't understand - even if you read a word register with a byte-
accessor everything should still work (aside from not reading the
amount you're *meant* to).
So this still seems odd to me. Can you rephrase or expand on what
you've said?
>
> >
> > > > > > + break;
> > > > + }
> > >
> > > +
> > > > + return rc;
> > >
> > > +}
> > > +
> > > +static struct pmbus_driver_info ibmps_info = {
> > > > > > > > + .pages = 1,
> > > > > > > > + .func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> > > > > > > > + PMBUS_HAVE_PIN | PMBUS_HAVE_FAN12 | PMBUS_HAVE_TEMP |
> > > > > > > > + PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3 | PMBUS_HAVE_STATUS_VOUT |
> > > > > > > > + PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> > > > > > > > + PMBUS_HAVE_STATUS_TEMP | PMBUS_HAVE_STATUS_FAN12,
> > > > > > > > + .read_byte_data = ibmps_read_byte_data,
> > > > + .read_word_data = ibmps_read_word_data,
> > >
> > > +};
> > > +
> > > +static int ibmps_probe(struct i2c_client *client,
> > > > + const struct i2c_device_id *id)
> > >
> > > +{
> > > + return pmbus_do_probe(client, id, &ibmps_info);
> >
> > Is there anything we need to check before probe, e.g.
> > MFR_ID/MFR_MODEL/MFR_REVISION? Are there options that might need to be
> > configured (and exposed via devicetree bindings)?
>
> Nothing in the spec that I see.
Okay.
>
> >
> > I note we support fans: Is FAN_CONFIG_1_2 configured as needed? Or do
> > we need to enable the fan, configure PWM vs RPM and the tach pulses
> > value? If it is pre-configured, is this register always going to be
> > pre-configured? Or do we need a property like my proposed 'use-stored-
> > presence' for the MAX31785?
>
> I believe they are all configured internally. FAN_CONFIG_1_2 is listed
> in the spec but doesn't state that anything needs to be setup.
>
> >
> > Have you had a look at my proposed bindings for the max31785? I think
> > it would be worth your input, as we have another case of fans here, and
> > it will help us shape a better picture of whether my generic properties
> > and node structure are appropriate.
>
> Sure, I'll take a look.
>
Thanks.
> >
> > > +}
> > > +
> > > +static int ibmps_remove(struct i2c_client *client)
> > > +{
> > > > + return pmbus_do_remove(client);
> > >
> > > +}
> > > +
> > > +enum chips { witherspoon };
> >
> > This doesn't look necessary.
>
> Agreed, it's somewhat of a pmbus convention, but maybe since there's
> just one I'll remove it.
Right, at least IMO it's better to add it when we come to needing it.
>
> >
> > > +
> > > +static const struct i2c_device_id ibmps_id[] = {
> > > > > > > > + { "witherspoon", witherspoon },
> > > > + { }
> > >
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, ibmps_id);
> > > +
> > > +static const struct of_device_id ibmps_of_match[] = {
> > > > > > > > + { .compatible = "ibm,ibmps" },
> > > > + {}
> > >
> > > +};
> > > +MODULE_DEVICE_TABLE(of, p8_i2c_occ_of_match);
> >
> > p8_i2c_occ_of_match isn't right. Do we need both device tables?
> > "witherspoon" as a device ID also seems unusual. What's going on there?
>
> Uhh, not sure how that got in there... How does this compile?? I'll fix
> it. The power supply doesn't have any device id or anything, so I just
> went with witherspoon...
My issue is "withersoon" identifies a class of machine, not a power
supply, even if the powersupply is specific to Witherspoon.
>
> >
> > > +
> > > +static struct i2c_driver ibmps_driver = {
> > > > > > > > + .driver = {
> > > > > > > > + .name = "ibmps",
> > > > > > > > + .of_match_table = ibmps_of_match,
> > > > > > > > + },
> > > > > > > > + .probe = ibmps_probe,
> > > > > > > > + .remove = ibmps_remove,
> > > > + .id_table = ibmps_id,
> > >
> > > +};
> > > +
> > > +module_i2c_driver(ibmps_driver);
> > > +
> > > +MODULE_AUTHOR("Eddie James");
> > > +MODULE_DESCRIPTION("PMBus driver for IBM power supply");
> >
> > So do we anticipate only ever having one power supply design? The name
> > feels a bit generic. In the device id table above you suggest it's
> > called 'witherspoon'. Is the design specific to Witherspoon? If so this
> > fact should be included in the driver name and description.
>
> I was told it is specific to witherspoon. I just hate to make it so
> specific when I'm pretty sure there will be future similar power
> supplies (in fact there was another compatible one that's buggy, present
> on some older witherspoons). But that will work.
Right, but if we can't really claim support power supplies that don't
yet exist. I think we should scope ourselves to what we currently have.
Cheers,
Andrew
>
> >
> > If there are other power supplies that are similar, then when we add
> > support we can rename the driver, and add any necessary module aliases,
> > and have a common device id table listing all of the supported devices.
> >
> > Cheers,
> >
> > Andrew
> >
> > > +MODULE_LICENSE("GPL");
>
>
-------------- 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/20170807/1ad0499f/attachment.sig>
More information about the openbmc
mailing list