[PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver
Andrew Jeffery
andrew at aj.id.au
Fri Aug 4 10:34:29 AEST 2017
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.
> + 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?
> > + 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?
> + 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?
> + 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)?
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?
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.
> +}
> +
> +static int ibmps_remove(struct i2c_client *client)
> +{
> > + return pmbus_do_remove(client);
> +}
> +
> +enum chips { witherspoon };
This doesn't look necessary.
> +
> +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?
> +
> +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.
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/20170804/c8dd211f/attachment-0001.sig>
More information about the openbmc
mailing list