[PATCH linux dev-4.10 v3 1/5] drivers/hwmon/pmbus: Add IBM power supply hwmon driver

[Eddie James]

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.

>
>>> +		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.

>
>> +		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.

>
> 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.

>
>> +}
>> +
>> +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.

>
>> +
>> +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...

>
>> +
>> +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.

>
> 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");