Re: [RFC PATCH v2 3/3] pmbus: Add MAX31785 driver

From: Andrew Jeffery
Date: Wed Jul 19 2017 - 21:26:50 EST

Next message: Boqun Feng: "Re: [PATCH] documentation: Fix two-CPU control-dependency example"
Previous message: Stephen Rothwell: "linux-next: manual merge of the drm-misc tree with the drm-intel tree"
In reply to: Guenter Roeck: "Re: [RFC PATCH v2 3/3] pmbus: Add MAX31785 driver"
Next in thread: Andrew Jeffery: "[RFC PATCH v2 2/3] pmbus: Add and expose pmbus_get_info()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, 2017-07-19 at 11:11 -0700, Guenter Roeck wrote:
> On Thu, Jul 20, 2017 at 12:35:09AM +0930, Joel Stanley wrote:
> > > > On Tue, Jul 18, 2017 at 1:06 PM, Andrew Jeffery <andrew@xxxxxxxx> wrote:
> > > The driver features fan control and basic dual-tachometer support.
> >
> > Say something about the hardware?
> >
> > Âmax31785 is a fancy fan controller with temp measurement, pwm, tach,
> > breakfast making from Maxim.
> >
> >
> > >
> > > The fan control makes use of the new virtual registers exposed by the
> > > pmbus core, mixing use of the default implementations with some
> > > overrides via the read/write handlers. FAN_COMMAND_1 on the MAX31785
> > > breaks the values into bands that depend on the RPM or PWM control mode.
> > >
> > > The dual tachometer feature is implemented in hardware with a TACHSEL
> > > input to indicate the rotor under measurement, and exposed on the bus by
> > > extending the READ_FAN_SPEED_1 word with two extra bytes*.
> > > The need to read the non-standard four-byte response leads to a cut-down
> > > implementation of i2c_smbus_xfer_emulated() included in the driver.
> > > Further, to expose the second rotor tachometer value to userspace,
> > > virtual fans are implemented by re-routing the FAN_CONFIG_1_2 register
> > > from the undefined (in hardware) pages 23-28 to the same register on
> > > pages 0-5, and similarly re-routing READ_FAN_SPEED_1 requests on 23-28
> > > to pages 0-5 but extracting the second word of the four-byte response.
> > >
> > > * The documentation recommends the slower rotor be associated with
> > > TACHSEL=0, which is the input used by the controller's closed-loop fan
> > > management.
> > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> > > ---
> > > v1 -> v2:
> > >
> > > * Implement in terms of virtual registers
> > > * Add support for dual-tachometer readings through 'virtual fans' (unused pages)
> > >
> > > Âdrivers/hwmon/pmbus/KconfigÂÂÂÂ|ÂÂ10 ++
> > > Âdrivers/hwmon/pmbus/MakefileÂÂÂ|ÂÂÂ1 +
> > > Âdrivers/hwmon/pmbus/max31785.c | 372 +++++++++++++++++++++++++++++++++++++++++
> > > Â3 files changed, 383 insertions(+)
> > > Âcreate mode 100644 drivers/hwmon/pmbus/max31785.c
> > >
> > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > > index cad1229b7e17..5f2f3c6c7499 100644
> > > --- a/drivers/hwmon/pmbus/Kconfig
> > > +++ b/drivers/hwmon/pmbus/Kconfig
> > > @@ -95,6 +95,16 @@ config SENSORS_MAX20751
> > > ÂÂÂÂÂÂÂÂÂÂThis driver can also be built as a module. If so, the module will
> > > ÂÂÂÂÂÂÂÂÂÂbe called max20751.
> > >
> > > +config SENSORS_MAX31785
> > > +ÂÂÂÂÂÂÂtristate "Maxim MAX31785 and compatibles"
> > > +ÂÂÂÂÂÂÂdefault n
> > > +ÂÂÂÂÂÂÂhelp
> > > +ÂÂÂÂÂÂÂÂÂIf you say yes here you get hardware monitoring support for Maxim
> > > +ÂÂÂÂÂÂÂÂÂMAX31785.
> > > +
> > > +ÂÂÂÂÂÂÂÂÂThis driver can also be built as a module. If so, the module will
> > > +ÂÂÂÂÂÂÂÂÂbe called max31785.
> > > +
> > > Âconfig SENSORS_MAX34440
> > > ÂÂÂÂÂÂÂÂtristate "Maxim MAX34440 and compatibles"
> > > ÂÂÂÂÂÂÂÂdefault n
> > > diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
> > > index 562132054aaf..4ea548a8af46 100644
> > > --- a/drivers/hwmon/pmbus/Makefile
> > > +++ b/drivers/hwmon/pmbus/Makefile
> > > @@ -10,6 +10,7 @@ obj-$(CONFIG_SENSORS_LTC2978) += ltc2978.o
> > > Âobj-$(CONFIG_SENSORS_LTC3815)ÂÂ+= ltc3815.o
> > > Âobj-$(CONFIG_SENSORS_MAX16064) += max16064.o
> > > Âobj-$(CONFIG_SENSORS_MAX20751) += max20751.o
> > > +obj-$(CONFIG_SENSORS_MAX31785) += max31785.o
> > > Âobj-$(CONFIG_SENSORS_MAX34440) += max34440.o
> > > Âobj-$(CONFIG_SENSORS_MAX8688)ÂÂ+= max8688.o
> > > Âobj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
> > > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > > new file mode 100644
> > > index 000000000000..1baa961f2eb1
> > > --- /dev/null
> > > +++ b/drivers/hwmon/pmbus/max31785.c
> > > @@ -0,0 +1,372 @@
> > > +/*
> > > + * Copyright (C) 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/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/init.h>
> > > +#include <linux/err.h>
> > > +#include <linux/i2c.h>
> > > +#include "pmbus.h"
> > > +
> > > +#define MFR_FAN_CONFIG_DUAL_TACHÂÂÂÂÂÂÂBIT(12)
> > > +#define MFR_FAN_CONFIG_TSFOÂÂÂÂÂÂÂÂÂÂÂÂBIT(9)
> > > +#define MFR_FAN_CONFIG_TACHOÂÂÂÂÂÂÂÂÂÂÂBIT(8)
> > > +
> > > +#define MAX31785_CAP_DUAL_TACHÂÂÂÂÂÂÂÂÂBIT(0)
> > > +
> > > +struct max31785 {
> > > +ÂÂÂÂÂÂÂstruct pmbus_driver_info info;
> > > +
> > > +ÂÂÂÂÂÂÂu32 capabilities;
> > > +};
> > > +
> > > +enum max31785_regs {
> > > +ÂÂÂÂÂÂÂPMBUS_MFR_FAN_CONFIGÂÂÂÂÂÂÂÂÂÂÂÂ= 0xF1,
> > > +ÂÂÂÂÂÂÂPMBUS_MFR_READ_FAN_PWMÂÂÂÂÂÂÂÂÂÂ= 0xF3,
> > > +ÂÂÂÂÂÂÂPMBUS_MFR_FAN_FAULT_LIMITÂÂÂÂÂÂÂ= 0xF5,
> > > +ÂÂÂÂÂÂÂPMBUS_MFR_FAN_WARN_LIMITÂÂÂÂÂÂÂÂ= 0xF6,
> > > +ÂÂÂÂÂÂÂPMBUS_MFR_FAN_PWM_AVGÂÂÂÂÂÂÂÂÂÂÂ= 0xF8,
> > > +};
> > > +
> > > +#define to_max31785(_c) container_of(pmbus_get_info(_c), struct max31785, info)
> > > +
> > > +static int max31785_read_byte_data(struct i2c_client *client, int page,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint reg)
> > > +{
> > > +ÂÂÂÂÂÂÂstruct max31785 *chip = to_max31785(client);
> > > +ÂÂÂÂÂÂÂint rv = -ENODATA;
> >
> > I'd drop this.
> >
> > > +
> > > +ÂÂÂÂÂÂÂswitch (reg) {
> > > +ÂÂÂÂÂÂÂcase PMBUS_VOUT_MODE:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (page < 23)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOTSUPP;
> > > +ÂÂÂÂÂÂÂcase PMBUS_FAN_CONFIG_12:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (page < 23)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (WARN_ON(!(chip->capabilities & MAX31785_CAP_DUAL_TACH)))
> >
> > Not sure about the WARN_ON here - won't users get spammed every time
> > someone reads a byte of data? When do we expect this case to happen?
> >
>
> Presumably this would be an implementation error (there is no page numbered 23
> or larger if the bit is not set). This can't happen, thus the check is really
> unnecessary and just bloats the code.
>
> This also means that the 'capabilities' variable is also unecessary, since it
> is only used to verify that the PMBus core really understood that there are
> only 24 pages if MAX31785_CAP_DUAL_TACH is not set (and, to be _really_ sure,
> it keeps verifying it over and over again).

This was mainly some (overzealous) sanity checking whilst I was
developing the driver (I have also developed a PMBus QEMU model in the
process - was trying to catch thinkos in on both sides). As you point
out it's unnecessary if everything works as it should.

I'll remove this and other unnecessary verifications before posting the
next revision.

>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOTSUPP;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = pmbus_read_byte_data(client, page - 23, reg);
> >
> > return pmbus_read_byte_data(client, page - 23, reg);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂreturn rv;
> >
> > return -ENODATA;
> >
>
> -ENODATA: Calling code is expected to handle the command.
>
> > Or something like ESUPP or EINVAL?
> >
>
> Only if you want to redefine the entire internal PMBus driver API.
>
> > Interestingly max34440_read_byte_data (may) return zero in this case.
> >
>
> zero: No error. Command has been handled.
>
> > > +}
> > > +
> > > +static long max31785_read_long_data(struct i2c_client *client, int page,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint reg)
> > > +{
> > > +ÂÂÂÂÂÂÂunsigned char cmdbuf[1];
> > > +ÂÂÂÂÂÂÂunsigned char rspbuf[4];
> > > +ÂÂÂÂÂÂÂs64 rc;
> >
> > This should be the same type as the function returns.
> >
>
> Also, long is likely s32 on 32-bit architectures, making it quite pointless
> as return type. In this case, it may be better to handle data and error return
> as separate parameters, such as in
>
> static init max31785_read32(struct i2c_client *client, int page, int reg,
> ÂÂÂÂu32 *data)

Sure, I'll take that approach.

>
> > > +
> > > +ÂÂÂÂÂÂÂstruct i2c_msg msg[2] = {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.addr = client->addr,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.flags = 0,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.len = sizeof(cmdbuf),
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.buf = cmdbuf,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ},
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.addr = client->addr,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.flags = I2C_M_RD,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.len = sizeof(rspbuf),
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.buf = rspbuf,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ},
> > > +ÂÂÂÂÂÂÂ};
> > > +
> > > +ÂÂÂÂÂÂÂcmdbuf[0] = reg;
> > > +
> > > +ÂÂÂÂÂÂÂrc = pmbus_set_page(client, page);
> > > +ÂÂÂÂÂÂÂif (rc < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> > > +
> > > +ÂÂÂÂÂÂÂrc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > > +ÂÂÂÂÂÂÂif (rc < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rc;
> > > +
> > > +ÂÂÂÂÂÂÂrc = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂ(rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
> >
> > It's getting late, so I might have confused myself. But can you just do:
> >
> > rc = le32_to_cpup(rspbuf);
> >
> > > +
> > > +ÂÂÂÂÂÂÂreturn rc;
> > > +}
> > > +
> > > +static int max31785_get_pwm(struct i2c_client *client, int page)
> > > +{
> > > +ÂÂÂÂÂÂÂint config;
> > > +ÂÂÂÂÂÂÂint command;
> > > +
> > > +ÂÂÂÂÂÂÂconfig = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
> > > +ÂÂÂÂÂÂÂif (config < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn config;
> > > +
> > > +ÂÂÂÂÂÂÂcommand = pmbus_read_word_data(client, page, PMBUS_FAN_COMMAND_1);
> > > +ÂÂÂÂÂÂÂif (command < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn command;
> > > +
> > > +ÂÂÂÂÂÂÂif (!(config & PB_FAN_1_RPM)) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (command >= 0x8000)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if (command >= 0x2711)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0x2710;
> >
> > What are the magic numbers?

Magic from the Maxim datasheet: See FAN_COMMAND_1, page 30[1]. I felt
macros would be more pain than gain but am happy to bikeshed. I
probably just wasn't being creative enough. In general I agree magic
numbers are not the way to go.

[1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf

> >
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse
>
> unnecessary else

Indeed. Will fix up other instances.

>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn command;
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂreturn 0;
> > > +}
> > > +
> > > +static int max31785_get_pwm_mode(struct i2c_client *client, int page)
> > > +{
> > > +ÂÂÂÂÂÂÂint config;
> > > +ÂÂÂÂÂÂÂint command;
> > > +
> > > +ÂÂÂÂÂÂÂconfig = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
> > > +ÂÂÂÂÂÂÂif (config < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn config;
> > > +
> > > +ÂÂÂÂÂÂÂcommand = pmbus_read_word_data(client, page, PMBUS_FAN_COMMAND_1);
> > > +ÂÂÂÂÂÂÂif (command < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn command;
> > > +
> > > +ÂÂÂÂÂÂÂif (!(config & PB_FAN_1_RPM)) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (command >= 0x8000)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 2;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse if (command >= 0x2711)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂelse
>
> Unecessary else
>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 1;
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂreturn (command >= 0x8000) ? 2 : 1;
> > > +}
> > > +
> > > +static int max31785_read_word_data(struct i2c_client *client, int page,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint reg)
> > > +{
> > > +ÂÂÂÂÂÂÂstruct max31785 *chip = to_max31785(client);
> > > +ÂÂÂÂÂÂÂlong rv = -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂswitch (reg) {
> > > +ÂÂÂÂÂÂÂcase PMBUS_READ_FAN_SPEED_1:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (likely(page < 23))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (WARN_ON(!(chip->capabilities & MAX31785_CAP_DUAL_TACH)))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOTSUPP;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = max31785_read_long_data(client, page - 23, reg);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (rv < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = (rv >> 16) & 0xffff;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > > +ÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_1:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = max31785_get_pwm(client, page);
>
> And ignore the error ?

Hah. Apparently. Will fix.

>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv *= 255;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv /= 100;
>
> max31785_get_pwm() returns 0..0x2710 (or 10000). This is translated to 0..25500.
> Not really sure if that makes sense. We don't report pwm values in fractions of
> 100 to user space.

Right, but this goes through DIRECT conversion to divide by 100 (m = 1,
b = 0, R = 2) before we hit userspace.

>
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > > +ÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_ENABLE_1:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = max31785_get_pwm_mode(client, page);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
>
> Not sure if PMBUS_VIRT_PWM_1 and PMBUS_VIRT_PWM_ENABLE_1 should return
> -ENOTSUPP for pages 23+.

Yeah, that's reasonable, however by similar reasoning you used to
eliminate the capabilities variable this case should never be hit as
those attributes shouldn't exist.

>
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂif (rv == -ENODATA && page >= 23)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
>
> That looks like another verification. Please drop.
>
> > > +
> > > +ÂÂÂÂÂÂÂreturn rv;
> > > +}
> > > +
> > > +static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0xffff };
> > > +
> > > +static int max31785_write_word_data(struct i2c_client *client, int page,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint reg, u16 word)
> > > +{
> > > +ÂÂÂÂÂÂÂint rv = -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂif (page >= 23)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
> > > +
> > > +ÂÂÂÂÂÂÂswitch (reg) {
> > > +ÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_ENABLE_1:
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (word >= ARRAY_SIZE(max31785_pwm_modes))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOTSUPP;
>
> That doesn't look correct. -EINVAL, possibly.

Yes, -EINVAL would be better.

>
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂmax31785_pwm_modes[word]);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂreturn rv;
> > > +}
> > > +
> > > +static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
> > > +{
> > > +ÂÂÂÂÂÂÂif (page < 23)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODATA;
> > > +
> > > +ÂÂÂÂÂÂÂreturn -ENOTSUPP;
> > > +}
> > > +
> > > +static struct pmbus_driver_info max31785_info = {
> > > +ÂÂÂÂÂÂÂ.pages = 23,
> > > +
> > > +ÂÂÂÂÂÂÂ.write_word_data = max31785_write_word_data,
> > > +ÂÂÂÂÂÂÂ.read_byte_data = max31785_read_byte_data,
> > > +ÂÂÂÂÂÂÂ.read_word_data = max31785_read_word_data,
> > > +ÂÂÂÂÂÂÂ.write_byte = max31785_write_byte,
> > > +
> > > +ÂÂÂÂÂÂÂ/* RPM */
> > > +ÂÂÂÂÂÂÂ.format[PSC_FAN] = direct,
> > > +ÂÂÂÂÂÂÂ.m[PSC_FAN] = 1,
> > > +ÂÂÂÂÂÂÂ.b[PSC_FAN] = 0,
> > > +ÂÂÂÂÂÂÂ.R[PSC_FAN] = 0,
> > > +ÂÂÂÂÂÂÂ/* PWM */
> > > +ÂÂÂÂÂÂÂ.format[PSC_PWM] = direct,
> > > +ÂÂÂÂÂÂÂ.m[PSC_PWM] = 1,
> > > +ÂÂÂÂÂÂÂ.b[PSC_PWM] = 0,
> > > +ÂÂÂÂÂÂÂ.R[PSC_PWM] = 2,
> > > +ÂÂÂÂÂÂÂ.func[0] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +ÂÂÂÂÂÂÂ.func[1] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +ÂÂÂÂÂÂÂ.func[2] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +ÂÂÂÂÂÂÂ.func[3] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +ÂÂÂÂÂÂÂ.func[4] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +ÂÂÂÂÂÂÂ.func[5] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
> > > +
> > > +ÂÂÂÂÂÂÂ.format[PSC_TEMPERATURE] = direct,
> > > +ÂÂÂÂÂÂÂ.m[PSC_TEMPERATURE] = 1,
> > > +ÂÂÂÂÂÂÂ.b[PSC_TEMPERATURE] = 0,
> > > +ÂÂÂÂÂÂÂ.R[PSC_TEMPERATURE] = 2,
> > > +ÂÂÂÂÂÂÂ.func[6]ÂÂ= PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[7]ÂÂ= PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[8]ÂÂ= PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[9]ÂÂ= PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[10] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[11] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[12] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[13] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[14] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[15] = PMBUS_HAVE_STATUS_TEMP,
> > > +ÂÂÂÂÂÂÂ.func[16] = PMBUS_HAVE_STATUS_TEMP,
> > > +
> > > +ÂÂÂÂÂÂÂ.format[PSC_VOLTAGE_OUT] = direct,
> > > +ÂÂÂÂÂÂÂ.m[PSC_VOLTAGE_OUT] = 1,
> > > +ÂÂÂÂÂÂÂ.b[PSC_VOLTAGE_OUT] = 0,
> > > +ÂÂÂÂÂÂÂ.R[PSC_VOLTAGE_OUT] = 0,
> > > +ÂÂÂÂÂÂÂ.func[17] = PMBUS_HAVE_STATUS_VOUT,
> > > +ÂÂÂÂÂÂÂ.func[18] = PMBUS_HAVE_STATUS_VOUT,
> > > +ÂÂÂÂÂÂÂ.func[19] = PMBUS_HAVE_STATUS_VOUT,
> > > +ÂÂÂÂÂÂÂ.func[20] = PMBUS_HAVE_STATUS_VOUT,
> > > +ÂÂÂÂÂÂÂ.func[21] = PMBUS_HAVE_STATUS_VOUT,
> > > +ÂÂÂÂÂÂÂ.func[22] = PMBUS_HAVE_STATUS_VOUT,
> > > +};
> > > +
> > > +static int max31785_probe(struct i2c_client *client,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst struct i2c_device_id *id)
> > > +{
> > > +ÂÂÂÂÂÂÂstruct max31785 *chip;
> > > +ÂÂÂÂÂÂÂint rv;
> > > +ÂÂÂÂÂÂÂint i;
> > > +
> > > +ÂÂÂÂÂÂÂchip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
> > > +ÂÂÂÂÂÂÂif (!chip)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > > +
> > > +ÂÂÂÂÂÂÂchip->info = max31785_info;
> > > +
> > > +ÂÂÂÂÂÂÂ/*
> > > +ÂÂÂÂÂÂÂÂ* Identify the chip firmware and configure capabilities.
> > > +ÂÂÂÂÂÂÂÂ*
> > > +ÂÂÂÂÂÂÂÂ* Bootstrap with i2c_smbus_*() calls as we need to understand the chip
> > > +ÂÂÂÂÂÂÂÂ* capabilities for before invoking pmbus_do_probe(). The pmbus_*()
> > > +ÂÂÂÂÂÂÂÂ* calls need access to memory that is only valid after
> > > +ÂÂÂÂÂÂÂÂ* pmbus_do_probe().
> >
> > I think this is common enough in pmbus drivers that the comment is redundant.

Will do.

> >
> > > +ÂÂÂÂÂÂÂÂ*/
> > > +ÂÂÂÂÂÂÂrv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
> > > +ÂÂÂÂÂÂÂif (rv < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > > +
> > > +ÂÂÂÂÂÂÂrv = i2c_smbus_read_word_data(client, PMBUS_MFR_REVISION);
> > > +ÂÂÂÂÂÂÂif (rv < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > > +
> > > +ÂÂÂÂÂÂÂif ((rv & 0xff) == 0x40) {
> >
> > DUAL_TACH_REV? Or a better name from the docs we have?

I'll have a think about a name.

Thanks for the feedback guys.

Andrew

> >
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->capabilities |= MAX31785_CAP_DUAL_TACH;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Punt the dual tach virtual fans to non-existent pages. This
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* ensures the pwm attributes appear in a contiguous block
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.pages = 29;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[23] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[24] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[25] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[26] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[27] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂchip->info.func[28] = PMBUS_HAVE_FAN12;
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂrv = pmbus_do_probe(client, id, &chip->info);
> > > +ÂÂÂÂÂÂÂif (rv < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > > +
> > > +ÂÂÂÂÂÂÂfor (i = 0; i < max31785_info.pages; i++) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint reg;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!(max31785_info.func[i] & (PMBUS_HAVE_FAN12)))
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = pmbus_read_word_data(client, i, PMBUS_MFR_FAN_CONFIG);
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (reg < 0)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* XXX: Purely for RFC/testing purposes, don't ramp fans on fan
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* or temperature sensor fault, or a failure to write
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* FAN_COMMAND_1 inside a 10s window (watchdog mode).
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* The TSFO bit controls both ramping on temp sensor failure
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* AND whether FAN_COMMAND_1 is in watchdog mode.
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ*/
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg |= MFR_FAN_CONFIG_TSFO | MFR_FAN_CONFIG_TACHO;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (chip->capabilities & MAX31785_CAP_DUAL_TACH)
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg |= MFR_FAN_CONFIG_DUAL_TACH;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg &= 0xffff;
> > > +
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = pmbus_write_word_data(client, i, PMBUS_MFR_FAN_CONFIG,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg);
> > > +ÂÂÂÂÂÂÂ}
> > > +
> > > +ÂÂÂÂÂÂÂreturn 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id max31785_id[] = {
> > > +ÂÂÂÂÂÂÂ{ "max31785", 0 },
> > > +ÂÂÂÂÂÂÂ{ },
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(i2c, max31785_id);
> > > +
> > > +static struct i2c_driver max31785_driver = {
> > > +ÂÂÂÂÂÂÂ.driver = {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ.name = "max31785",
> > > +ÂÂÂÂÂÂÂ},
> > > +ÂÂÂÂÂÂÂ.probe = max31785_probe,
> > > +ÂÂÂÂÂÂÂ.remove = pmbus_do_remove,
> > > +ÂÂÂÂÂÂÂ.id_table = max31785_id,
> > > +};
> > > +
> > > +module_i2c_driver(max31785_driver);
> > > +
> > > > > > +MODULE_AUTHOR("Andrew Jeffery <andrew@xxxxxxxx>");
> > > +MODULE_DESCRIPTION("PMBus driver for the Maxim MAX31785");
> > > +MODULE_LICENSE("GPL");
> > > --
> > > 2.11.0
> > >

Attachment: signature.asc
Description: This is a digitally signed message part

Next message: Boqun Feng: "Re: [PATCH] documentation: Fix two-CPU control-dependency example"
Previous message: Stephen Rothwell: "linux-next: manual merge of the drm-misc tree with the drm-intel tree"
In reply to: Guenter Roeck: "Re: [RFC PATCH v2 3/3] pmbus: Add MAX31785 driver"
Next in thread: Andrew Jeffery: "[RFC PATCH v2 2/3] pmbus: Add and expose pmbus_get_info()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]