Re: [PATCH 1/2] ARM: dts: aspeed: tiogapass: Add VR devices

[Andrew Jeffery]

Hi Vijay,

A few nitpicks.

On Tue, 23 Jul 2019, at 05:10, Vijay Khemka wrote:
> Addes

Typo: Adds

> Voltage

Unnecessary capitalisation.

> regulators Infineon pxe1610 devices to Facebook
> tiogapass platform.
> 
> Signed-off-by: Vijay Khemka 
> ---
>  .../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts 
> b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index c4521eda787c..b7783833a58c 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -144,6 +144,42 @@
>   {
>   status = "okay";
>   // CPU Voltage regulators
> + vr@48 {

The recommended generic name is 'regulator', so e.g. regulator@48

> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> + };
> + vr@4a {
> + compatible = "infineon,pxe1610";
> + reg = <0x4a>;
> + };
> + vr@50 {
> + compatible = "infineon,pxe1610";
> + reg = <0x50>;
> + };
> + vr@52 {
> + compatible = "infineon,pxe1610";
> + reg = <0x52>;
> + };
> + vr@58 {
> + compatible = "infineon,pxe1610";
> + reg = <0x58>;
> + };
> + vr@5a {
> + compatible = "infineon,pxe1610";
> + reg = <0x5a>;
> + };
> + vr@68 {
> + compatible = "infineon,pxe1610";
> + reg = <0x68>;
> + };
> + vr@70 {
> + compatible = "infineon,pxe1610";
> + reg = <0x70>;
> + };
> + vr@72 {
> + compatible = "infineon,pxe1610";
> + reg = <0x72>;
> + };
>  };
>  
>   {
> -- 
> 2.17.1
> 
>

Re: [PATCH v1 1/2] dt-binding: misc: Add common LPC snoop documentation

[Andrew Jeffery]

On Mon, 15 Apr 2019, at 21:51, Tomer Maimon wrote:
> Added device tree binding documentation for Nuvoton BMC
> NPCM BIOS Post Code (BPC) and Apeed AST2500 LPC snoop.
> The LPC snoop monitoring two configurable I/O addresses
> written by the host on Low Pin Count (LPC) bus.
> 
> Signed-off-by: Tomer Maimon 
> ---
>  .../devicetree/bindings/misc/lpc-snoop.txt | 27 
> ++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/lpc-snoop.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/lpc-snoop.txt 
> b/Documentation/devicetree/bindings/misc/lpc-snoop.txt
> new file mode 100644
> index ..7deac244ef9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/lpc-snoop.txt
> @@ -0,0 +1,27 @@
> +LPC snoop interface
> +
> +The LPC snoop (BIOS Post Code) interface can monitor
> +two configurable I/O addresses written by the host on
> +the Low Pin Count (LPC) bus.

Wrapping looks kind of odd and there's a missing newline at the
end of the file, but for the content:

Reviewed-by: Andrew Jeffery 

> +
> +Nuvoton NPCM7xx LPC snoop supports capture double words,
> +when using capture double word only I/O address 1 is monitored.
> +
> +Required properties for lpc-snoop node
> +- compatible   : "nuvoton,npcm750-lpc-bpc-snoop" for Poleg NPCM7XX
> + "aspeed,ast2500-lpc-snoop" for Aspeed AST2500.
> +- reg  : specifies physical base address and size of the 
> registers.
> +- interrupts   : contain the LPC snoop interrupt with flags for 
> falling edge.
> +- snoop-ports  : contain monitor I/O addresses, at least one monitor 
> I/O
> + address required
> +
> +Optional property for NPCM7xx lpc-snoop node
> +- nuvoton,lpc-en-dwcapture : enable capture double words support.
> +
> +Example:
> + lpc-snoop: lpc_snoop@f0007040 {
> + compatible = "nuvoton,npcm750-lpc-bpc-snoop";
> + reg = <0xf0007040 0x14>;
> + snoop-ports = <0x80>;
> + interrupts = ;
> + };
> \ No newline at end of file
> -- 
> 2.14.1
> 
>

Re: [PATCH v2 10/21] docs: hwmon: aspeed-pwm-tacho: convert to ReST format

[Andrew Jeffery]

On Thu, 11 Apr 2019, at 04:54, Mauro Carvalho Chehab wrote:
> Convert aspeed-pwm-tacho to ReST format, in order to allow it to
> be parsed by Sphinx.
> 
> Signed-off-by: Mauro Carvalho Chehab 

Acked-by: Andrew Jeffery 

> ---
>  Documentation/hwmon/aspeed-pwm-tacho | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/hwmon/aspeed-pwm-tacho 
> b/Documentation/hwmon/aspeed-pwm-tacho
> index 7cfb34977460..6dcec845fbc7 100644
> --- a/Documentation/hwmon/aspeed-pwm-tacho
> +++ b/Documentation/hwmon/aspeed-pwm-tacho
> @@ -15,8 +15,10 @@ controller supports up to 16 tachometer inputs.
>  
>  The driver provides the following sensor accesses in sysfs:
>  
> +=== === =
>  fanX_input   ro  provide current fan rotation value in RPM as reported
>   by the fan to the device.
>  
>  pwmX rw  get or set PWM fan control value. This is an integer
>   value between 0(off) and 255(full speed).
> +=== === =
> -- 
> 2.20.1
> 
>

Re: [PATCH v1 1/2] dt-binding: bmc: Add NPCM7xx LPC BPC documentation

[Andrew Jeffery]

On Thu, 12 Jul 2018, at 05:51, Rob Herring wrote:
> On Wed, Jul 04, 2018 at 12:14:26PM +0300, Tomer Maimon wrote:
> > Added device tree binding documentation for Nuvoton BMC
> > NPCM7xx BIOS Post Code (BPC).
> > The NPCM7xx BPC monitoring two configurable I/O addresses
> > written by the host on Low Pin Count (LPC) bus.
> > 
> > Signed-off-by: Tomer Maimon 
> > ---
> >  .../devicetree/bindings/bmc/npcm7xx-lpc-bpc.txt| 26 
> > ++
> >  1 file changed, 26 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/bmc/npcm7xx-lpc-bpc.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/bmc/npcm7xx-lpc-bpc.txt 
> > b/Documentation/devicetree/bindings/bmc/npcm7xx-lpc-bpc.txt
> > new file mode 100644
> > index ..0832c9cbea32
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/bmc/npcm7xx-lpc-bpc.txt
> > @@ -0,0 +1,26 @@
> > +Nuvoton NPCM7xx LPC BPC interface
> > +
> > +Nuvoton BMC NPCM7xx BIOS Post Code (BPC) monitoring two
> > +configurable I/O addresses written by the host on the
> > +Low Pin Count (LPC) bus, the capure data stored in 128-word FIFO.
> 
> s/capure/capture/
> 
> Otherwise,
> 
> Reviewed-by: Rob Herring 

So uncovering a bit of a dirty secret here, we should probably consider this in 
the context of the ASPEED snoop function. A driver snuck into the misc tree 
without an associated bindings document:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/aspeed-lpc-snoop.c

(it wasn't me, I'm happy to discuss my proposed bindings, please don't shoot 
the messenger :))

The devicetree node looks something like:

lpc_snoop: lpc-snoop@0 {
compatible = 
"aspeed,ast2500-lpc-snoop";
reg = <0x0 0x80>;
interrupts = <8>;
status = "disabled";
};

and also has a required "snoop-ports" property with at least one and up to two 
cells that describe the channels to snoop. Channel values are either 0 or 1. 
It's feasible that the double-word capture could also be supported as both 
supported channels have their data captured in different fields of the same 
register.

Do we go with what's already in the tree in supporting 'snoop-ports' or keep 
'monitor-ports' as proposed by Tomer?

Andrew

> 
> 
> > +
> > +NPCM7xx BPC supports capture double words, when using capture
> > +double word only I/O address 1 is monitored.
> > +
> > +Required properties for lpc_bpc node
> > +- compatible   : "nuvoton,npcm750-lpc-bpc" for Poleg NPCM7XX.
> > +- reg  : specifies physical base address and size of 
> > the registers.
> > +- interrupts   : contain the LPC BPC with flags for falling edge.
> > +- monitor-ports : contain monitor I/O addresses, at least one monitor I/O
> > + address required
> > +
> > +Optional property for lpc_bpc node
> > +- bpc-en-dwcapture : enable capture double words support.
> > +
> > +Example:
> > +   lpc_bpc: lpc-bpc@f0007040 {
> > +   compatible = "nuvoton,npcm7xx-lpc-bpc";
> > +   reg = <0xf0007040 0x14>;
> > +   monitor-ports = <0x80>;
> > +   interrupts = ;
> > +   };
> > -- 
> > 2.14.1
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 2/4] pmbus (max31785): Add fan control

[Andrew Jeffery]

The implementation makes use of the new fan control virtual registers exposed
by the pmbus core. It mixes use of the default implementations with some
overrides via the read/write handlers to handle FAN_COMMAND_1 on the MAX31785,
whose definition breaks the value range into various control bands dependent on
RPM or PWM mode.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |   7 ++-
 drivers/hwmon/pmbus/max31785.c | 138 +-
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 45fb6093dec2..7b0a0a8cdb6b 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -32,6 +32,7 @@ Sysfs attributes
 fan[1-4]_alarm Fan alarm.
 fan[1-4]_fault Fan fault.
 fan[1-4]_input Fan RPM.
+fan[1-4]_targetFan input target
 
 in[1-6]_crit   Critical maximum output voltage
 in[1-6]_crit_alarm Output voltage critical high alarm
@@ -44,6 +45,12 @@ in[1-6]_max_alarmOutput voltage high alarm
 in[1-6]_minMinimum output voltage
 in[1-6]_min_alarm  Output voltage low alarm
 
+pwm[1-4]   Fan target duty cycle (0..255)
+pwm[1-4]_enable0: Full-speed
+   1: Manual PWM control
+   2: Automatic PWM (tach-feedback RPM fan-control)
+   3: Automatic closed-loop (temp-feedback fan-control)
+
 temp[1-11]_critCritical high temperature
 temp[1-11]_crit_alarm  Chip temperature critical high alarm
 temp[1-11]_input   Measured temperature
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 9313849d5160..8706a696c89a 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -20,8 +20,136 @@ enum max31785_regs {
 
 #define MAX31785_NR_PAGES  23
 
+static int max31785_get_pwm(struct i2c_client *client, int page)
+{
+   int rv;
+
+   rv = pmbus_get_fan_rate_device(client, page, 0, percent);
+   if (rv < 0)
+   return rv;
+   else if (rv >= 0x8000)
+   return 0;
+   else if (rv >= 0x2711)
+   return 0x2710;
+
+   return rv;
+}
+
+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)
+   return (command >= 0x8000) ? 3 : 2;
+
+   if (command >= 0x8000)
+   return 3;
+   else if (command >= 0x2711)
+   return 0;
+
+   return 1;
+}
+
+static int max31785_read_word_data(struct i2c_client *client, int page,
+  int reg)
+{
+   int rv;
+
+   switch (reg) {
+   case PMBUS_VIRT_PWM_1:
+   rv = max31785_get_pwm(client, page);
+   break;
+   case PMBUS_VIRT_PWM_ENABLE_1:
+   rv = max31785_get_pwm_mode(client, page);
+   break;
+   default:
+   rv = -ENODATA;
+   break;
+   }
+
+   return rv;
+}
+
+static inline u32 max31785_scale_pwm(u32 sensor_val)
+{
+   /*
+* The datasheet describes the accepted value range for manual PWM as
+* [0, 0x2710], while the hwmon pwmX sysfs interface accepts values in
+* [0, 255]. The MAX31785 uses DIRECT mode to scale the FAN_COMMAND
+* registers and in PWM mode the coefficients are m=1, b=0, R=2. The
+* important observation here is that 0x2710 == 1 == 100 * 100.
+*
+* R=2 (== 10^2 == 100) accounts for scaling the value provided at the
+* sysfs interface into the required hardware resolution, but it does
+* not yet yield a value that we can write to the device (this initial
+* scaling is handled by pmbus_data2reg()). Multiplying by 100 below
+* translates the parameter value into the percentage units required by
+* PMBus, and then we scale back by 255 as required by the hwmon pwmX
+* interface to yield the percentage value at the appropriate
+* resolution for hardware.
+*/
+   return (sensor_val * 100) / 255;
+}
+
+static int max31785_pwm_enable(struct i2c_client *client, int page,
+   u16 word)
+{
+   int config = 0;
+   int rate;
+
+   switch (word) {
+   case 0:
+   rate = 0x7fff;
+   break;
+   case 1:
+   rate = pmbus_get_fan_rate_cached(client, page, 0, percent);
+   if (rate < 0)
+   return rate;
+   rate = max31785_scale_pwm(rate);

[PATCH v6 3/4] pmbus (core): Add virtual page config bit

[Andrew Jeffery]

Some circumstances call for virtual pages, to expose multiple values
packed into an extended PMBus register in a manner non-compliant with
the PMBus standard. An example of this is the Maxim MAX31785 controller, which
extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
tach readings for both rotors of a dual rotor fan. This extended register
contains two word-sized values, one reporting the rate of the fastest rotor,
the other the rate of the slowest. The concept of virtual pages aids this
situation by mapping the page number onto the value to be selected from the
vectored result.

We should not try to set virtual pages on the device as such a page explicitly
doesn't exist; add a flag so we can avoid doing so.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 27 ++-
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index b54d7604d3ef..d39d506aa63e 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -372,6 +372,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12   BIT(20)
 #define PMBUS_HAVE_PWM34   BIT(21)
 
+#define PMBUS_PAGE_VIRTUAL BIT(31)
+
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12, vr13 };
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index edc25efe7552..e215bbd403a5 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -161,18 +161,27 @@ EXPORT_SYMBOL_GPL(pmbus_clear_cache);
 int pmbus_set_page(struct i2c_client *client, int page)
 {
struct pmbus_data *data = i2c_get_clientdata(client);
-   int rv = 0;
-   int newpage;
+   int rv;
+
+   if (page < 0 || page == data->currpage)
+   return 0;
 
-   if (page >= 0 && page != data->currpage) {
+   if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
-   newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
-   if (newpage != page)
-   rv = -EIO;
-   else
-   data->currpage = page;
+   if (rv < 0)
+   return rv;
+
+   rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
+   if (rv < 0)
+   return rv;
+
+   if (rv != page)
+   return -EIO;
}
-   return rv;
+
+   data->currpage = page;
+
+   return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
 
-- 
git-series 0.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 4/4] pmbus (max31785): Add dual tachometer support

[Andrew Jeffery]

The dual tachometer feature is implemented in hardware with a TACHSEL
input to indicate the rotor under measurement, and exposed on the device
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 the
values are exposed through virtual pages. We re-route accesses to
FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on pages 23-28 (not defined by the
hardware) to the same registers on pages 0-5, and with the latter command we
extract the value from the second word of the four-byte response.

* The documentation recommends the slower rotor be associated with
TACHSEL=0, which corresponds to the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management to judge
target fan rate.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |   8 +-
 drivers/hwmon/pmbus/max31785.c | 147 ++-
 2 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 7b0a0a8cdb6b..270c5f865261 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -17,8 +17,9 @@ management with temperature and remote voltage sensing. 
Various fan control
 features are provided, including PWM frequency control, temperature hysteresis,
 dual tachometer measurements, and fan health monitoring.
 
-For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
-two in the fan[1-4]_input attributes.
+For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
+readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
+the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
 
 Usage Notes
 ---
@@ -31,7 +32,8 @@ Sysfs attributes
 
 fan[1-4]_alarm Fan alarm.
 fan[1-4]_fault Fan fault.
-fan[1-4]_input Fan RPM.
+fan[1-8]_input Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
+   second rotor of fans 1-4
 fan[1-4]_targetFan input target
 
 in[1-6]_crit   Critical maximum output voltage
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 8706a696c89a..bffab449be39 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,9 +16,79 @@
 
 enum max31785_regs {
MFR_REVISION= 0x9b,
+   MFR_FAN_CONFIG  = 0xf1,
 };
 
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+
 #define MAX31785_NR_PAGES  23
+#define MAX31785_NR_FAN_PAGES  6
+
+static int max31785_read_byte_data(struct i2c_client *client, int page,
+  int reg)
+{
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   switch (reg) {
+   case PMBUS_VOUT_MODE:
+   return -ENOTSUPP;
+   case PMBUS_FAN_CONFIG_12:
+   return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
+   reg);
+   }
+
+   return -ENODATA;
+}
+
+static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
+{
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   return -ENOTSUPP;
+}
+
+static int max31785_read_long_data(struct i2c_client *client, int page,
+  int reg, u32 *data)
+{
+   unsigned char cmdbuf[1];
+   unsigned char rspbuf[4];
+   int rc;
+
+   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;
+
+   *data = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
+   (rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
+
+   return rc;
+}
 
 static int max31785_get_pwm(struct i2c_client *client, int page)
 {
@@ -62,9 +132,30 @@ static int max31785_get_pwm_mode(struct i2c_client *client, 
int page)
 static int max31785_read_word_data(struct i2c_client *client, int page,
   int reg)
 {
+   u32 val;
  

[PATCH v6 0/4] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

This series introduces support for the MAX31785 intelligent fan controller, a
PMBus device providing closed-loop fan control among a number of other
features. Along the way the series adds support to control fans and create
virtual pages to the PMBus core, the latter to support some of the more
annoying design decisions found in the 'A' variant of the chip.

This is the sixth spin of the series. v5 can be found here:

https://lkml.org/lkml/2017/11/17/910

v6 fixes an error-handling bug I introduced in pmbus_set_page() with PATCH 3/4,
as pointed out by Guenter on v5[1]. The rework implements his suggested
solution.

A few other other minor fixes are included:

* PATCH 1/4:
  * Avoid potential NULL dereference in pmbus_get_fan_rate(). It
amounts to being an error path that should never happen in practice, but
lets make sure we don't die if we do hit it.
  * Drop a redundant test for the presence of FAN_COMMAND_x in
pmbus_add_fan_ctrl(); document that its presence is a precondition to
calling it (this was already tested at the one call-site).

* PATCH 2/4: Avoid scaling an error code in pmbus_pwm_enable(). Rather, test
  the result of pmbus_get_fan_rate_cached() before performing the scaling when
  switching to PWM mode.

[1] https://lkml.org/lkml/2017/11/19/280

Please review!

Andrew

Andrew Jeffery (4):
  pmbus (core): Add fan control support
  pmbus (max31785): Add fan control
  pmbus (core): Add virtual page config bit
  pmbus (max31785): Add dual tachometer support

 Documentation/hwmon/max31785 |  15 +-
 drivers/hwmon/pmbus/max31785.c   | 285 +++-
 drivers/hwmon/pmbus/pmbus.h  |  41 -
 drivers/hwmon/pmbus/pmbus_core.c | 265 +++---
 4 files changed, 575 insertions(+), 31 deletions(-)

base-commit: ded0eb83449e8fcba22fd2736826336e101ffbcb
-- 
git-series 0.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/4] pmbus (core): Add virtual page config bit

[Andrew Jeffery]

On Sun, 2017-11-19 at 08:49 -0800, Guenter Roeck wrote:
> On 11/17/2017 07:26 PM, Andrew Jeffery wrote:
> > Some circumstances call for virtual pages, to expose multiple values
> > packed into an extended PMBus register in a manner non-compliant with
> > the PMBus standard. An example of this is the Maxim MAX31785 controller, 
> > which
> > extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to 
> > support
> > tach readings for both rotors of a dual rotor fan. This extended register
> > contains two word-sized values, one reporting the rate of the fastest rotor,
> > the other the rate of the slowest. The concept of virtual pages aids this
> > situation by mapping the page number onto the value to be selected from the
> > vectored result.
> > 
> > We should not try to set virtual pages on the device as such a page 
> > explicitly
> > doesn't exist; add a flag so we can avoid doing so.
> > 
> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >   drivers/hwmon/pmbus/pmbus.h  |  2 ++
> >   drivers/hwmon/pmbus/pmbus_core.c | 10 +++---
> >   2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index b54d7604d3ef..d39d506aa63e 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -372,6 +372,8 @@ enum pmbus_sensor_classes {
> >   #define PMBUS_HAVE_PWM12  BIT(20)
> >   #define PMBUS_HAVE_PWM34  BIT(21)
> >   
> > +#define PMBUS_PAGE_VIRTUAL BIT(31)
> > +
> >   enum pmbus_data_format { linear = 0, direct, vid };
> >   enum vrm_version { vr11 = 0, vr12, vr13 };
> >   
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
> > b/drivers/hwmon/pmbus/pmbus_core.c
> > index 631db88526b6..ba97230fcde7 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, int 
> > page)
> > int rv = 0;
> > int newpage;
> >   
> > -   if (page >= 0 && page != data->currpage) {
> > +   if (page < 0 || page == data->currpage)
> > +   return 0;
> > +
> > +   if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
> > rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> > newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> > if (newpage != page)
> > rv = -EIO;
> 
> This should probably be something like
>   rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
>   if (rv < 0)
>   return rv;
>   newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>   if (newpage < 0)
>   return newpage;
>   if (newpage != page)
>   return -EIO;
> 
> We can actually drop 'newpage' and just use 'rv'.
> 
> > -   else
> > -   data->currpage = page;
> > }
> > +
> > +   data->currpage = page;
> > +
> 
> ... otherwise currpage is set even on error.

Thanks for catching that, clearly I needed to pay more attention
resolving the conflicts when rebasing on hwmon-next. I'll address all
your points.

Cheers,

Andrew

> 
> > return rv;
> 
> this can then be
>   return 0;
> 
> >   }
> >   EXPORT_SYMBOL_GPL(pmbus_set_page);
> > 
> 
> 

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

[PATCH v5 3/4] pmbus (core): Add virtual page config bit

[Andrew Jeffery]

Some circumstances call for virtual pages, to expose multiple values
packed into an extended PMBus register in a manner non-compliant with
the PMBus standard. An example of this is the Maxim MAX31785 controller, which
extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
tach readings for both rotors of a dual rotor fan. This extended register
contains two word-sized values, one reporting the rate of the fastest rotor,
the other the rate of the slowest. The concept of virtual pages aids this
situation by mapping the page number onto the value to be selected from the
vectored result.

We should not try to set virtual pages on the device as such a page explicitly
doesn't exist; add a flag so we can avoid doing so.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 10 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index b54d7604d3ef..d39d506aa63e 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -372,6 +372,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12   BIT(20)
 #define PMBUS_HAVE_PWM34   BIT(21)
 
+#define PMBUS_PAGE_VIRTUAL BIT(31)
+
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12, vr13 };
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 631db88526b6..ba97230fcde7 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, int page)
int rv = 0;
int newpage;
 
-   if (page >= 0 && page != data->currpage) {
+   if (page < 0 || page == data->currpage)
+   return 0;
+
+   if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
if (newpage != page)
rv = -EIO;
-   else
-   data->currpage = page;
}
+
+   data->currpage = page;
+
return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
-- 
git-series 0.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/4] pmbus (core): Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers,
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding configuration in FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers to
facilitate the necessary side-effects of each access:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

Some complexity arises with the fanX_target and pwmX attributes both mapping
onto FAN_COMMAND_x: There is no general mapping between PWM percent duty and
RPM, so we can't display values in either attribute in terms of the other
(which in my mind is the intuitive, if impossible, behaviour). This problem
also affects the pwmX_enable attribute which allows userspace to switch between
full speed, manual PWM and a number of automatic control modes, possibly
including a switch to RPM behaviour (e.g. automatically adjusting PWM duty to
reach a RPM target, the behaviour of fanX_target).

The next most intuitive behaviour is for fanX_target and pwmX to simply be
independent, to retain their most recently set value even if that value is not
active on the hardware (due to switching to the alternative control mode). This
property of retaining the value independent of the hardware state has useful
results for both userspace and the kernel: Userspace always sees a sensible
value in the attribute (the last thing it was set to, as opposed to 0 or
receiving an error on read), and the kernel can use the attributes as a value
cache. This latter point eases the implementation of pwmX_enable, which can
look up the associated pmbus_sensor object, take its cached value and apply it
to hardware on changing control mode. This ensures we will not arbitrarily set
a PWM value as an RPM value or vice versa, and we can assume that the RPM or
PWM value set was sensible at least at some point in the past.

Finally, the DIRECT mode coefficients of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to capture the
necessary coefficients. As pmbus core had no PWM support previously PSC_FAN
continues to be used to capture the RPM DIRECT coefficients, but in order to
avoid falsely applying RPM scaling to PWM values I have introduced the
PMBUS_HAVE_PWM12 and PMB_BUS_HAVE_PWM34 feature bits. These feature bits allow
drivers to explicitly declare PWM support in order to have the attributes
exposed.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  39 -
 drivers/hwmon/pmbus/pmbus_core.c | 240 +---
 2 files changed, 261 insertions(+), 18 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa613bd209e3..b54d7604d3ef 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,33 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /*
+* RPM and PWM Fan control
+*
+* Drivers wanting to expose PWM control must define the behaviour of
+* PMBUS_VIRT_PWM_[1-4] and PMBUS_VIRT_PWM_ENABLE_[1-4] in the
+* {read,write}_word_data callback.
+*
+* pmbus core provides a default implementation for
+* PMBUS_VIRT_FAN_TARGET_[1-4].
+*
+* TARGET, PWM and PWM_ENABLE members must be defined sequentially;
+* pmbus core uses the difference between the provided register and
+* it's _1 counterpart to calculate the FAN/PWM ID.
+*/
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +250,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +342,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -339,6 +369,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34BIT(17)
 #define PMBUS_HAVE_VMONBIT(18)
 #define PMBUS_HAVE_STATUS_VMON BIT(19)
+#define PMBUS_HAVE_PWM12   BIT(20)
+#define PMBUS_HAVE_PWM34   

[PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

This series introduces support for the MAX31785 intelligent fan controller, a
PMBus device providing closed-loop fan control among a number of other
features. Along the way the series adds support to control fans and create
virtual pages to the PMBus core, the latter to support some of the more
annoying design decisions found in the 'A' variant of the chip.

This is the fifth spin of the series. v4 can be found here:

https://lkml.org/lkml/2017/11/3/15

The changes over v4 include a rework of the fan control support to provide a
more intuitive behaviour for fanX_target, pwmX. They now always read the value
last set (in v4 they returned 0 if they were not the active control mode, and
in even earlier spins they returned an error), which also allows implementation
of sane behaviour for pwmX_enable when switching control modes.

The default implementation for the PWM virtual registers is removed from pmbus
core in light of having no consumers (the max31785 driver implements them
itself), though whilst I was unsure about the generality of the scaling in
replies on v4, after some more thought I have reason to believe it should hold
in general. Regardless, it's gone for the moment, and I've added some
commentary about the scaling in the max31785 implementation.

Please review!

Andrew

Andrew Jeffery (4):
  pmbus (core): Add fan control support
  pmbus (max31785): Add fan control
  pmbus (core): Add virtual page config bit
  pmbus (max31785): Add dual tachometer support

 Documentation/hwmon/max31785 |  15 +-
 drivers/hwmon/pmbus/max31785.c   | 285 +++-
 drivers/hwmon/pmbus/pmbus.h  |  41 -
 drivers/hwmon/pmbus/pmbus_core.c | 250 +---
 4 files changed, 566 insertions(+), 25 deletions(-)

base-commit: ded0eb83449e8fcba22fd2736826336e101ffbcb
-- 
git-series 0.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 4/4] pmbus (max31785): Add dual tachometer support

[Andrew Jeffery]

The dual tachometer feature is implemented in hardware with a TACHSEL
input to indicate the rotor under measurement, and exposed on the device
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 the
values are exposed through virtual pages. We re-route accesses to
FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on pages 23-28 (not defined by the
hardware) to the same registers on pages 0-5, and with the latter command we
extract the value from the second word of the four-byte response.

* The documentation recommends the slower rotor be associated with
TACHSEL=0, which corresponds to the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management to judge
target fan rate.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |   8 +-
 drivers/hwmon/pmbus/max31785.c | 147 ++-
 2 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 7b0a0a8cdb6b..270c5f865261 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -17,8 +17,9 @@ management with temperature and remote voltage sensing. 
Various fan control
 features are provided, including PWM frequency control, temperature hysteresis,
 dual tachometer measurements, and fan health monitoring.
 
-For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
-two in the fan[1-4]_input attributes.
+For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
+readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
+the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
 
 Usage Notes
 ---
@@ -31,7 +32,8 @@ Sysfs attributes
 
 fan[1-4]_alarm Fan alarm.
 fan[1-4]_fault Fan fault.
-fan[1-4]_input Fan RPM.
+fan[1-8]_input Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
+   second rotor of fans 1-4
 fan[1-4]_targetFan input target
 
 in[1-6]_crit   Critical maximum output voltage
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 2699134dfbe7..b5389070ae06 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,9 +16,79 @@
 
 enum max31785_regs {
MFR_REVISION= 0x9b,
+   MFR_FAN_CONFIG  = 0xf1,
 };
 
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+
 #define MAX31785_NR_PAGES  23
+#define MAX31785_NR_FAN_PAGES  6
+
+static int max31785_read_byte_data(struct i2c_client *client, int page,
+  int reg)
+{
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   switch (reg) {
+   case PMBUS_VOUT_MODE:
+   return -ENOTSUPP;
+   case PMBUS_FAN_CONFIG_12:
+   return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
+   reg);
+   }
+
+   return -ENODATA;
+}
+
+static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
+{
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   return -ENOTSUPP;
+}
+
+static int max31785_read_long_data(struct i2c_client *client, int page,
+  int reg, u32 *data)
+{
+   unsigned char cmdbuf[1];
+   unsigned char rspbuf[4];
+   int rc;
+
+   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;
+
+   *data = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
+   (rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
+
+   return rc;
+}
 
 static int max31785_get_pwm(struct i2c_client *client, int page)
 {
@@ -62,9 +132,30 @@ static int max31785_get_pwm_mode(struct i2c_client *client, 
int page)
 static int max31785_read_word_data(struct i2c_client *client, int page,
   int reg)
 {
+   u32 val;
  

Re: [v4,6/6] pmbus: max31785: Add dual tachometer support

[Andrew Jeffery]

On Sun, 2017-11-05 at 06:58 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:06PM +1100, Andrew Jeffery wrote:
> > The dual tachometer feature is implemented in hardware with a TACHSEL
> > input to indicate the rotor under measurement, and exposed on the device
> > 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 the
> > values are exposed through virtual pages. We re-route accesses to
> > FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on undefined (in hardware) pages
> > 23-28 to the same registers on pages 0-5, and with the latter command we
> > extract the value from the second word of the four-byte response.
> > 
> > * The documentation recommends the slower rotor be associated with
> > TACHSEL=0, which provides the first word of the response. The TACHSEL=0
> > measurement is used by the controller's closed-loop fan management.
> > 
> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  Documentation/hwmon/max31785   |   8 ++-
> >  drivers/hwmon/pmbus/max31785.c | 159 
> > -
> >  2 files changed, 163 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> > index e9edbf11948f..8e75efc5e4b9 100644
> > --- a/Documentation/hwmon/max31785
> > +++ b/Documentation/hwmon/max31785
> > @@ -17,8 +17,9 @@ management with temperature and remote voltage sensing. 
> > Various fan control
> >  features are provided, including PWM frequency control, temperature 
> > hysteresis,
> >  dual tachometer measurements, and fan health monitoring.
> >  
> > -For dual rotor fan configuration, the MAX31785 exposes the slowest rotor 
> > of the
> > -two in the fan[1-4]_input attributes.
> > +For dual-rotor configurations the MAX31785A exposes the second rotor 
> > tachometer
> > +readings in attributes fan[5-8]_input. By contrast the MAX31785 only 
> > exposes
> > +the slowest rotor measurement, and does so in the fan[1-4]_input 
> > attributes.
> >  
> >  Usage Notes
> >  ---
> > @@ -31,7 +32,8 @@ Sysfs attributes
> >  
> >  fan[1-4]_alarm Fan alarm.
> >  fan[1-4]_fault Fan fault.
> > -fan[1-4]_input Fan RPM.
> > +fan[1-8]_input Fan RPM. On the MAX31785A, inputs 5-8 
> > correspond to the
> > +   second rotor of fans 1-4
> >  fan[1-4]_targetFan input target
> >  
> >  in[1-6]_crit   Critical maximum output voltage
> > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > index 0d97ddf67079..2ca7febb2843 100644
> > --- a/drivers/hwmon/pmbus/max31785.c
> > +++ b/drivers/hwmon/pmbus/max31785.c
> > @@ -16,10 +16,82 @@
> >  
> >  enum max31785_regs {
> > MFR_REVISION= 0x9b,
> > +   MFR_FAN_CONFIG  = 0xf1,
> >  };
> >  
> > +#define MAX31785   0x3030
> > +#define MAX31785A  0x3040
> > +
> > +#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
> > +
> >  #define MAX31785_NR_PAGES  23
> >  
> > +static int max31785_read_byte_data(struct i2c_client *client, int page,
> > +  int reg)
> > +{
> > +   switch (reg) {
> > +   case PMBUS_VOUT_MODE:
> > +   if (page < MAX31785_NR_PAGES)
> > +   return -ENODATA;
> > +
> > +   return -ENOTSUPP;
> > +   case PMBUS_FAN_CONFIG_12:
> > +   if (page < MAX31785_NR_PAGES)
> > +   return -ENODATA;
> > +
> > +   return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
> > +   reg);
> > +   }
> > +
> > +   return -ENODATA;
> > +}
> > +
> > +static int max31785_write_byte(struct i2c_client *client, int page, u8 
> > value)
> > +{
> > +   if (page < MAX31785_NR_PAGES)
> > +   return -ENODATA;
> > +
> > +   return -ENOTSUPP;
> > +}
> > +
> > +static int max31785_read_long_data(struct i2c_client *client, int page,
> > +  int reg, u32 *data)
> > +{
> > +   unsigned char cmdbuf[1];
> > +   unsigned char rspbuf[4];
> > +   int rc;
> > +
> > +   struct i2c_msg msg[2

Re: [v4,4/6] pmbus: max31785: Add fan control

[Andrew Jeffery]

On Sun, 2017-11-05 at 07:04 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:04PM +1100, Andrew Jeffery wrote:
> > The implementation makes use of the new fan control virtual registers
> > exposed by the pmbus core. It mixes use of the default implementations
> > with some overrides via the read/write handlers to handle FAN_COMMAND_1
> > on the MAX31785, whose definition breaks the value range into various
> > control bands dependent on RPM or PWM mode.
> > 
> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  Documentation/hwmon/max31785   |   4 ++
> >  drivers/hwmon/pmbus/max31785.c | 104 
> > -
> >  2 files changed, 107 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> > index 45fb6093dec2..e9edbf11948f 100644
> > --- a/Documentation/hwmon/max31785
> > +++ b/Documentation/hwmon/max31785
> > @@ -32,6 +32,7 @@ Sysfs attributes
> >  fan[1-4]_alarm Fan alarm.
> >  fan[1-4]_fault Fan fault.
> >  fan[1-4]_input Fan RPM.
> > +fan[1-4]_targetFan input target
> >  
> >  in[1-6]_crit   Critical maximum output voltage
> >  in[1-6]_crit_alarm Output voltage critical high alarm
> > @@ -44,6 +45,9 @@ in[1-6]_max_alarm Output voltage high alarm
> >  in[1-6]_minMinimum output voltage
> >  in[1-6]_min_alarm  Output voltage low alarm
> >  
> > +pwm[1-4]   Fan target duty cycle (0..255)
> > +pwm[1-4]_enable0: full-speed, 1: manual control, 2: automatic
> > +
> >  temp[1-11]_critCritical high temperature
> >  temp[1-11]_crit_alarm  Chip temperature critical high alarm
> >  temp[1-11]_input   Measured temperature
> > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > index 9313849d5160..0d97ddf67079 100644
> > --- a/drivers/hwmon/pmbus/max31785.c
> > +++ b/drivers/hwmon/pmbus/max31785.c
> > @@ -20,8 +20,102 @@ enum max31785_regs {
> >  
> >  #define MAX31785_NR_PAGES  23
> >  
> > +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;
> > +
> > +   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;
> > +
> > +   return 1;
> > +   }
> > +
> > +   return (command >= 0x8000) ? 2 : 1;
> > +}
> > +
> > +static int max31785_read_word_data(struct i2c_client *client, int page,
> > +  int reg)
> > +{
> > +   int rv;
> > +
> > +   switch (reg) {
> > +   case PMBUS_VIRT_PWM_1:
> > +   rv = max31785_get_pwm(client, page);
> > +   if (rv < 0)
> > +   return rv;
> > +
> > +   rv *= 255;
> > +   rv /= 100;
> > +   break;
> > +   case PMBUS_VIRT_PWM_ENABLE_1:
> > +   rv = max31785_get_pwm_mode(client, page);
> > +   break;
> 
> I do wonder ... does it even make sense to specify generic code
> for the new virtual attributes in the pmbus core code, or would
> it be better to have it all in this driver, at least for now ?

I think I'll pull the generic implementations out in light of my
response on 3/6. At best the generic implementation for the PWM virtual
regs is a guess. We can always put it back if others come to need it.

Cheers,

Andrew

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

Re: [v4,3/6] pmbus: core: Add fan control support

[Andrew Jeffery]

On Sun, 2017-11-05 at 06:39 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:
> > Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.
> > 
> > Fans in a PMBus device are driven by the configuration of two registers:
> > FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
> > and the tacho operate (if installed), while FAN_COMMAND_x sets the
> > desired fan rate. The unit of FAN_COMMAND_x is dependent on the
> > operational fan mode, RPM or PWM percent duty, as determined by the
> > corresponding FAN_CONFIG_x_y.
> > 
> > The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
> > FAN_COMMAND_x is implemented with the addition of virtual registers and
> > generic implementations in the core:
> > 
> > 1. PMBUS_VIRT_FAN_TARGET_x
> > 2. PMBUS_VIRT_PWM_x
> > 3. PMBUS_VIRT_PWM_ENABLE_x
> > 
> > The virtual registers facilitate the necessary side-effects of each
> > access. Examples of the read case, assuming m = 1, b = 0, R = 0:
> > 
> >  Read |  With  || Gives
> >  ---
> >Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
> >  --++---
> >   fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
> >   pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
> >   pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
> >   fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
> >   pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
> >   pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1
> > 
> > And the write case:
> > 
> >  Write| With  ||   Sets
> >  -+---+++---
> >Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
> >  -+---+++---
> >   fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
> >   pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
> >   pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064
> > 
> > Also, the DIRECT mode scaling of some controllers is different between
> > RPM and PWM percent duty control modes, so PSC_PWM is introduced to
> > capture the necessary coefficients.
> > 
> > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  drivers/hwmon/pmbus/pmbus.h  |  29 +
> >  drivers/hwmon/pmbus/pmbus_core.c | 224 
> > ---
> >  2 files changed, 238 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index 4efa2bd4f6d8..cdf3e288e626 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -190,6 +190,28 @@ enum pmbus_regs {
> > PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
> > PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
> > PMBUS_VIRT_STATUS_VMON,
> > +
> > +   /*
> > +* RPM and PWM Fan control
> > +*
> > +* Drivers wanting to expose PWM control must define the behaviour of
> > +* PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
> > +*
> > +* pmbus core provides default implementations for
> > +* PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
> > +*/
> > +   PMBUS_VIRT_FAN_TARGET_1,
> > +   PMBUS_VIRT_FAN_TARGET_2,
> > +   PMBUS_VIRT_FAN_TARGET_3,
> > +   PMBUS_VIRT_FAN_TARGET_4,
> > +   PMBUS_VIRT_PWM_1,
> > +   PMBUS_VIRT_PWM_2,
> > +   PMBUS_VIRT_PWM_3,
> > +   PMBUS_VIRT_PWM_4,
> > +   PMBUS_VIRT_PWM_ENABLE_1,
> > +   PMBUS_VIRT_PWM_ENABLE_2,
> > +   PMBUS_VIRT_PWM_ENABLE_3,
> > +   PMBUS_VIRT_PWM_ENABLE_4,
> >  };
> >  
> >  /*
> > @@ -223,6 +245,8 @@ enum pmbus_regs {
> >  #define PB_FAN_1_RPM   BIT(6)
> >  #define PB_FAN_1_INSTALLED BIT(7)
> >  
> > +enum pmbus_fan_mode { percent = 0, rpm };
> > +
> >  /*
> >   * STATUS_BYTE, STATUS_WORD (lower)
> >   */
> > @@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
> > PSC_POWER,
> > PSC_TEMPERATURE,
> > PSC_FAN,
> > +   PSC_PWM,
> > PSC_NUM_CLASSES /* Number of power sensor classes */
> >  };
> >  
> > @@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
> >  #define PMBUS_HAVE_STATUS_FAN34BIT(17)
> >  #define PMBUS_HAVE_VMONBIT(

[PATCH v4 0/6] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

This series introduces support for the MAX31785 intelligent fan controller, a
PMBus device providing closed-loop fan control among a number of other
features. Along the way the series adds support to control fans and create
virtual pages to the PMBus core, the latter to support some of the more
annoying design decisions found in the 'A' variant of the chip.

This is the fourth spin of the series, v3 can be found here[1].

I've been running aground with the described devicetree bindings in the
previous iterations, so in order to get *some* support upstream I've gutted the
documentation and removed the corresponding support from the driver. I'll save
posting that for a later date once Guenter and I have some input from Rob about
what direction to take with respect to describing PMBus devices.

As mentioned, adding full support for the features of the MAX31785 requires
modifications to the PMBus core, so I've split the addition of features into
separate patches, in the hope that some can be incrementally applied while we
iterate on the details of any suboptimal parts.

Please review!

Andrew

[1] https://lkml.org/lkml/2017/9/8/4

Andrew Jeffery (6):
  dt-bindings: pmbus: Add Maxim MAX31785 documentation
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller
  pmbus: core: Add fan control support
  pmbus: max31785: Add fan control
  pmbus: core: Add virtual page config bit
  pmbus: max31785: Add dual tachometer support

 .../devicetree/bindings/hwmon/max31785.txt |  22 ++
 Documentation/hwmon/max31785   |  57 
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 375 +
 drivers/hwmon/pmbus/pmbus.h|  31 ++
 drivers/hwmon/pmbus/pmbus_core.c   | 236 +++--
 7 files changed, 713 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/max31785.txt
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/6] dt-bindings: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 .../devicetree/bindings/hwmon/max31785.txt | 22 ++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/max31785.txt

diff --git a/Documentation/devicetree/bindings/hwmon/max31785.txt 
b/Documentation/devicetree/bindings/hwmon/max31785.txt
new file mode 100644
index ..106e08c56aaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max31785.txt
@@ -0,0 +1,22 @@
+Bindings for the Maxim MAX31785 Intelligent Fan Controller
+==
+
+Reference:
+
+https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+The Maxim MAX31785 is a PMBus device providing closed-loop, multi-channel fan
+management with temperature and remote voltage sensing. Various fan control
+features are provided, including PWM frequency control, temperature hysteresis,
+dual tachometer measurements, and fan health monitoring.
+
+Required properties:
+- compatible : One of "maxim,max31785" or "maxim,max31785a"
+- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
+
+Example:
+
+fans@52 {
+compatible = "maxim,max31785";
+reg = <0x52>;
+};
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 6/6] pmbus: max31785: Add dual tachometer support

[Andrew Jeffery]

The dual tachometer feature is implemented in hardware with a TACHSEL
input to indicate the rotor under measurement, and exposed on the device
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 the
values are exposed through virtual pages. We re-route accesses to
FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on undefined (in hardware) pages
23-28 to the same registers on pages 0-5, and with the latter command we
extract the value from the second word of the four-byte response.

* The documentation recommends the slower rotor be associated with
TACHSEL=0, which provides the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |   8 ++-
 drivers/hwmon/pmbus/max31785.c | 159 -
 2 files changed, 163 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index e9edbf11948f..8e75efc5e4b9 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -17,8 +17,9 @@ management with temperature and remote voltage sensing. 
Various fan control
 features are provided, including PWM frequency control, temperature hysteresis,
 dual tachometer measurements, and fan health monitoring.
 
-For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
-two in the fan[1-4]_input attributes.
+For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
+readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
+the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
 
 Usage Notes
 ---
@@ -31,7 +32,8 @@ Sysfs attributes
 
 fan[1-4]_alarm Fan alarm.
 fan[1-4]_fault Fan fault.
-fan[1-4]_input Fan RPM.
+fan[1-8]_input Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
+   second rotor of fans 1-4
 fan[1-4]_targetFan input target
 
 in[1-6]_crit   Critical maximum output voltage
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 0d97ddf67079..2ca7febb2843 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,10 +16,82 @@
 
 enum max31785_regs {
MFR_REVISION= 0x9b,
+   MFR_FAN_CONFIG  = 0xf1,
 };
 
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+
 #define MAX31785_NR_PAGES  23
 
+static int max31785_read_byte_data(struct i2c_client *client, int page,
+  int reg)
+{
+   switch (reg) {
+   case PMBUS_VOUT_MODE:
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   return -ENOTSUPP;
+   case PMBUS_FAN_CONFIG_12:
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
+   reg);
+   }
+
+   return -ENODATA;
+}
+
+static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
+{
+   if (page < MAX31785_NR_PAGES)
+   return -ENODATA;
+
+   return -ENOTSUPP;
+}
+
+static int max31785_read_long_data(struct i2c_client *client, int page,
+  int reg, u32 *data)
+{
+   unsigned char cmdbuf[1];
+   unsigned char rspbuf[4];
+   int rc;
+
+   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;
+
+   *data = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
+   (rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
+
+   return rc;
+}
+
 static int max31785_get_pwm(struct i2c_client *client, int page)
 {
int config;
@@ -76,7 +148,25 @@ static int max31785_read_word_data(struct i2c_client 
*client, int page,
int rv;
 
switch (reg) {
+   case PMBUS_READ_FAN_SPEED_1:
+  

[PATCH v4 4/6] pmbus: max31785: Add fan control

[Andrew Jeffery]

The implementation makes use of the new fan control virtual registers
exposed by the pmbus core. It mixes use of the default implementations
with some overrides via the read/write handlers to handle FAN_COMMAND_1
on the MAX31785, whose definition breaks the value range into various
control bands dependent on RPM or PWM mode.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |   4 ++
 drivers/hwmon/pmbus/max31785.c | 104 -
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 45fb6093dec2..e9edbf11948f 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -32,6 +32,7 @@ Sysfs attributes
 fan[1-4]_alarm Fan alarm.
 fan[1-4]_fault Fan fault.
 fan[1-4]_input Fan RPM.
+fan[1-4]_targetFan input target
 
 in[1-6]_crit   Critical maximum output voltage
 in[1-6]_crit_alarm Output voltage critical high alarm
@@ -44,6 +45,9 @@ in[1-6]_max_alarm Output voltage high alarm
 in[1-6]_minMinimum output voltage
 in[1-6]_min_alarm  Output voltage low alarm
 
+pwm[1-4]   Fan target duty cycle (0..255)
+pwm[1-4]_enable0: full-speed, 1: manual control, 2: automatic
+
 temp[1-11]_critCritical high temperature
 temp[1-11]_crit_alarm  Chip temperature critical high alarm
 temp[1-11]_input   Measured temperature
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 9313849d5160..0d97ddf67079 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -20,8 +20,102 @@ enum max31785_regs {
 
 #define MAX31785_NR_PAGES  23
 
+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;
+
+   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;
+
+   return 1;
+   }
+
+   return (command >= 0x8000) ? 2 : 1;
+}
+
+static int max31785_read_word_data(struct i2c_client *client, int page,
+  int reg)
+{
+   int rv;
+
+   switch (reg) {
+   case PMBUS_VIRT_PWM_1:
+   rv = max31785_get_pwm(client, page);
+   if (rv < 0)
+   return rv;
+
+   rv *= 255;
+   rv /= 100;
+   break;
+   case PMBUS_VIRT_PWM_ENABLE_1:
+   rv = max31785_get_pwm_mode(client, page);
+   break;
+   default:
+   rv = -ENODATA;
+   break;
+   }
+
+   return rv;
+}
+
+static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0x };
+
+static int max31785_write_word_data(struct i2c_client *client, int page,
+   int reg, u16 word)
+{
+   switch (reg) {
+   case PMBUS_VIRT_PWM_ENABLE_1:
+   if (word >= ARRAY_SIZE(max31785_pwm_modes))
+   return -EINVAL;
+
+   return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
+   max31785_pwm_modes[word]);
+   default:
+   break;
+   }
+
+   return -ENODATA;
+}
+
 #define MAX31785_FAN_FUNCS \
-   (PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
+   (PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_PWM12)
 
 #define MAX31785_TEMP_FUNCS \
(PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP)
@@ -32,11 +126,19 @@ enum max31785_regs {
 static const struct pmbus_driver_info max31785_info = {
.pages = MAX31785_NR_PAGES,
 
+   .write_word_data = max31785_write_word_data,
+   .read_word_data = max31785_read_word_data,
+
/* RPM */
.format[PSC_FAN] = direct,
.m[PSC_FAN] = 1,
.b[PSC_FAN] = 0,
.R[PSC_FAN] = 0,
+   /* PWM */
+   .format[PSC_PWM] = dire

[PATCH v4 2/6] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Andrew Jeffery]

The Maxim MAX31785 is a PMBus device providing closed-loop, multi-channel fan
management with temperature and remote voltage sensing. Various fan control
features are provided, including PWM frequency control, temperature hysteresis,
dual tachometer measurements, and fan health monitoring.

This patch presents a basic driver using only the existing features of the
PMBus subsystem.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/max31785   |  51 ++
 drivers/hwmon/pmbus/Kconfig|  10 
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 116 +
 4 files changed, 178 insertions(+)
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/pmbus/max31785.c

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
new file mode 100644
index ..45fb6093dec2
--- /dev/null
+++ b/Documentation/hwmon/max31785
@@ -0,0 +1,51 @@
+Kernel driver max31785
+==
+
+Supported chips:
+  * Maxim MAX31785, MAX31785A
+Prefix: 'max31785' or 'max31785a'
+Addresses scanned: -
+Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+Author: Andrew Jeffery <and...@aj.id.au>
+
+Description
+---
+
+The Maxim MAX31785 is a PMBus device providing closed-loop, multi-channel fan
+management with temperature and remote voltage sensing. Various fan control
+features are provided, including PWM frequency control, temperature hysteresis,
+dual tachometer measurements, and fan health monitoring.
+
+For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
+two in the fan[1-4]_input attributes.
+
+Usage Notes
+---
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+Sysfs attributes
+
+
+fan[1-4]_alarm Fan alarm.
+fan[1-4]_fault Fan fault.
+fan[1-4]_input Fan RPM.
+
+in[1-6]_crit   Critical maximum output voltage
+in[1-6]_crit_alarm Output voltage critical high alarm
+in[1-6]_input  Measured output voltage
+in[1-6]_label  "vout[18-23]"
+in[1-6]_lcrit  Critical minimum output voltage
+in[1-6]_lcrit_alarmOutput voltage critical low alarm
+in[1-6]_maxMaximum output voltage
+in[1-6]_max_alarm  Output voltage high alarm
+in[1-6]_minMinimum output voltage
+in[1-6]_min_alarm  Output voltage low alarm
+
+temp[1-11]_critCritical high temperature
+temp[1-11]_crit_alarm  Chip temperature critical high alarm
+temp[1-11]_input   Measured temperature
+temp[1-11]_max Maximum temperature
+temp[1-11]_max_alarm   Chip temperature high alarm
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 40019325b517..08479006c7f9 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -114,6 +114,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 459a6be3390e..a8bf0e490db9 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -12,6 +12,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 ..9313849d5160
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,116 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+enum max31785_regs {
+   MFR_REVISION= 0x9b,
+};
+
+#define MAX31785_NR_PAGES  23
+
+#define MAX31785_FAN_FUNCS \
+   (PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
+
+#define MAX31785_TEMP_FUNCS \
+   (PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP)
+
+#d

[PATCH v4 5/6] pmbus: core: Add virtual page config bit

[Andrew Jeffery]

Some circumstances call for virtual pages to expose multiple values
packed into an extended PMBus register in a manner non-compliant with
the PMBus standard. We should not try to set virtual pages on the
device; add a flag so we can avoid doing so.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 12 
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index cdf3e288e626..0560a8dbcee0 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -367,6 +367,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12   BIT(20)
 #define PMBUS_HAVE_PWM34   BIT(21)
 
+#define PMBUS_PAGE_VIRTUAL BIT(31)
+
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12, vr13 };
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 55838b69e99a..af7362de405d 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
int rv = 0;
int newpage;
 
-   if (page != data->currpage) {
+   if (page == data->currpage)
+   return 0;
+
+   if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
if (newpage != page)
-   rv = -EIO;
-   else
-   data->currpage = page;
+   return -EIO;
}
+
+   data->currpage = page;
+
return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/6] pmbus: core: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The virtual registers facilitate the necessary side-effects of each
access. Examples of the read case, assuming m = 1, b = 0, R = 0:

 Read |  With  || Gives
 ---
   Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
 --++---
  fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
  pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
  pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
  fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
  pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
  pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1

And the write case:

 Write| With  ||   Sets
 -+---+++---
   Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
 -+---+++---
  fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
  pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
  pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  29 +
 drivers/hwmon/pmbus/pmbus_core.c | 224 ---
 2 files changed, 238 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 4efa2bd4f6d8..cdf3e288e626 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,28 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /*
+* RPM and PWM Fan control
+*
+* Drivers wanting to expose PWM control must define the behaviour of
+* PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
+*
+* pmbus core provides default implementations for
+* PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
+*/
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +245,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34BIT(17)
 #define PMBUS_HAVE_VMONBIT(18)
 #define PMBUS_HAVE_STATUS_VMON BIT(19)
+#define PMBUS_HAVE_PWM12   BIT(20)
+#define PMBUS_HAVE_PWM34   BIT(21)
 
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12, vr13 };
@@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+u8 config, u8 mask, u16 command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 302f0aef59de..55838b69e99a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -64,6 +64,7 @@ struct pmbus_sensor {
u

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

On Mon, 2017-09-18 at 13:15 -0700, Guenter Roeck wrote:
> On Mon, Sep 18, 2017 at 02:26:38PM -0500, Rob Herring wrote:
> > On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > > ---
> > >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 
> > > +
> > 
> > I think this needs to be located by what it does (fan control), not what 
> > interface it has (pmbus).
> > 
> > I'm not all that happy with hwmon either because things here seem to 
> > just be based on being Linux hwmon devices which is sometimes arbitrary. 
> > 
> 
> The chip also measures temperatures. Other PMBus chips may do fan control,
> measure temperatures, measure and/or control voltages, current, power ...
> Strictly speaking pretty much all PMBus chips are multi-function devices.
> I personally don't really care if the documentation is spread across
> several directories, but even here this is already challenging.
> 
> Only solution I can think of would be to create separate documents for each
> functionality, ie here one for the device itself, one for fan control,
> and one for temperature control (if that needs separate bindings). That
> would be similar to mfd. But then we would still have to sort out where
> to store the various bindings. Like iio, in subdirectories ? Like mfd,
> in the matching subsystems ? If so, what to do if there is no matching
> subsystem ?
> 
> Lots of questions. I'll be happy to spend some time sorting it out,
> but I would need some directions.
> 

Likewise - I'm keen to discuss and iterate on this so we get something
satisfactory.

At least I could split out the PMBus-specific bindings from the Maxim-
specific bindings in the current document, but there's still the
question of how to arrange it as Guenter has queried above, and also
how much of PMBus to define bindings for. I'm hesitant to take a stab
at describing bindings across the whole spec if I don't have useful
driver implementations to test against. I know that the bindings
describe the hardware and not the driver, but there are probably more
or less clumsy ways to describe devices that could be ironed out with a
corresponding implementation (e.g. the Aspeed PWM/Tacho...).

Thoughts?

Andrew

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

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

On Mon, 2017-09-18 at 14:26 -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 
> > +
> 
> I think this needs to be located by what it does (fan control), not what 
> interface it has (pmbus).
> 
> I'm not all that happy with hwmon either because things here seem to 
> just be based on being Linux hwmon devices which is sometimes arbitrary.

I'll follow-up on this on Guenter's reply if necessary.

>  
> 
> >  1 file changed, 158 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt 
> > b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > new file mode 100644
> > index ..af9578e7742c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > @@ -0,0 +1,158 @@
> > +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> > +==
> > +
> > +Reference:
> > +
> > +https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> > +
> > +Required properties:
> > +- compatible : One of "maxim,max31785" or "maxim,max31785a"
> > +- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
> > +- #address-cells : Must be 1
> > +- #size-cells: Must be 0
> > +- #thermal-sensor-cells  : Should be 1. The device supports:
> > +   - One internal sensor
> > +   - Four external I2C digital sensors
> > +   - Six external thermal diodes
> 
> You should define the IDs here, not just how many of each type.

Okay.

> 
> > +
> > +Optional properties:
> > +- use-stored-presence: Do not treat the devicetree description as 
> > canon for
> > +   fan presence (the 'installed' bit of 
> > FAN_CONFIG_*).
> > +   Instead, rely on the on the default value store 
> > of
> > +   the device to populate it.
> 
> Boolean? Please be explicit.

Okay.

> 
> Needs a vendor prefix.

We've discussed this previously. It's describing how to interpret part
of the PMBus spec, not something Maxim have done, so I don't think it
should have a vendor prefix.

But maybe I'm representing it wrong?

> 
> > +
> > +Capabilities are configured through subnodes of the controller's node.
> > +
> > +Fans
> > +
> > +
> > +Only fans with subnodes present will be considered as installed. If
> > +use-stored-presence is present in the parent node, then only fans that are 
> > both
> > +defined in the devicetree and have their installed bit set are considered
> > +installed.
> > +
> > +Required subnode properties:
> > +- compatible : Must be "pmbus-fan"
> 
> "pmbus" is a property of the controller, not the fan, right? We should 
> have compatibles along the lines of the type of fan like 4-wire, 3-wire, 
> etc. 

Yes, PMBus is a property of the controller. But a controller that
implements PMBus fan control and monitoring abstracts away most of the
details of the fan hardware, so I don't see it fit to describe e.g. 4-
wire or 3-wire properties here.

Perhaps this is resolved by having a "pmbus-fan-control" compatible
node, and nesting a fan node under that? My concern would be that the
only element of the fan subnode would be the "tach-pulses" property
(though maybe also dual-tach if we generalise the maxim,fan-dual-tach
property below).

Regardless, what I'm trying to describe here with the non-vendor-
prefixed properties are configurable elements of the PMBus interface
for fan control. Adherence to PMBus by a device dictates the command
interface, and is therefore a property of the controller hardware at
the end of the day. In this instance (PMBus) I don't see how we can
draw the distinction between "what it does" and "what interface it has"
- adherence to relevant parts of the PMBus spec defines the minimum of
what the controller does (PMBus allows for vendor extensions).
Unfortunately the PMBus spec is fairly loose in terms of what it
*requires* you to implement, but I don't think that's relevant here.

> 
> > +- reg: The PMBus page the properties apply to.
> > +- #cooling-cells : Should be 2. See the thermal bindings at [1].
> > +- maxim,fan-rotor-input  : The

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 22:14 -0700, Guenter Roeck wrote:
> On 09/07/2017 08:40 PM, Andrew Jeffery wrote:
> > On Fri, 2017-09-08 at 12:51 +1000, Andrew Jeffery wrote:
> > > > I can't test with devicetree. x86 system.
> > > >   
> > > > 2,100+ iterations with your driver, no failures.
> > > 
> > > Great. I really appreciate your testing here, so thanks for your
> > > efforts. I owe you a few drinks if we ever happen to meet.
> > 
> > Actually, on that, how did you chop out the devicetree parts? Did you
> > keep the configuration writes at the end of max31785_of_fan_config()
> > and max31785_of_tmp_config()? Or did you just not call them? These two
> > functions cause the bulk of the bus traffic with on probe.
> > 
> 
> fan config hardcoded, four fans connected. Still no failure.

Great. Thanks again for your efforts here.

Andrew

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 21:43 -0700, Guenter Roeck wrote:
> On 09/07/2017 08:40 PM, Andrew Jeffery wrote:
> > On Fri, 2017-09-08 at 12:51 +1000, Andrew Jeffery wrote:
> > > > I can't test with devicetree. x86 system.
> > > >   
> > > > 2,100+ iterations with your driver, no failures.
> > > 
> > > Great. I really appreciate your testing here, so thanks for your
> > > efforts. I owe you a few drinks if we ever happen to meet.
> > 
> > Actually, on that, how did you chop out the devicetree parts? Did you
> > keep the configuration writes at the end of max31785_of_fan_config()
> > and max31785_of_tmp_config()? Or did you just not call them? These two
> > functions cause the bulk of the bus traffic with on probe.
> > 
> 
> I didn't change to code, just compiled and run it. Guess that means
> this part was skipped.

Right, yeah looking at it a bit more, dev->of_node will be NULL for
for_each_child_of_node(dev->of_node, child), therefore the loop body
won't execute.

> 
> I'll replaced the fan configuration with some hard-coded values.
> We'll see if it makes a difference.

Sounds good.

Andrew

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

[PATCH v3 3/3] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Andrew Jeffery]

The Maxim MAX31785 is a PMBus device providing closed-loop,
multi-channel fan management with temperature and remote voltage
sensing. Various fan control features are provided, including PWM
frequency control, temperature hysteresis, dual tachometer measurements,
and fan health monitoring.

The driver implementation makes use of the new fan control virtual
registers exposed by the pmbus core. It mixes use of the default
implementations with some overrides via the read/write handlers to
handle FAN_COMMAND_1 on the MAX31785, whose definition breaks the value
range into various control bands dependent on RPM or PWM 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 provides the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 673 +
 3 files changed, 684 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/max31785.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 68d717a3fd59..04f6baa98a14 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -105,6 +105,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 75bb7ca619d9..663801c57a82 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -11,6 +11,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 ..a83f59ca31c7
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,673 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+enum max31785_regs {
+   MFR_REVISION= 0x9b,
+   MFR_FAULT_RESPONSE  = 0xd9,
+   MFR_TEMP_SENSOR_CONFIG  = 0xf0,
+   MFR_FAN_CONFIG  = 0xf1,
+   MFR_READ_FAN_PWM= 0xf3,
+   MFR_FAN_FAULT_LIMIT = 0xf5,
+   MFR_FAN_WARN_LIMIT  = 0xf6,
+   MFR_FAN_PWM_AVG = 0xf8,
+};
+
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_TEMP_SENSOR_CONFIG_ENABLE  BIT(15)
+#define MFR_TEMP_SENSOR_CONFIG_OFFSET  GENMASK(14, 10)
+
+#define MFR_FAN_CONFIG_FREQGENMASK(15, 13)
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+#define MFR_FAN_CONFIG_HYS GENMASK(11, 10)
+#define MFR_FAN_CONFIG_TSFOBIT(9)
+#define MFR_FAN_CONFIG_TACHO   BIT(8)
+#define MFR_FAN_CONFIG_RAMPGENMASK(7, 5)
+#define MFR_FAN_CONFIG_HEALTH  BIT(4)
+#define MFR_FAN_CONFIG_ROTOR_HI_LO BIT(3)
+#define MFR_FAN_CONFIG_ROTOR   BIT(2)
+#define MFR_FAN_CONFIG_SPINGENMASK(1, 0)
+
+#define MFR_FAULT_RESPONSE_MONITOR BIT(0)
+
+#define MAX31785_CAP_READ_DUAL_TACHBIT(0)
+
+#define MAX31785_NR_PAGES  23
+
+static int max31785_read_byte_data

[PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt 
b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
new file mode 100644
index ..af9578e7742c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
@@ -0,0 +1,158 @@
+Bindings for the Maxim MAX31785 Intelligent Fan Controller
+==
+
+Reference:
+
+https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+Required properties:
+- compatible : One of "maxim,max31785" or "maxim,max31785a"
+- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
+- #address-cells : Must be 1
+- #size-cells: Must be 0
+- #thermal-sensor-cells  : Should be 1. The device supports:
+   - One internal sensor
+   - Four external I2C digital sensors
+   - Six external thermal diodes
+
+Optional properties:
+- use-stored-presence: Do not treat the devicetree description as canon for
+   fan presence (the 'installed' bit of FAN_CONFIG_*).
+   Instead, rely on the on the default value store of
+   the device to populate it.
+
+Capabilities are configured through subnodes of the controller's node.
+
+Fans
+
+
+Only fans with subnodes present will be considered as installed. If
+use-stored-presence is present in the parent node, then only fans that are both
+defined in the devicetree and have their installed bit set are considered
+installed.
+
+Required subnode properties:
+- compatible : Must be "pmbus-fan"
+- reg: The PMBus page the properties apply to.
+- #cooling-cells : Should be 2. See the thermal bindings at [1].
+- maxim,fan-rotor-input  : The type of rotor measurement provided to the
+   controller. Must be either "tach" for tachometer
+   pulses or "lock" for a locked-rotor signal.
+- maxim,fan-lock-polarity: Required iff maxim,fan-rotor-input is "lock". Valid
+   values are "low" for active low, "high" for active
+   high.
+
+Optional subnode properties:
+- fan-mode   : "rpm" or "pwm". Default value is "pwm".
+- tach-pulses: Tachometer pulses per revolution. Valid values are
+   1, 2, 3 or 4. The default is 1.
+- cooling-min-level  : Smallest cooling state accepted. See [1].
+- cooling-max-level  : Largest cooling state accepted. See [1].
+- maxim,fan-no-fault-ramp: Do not ramp the fan to 100% PWM duty on detecting a
+   fan fault
+- maxim,fan-startup  : The number of rotations required before taking
+   emergency action for an unresponsive fan and driving
+   it with 100% or 0% PWM duty, depending on the state
+   of maxim,fan-no-fault-ramp. Valid values are 0
+   (automatic spin-up disabled), 2, 4, or 8. Default
+   value is 0.
+- maxim,fan-health   : Enable automated fan health check
+- maxim,fan-ramp : Configures how fast the device ramps the PWM duty
+   cycle from one value to another. Valid values are 0
+   to 7 inclusive, with values 0 - 2 configuring a
+   1000ms update rate and 1 - 3% duty respective duty
+   increase, and 3 - 7 a 200ms update rate with a 1 -
+   5% respective duty increase. Default value is 0.
+- maxim,fan-no-watchdog  : Do not ramp fan to 100% PWM duty on failure to
+   update desired fan rate inside 10s. This implies
+   maxim,tmp-no-fault-ramp
+- maxim,tmp-no-fault-ramp: Do not ramp fan to 100% PWM duty on temperature
+   sensor fault detection. This implies
+   maxim,fan-no-watchdog
+- maxim,tmp-hysteresis   : The temperature hysteresis used to determine
+   transitions to lower fan speed bands in the
+   temperature/fan rate lookup table. Valid values are
+   2, 4, 6 or 8 (degrees celcius). Default value is 2.
+- maxim,fan-dual-tach: Enable dual tachometer functionality
+- maxim,fan-pwm-freq : The PWM frequency. Valid values are 30, 50, 100, 150
+   and 25000 (Hz). Default value is 30Hz.
+- maxim,fan-lookup-table : A 16-elem

[PATCH v3 0/3] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

These three patches lay the ground work for fan control in the pmbus core and
introduce a driver for the MAX31785 Intelligent Fan Controller that makes use
of the new control features.

Since v2[1] I've addressed Rob's comments on the bindings, integrating the 
thermal
support and cleaning up some warts. One of the warts was regarding the
maxim,tmp-fans property which was switched to use a list of fan phandles rather
than indexes. A corresponding change is made in the driver. I also did a
comparison with the Aspeed PWM/Tach bindings but I'm not convinced there's much
to take away from it other than those bindings are somewhat broken.

In v3 I've dropped patch 4 from v2, which was a bunch of work-arounds for bad
behaviour I observed in testing. At this stage the bad behaviour appears to be
a product of the larger hardware design, not necessarily a problem with the
Maxim chip. Guenter has performed some quick testing of a cut-down v2 driver
against the MAX31785 evaluation board and couldn't reproduce the results I was
seeing[2][3], which increases the likelyhood that it's not the Maxim chip and
the work-around patch is inappropriate.

Please review!

Andrew

[1] https://lkml.org/lkml/2017/8/2/88
[2] https://lkml.org/lkml/2017/9/7/669
[3] https://lkml.org/lkml/2017/9/5/53

Andrew Jeffery (3):
  dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation
  hwmon: pmbus: Add fan control support
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 673 +
 drivers/hwmon/pmbus/pmbus.h|  29 +
 drivers/hwmon/pmbus/pmbus_core.c   | 224 ++-
 6 files changed, 1080 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The virtual registers facilitate the necessary side-effects of each
access. Examples of the read case, assuming m = 1, b = 0, R = 0:

 Read |  With  || Gives
 ---
   Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
 --++---
  fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
  pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
  pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
  fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
  pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
  pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1

And the write case:

 Write| With  ||   Sets
 -+---+++---
   Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
 -+---+++---
  fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
  pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
  pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  29 +
 drivers/hwmon/pmbus/pmbus_core.c | 224 ---
 2 files changed, 238 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..a863b8fed16f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,28 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /*
+* RPM and PWM Fan control
+*
+* Drivers wanting to expose PWM control must define the behaviour of
+* PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
+*
+* pmbus core provides default implementations for
+* PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
+*/
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +245,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34BIT(17)
 #define PMBUS_HAVE_VMONBIT(18)
 #define PMBUS_HAVE_STATUS_VMON BIT(19)
+#define PMBUS_HAVE_PWM12   BIT(20)
+#define PMBUS_HAVE_PWM34   BIT(21)
 
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
@@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+u8 config, u8 mask, u16 command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f1eff6b6c798..5ed9cbf1daf9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -63,6 +63,7 @@ struct pmbus_sensor {
u

Re: [PATCH v2 1/4] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

On Mon, 2017-08-14 at 11:25 +0930, Andrew Jeffery wrote:
> On Thu, 2017-08-10 at 11:13 -0500, Rob Herring wrote:
> > On Wed, Aug 02, 2017 at 04:45:38PM +0930, Andrew Jeffery wrote:
> > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > > 
> > > ---
> > >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 126 
> > > +
> > >  1 file changed, 126 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > >  
> > > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt 
> > > b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > > new file mode 100644
> > > index ..8ddc4ea1958d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > > @@ -0,0 +1,126 @@
> > > +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> > > +==
> > 
> >  
> > This is the second fan controller binding I've seen recently. The other 
> > being hwmon/aspeed-pwm-tacho.txt. I think some of this needs to be 
> > common. There's only so many types of fans, right?
> 
> Heh, you'd think so, I'll take a look. However much of this is driven by the
> PMBus specification and the Aspeed PWM/Tacho isn't a PMBus-based device.

Following up on this, there's not much that conflicts between the two fan
descriptions. But there's not much in common either, just because there's
really not that much to it all. In both cases the interesting bits are in the
manufacturer specific properties. reg is applicable in either case. I require a
compatible here to separate fan support from temperature sensing.

What would you be looking for in a common fan description?

However, as an aside, I think there's a fundamental problem with the Aspeed
PWM/Tacho bindings. For reference, a vendor submitted this devicetree patch to
OpenBMC:

http://patchwork.ozlabs.org/patch/804862/

Particularly, I'd draw your attention to this part of the linked patch:

+   fan@0 {
+   reg = <0x00>;
+   cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+   aspeed,fan-tach-ch = /bits/ 8 <0x00>;
+   };
+
+   fan@1 {
+   reg = <0x00>;
+   aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+   };
+
+   fan@2 {
+   reg = <0x00>;
+   aspeed,fan-tach-ch = /bits/ 8 <0x02>;
+   };

As outlined in my comments on the patch in the patchwork link above, the
bindings are backwards for what is being described: Eight fans driven by one
PWM, so there is a mismatch between the unit and reg. reg is being used to
index the one PWM, and tach channels are being associated to the PWM.

I rather think fans should be represented by their tach input (the tach ID
assigned to reg), and a PWM channel be associated with the node via e.g. an
aspeed,pwm-ch property. There is the issue of dual-tach fans, but in the case
of the Aspeed PWM/Tach block the dual-tach lines would need to be wired to
separate tach inputs, still leaving us free to select the appropriate tach
input to drive the fan loop.

There are other issues such as the incorrect suggestion for the value of
#size-cells in the Aspeed PWM/Tach bindings.

> 
> I was hesitant to start a generic PMBus bindings document without having more
> PMBus devices with bindings, but maybe that's the way I should go? It would
> make clear what's from the fan-control parts of the PMBus specification and
> what's here for the purpose of supporting the MAX31785.
> 
> >  
> > And we have the thermal binding which you don't appear to tie into.
> 
> I'll look into that also.

I've done what I think I need to in order to resolve this.

> 
> >  
> > > +
> > > +Reference:
> > > +[1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> > > +
> > > +Required properties:
> > > +- compatible : One of "maxim,max31785" or "maxim,max31785a"
> > > +- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
> > > +- #address-cells : Must be 1
> > > +- #size-cells: Must be 0
> > > +
> > > +Optional properties:
> > > +- use-stored-presence: Do not treat the devicetree description as 
> > > canon for
> > 
> >  
> > Is this maxim specific? If so, needs a vendor prefix.
> 
> PMBus specifies two 8-bit registers of the same structure: FAN_CONFIG_1_2
> and FAN_CONFIG_3_4. It's not intended to be manufacturer-specific.
> 
> A bit in each nibble of FAN_CONFIG_* is specified as mar

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Fri, 2017-09-08 at 12:51 +1000, Andrew Jeffery wrote:
> > I can't test with devicetree. x86 system.
> > 
> > 2,100+ iterations with your driver, no failures.
> 
> Great. I really appreciate your testing here, so thanks for your
> efforts. I owe you a few drinks if we ever happen to meet.

Actually, on that, how did you chop out the devicetree parts? Did you
keep the configuration writes at the end of max31785_of_fan_config()
and max31785_of_tmp_config()? Or did you just not call them? These two
functions cause the bulk of the bus traffic with on probe.

Andrew

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 19:17 -0700, Guenter Roeck wrote:
> On 09/07/2017 07:06 PM, Andrew Jeffery wrote:
> > On Thu, 2017-09-07 at 18:26 -0700, Guenter Roeck wrote:
> > > On 09/07/2017 06:02 PM, Andrew Jeffery wrote:
> > > > On Thu, 2017-09-07 at 17:27 -0700, Guenter Roeck wrote:
> > > > > On 09/07/2017 08:22 AM, Andrew Jeffery wrote:
> > > > > > On Thu, 2017-09-07 at 06:40 -0700, Guenter Roeck wrote:
> > > > > > > On 09/06/2017 04:32 PM, Andrew Jeffery wrote:
> > > > > > > 
> > > > > > > > >  
> > > > > > > > > Guess I need to dig up my eval board and see if I can 
> > > > > > > > > reproduce the problem.
> > > > > > > > > Seems you are saying that the problem is always seen when 
> > > > > > > > > issuing a sequence
> > > > > > > > > of "clear faults" commands on multiple pages ?
> > > > > > > > 
> > > > > > > > Yeah. We're also seeing bad behaviour under other command 
> > > > > > > > sequences as well,
> > > > > > > > which lead to this hack of a work-around patch[1].
> > > > > > > > 
> > > > > > > > I'd be very interested in the results of testing against the 
> > > > > > > > eval board. I
> > > > > > > > don't have access to one and it seems Maxim have discontinued 
> > > > > > > > them.
> > > > > > > > 
> > > > > > > 
> > > > > > > Do you have a somewhat reliable means to reproduce the problem ?
> > > > > > 
> > > > > > It seems we hit a bunch of problems by just continually
> > > > > > binding/unbinding the driver, if you don't apply that hacky oneshot
> > > > > > retry patch. We can hit problems (in our design?) with something 
> > > > > > like:
> > > > > > 
> > > > > > # cd /sys/bus/i2c/drivers/max31785; \
> > > > > > echo $addr > unbind; \
> > > > > > while echo $addr > bind; \
> > > > > > do echo $addr > unbind; echo -n .; done;
> > > > > > 
> > > > > > It should hit issues covered by this patch, as the register checks 
> > > > > > are
> > > > > > used in the operations used by probe.
> > > > > > 
> > > > > 
> > > > > Hmm ... I didn't use your driver but my prototype driver which also 
> > > > > supports
> > > > > temperature and voltage attributes, so if anything it should create 
> > > > > more
> > > > > stress on the chip.
> > > > 
> > > > I did add the temp and voltage attributes...
> > > > 
> > > > Any chance you can give mine a try? I don't know what I would have done
> > > > to invoke this kind of behaviour, so it would be useful to know whether
> > > > or not it happens with one driver but not the other.
> > > > 
> > > 
> > > Will do.
> > 
> > Thanks. For reference, here's a devicetree description:
> > 
> > https://github.com/openbmc/linux/blob/dev-4.10/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts#L283
> > 
> 
> I can't test with devicetree. x86 system.
> 
> 2,100+ iterations with your driver, no failures.

Great. I really appreciate your testing here, so thanks for your
efforts. I owe you a few drinks if we ever happen to meet.

> 
> Either it is because my chip is a MAX31785 (not A), or the configuration 
> makes a difference,
> or it is your hardware.

Yep. My understanding is the A variant is just a difference of
microcode, but who knows what affect that could have. 

> 
> I'll try to connect a couple of fans next (so far I did without) and try 
> again.

Keep me posted if you do.

Thanks again.

Andrew

> 
> Guenter

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 18:26 -0700, Guenter Roeck wrote:
> On 09/07/2017 06:02 PM, Andrew Jeffery wrote:
> > On Thu, 2017-09-07 at 17:27 -0700, Guenter Roeck wrote:
> > > On 09/07/2017 08:22 AM, Andrew Jeffery wrote:
> > > > On Thu, 2017-09-07 at 06:40 -0700, Guenter Roeck wrote:
> > > > > On 09/06/2017 04:32 PM, Andrew Jeffery wrote:
> > > > > 
> > > > > > > 
> > > > > > > Guess I need to dig up my eval board and see if I can reproduce 
> > > > > > > the problem.
> > > > > > > Seems you are saying that the problem is always seen when issuing 
> > > > > > > a sequence
> > > > > > > of "clear faults" commands on multiple pages ?
> > > > > > 
> > > > > > Yeah. We're also seeing bad behaviour under other command sequences 
> > > > > > as well,
> > > > > > which lead to this hack of a work-around patch[1].
> > > > > > 
> > > > > > I'd be very interested in the results of testing against the eval 
> > > > > > board. I
> > > > > > don't have access to one and it seems Maxim have discontinued them.
> > > > > > 
> > > > > 
> > > > > Do you have a somewhat reliable means to reproduce the problem ?
> > > > 
> > > > It seems we hit a bunch of problems by just continually
> > > > binding/unbinding the driver, if you don't apply that hacky oneshot
> > > > retry patch. We can hit problems (in our design?) with something like:
> > > > 
> > > > # cd /sys/bus/i2c/drivers/max31785; \
> > > > echo $addr > unbind; \
> > > > while echo $addr > bind; \
> > > > do echo $addr > unbind; echo -n .; done;
> > > > 
> > > > It should hit issues covered by this patch, as the register checks are
> > > > used in the operations used by probe.
> > > > 
> > > 
> > > Hmm ... I didn't use your driver but my prototype driver which also 
> > > supports
> > > temperature and voltage attributes, so if anything it should create more
> > > stress on the chip.
> > 
> > I did add the temp and voltage attributes...
> > 
> > Any chance you can give mine a try? I don't know what I would have done
> > to invoke this kind of behaviour, so it would be useful to know whether
> > or not it happens with one driver but not the other.
> > 
> 
> Will do.

Thanks. For reference, here's a devicetree description:

https://github.com/openbmc/linux/blob/dev-4.10/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts#L283


> 
> > >   No error so far, after running the script for a couple
> > > of minutes. How long does it take for errors to appear, and how do I see
> > > that there is an error ?
> > 
> > I'm seeing failures after anything from a handful of bind/unbinds, to
> > hundreds of bind/unbinds. It seems to vary.
> > 
> > > Does the driver fail to instantiate ?
> > 
> > Typically probe fails so the loop exits. It usually gets -EIO and the
> > shell spits out "No such device".
> > 
> > Thanks for testing, it's a useful data point for us hunting down the
> > source of our problems.
> > 
> 
> I aborted the test after ~2,500 loops without error.

Yeah, I'd consider that fairly stable.

Cheers,

Andrew

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 17:27 -0700, Guenter Roeck wrote:
> On 09/07/2017 08:22 AM, Andrew Jeffery wrote:
> > On Thu, 2017-09-07 at 06:40 -0700, Guenter Roeck wrote:
> > > On 09/06/2017 04:32 PM, Andrew Jeffery wrote:
> > > 
> > > > >    
> > > > > Guess I need to dig up my eval board and see if I can reproduce the 
> > > > > problem.
> > > > > Seems you are saying that the problem is always seen when issuing a 
> > > > > sequence
> > > > > of "clear faults" commands on multiple pages ?
> > > > 
> > > > Yeah. We're also seeing bad behaviour under other command sequences as 
> > > > well,
> > > > which lead to this hack of a work-around patch[1].
> > > > 
> > > > I'd be very interested in the results of testing against the eval 
> > > > board. I
> > > > don't have access to one and it seems Maxim have discontinued them.
> > > > 
> > > 
> > > Do you have a somewhat reliable means to reproduce the problem ?
> > 
> > It seems we hit a bunch of problems by just continually
> > binding/unbinding the driver, if you don't apply that hacky oneshot
> > retry patch. We can hit problems (in our design?) with something like:
> > 
> > # cd /sys/bus/i2c/drivers/max31785; \
> > echo $addr > unbind; \
> > while echo $addr > bind; \
> > do echo $addr > unbind; echo -n .; done;
> > 
> > It should hit issues covered by this patch, as the register checks are
> > used in the operations used by probe.
> > 
> 
> Hmm ... I didn't use your driver but my prototype driver which also supports
> temperature and voltage attributes, so if anything it should create more
> stress on the chip.

I did add the temp and voltage attributes...

Any chance you can give mine a try? I don't know what I would have done
to invoke this kind of behaviour, so it would be useful to know whether
or not it happens with one driver but not the other.

>  No error so far, after running the script for a couple
> of minutes. How long does it take for errors to appear, and how do I see
> that there is an error ? 

I'm seeing failures after anything from a handful of bind/unbinds, to
hundreds of bind/unbinds. It seems to vary. 

> Does the driver fail to instantiate ?

Typically probe fails so the loop exits. It usually gets -EIO and the
shell spits out "No such device".

Thanks for testing, it's a useful data point for us hunting down the
source of our problems.

Andrew

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Thu, 2017-09-07 at 06:40 -0700, Guenter Roeck wrote:
> On 09/06/2017 04:32 PM, Andrew Jeffery wrote:
> 
> > >   
> > > Guess I need to dig up my eval board and see if I can reproduce the 
> > > problem.
> > > Seems you are saying that the problem is always seen when issuing a 
> > > sequence
> > > of "clear faults" commands on multiple pages ?
> > 
> > Yeah. We're also seeing bad behaviour under other command sequences as well,
> > which lead to this hack of a work-around patch[1].
> > 
> > I'd be very interested in the results of testing against the eval board. I
> > don't have access to one and it seems Maxim have discontinued them.
> > 
> 
> Do you have a somewhat reliable means to reproduce the problem ?

It seems we hit a bunch of problems by just continually
binding/unbinding the driver, if you don't apply that hacky oneshot
retry patch. We can hit problems (in our design?) with something like:

# cd /sys/bus/i2c/drivers/max31785; \
echo $addr > unbind; \
while echo $addr > bind; \
do echo $addr > unbind; echo -n .; done;

It should hit issues covered by this patch, as the register checks are
used in the operations used by probe.

Andrew

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

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Wed, 2017-09-06 at 15:51 -0700, Guenter Roeck wrote:
> On Wed, Sep 06, 2017 at 10:23:37AM +1000, Andrew Jeffery wrote:
> > On Tue, 2017-09-05 at 10:00 -0700, Guenter Roeck wrote:
> > > On Tue, Sep 05, 2017 at 05:01:32PM +1000, Andrew Jeffery wrote:
> > > > Some functions exposed by pmbus core conflated errors that occurred when
> > > > setting the page to access with errors that occurred when accessing
> > > > registers in a page. In some cases, this caused legitimate errors to be
> > > > hidden under the guise of the register not being supported.
> > > > 
> > > 
> > > I'll have to look into this. If I recall correctly, the reason for not 
> > > returning
> > > errors from the "clear faults" command was that some early chips did not 
> > > support
> > > it, or did not support it on all pages. Those chips would now always fail.
> > 
> > Yes, that is a concern.
> > 
> > However, shouldn't the lack of support for CLEAR_FAULTS be a described
> > property of the chip or page?
> > 
> > Regardless, the issue here is the PAGE command is sometimes failing
> > (more below). This means we won't have issued a CLEAR_FAULTS against
> > the page even if the page supports CLEAR_FAULTS. That to me is
> > something that should be propagated back up the call chain, as faults
> > that can be cleared will not have been.
> > 
> 
> We could also consider
> - retry

I guess that leads to the usual retries problem: How many? Oneshot? More?

My expectation is that oneshot would be enough, but we'd still need to consider
what to do if that wasn't successful.

Does the retry policy need to be in the kernel? I guess if it's not we would
need all operations in the path to the failure to be idempotent.

> - Add a marker indicating that faults (specifically command errors)
>   are unreliable after clearing faults failed

What kind of marker were you thinking? Something in dmesg? If it's anything
else we'd probably want something to respond to the condition, which nothing
would do currently.

> 
> If we can't reliably clear faults, all bets are off.

Yeah, it's a messy problem. However even if we resolve our issues in hardware
(i.e. it's discovered that it is a design failure or something), I don't think
we should drop a resolution to the problem highlighted by this patch.

> 
> > > 
> > > On a higher level, I am not sure if it is a good idea to return an error
> > > from a function intended to clear faults (and nothing else). That seems
> > > counterproductive.
> > > 
> > > Is this a problem you have actually observed ? 
> > 
> > Unfortunately yes. I was trying to reproduce some issues that we are
> > seeing in userspace but wasn't having much luck (getting -EIO on
> > reading a hwmon attribute). I ended up instrumenting our I2C bus
> > driver, and found that the problem was more prevalent than what the
> > read failures in userspace were indicating. The paths that were
> > triggering these unreported conditions all traced through the check
> > register and clear fault functions, which on analysis were swallowing
> > the error.
> > 
> > > If so, what is the chip ?
> > 
> > Well, the chip that I'm *communicating* with is the Maxim MAX31785
> > Intelligent Fan Controller. As to whether that's what is *causing* the
> > PAGE command to fail is still up in the air. I would not be surprised
> > if we have other issues in the hardware design.
> > 
> > I haven't sent a follow-up to the series introducing the MAX31785
> > driver because I've been chasing down communication issues. I felt it
> > was important to understand whether the device has quirks that need to
> > be worked around in the driver, or if our hardware design has bigger
> > problems that should be handled in other ways. I've been in touch with
> > Maxim who have asserted that some of the problems we're seeing cannot
> > be caused by their chip.
> > 
> 
> Guess I need to dig up my eval board and see if I can reproduce the problem.
> Seems you are saying that the problem is always seen when issuing a sequence
> of "clear faults" commands on multiple pages ?

Yeah. We're also seeing bad behaviour under other command sequences as well,
which lead to this hack of a work-around patch[1].

I'd be very interested in the results of testing against the eval board. I
don't have access to one and it seems Maxim have discontinued them.

[1] https://patchwork.kernel.org/patch/9876083/

> 
> The MAX31785 is microcode based, so I would not be entirely surprised if
> it sometimes fails to reply if a sequence of 
> comma

Re: [PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

On Tue, 2017-09-05 at 10:00 -0700, Guenter Roeck wrote:
> On Tue, Sep 05, 2017 at 05:01:32PM +1000, Andrew Jeffery wrote:
> > Some functions exposed by pmbus core conflated errors that occurred when
> > setting the page to access with errors that occurred when accessing
> > registers in a page. In some cases, this caused legitimate errors to be
> > hidden under the guise of the register not being supported.
> > 
> 
> I'll have to look into this. If I recall correctly, the reason for not 
> returning
> errors from the "clear faults" command was that some early chips did not 
> support
> it, or did not support it on all pages. Those chips would now always fail.

Yes, that is a concern.

However, shouldn't the lack of support for CLEAR_FAULTS be a described
property of the chip or page?

Regardless, the issue here is the PAGE command is sometimes failing
(more below). This means we won't have issued a CLEAR_FAULTS against
the page even if the page supports CLEAR_FAULTS. That to me is
something that should be propagated back up the call chain, as faults
that can be cleared will not have been.

> 
> On a higher level, I am not sure if it is a good idea to return an error
> from a function intended to clear faults (and nothing else). That seems
> counterproductive.
> 
> Is this a problem you have actually observed ? 

Unfortunately yes. I was trying to reproduce some issues that we are
seeing in userspace but wasn't having much luck (getting -EIO on
reading a hwmon attribute). I ended up instrumenting our I2C bus
driver, and found that the problem was more prevalent than what the
read failures in userspace were indicating. The paths that were
triggering these unreported conditions all traced through the check
register and clear fault functions, which on analysis were swallowing
the error.

> If so, what is the chip ?

Well, the chip that I'm *communicating* with is the Maxim MAX31785
Intelligent Fan Controller. As to whether that's what is *causing* the
PAGE command to fail is still up in the air. I would not be surprised
if we have other issues in the hardware design.

I haven't sent a follow-up to the series introducing the MAX31785
driver because I've been chasing down communication issues. I felt it
was important to understand whether the device has quirks that need to
be worked around in the driver, or if our hardware design has bigger
problems that should be handled in other ways. I've been in touch with
Maxim who have asserted that some of the problems we're seeing cannot
be caused by their chip.

Andrew

> 
> Thanks,
> Guenter
> 
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  Documentation/hwmon/pmbus-core   |  12 ++--
> >  drivers/hwmon/pmbus/pmbus.h  |   6 +-
> >  drivers/hwmon/pmbus/pmbus_core.c | 115 
> > +--
> >  3 files changed, 95 insertions(+), 38 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/pmbus-core b/Documentation/hwmon/pmbus-core
> > index 8ed10e9ddfb5..3e9f41bb756f 100644
> > --- a/Documentation/hwmon/pmbus-core
> > +++ b/Documentation/hwmon/pmbus-core
> > @@ -218,17 +218,17 @@ Specifically, it provides the following information.
> >    This function calls the device specific write_byte function if defined.
> >    Therefore, it must _not_ be called from that function.
> >  
> > -  bool pmbus_check_byte_register(struct i2c_client *client, int page, int 
> > reg);
> > +  int pmbus_check_byte_register(struct i2c_client *client, int page, int 
> > reg);
> >  
> > -  Check if byte register exists. Return true if the register exists, false
> > -  otherwise.
> > +  Check if byte register exists. Returns 1 if the register exists, 0 if it 
> > does
> > +  not, and less than zero on an unexpected error.
> >    This function calls the device specific write_byte function if defined to
> >    obtain the chip status. Therefore, it must _not_ be called from that 
> > function.
> >  
> > -  bool pmbus_check_word_register(struct i2c_client *client, int page, int 
> > reg);
> > +  int pmbus_check_word_register(struct i2c_client *client, int page, int 
> > reg);
> >  
> > -  Check if word register exists. Return true if the register exists, false
> > -  otherwise.
> > +  Check if word register exists. Returns 1 if the register exists, 0 if it 
> > does
> > +  not, and less than zero on an unexpected error.
> >    This function calls the device specific write_byte function if defined to
> >    obtain the chip status. Therefore, it must _not_ be called from that 
> > function.
> >  
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > in

[PATCH] hwmon: pmbus: Make reg check and clear faults functions return errors

[Andrew Jeffery]

Some functions exposed by pmbus core conflated errors that occurred when
setting the page to access with errors that occurred when accessing
registers in a page. In some cases, this caused legitimate errors to be
hidden under the guise of the register not being supported.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 Documentation/hwmon/pmbus-core   |  12 ++--
 drivers/hwmon/pmbus/pmbus.h  |   6 +-
 drivers/hwmon/pmbus/pmbus_core.c | 115 +--
 3 files changed, 95 insertions(+), 38 deletions(-)

diff --git a/Documentation/hwmon/pmbus-core b/Documentation/hwmon/pmbus-core
index 8ed10e9ddfb5..3e9f41bb756f 100644
--- a/Documentation/hwmon/pmbus-core
+++ b/Documentation/hwmon/pmbus-core
@@ -218,17 +218,17 @@ Specifically, it provides the following information.
   This function calls the device specific write_byte function if defined.
   Therefore, it must _not_ be called from that function.
 
-  bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
+  int pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 
-  Check if byte register exists. Return true if the register exists, false
-  otherwise.
+  Check if byte register exists. Returns 1 if the register exists, 0 if it does
+  not, and less than zero on an unexpected error.
   This function calls the device specific write_byte function if defined to
   obtain the chip status. Therefore, it must _not_ be called from that 
function.
 
-  bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
+  int pmbus_check_word_register(struct i2c_client *client, int page, int reg);
 
-  Check if word register exists. Return true if the register exists, false
-  otherwise.
+  Check if word register exists. Returns 1 if the register exists, 0 if it does
+  not, and less than zero on an unexpected error.
   This function calls the device specific write_byte function if defined to
   obtain the chip status. Therefore, it must _not_ be called from that 
function.
 
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..c53032a04a6f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -413,9 +413,9 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
-void pmbus_clear_faults(struct i2c_client *client);
-bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
-bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
+int pmbus_clear_faults(struct i2c_client *client);
+int pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
+int pmbus_check_word_register(struct i2c_client *client, int page, int reg);
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
   struct pmbus_driver_info *info);
 int pmbus_do_remove(struct i2c_client *client);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f1eff6b6c798..153700e35431 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -304,18 +304,24 @@ static int _pmbus_read_byte_data(struct i2c_client 
*client, int page, int reg)
return pmbus_read_byte_data(client, page, reg);
 }
 
-static void pmbus_clear_fault_page(struct i2c_client *client, int page)
+static int pmbus_clear_fault_page(struct i2c_client *client, int page)
 {
-   _pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
+   return _pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
 }
 
-void pmbus_clear_faults(struct i2c_client *client)
+int pmbus_clear_faults(struct i2c_client *client)
 {
struct pmbus_data *data = i2c_get_clientdata(client);
+   int rv;
int i;
 
-   for (i = 0; i < data->info->pages; i++)
-   pmbus_clear_fault_page(client, i);
+   for (i = 0; i < data->info->pages; i++) {
+   rv = pmbus_clear_fault_page(client, i);
+   if (rv)
+   return rv;
+   }
+
+   return 0;
 }
 EXPORT_SYMBOL_GPL(pmbus_clear_faults);
 
@@ -333,28 +339,45 @@ static int pmbus_check_status_cml(struct i2c_client 
*client)
return 0;
 }
 
-static bool pmbus_check_register(struct i2c_client *client,
+static int pmbus_check_register(struct i2c_client *client,
 int (*func)(struct i2c_client *client,
 int page, int reg),
 int page, int reg)
 {
+   struct pmbus_data *data;
+   int check;
int rv;
-   struct pmbus_data *data = i2c_get_clientdata(client);
 
-   rv = func(client, page, reg);
-   if (rv >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK))
-   rv = pmbus_check_status_cml(clie

Re: [PATCH v2 4/4] pmbus: max31785: Work around back-to-back writes with FAN_CONFIG_1_2

[Andrew Jeffery]

On Wed, 2017-08-02 at 16:45 +0930, Andrew Jeffery wrote:
> Testing of the pmbus max31785 driver implementation revealed occasional
> NACKs from the device. Attempting the same transaction immediately after
> the failure appears to always succeed. The NACK has consistently been
> observed to happen on the second write of back-to-back writes to the
> device, where the first write is to FAN_CONFIG_1_2. The NACK occurs
> against the first data byte transmitted by the master on the second
> write. The behaviour has the hallmarks of a PMBus Device Busy response,
> but the busy bit is not set in the status byte.
> 
> Work around this undocumented behaviour by retrying any back-to-back
> writes that occur after first writing FAN_CONFIG_1_2.
> 
> Signed-off-by: Andrew Jeffery <and...@aj.id.au>

I expect I'll be dropping this patch. At this point it looks like we
have another chip on the bus interfering with transactions to the
MAX31785. Checking the behaviour with a scope showed that SCL was being
held down by something that wasn't the master, but according to Maxim
the SCL pin on the MAX31785 is input-only and therefore it cannot
interfere in the manner we observed. Further testing has narrowed down
the list of candidates for the interference, but our investigation is
ongoing.

Andrew
 
> ---
>  drivers/hwmon/pmbus/max31785.c | 105 
> +
>  1 file changed, 97 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index c2b693badcea..509b1a5a49b9 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -48,6 +48,55 @@ enum max31785_regs {
>  
> >  #define MAX31785_NR_PAGES  23
>  
> +/*
> + * MAX31785 dragons ahead
> + *
> + * It seems there's an undocumented timing constraint when performing
> + * back-to-back writes where the first write is to FAN_CONFIG_1_2. The device
> + * provides no indication of this besides NACK'ing master Txs; no bits are 
> set
> + * in STATUS_BYTE to suggest anything has gone wrong.
> + *
> + * Given that, do a one-shot retry of the write.
> + *
> + * The max31785_*_write_*_data() functions should be used at any point where
> + * the prior write is to FAN_CONFIG_1_2.
> + */
> +static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client,
> > +     int command, u16 data)
> +{
> > +   int ret;
> +
> > +   ret = i2c_smbus_write_byte_data(client, command, data);
> > +   if (ret == -EIO)
> > +   ret = i2c_smbus_write_byte_data(client, command, data);
> +
> > +   return ret;
> +}
> +
> +static int max31785_i2c_smbus_write_word_data(struct i2c_client *client,
> > +     int command, u16 data)
> +{
> > +   int ret;
> +
> > +   ret = i2c_smbus_write_word_data(client, command, data);
> > +   if (ret == -EIO)
> > +   ret = i2c_smbus_write_word_data(client, command, data);
> +
> > +   return ret;
> +}
> +
> +static int max31785_pmbus_write_word_data(struct i2c_client *client, int 
> page,
> > +     int command, u16 data)
> +{
> > +   int ret;
> +
> > +   ret = pmbus_write_word_data(client, page, command, data);
> > +   if (ret == -EIO)
> > +   ret = pmbus_write_word_data(client, page, command, data);
> +
> > +   return ret;
> +}
> +
>  static int max31785_read_byte_data(struct i2c_client *client, int page,
> >        int reg)
>  {
> @@ -210,6 +259,31 @@ static int max31785_read_word_data(struct i2c_client 
> *client, int page,
> >     return rv;
>  }
>  
> +static int max31785_update_fan(struct i2c_client *client, int page,
> > +      u8 config, u8 mask, u16 command)
> +{
> > +   int from, rv;
> > +   u8 to;
> +
> > +   from = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
> > +   if (from < 0)
> > +   return from;
> +
> > +   to = (from & ~mask) | (config & mask);
> +
> > +   if (to != from) {
> > +   rv = pmbus_write_byte_data(client, page, PMBUS_FAN_CONFIG_12,
> > +      to);
> > +   if (rv < 0)
> > +   return rv;
> > +   }
> +
> > +   rv = max31785_pmbus_write_word_data(client, page, PMBUS_FAN_COMMAND_1,
> > +   command);
> +
> > +   return rv;
> +}
> +
>  static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0x };
>  
>  static int max31785_write_word_data(struct i2c_client *client, int pag

Re: [PATCH v2 1/4] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

On Thu, 2017-08-10 at 11:13 -0500, Rob Herring wrote:
> On Wed, Aug 02, 2017 at 04:45:38PM +0930, Andrew Jeffery wrote:
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 126 
> > +
> >  1 file changed, 126 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt 
> > b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > new file mode 100644
> > index ..8ddc4ea1958d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
> > @@ -0,0 +1,126 @@
> > +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> > +==
> 
> This is the second fan controller binding I've seen recently. The other 
> being hwmon/aspeed-pwm-tacho.txt. I think some of this needs to be 
> common. There's only so many types of fans, right?

Heh, you'd think so, I'll take a look. However much of this is driven by the
PMBus specification and the Aspeed PWM/Tacho isn't a PMBus-based device.

I was hesitant to start a generic PMBus bindings document without having more
PMBus devices with bindings, but maybe that's the way I should go? It would
make clear what's from the fan-control parts of the PMBus specification and
what's here for the purpose of supporting the MAX31785.

> 
> And we have the thermal binding which you don't appear to tie into.

I'll look into that also.

> 
> > +
> > +Reference:
> > +[1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> > +
> > +Required properties:
> > +- compatible : One of "maxim,max31785" or "maxim,max31785a"
> > +- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
> > +- #address-cells : Must be 1
> > +- #size-cells: Must be 0
> > +
> > +Optional properties:
> > +- use-stored-presence: Do not treat the devicetree description as 
> > canon for
> 
> Is this maxim specific? If so, needs a vendor prefix.

PMBus specifies two 8-bit registers of the same structure: FAN_CONFIG_1_2
and FAN_CONFIG_3_4. It's not intended to be manufacturer-specific.

A bit in each nibble of FAN_CONFIG_* is specified as marking whether or not the
fan is installed. The intent of this property is to define whether the consumer
should consider the devicetree as canonical, or the device. In the absence of
the property consumer of the node should mark fans described in the devicetree
as installed (set the bit, and clear the bit for any fan pages that are not
described in the devicetree). Alternatively, the device may be pre-programmed
with a default register value that is suitable for the system the controller
resides in, in which case the devicetree consumer should itself consume the
installed bit from the device rather than set/clear it.

The two remaining fields in FAN_CONFIG_*, fan mode (RPM/PWM) and
tach-pulses-per-revolution (1-4), are covered by optional properties below.

> 
> > +   fan presence (the 'installed' bit of 
> > FAN_CONFIG_*).
> > +   Instead, rely on the on the default value store 
> > of
> > +   the device to populate it.
> > +
> > +Capabilities are configured through subnodes of the controller's node.
> > +
> > +Fans
> > +
> > +
> > +Only fans with subnodes present will be considered as installed. If
> > +use-stored-presence is present in the parent node, then only fans that are 
> > both
> > +defined in the devicetree and have their installed bit set are considered
> > +installed.
> > +
> > +Required subnode properties:
> > +- compatible : Must be "pmbus-fan"
> 
> What defines a pmbus-fan? Sounds generic, but then you have all these 
> maxim specific properties.

It's driven by the two optional properties, fan-mode and tach-pulses, which
make up the remaining fields of the FAN_CONFIG_* registers from the PMBus
specification. PMBus reserves command ranges for manufacturer-specific use, and
Maxim chose to use one of these reserved commands as MFR_FAN_CONFIG
(Manufacturer-specific Fan Configuration). The vendor-prefixed properties deal
with the properties described in the 16-bit MFR_FAN_CONFIG register.

> 
> > +- reg: The PMBus page the properties apply to.
> > +- maxim,fan-rotor-input  : The type of rotor measurement provided to the
> > +   controller. Must be either "tach" for tachometer
> > + 

[PATCH v2 2/4] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The virtual registers facilitate the necessary side-effects of each
access. Examples of the read case, assuming m = 1, b = 0, R = 0:

 Read |  With  || Gives
 ---
   Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
 --++---
  fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
  pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
  pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
  fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
  pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
  pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1

And the write case:

 Write| With  ||   Sets
 -+---+++---
   Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
 -+---+++---
  fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
  pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
  pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  29 +
 drivers/hwmon/pmbus/pmbus_core.c | 224 ---
 2 files changed, 238 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..a863b8fed16f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,28 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /*
+* RPM and PWM Fan control
+*
+* Drivers wanting to expose PWM control must define the behaviour of
+* PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
+*
+* pmbus core provides default implementations for
+* PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
+*/
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +245,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34BIT(17)
 #define PMBUS_HAVE_VMONBIT(18)
 #define PMBUS_HAVE_STATUS_VMON BIT(19)
+#define PMBUS_HAVE_PWM12   BIT(20)
+#define PMBUS_HAVE_PWM34   BIT(21)
 
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
@@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+u8 config, u8 mask, u16 command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ba59eaef2e07..93fb58787b99 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -63,6 +63,7 @@ struct pmbus_sensor {
u

[PATCH v2 1/4] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 126 +
 1 file changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt 
b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
new file mode 100644
index ..8ddc4ea1958d
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
@@ -0,0 +1,126 @@
+Bindings for the Maxim MAX31785 Intelligent Fan Controller
+==
+
+Reference:
+[1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+Required properties:
+- compatible : One of "maxim,max31785" or "maxim,max31785a"
+- reg: I2C address, one of 0x52, 0x53, 0x54, 0x55.
+- #address-cells : Must be 1
+- #size-cells: Must be 0
+
+Optional properties:
+- use-stored-presence: Do not treat the devicetree description as canon for
+   fan presence (the 'installed' bit of FAN_CONFIG_*).
+   Instead, rely on the on the default value store of
+   the device to populate it.
+
+Capabilities are configured through subnodes of the controller's node.
+
+Fans
+
+
+Only fans with subnodes present will be considered as installed. If
+use-stored-presence is present in the parent node, then only fans that are both
+defined in the devicetree and have their installed bit set are considered
+installed.
+
+Required subnode properties:
+- compatible : Must be "pmbus-fan"
+- reg: The PMBus page the properties apply to.
+- maxim,fan-rotor-input  : The type of rotor measurement provided to the
+   controller. Must be either "tach" for tachometer
+   pulses or "lock" for a locked-rotor signal.
+- maxim,fan-lock-polarity: Required iff maxim,fan-rotor-input is "lock". Valid
+   values are "low" for active low, "high" for active
+   high.
+
+Optional subnode properties:
+- fan-mode   : "rpm" or "pwm". Default value is "pwm".
+- tach-pulses: Tachometer pulses per revolution. Valid values are
+   1, 2, 3 or 4. The default is 1.
+- maxim,fan-no-fault-ramp: Do not ramp the fan to 100% PWM duty on detecting a
+   fan fault
+- maxim,fan-startup  : The number of rotations required before taking
+   emergency action for an unresponsive fan and driving
+   it with 100% or 0% PWM duty, depending on the state
+   of maxim,fan-no-fault-ramp. Valid values are 0
+   (automatic spin-up disabled), 2, 4, or 8. Default
+   value is 0.
+- maxim,fan-health   : Enable automated fan health check
+- maxim,fan-ramp : Configures how fast the device ramps the PWM duty
+   cycle from one value to another. Valid values are 0
+   to 7 inclusive, with values 0 - 2 configuring a
+   1000ms update rate and 1 - 3% duty respective duty
+   increase, and 3 - 7 a 200ms update rate with a 1 -
+   5% respective duty increase. Default value is 0.
+- maxim,fan-no-watchdog  : Do not ramp fan to 100% PWM duty on failure to
+   update desired fan rate inside 10s. This implies
+   maxim,tmp-no-fault-ramp
+- maxim,tmp-no-fault-ramp: Do not ramp fan to 100% PWM duty on temperature
+   sensor fault detection. This implies
+   maxim,fan-no-watchdog
+- maxim,tmp-hysteresis   : The temperature hysteresis used to determine
+   transitions to lower fan speed bands in the
+   temperature/fan rate lookup table. Valid values are
+   2, 4, 6 or 8 (degrees celcius). Default value is 2.
+- maxim,fan-dual-tach: Enable dual tachometer functionality
+- maxim,fan-pwm-freq : The PWM frequency. Valid values are 30, 50, 100, 150
+   and 25000 (Hz). Default value is 30Hz.
+- maxim,fan-lookup-table : A 16-element cell array of alternating temperature
+   and rate values representing the look up table. The
+   rate units are set through the fan-mode property.
+- maxim,fan-fault-pin-mon: Ramp fans to 100% PWM duty when the FAULT pin is
+   asserted
+
+Temperature
+---
+
+Required subnode properties:
+- compatible: Must be "pmbus-temperature"
+- reg   : The PMBus pag

[PATCH v2 3/4] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Andrew Jeffery]

The Maxim MAX31785 is a PMBus device providing closed-loop,
multi-channel fan management with temperature and remote voltage
sensing. Various fan control features are provided, including PWM
frequency control, temperature hysteresis, dual tachometer measurements,
and fan health monitoring.

The driver implementation makes use of the new fan control virtual
registers exposed by the pmbus core. It mixes use of the default
implementations with some overrides via the read/write handlers to
handle FAN_COMMAND_1 on the MAX31785, whose definition breaks the value
range into various control bands dependent on RPM or PWM 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 provides the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 673 +
 3 files changed, 684 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 ..c2b693badcea
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,673 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+enum max31785_regs {
+   MFR_REVISION= 0x9b,
+   MFR_FAULT_RESPONSE  = 0xd9,
+   MFR_TEMP_SENSOR_CONFIG  = 0xf0,
+   MFR_FAN_CONFIG  = 0xf1,
+   MFR_READ_FAN_PWM= 0xf3,
+   MFR_FAN_FAULT_LIMIT = 0xf5,
+   MFR_FAN_WARN_LIMIT  = 0xf6,
+   MFR_FAN_PWM_AVG = 0xf8,
+};
+
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_TEMP_SENSOR_CONFIG_ENABLE  BIT(15)
+#define MFR_TEMP_SENSOR_CONFIG_OFFSET  GENMASK(14, 10)
+
+#define MFR_FAN_CONFIG_FREQGENMASK(15, 13)
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+#define MFR_FAN_CONFIG_HYS GENMASK(11, 10)
+#define MFR_FAN_CONFIG_TSFOBIT(9)
+#define MFR_FAN_CONFIG_TACHO   BIT(8)
+#define MFR_FAN_CONFIG_RAMPGENMASK(7, 5)
+#define MFR_FAN_CONFIG_HEALTH  BIT(4)
+#define MFR_FAN_CONFIG_ROTOR_HI_LO BIT(3)
+#define MFR_FAN_CONFIG_ROTOR   BIT(2)
+#define MFR_FAN_CONFIG_SPINGENMASK(1, 0)
+
+#define MFR_FAULT_RESPONSE_MONITOR BIT(0)
+
+#define MAX31785_CAP_READ_DUAL_TACHBIT(0)
+
+#define MAX31785_NR_PAGES  23
+
+static int max31785_read_byte_data

[PATCH v2 0/4] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

v1[1] spent some time in the OpenBMC kernel tree and it shook out a few issues:

1. The machines I was testing against had pre-prammed the installed-bit in
   FAN_CONFIG_1_2
2. There appears to be a hardware issue with some back-to-back writes to the
   MAX31785

Point 1. is a policy issue so we should have the ability to specify behaviour
in the devicetree. As such the bindings documentation has been updated to suit.

Point 2, well, it's a little ugly. We're in contact with Maxim to better
understand the nature of the issue. Patch 4 introduces a work-around. I've kept
this separate from the introduction of the driver so it's implementation and
the demonstration of new pmbus core features isn't obscured. The work-around
has proved reliable so-far, across a number of machines.

[1] https://lkml.org/lkml/2017/7/27/116

Changes from v1 to v2:

* Add the use-stored-presence devicetree property describing the hardware as
  canonical in terms of fan presence.
* Small optimsation in pmbus_update_fans(): Don't write FAN_CONFIG_x_y unless
  the new value is different to the current value
* Update the MAX31785 driver to consume use-stored-presence from the devicetree
* Add the back-to-back write work-around

Changes between RFC v2 to (non-RFC) v1

* Clean up issues identified in comments on the pmbus core changes 
* Define and implement bindings for the MAX31785
* Clean up issues identified in comments on the max31785 driver

Andrew Jeffery (4):
  dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation
  hwmon: pmbus: Add fan control support
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller
  pmbus: max31785: Work around back-to-back writes with FAN_CONFIG_1_2

 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 126 
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 762 +
 drivers/hwmon/pmbus/pmbus.h|  29 +
 drivers/hwmon/pmbus/pmbus_core.c   | 224 +-
 6 files changed, 1137 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.

[Andrew Jeffery]

On Thu, 2017-07-27 at 08:36 +, Mykola Kostenok wrote:
> > -Original Message-
> > > > From: Andrew Jeffery [mailto:and...@aj.id.au]
> > Sent: Thursday, July 27, 2017 9:01 AM
> > > > To: Mykola Kostenok <c_myko...@mellanox.com>; Joel Stanley
> > > > <j...@jms.id.au>
> > > > Cc: linux-hwmon@vger.kernel.org; Jaghathiswari Rankappagounder
> > > > > > Natarajan <ja...@google.com>; Jean Delvare <jdelv...@suse.com>; Ohad
> > > > > > Oz <oh...@mellanox.com>; Vadim Pasternak <vad...@mellanox.com>;
> > > > Patrick Venture <vent...@google.com>; OpenBMC Maillist
> > > > > > <open...@lists.ozlabs.org>; Rob Herring <robh...@kernel.org>; 
> > > > > > Guenter
> > > > Roeck <li...@roeck-us.net>
> > Subject: Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.
> > 
> > Hi Mykola,
> > 
> > I know you've sent out subsequent versions, but I wanted to address one of
> > your arguments here:
> > 
> > On Wed, 2017-07-26 at 11:08 +, Mykola Kostenok wrote:
> > > > > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct
> > > > > platform_device *pdev)
> > > > > 
> > > > >  for_each_child_of_node(np, child) {
> > > > >  ret = aspeed_create_fan(dev, child, priv);
> > > > > -   of_node_put(child);
> > > > > -   if (ret)
> > > > > +   if (ret) {
> > > > > +   of_node_put(child);
> > > > >  return ret;
> > > > > +   }
> > > > >  }
> > > > > +   of_node_put(child);
> > > > 
> > > > I'm not sure about these.
> > > > 
> > > > Cheers,
> > > > 
> > > > Joel
> > > 
> > > If CONFIG_OF_UNITTEST is set, system initialization  fails on this
> > 
> > of_node_put.
> > > I checked and found that for_each_child_of_node is macro witch use
> > > __of_get_next_child
> > >  in cycle. __of_get_next_child do of_node_put previous child but not last.
> > > 
> > > static struct device_node *__of_get_next_child(const struct
> > > device_node *node,
> > > struct device_node
> > > *prev) {
> > > struct device_node *next;
> > > 
> > > if (!node)
> > > return NULL;
> > > 
> > > next = prev ? prev->sibling : node->child;
> > > for (; next; next = next->sibling)
> > > if (of_node_get(next))
> > > break;
> > > of_node_put(prev);
> > > return next;
> > > }
> > > #define __for_each_child_of_node(parent, child) \
> > > for (child = __of_get_next_child(parent, NULL); child != NULL;
> > > \
> > >  child = __of_get_next_child(parent, child))
> > > 
> > > So inside cycle we should not use of_node_put on each child. We must use
> > 
> > it only on last child in cycle.
> > 
> > I was just looking at this myself for a different driver, and I don't think 
> > this
> > argument is right. The natural terminating condition of the loop is child ==
> > NULL. child == NULL occurs if we have zero-or-more- children; the child is
> > always initialised as part of the loop header and would be NULL if there are
> > no children. If we have more than one child, the reference to the last valid
> > child is passed to of_node_put() in __of_get_next_child() in order to return
> > the terminating NULL. Given
> > __of_get_next_child() is passed the last node and the result is NULL, the 
> > call
> > to of_node_put() after the loop is always invoked on NULL, which performs
> > an early return.
> > 
> > As such I think it is unnecessary.
> > 
> > Cheers,
> > 
> > Andrew
> 
> Ok, I agree that we don’t need of_node_put after loop. 
> We must do of_node_put only in case of error.

Yep, this is the right approach as far as I can see.

Thanks,

Andrew

> 
> So I will send next patch v6 with this:
>    for_each_child_of_node(np, child) {
>    ret = aspeed_create_fan(dev, child, priv);
>   -   of_node_put(child);
>   -   if (ret)
>  +   if (ret) {
>  +  

[PATCH 2/3] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The facilitate the necessary side-effects of each access. Examples of
the read case, assuming m = 1, b = 0, R = 0:

 Read |  With  || Gives
 ---
   Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
 --++---
  fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
  pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
  pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
  fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
  pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
  pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1

And the write case:

 Write| With  ||   Sets
 -+---+++---
   Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
 -+---+++---
  fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
  pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
  pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  29 +
 drivers/hwmon/pmbus/pmbus_core.c | 222 ---
 2 files changed, 236 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..a863b8fed16f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,28 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /*
+* RPM and PWM Fan control
+*
+* Drivers wanting to expose PWM control must define the behaviour of
+* PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
+*
+* pmbus core provides default implementations for
+* PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
+*/
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +245,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34BIT(17)
 #define PMBUS_HAVE_VMONBIT(18)
 #define PMBUS_HAVE_STATUS_VMON BIT(19)
+#define PMBUS_HAVE_PWM12   BIT(20)
+#define PMBUS_HAVE_PWM34   BIT(21)
 
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
@@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+u8 config, u8 mask, u16 command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ba59eaef2e07..3c8bcbdf1d88 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -63,6 +63,7 @@ struct pmbus_sensor {
u

[PATCH 3/3] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Andrew Jeffery]

The Maxim MAX31785 is a PMBus device providing closed-loop,
multi-channel fan management with temperature and remote voltage
sensing. Various fan control features are provided, including PWM
frequency control, temperature hysteresis, dual tachometer measurements,
and fan health monitoring.

The driver implementation makes use of the new fan control virtual
registers exposed by the pmbus core. It mixes use of the default
implementations with some overrides via the read/write handlers to
handle FAN_COMMAND_1 on the MAX31785, whose definition breaks the value
range into various control bands dependent on RPM or PWM mode.

The dual tachometer feature is implemented in hardware with a TACHSEL
line 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 provides the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 670 +
 3 files changed, 681 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 ..1c2648ce5322
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,670 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+enum max31785_regs {
+   MFR_REVISION= 0x9b,
+   MFR_FAULT_RESPONSE  = 0xd9,
+   MFR_TEMP_SENSOR_CONFIG  = 0xf0,
+   MFR_FAN_CONFIG  = 0xf1,
+   MFR_READ_FAN_PWM= 0xf3,
+   MFR_FAN_FAULT_LIMIT = 0xf5,
+   MFR_FAN_WARN_LIMIT  = 0xf6,
+   MFR_FAN_PWM_AVG = 0xf8,
+};
+
+#define MAX31785   0x3030
+#define MAX31785A  0x3040
+
+#define MFR_TEMP_SENSOR_CONFIG_ENABLE  BIT(15)
+#define MFR_TEMP_SENSOR_CONFIG_OFFSET  GENMASK(14, 10)
+
+#define MFR_FAN_CONFIG_FREQGENMASK(15, 13)
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+#define MFR_FAN_CONFIG_HYS GENMASK(11, 10)
+#define MFR_FAN_CONFIG_TSFOBIT(9)
+#define MFR_FAN_CONFIG_TACHO   BIT(8)
+#define MFR_FAN_CONFIG_RAMPGENMASK(7, 5)
+#define MFR_FAN_CONFIG_HEALTH  BIT(4)
+#define MFR_FAN_CONFIG_ROTOR_HI_LO BIT(3)
+#define MFR_FAN_CONFIG_ROTOR   BIT(2)
+#define MFR_FAN_CONFIG_SPINGENMASK(1, 0)
+
+#define MFR_FAULT_RESPONSE_MONITOR BIT(0)
+
+#define MAX31785_CAP_READ_DUAL_TACHBIT(0)
+
+#define MAX31785_NR_PAGES  23
+
+static int max31785_read_byte_data

[PATCH 0/3] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

Previously I'd posted a second RFC series implementing fan control support in
PMBus core and adding a driver for the MAX31785 fan controller:

  https://www.spinics.net/lists/kernel/msg2558128.html

I think the core changes have settled enough to move the effort out of RFC-land.

This series is some modest refinements on the core based on previous feedback,
and polishes the driver implementation by defining and implementing devicetree
bindings for the controller.

The series has been tested on a number of IBM Witherspoon systems, which
contain the fan controller of interest.

Please review!

Andrew

Changes between RFC v2 to (non-RFC) v1

* Clean up issues identified in comments on the pmbus core changes 
* Define and implement bindings for the MAX31785
* Clean up issues identified in comments on the max31785 driver

Andrew Jeffery (3):
  dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation
  hwmon: pmbus: Add fan control support
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 117 
 drivers/hwmon/pmbus/Kconfig|  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 670 +
 drivers/hwmon/pmbus/pmbus.h|  29 +
 drivers/hwmon/pmbus/pmbus_core.c   | 222 ++-
 6 files changed, 1034 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v5] hwmon: (aspeed-pwm-tacho) cooling device support.

[Andrew Jeffery]

On Wed, 2017-07-26 at 17:52 +0300, Mykola Kostenok wrote:
> Add support in aspeed-pwm-tacho driver for cooling device creation.
> This cooling device could be bound to a thermal zone
> for the thermal control. Device will appear in /sys/class/thermal
> folder as cooling_deviceX. Then it could be bound to particular
> thermal zones. Allow specification of the cooling levels
> vector - PWM duty cycle values in a range from 0 to 255
> which correspond to thermal cooling states.
> 
> > Signed-off-by: Mykola Kostenok 
> 
> v1 -> v2:
>  - Remove device tree binding file from the patch.
>  - Move of_node_put out of cycle because of_get_next_child
>    already do of_put_node on previous child.
> 
> v2 -> v3:
>  Pointed out by Rob Herring:
>  - Put cooling-levels under fan subnodes.
> 
> v3 -> v4:
>  Pointed out by Joel Stanley:
>  - Move patch history after Signoff.
>  - Remove unnecessary type cast.
>  - Use array instead of pointers for colling_levels.
>  - Rename num_level to num_levels.
>  - Use local variable to make function easier to read.
>  - Drop unnesesary sizeof(u8).
>  - Use IS_ERR instead of PTR_ERR_OR_ZERO.
> 
> v4 -> v5:
>  Pointed out by Joel Stanley:
>  - Use snprintf to fill cdev->name[16].
> ---
>  drivers/hwmon/aspeed-pwm-tacho.c | 117 
> ++-
>  1 file changed, 115 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c 
> b/drivers/hwmon/aspeed-pwm-tacho.c
> index ddfe66b..049e4eb 100644
> --- a/drivers/hwmon/aspeed-pwm-tacho.c
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /* ASPEED PWM & FAN Tach Register Definition */
> >  #define ASPEED_PTCR_CTRL   0x00
> @@ -166,6 +167,18 @@
>  /* How long we sleep in us while waiting for an RPM result. */
> >  #define ASPEED_RPM_STATUS_SLEEP_USEC   500
>  
> +#define MAX_CDEV_NAME_LEN 16
> +
> +struct aspeed_cooling_device {
> > +   char name[16];
> > +   struct aspeed_pwm_tacho_data *priv;
> > +   struct thermal_cooling_device *tcdev;
> > +   int pwm_port;
> > +   u8 *cooling_levels;
> > +   u8 max_state;
> > +   u8 cur_state;
> +};
> +
>  struct aspeed_pwm_tacho_data {
> >     struct regmap *regmap;
> >     unsigned long clk_freq;
> @@ -180,6 +193,7 @@ struct aspeed_pwm_tacho_data {
> >     u8 pwm_port_type[8];
> >     u8 pwm_port_fan_ctrl[8];
> >     u8 fan_tach_ch_source[16];
> > +   struct aspeed_cooling_device *cdev[8];
> >     const struct attribute_group *groups[3];
>  };
>  
> @@ -765,6 +779,94 @@ static void aspeed_create_fan_tach_channel(struct 
> aspeed_pwm_tacho_data *priv,
> >     }
>  }
>  
> +static int
> +aspeed_pwm_cz_get_max_state(struct thermal_cooling_device *tcdev,
> > +   unsigned long *state)
> +{
> > +   struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> > +   *state = cdev->max_state;
> +
> > +   return 0;
> +}
> +
> +static int
> +aspeed_pwm_cz_get_cur_state(struct thermal_cooling_device *tcdev,
> > +   unsigned long *state)
> +{
> > +   struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> > +   *state = cdev->cur_state;
> +
> > +   return 0;
> +}
> +
> +static int
> +aspeed_pwm_cz_set_cur_state(struct thermal_cooling_device *tcdev,
> > +   unsigned long state)
> +{
> > +   struct aspeed_cooling_device *cdev = tcdev->devdata;
> +
> > +   if (state > cdev->max_state)
> > +   return -EINVAL;
> +
> > +   cdev->cur_state = state;
> > +   cdev->priv->pwm_port_fan_ctrl[cdev->pwm_port] =
> > +   cdev->cooling_levels[cdev->cur_state];
> > +   aspeed_set_pwm_port_fan_ctrl(cdev->priv, cdev->pwm_port,
> > +    cdev->cooling_levels[cdev->cur_state]);
> +
> > +   return 0;
> +}
> +
> +static const struct thermal_cooling_device_ops aspeed_pwm_cool_ops = {
> > +   .get_max_state = aspeed_pwm_cz_get_max_state,
> > +   .get_cur_state = aspeed_pwm_cz_get_cur_state,
> > +   .set_cur_state = aspeed_pwm_cz_set_cur_state,
> +};
> +
> +static int aspeed_create_pwm_cooling(struct device *dev,
> > +    struct device_node *child,
> > +    struct aspeed_pwm_tacho_data *priv,
> > +    u32 pwm_port, u8 num_levels)
> +{
> > +   int ret;
> > +   struct aspeed_cooling_device *cdev;
> +
> > +   cdev = devm_kzalloc(dev, sizeof(*cdev), GFP_KERNEL);
> +
> > +   if (!cdev)
> > +   return -ENOMEM;
> +
> > +   cdev->cooling_levels = devm_kzalloc(dev, num_levels, GFP_KERNEL);
> > +   if (!cdev->cooling_levels)
> > +   return -ENOMEM;
> +
> > +   cdev->max_state = num_levels - 1;
> > +   ret = of_property_read_u8_array(child, "cooling-levels",
> > +   cdev->cooling_levels,
> > +   num_levels);
> > +   if (ret) {
> > +   dev_err(dev, "Property 'cooling-levels' cannot be read.\n");
> > +   

Re: [patch v3] hwmon: (aspeed-pwm-tacho) cooling device support.

[Andrew Jeffery]

Hi Mykola,

I know you've sent out subsequent versions, but I wanted to address one
of your arguments here:

On Wed, 2017-07-26 at 11:08 +, Mykola Kostenok wrote:
> > > @@ -834,10 +946,12 @@ static int aspeed_pwm_tacho_probe(struct
> > > platform_device *pdev)
> > > 
> > >  for_each_child_of_node(np, child) {
> > >  ret = aspeed_create_fan(dev, child, priv);
> > > -   of_node_put(child);
> > > -   if (ret)
> > > +   if (ret) {
> > > +   of_node_put(child);
> > >  return ret;
> > > +   }
> > >  }
> > > +   of_node_put(child);
> > 
> > I'm not sure about these.
> > 
> > Cheers,
> > 
> > Joel
> 
> If CONFIG_OF_UNITTEST is set, system initialization  fails on this 
> of_node_put.
> I checked and found that for_each_child_of_node is macro witch use 
> __of_get_next_child
>  in cycle. __of_get_next_child do of_node_put previous child but not last.
> 
> static struct device_node *__of_get_next_child(const struct device_node *node,
> struct device_node *prev)
> {
> struct device_node *next;
> 
> if (!node)
> return NULL;
> 
> next = prev ? prev->sibling : node->child;
> for (; next; next = next->sibling)
> if (of_node_get(next))
> break;
> of_node_put(prev);
> return next;
> }
> #define __for_each_child_of_node(parent, child) \
> for (child = __of_get_next_child(parent, NULL); child != NULL; \
>  child = __of_get_next_child(parent, child))
> 
> So inside cycle we should not use of_node_put on each child. We must use it 
> only on last child in cycle.

I was just looking at this myself for a different driver, and I don't
think this argument is right. The natural terminating condition of the
loop is child == NULL. child == NULL occurs if we have zero-or-more-
children; the child is always initialised as part of the loop header
and would be NULL if there are no children. If we have more than one
child, the reference to the last valid child is passed to of_node_put()
in __of_get_next_child() in order to return the terminating NULL. Given
__of_get_next_child() is passed the last node and the result is NULL,
the call to of_node_put() after the loop is always invoked on NULL,
which performs an early return.

As such I think it is unnecessary.

Cheers,

Andrew

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

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

[Andrew Jeffery]

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 <and...@aj.id.au> 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 <and...@aj.id.au>
> > > ---
> > > 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 ..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
> > >

Re: [RFC PATCH v2 1/3] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

On Thu, 2017-07-20 at 00:35 +0930, Joel Stanley wrote:
> > On Tue, Jul 18, 2017 at 1:06 PM, Andrew Jeffery <and...@aj.id.au> wrote:
> > Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.
> 
> Nice! I had a bit of a read. I'm not a pmbus expert, so most of the
> review is nitpicks about types, etc.
> 
> I'll defer to Guenter for the real stuff.

Thanks for taking a look.

A lot of the issues are a case of overzealous caution so I didn't trip
myself up during development. A lot of it can be cleaned up. I won't
respond to all of them.

> 
> > 
> > Fans in a PMBus device are driven by the configuration of two registers:
> > FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
> > and the tacho operate (if installed), while FAN_COMMAND_x sets the
> > desired fan rate. The unit of FAN_COMMAND_x is dependent on the
> > operational fan mode, RPM or PWM percent duty, as determined by the
> > corresponding FAN_CONFIG_x_y.
> > 
> > The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
> > FAN_COMMAND_x is implemented with the addition of virtual registers and
> > generic implementations in the core:
> > 
> > 1. PMBUS_VIRT_FAN_TARGET_x
> > 2. PMBUS_VIRT_PWM_x
> > 3. PMBUS_VIRT_PWM_ENABLE_x
> > 
> > The facilitate the necessary side-effects of each access. Examples of
> > the read case, assuming m = 1, b = 0, R = 0:
> > 
> >  Read |  With  || Gives
> >  ---
> >    Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
> >  --++---
> >   fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
> >   pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
> >   pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
> >   fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
> >   pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
> >   pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1
> > 
> > And the write case:
> > 
> >  Write| With  ||   Sets
> >  -+---+++---
> >    Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
> >  -+---+++---
> >   fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
> >   pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
> >       pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064
> > 
> > Also, the DIRECT mode scaling of some controllers is different between
> > RPM and PWM percent duty control modes, so PSC_PWM is introduced to
> > capture the necessary coefficients.
> > 
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> > 
> > v1 -> v2:
> > * Convert to using virtual registers
> > * Drop struct pmbus_fan_ctrl
> > * Introduce PSC_PWM
> > * Drop struct pmbus_coeffs
> > * Drop additional callbacks
> > 
> >  drivers/hwmon/pmbus/pmbus.h  |  19 
> >  drivers/hwmon/pmbus/pmbus_core.c | 215 
> > +++
> >  2 files changed, 217 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index bfcb13bae34b..226a37bd525f 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -190,6 +190,20 @@ enum pmbus_regs {
> > PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
> > PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
> > PMBUS_VIRT_STATUS_VMON,
> > +
> > +   /* Fan control */
> > +   PMBUS_VIRT_FAN_TARGET_1,
> > +   PMBUS_VIRT_FAN_TARGET_2,
> > +   PMBUS_VIRT_FAN_TARGET_3,
> > +   PMBUS_VIRT_FAN_TARGET_4,
> > +   PMBUS_VIRT_PWM_1,
> > +   PMBUS_VIRT_PWM_2,
> > +   PMBUS_VIRT_PWM_3,
> > +   PMBUS_VIRT_PWM_4,
> > +   PMBUS_VIRT_PWM_ENABLE_1,
> > +   PMBUS_VIRT_PWM_ENABLE_2,
> > +   PMBUS_VIRT_PWM_ENABLE_3,
> > +   PMBUS_VIRT_PWM_ENABLE_4,
> >  };
> > 
> >  /*
> > @@ -223,6 +237,8 @@ enum pmbus_regs {
> >  #define PB_FAN_1_RPM   BIT(6)
> >  #define PB_FAN_1_INSTALLED BIT(7)
> > 
> > +enum pmbus_fan_mode { percent = 0, rpm };
> > +
> >  /*
> >   * STATUS_BYTE, STATUS_WORD (lower)
> >   */
> > @@ -313,6 +329,7 @@ enum pmbus_sensor_classes {
> > PSC_POWER,
> > PSC_TEMPERATURE,
&

[RFC PATCH v2 1/3] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The facilitate the necessary side-effects of each access. Examples of
the read case, assuming m = 1, b = 0, R = 0:

 Read |  With  || Gives
 ---
   Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
 --++---
  fanX_target | ~PB_FAN_z_RPM  | 0x0001|| 1
  pwm1| ~PB_FAN_z_RPM  | 0x0064|| 255
  pwmX_enable | ~PB_FAN_z_RPM  | 0x0001|| 1
  fanX_target |  PB_FAN_z_RPM  | 0x0001|| 1
  pwm1|  PB_FAN_z_RPM  | 0x0064|| 0
  pwmX_enable |  PB_FAN_z_RPM  | 0x0001|| 1

And the write case:

 Write| With  ||   Sets
 -+---+++---
   Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
 -+---+++---
  fanX_target | 1 ||  PB_FAN_z_RPM  | 0x0001
  pwmX| 255   || ~PB_FAN_z_RPM  | 0x0064
  pwmX_enable | 1 || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---

v1 -> v2:
* Convert to using virtual registers
* Drop struct pmbus_fan_ctrl
* Introduce PSC_PWM
* Drop struct pmbus_coeffs
* Drop additional callbacks

 drivers/hwmon/pmbus/pmbus.h  |  19 
 drivers/hwmon/pmbus/pmbus_core.c | 215 +++
 2 files changed, 217 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..226a37bd525f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,20 @@ enum pmbus_regs {
PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
PMBUS_VIRT_STATUS_VMON,
+
+   /* Fan control */
+   PMBUS_VIRT_FAN_TARGET_1,
+   PMBUS_VIRT_FAN_TARGET_2,
+   PMBUS_VIRT_FAN_TARGET_3,
+   PMBUS_VIRT_FAN_TARGET_4,
+   PMBUS_VIRT_PWM_1,
+   PMBUS_VIRT_PWM_2,
+   PMBUS_VIRT_PWM_3,
+   PMBUS_VIRT_PWM_4,
+   PMBUS_VIRT_PWM_ENABLE_1,
+   PMBUS_VIRT_PWM_ENABLE_2,
+   PMBUS_VIRT_PWM_ENABLE_3,
+   PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +237,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +329,7 @@ enum pmbus_sensor_classes {
PSC_POWER,
PSC_TEMPERATURE,
PSC_FAN,
+   PSC_PWM,
PSC_NUM_CLASSES /* Number of power sensor classes */
 };
 
@@ -413,6 +430,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int 
page, u8 reg,
  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+int config, int mask, int command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ba59eaef2e07..712a8b6c4bd6 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -63,6 +63,7 @@ struct pmbus_sensor {
u16 reg;/* register */
enum pmbus_sensor_classes class;/* sensor class */
bool update;/* runtime sensor update needed */
+   bool convert;   /* Whether or not to apply linear/vid/direct */
int data;   /* Sensor data.
   Negative if there was a read error */
 };
@@ -118,6 +119,27 @@ struct pmbus_data {
u8 currpage;
 };
 
+static const int pmbus_fan_rpm_mask[] = {
+   PB_FAN_1_RPM,
+   PB_FAN_2_RPM,
+   PB_

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

[Andrew Jeffery]

The driver features fan control and basic dual-tachometer support.

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 <and...@aj.id.au>
---
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 ..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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+#define MFR_FAN_CONFIG_DUAL_TACH   BIT(12)
+#define MFR_FAN_CONFIG_TSFOBIT(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;
+
+   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)))
+   return -ENOTSUPP;
+
+   rv = pmbus_read_byte_data(client, page - 23, reg);
+   break;
+   }
+
+   return rv;
+}
+
+static long max31785_read_long_data(struct i2c_client *client, int page,
+   

[RFC PATCH v2 0/3] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

This is a follow-up to the first RFC series[1] and includes some significant
reworks based on Guenter's feedback.

[1] https://lkml.org/lkml/2017/7/10/338

v2 retains the goal of exposing the fan[1-*]_target, pwm[1-*] and
pwm[1-*]_enable attributes, and implementing support for the MAX31785, but does
so in terms of virtual registers. The virtual register approach cuts the size
of the patch to the core roughly in half, reuses the struct pmbus_sensor
infrastructure and retains the same level of functionality. The additional
callbacks are dropped, I've reverted the change to the way the DIRECT
coefficients were structured, and there are no-longer modifications to struct
pmbus_sensor. I feel that the change is starting to become palatable.

With respect to the MAX31785 driver, I've thought about the dual tachometer
problem and resolved it with use of the read/write callbacks and only a small
modification to the core. The MAX31785 driver needs to track which fan inputs
have dual tachometer fans, and thus needs a context struct. To enable use of a
context struct I've exposed a pmbus_get_info() function that returns the
pointer to struct pmbus_driver_info provided at pmbus_do_probe(). This allows
the read/write callbacks to hoist themselves out to the containing context
struct without exposing the internals of struct pmbus_data, or injecting
driver-specific details into members of struct pmbus_data. I'm interested in
feedback here.

I've sent this as an RFC again to get feedback on the new approaches, but also
because the MAX31785 implementation is not quite where I want it to be:
bindings need to be developed, and the driver needs support for individual
sensor dual-tach support, not just assume that all present fans support it.
I'll work on these issues in parallel to feedback on the patches.

Andrew Jeffery (3):
  hwmon: pmbus: Add fan control support
  pmbus: Add and expose pmbus_get_info()
  pmbus: Add MAX31785 driver

 drivers/hwmon/pmbus/Kconfig  |  10 ++
 drivers/hwmon/pmbus/Makefile |   1 +
 drivers/hwmon/pmbus/max31785.c   | 372 +++
 drivers/hwmon/pmbus/pmbus.h  |  20 +++
 drivers/hwmon/pmbus/pmbus_core.c | 221 +--
 5 files changed, 607 insertions(+), 17 deletions(-)
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 2/4] pmbus: Add fan configuration support

[Andrew Jeffery]

On Tue, 2017-07-11 at 20:43 -0700, Guenter Roeck wrote:
> On 07/11/2017 05:39 PM, Andrew Jeffery wrote:
> > On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> > > On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > > > Augment PMBus support to include control of fans via the
> > > > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > > > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > > > their interactions do not fit the existing use of struct pmbus_sensor.
> > > > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > > > simple sensor case, along with associated sysfs show/set 
> > > > implementations.
> > > > 
> > > > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > > > the current fan mode (RPM or PWM, as configured in
> > > > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > > > register. For example, the MAX31785 chip defines the following:
> > > > 
> > > > PWM (m = 1, b = 0, R = 2):
> > > >    0x - 0x2710: 0 - 100% fan PWM duty cycle
> > > >    0x2711 - 0x7fff: 100% fan PWM duty cycle
> > > >    0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan 
> > > > control
> > > > 
> > > > RPM (m = 1, b = 0, R = 0):
> > > >    0x - 0x7FFF: 0 - 32,767 RPM
> > > >    0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan 
> > > > control
> > > > 
> > > > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > > > add an optional callbacks to the info struct to get/set the 'mode'
> > > > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > > > calculation exists if the callbacks are not populated; the fallback
> > > > ignores device-specific ranges and tries to determine a reasonable value
> > > > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> > > > 
> > > 
> > > This seems overly complex, but unfortunately I don't have time for a 
> > > detailed
> > > analysis right now.
> > 
> > No worries. It turned out more complex than I was hoping as well, and I
> >   am keen to hear any insights to trim it down.
> > 
> > > Couple of comments below.
> > 
> > Yep, thanks for taking a look.
> > 
> > > 
> > > Guenter
> > > 
> > > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > > > 
> > > > ---
> > > >    drivers/hwmon/pmbus/pmbus.h  |   7 +
> > > >    drivers/hwmon/pmbus/pmbus_core.c | 335 
> > > > +++
> > > >    2 files changed, 342 insertions(+)
> > > > 
> > > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > > > index bfcb13bae34b..927eabc1b273 100644
> > > > --- a/drivers/hwmon/pmbus/pmbus.h
> > > > +++ b/drivers/hwmon/pmbus/pmbus.h
> > > > @@ -223,6 +223,8 @@ enum pmbus_regs {
> > > > > > > > > > > >    #define PB_FAN_1_RPM BIT(6)
> > > > > >    #define PB_FAN_1_INSTALLED   BIT(7)
> > > > 
> > > >    
> > > > +enum pmbus_fan_mode { percent = 0, rpm };
> > > > +
> > > >    /*
> > > > * STATUS_BYTE, STATUS_WORD (lower)
> > > > */
> > > > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > > > > > > > > > >     int (*identify)(struct i2c_client *client,
> > > > > >     struct pmbus_driver_info *info);
> > > > 
> > > >    
> > > > > > > > > > > > +   /* Allow the driver to interpret the fan 
> > > > > > > > > > > > command value */
> > > > > > > > > > > > +   int (*get_pwm_mode)(int id, u8 fan_config, u16 
> > > > > > > > > > > > fan_command);
> > > > > > > > > > > > +   int (*set_pwm_mode)(int id, long mode, u8 
> > > > > > > > > > > > *fan_config,
> > > > > > +   u16 *fan_command);
> > > > 
> > > > +
> > > 
> > > It is not entirely obvious to me why this would require new call

Re: [RFC PATCH 2/4] pmbus: Add fan configuration support

[Andrew Jeffery]

On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Augment PMBus support to include control of fans via the
> > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > their interactions do not fit the existing use of struct pmbus_sensor.
> > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > simple sensor case, along with associated sysfs show/set implementations.
> > 
> > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > the current fan mode (RPM or PWM, as configured in
> > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > register. For example, the MAX31785 chip defines the following:
> > 
> > PWM (m = 1, b = 0, R = 2):
> >   0x - 0x2710: 0 - 100% fan PWM duty cycle
> >   0x2711 - 0x7fff: 100% fan PWM duty cycle
> >   0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan 
> > control
> > 
> > RPM (m = 1, b = 0, R = 0):
> >   0x - 0x7FFF: 0 - 32,767 RPM
> >   0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan 
> > control
> > 
> > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > add an optional callbacks to the info struct to get/set the 'mode'
> > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > calculation exists if the callbacks are not populated; the fallback
> > ignores device-specific ranges and tries to determine a reasonable value
> > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> > 
> 
> This seems overly complex, but unfortunately I don't have time for a detailed
> analysis right now. 

No worries. It turned out more complex than I was hoping as well, and I
 am keen to hear any insights to trim it down. 

> Couple of comments below.

Yep, thanks for taking a look.

> 
> Guenter
> 
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> >   drivers/hwmon/pmbus/pmbus.h  |   7 +
> >   drivers/hwmon/pmbus/pmbus_core.c | 335 
> > +++
> >   2 files changed, 342 insertions(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index bfcb13bae34b..927eabc1b273 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -223,6 +223,8 @@ enum pmbus_regs {
> > > >   #define PB_FAN_1_RPM  BIT(6)
> > > >   #define PB_FAN_1_INSTALLEDBIT(7)
> >   
> > +enum pmbus_fan_mode { percent = 0, rpm };
> > +
> >   /*
> >    * STATUS_BYTE, STATUS_WORD (lower)
> >    */
> > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > >     int (*identify)(struct i2c_client *client,
> > > >     struct pmbus_driver_info *info);
> >   
> > > > +   /* Allow the driver to interpret the fan command value */
> > > > +   int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > +   int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > > > +   u16 *fan_command);
> > +
> 
> It is not entirely obvious to me why this would require new callback 
> functions.
> Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does 
> not
> work for some reason, introduce a virtual register, such as 
> PMBUS_VIRT_PWM_MODE ?

Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?

Regarding virtual registers, I saw references to them whilst I was
working my way through the core code but didn't stop to investigate.
I'll take a deeper look.

However, the addition of the callbacks was driven by the behaviour of
the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
automated control, while others retain manual control. Patch 4/4 should
provide a bit more context, though I've also outlined the behaviour in
the commit message for this patch. I don't have a lot of experience
with PMBus devices so I don't have a good idea if there's a better way
to capture the behaviour that isn't so unconstrained in its approach.

> 
> > > >     /* Regulator functionality, if supported by this chip driver. */
> > > >     int num_regulators;
> > > >     const struct regulator_desc *reg_desc;
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
> > b/drivers/hwmon/pmbus/pmbus_core.c
> > index ba59eaef2e07..3b0a55bbbd2c 100644
> > --- a/drivers/hwmon/pmbus/p

[RFC PATCH 2/4] pmbus: Add fan configuration support

[Andrew Jeffery]

Augment PMBus support to include control of fans via the
FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
their interactions do not fit the existing use of struct pmbus_sensor.
The patch introduces struct pmbus_fan_ctrl to distinguish from the
simple sensor case, along with associated sysfs show/set implementations.

Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
the current fan mode (RPM or PWM, as configured in
FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
register. For example, the MAX31785 chip defines the following:

PWM (m = 1, b = 0, R = 2):
 0x - 0x2710: 0 - 100% fan PWM duty cycle
 0x2711 - 0x7fff: 100% fan PWM duty cycle
 0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan control

RPM (m = 1, b = 0, R = 0):
 0x - 0x7FFF: 0 - 32,767 RPM
 0x8000 - 0x: Ignore FAN_COMMAND_[1-4], use automatic fan control

To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
add an optional callbacks to the info struct to get/set the 'mode'
value required for the pwm[1-n]_enable sysfs attribute. A fallback
calculation exists if the callbacks are not populated; the fallback
ignores device-specific ranges and tries to determine a reasonable value
from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |   7 +
 drivers/hwmon/pmbus/pmbus_core.c | 335 +++
 2 files changed, 342 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..927eabc1b273 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -223,6 +223,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM   BIT(6)
 #define PB_FAN_1_INSTALLED BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -380,6 +382,11 @@ struct pmbus_driver_info {
int (*identify)(struct i2c_client *client,
struct pmbus_driver_info *info);
 
+   /* Allow the driver to interpret the fan command value */
+   int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
+   int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
+   u16 *fan_command);
+
/* Regulator functionality, if supported by this chip driver. */
int num_regulators;
const struct regulator_desc *reg_desc;
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ba59eaef2e07..3b0a55bbbd2c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -69,6 +69,38 @@ struct pmbus_sensor {
 #define to_pmbus_sensor(_attr) \
container_of(_attr, struct pmbus_sensor, attribute)
 
+#define PB_FAN_CONFIG_RPM  PB_FAN_2_RPM
+#define PB_FAN_CONFIG_INSTALLEDPB_FAN_2_INSTALLED
+#define PB_FAN_CONFIG_MASK(i)  (0xff << (4 * !((i) & 1)))
+#define PB_FAN_CONFIG_GET(i, n)(((n) >> (4 * !((i) & 1))) & 
0xff)
+#define PB_FAN_CONFIG_PUT(i, n)(((n) & 0xff) << (4 * !((i) & 
1)))
+
+struct pmbus_fan_ctrl_attr {
+   struct device_attribute attribute;
+   char name[PMBUS_NAME_SIZE];
+};
+
+struct pmbus_fan_ctrl {
+   struct pmbus_fan_ctrl_attr fan_target;
+   struct pmbus_fan_ctrl_attr pwm;
+   struct pmbus_fan_ctrl_attr pwm_enable;
+   int index;
+   u8 page;
+   u8 id;
+   u8 config;
+   u16 command;
+};
+#define to_pmbus_fan_ctrl_attr(_attr) \
+   container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
+#define fan_target_to_pmbus_fan_ctrl(_attr) \
+   container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
+   fan_target)
+#define pwm_to_pmbus_fan_ctrl(_attr) \
+   container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
+#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
+   container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
+   pwm_enable)
+
 struct pmbus_boolean {
char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
struct sensor_device_attribute attribute;
@@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
 }
 
+static ssize_t pmbus_show_fan_command(struct device *dev,
+ enum pmbus_fan_mode mode,
+ struct pmbus_fan_ctrl *fan, char *buf)
+{
+   struct i2c_client *client = to_i2c_client(dev->parent);
+   struct pmbus_data *data = i2c_get_clientdata(client);
+   struct pmbus_sensor sensor;
+   long val;
+
+   mutex_lock(>update_lock);
+
+

[RFC PATCH 4/4] pmbus: Add MAX31785 driver

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/Kconfig|  10 ++
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 201 +
 3 files changed, 212 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 24ff7ee7f8bb..acba1be0d9ad 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SENSORS_PMBUS)   += pmbus.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 ..8432ed5a0ad6
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,201 @@
+/*
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include "pmbus.h"
+
+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,
+};
+
+static const struct pmbus_coeffs fan_coeffs[] = {
+   [percent]   = { .m = 1, .b = 0, .R = 2 },
+   [rpm]   = { .m = 1, .b = 0, .R = 0 },
+};
+
+static const struct pmbus_coeffs *max31785_get_fan_coeffs(
+   const struct pmbus_driver_info *info, int index,
+   enum pmbus_fan_mode mode, int command)
+{
+   switch (command) {
+   case PMBUS_FAN_COMMAND_1:
+   case PMBUS_MFR_FAN_FAULT_LIMIT:
+   case PMBUS_MFR_FAN_WARN_LIMIT:
+   return _coeffs[mode];
+   case PMBUS_READ_FAN_SPEED_1:
+   return _coeffs[rpm];
+   case PMBUS_MFR_FAN_PWM_AVG:
+   return _coeffs[percent];
+   default:
+   break;
+   }
+
+   return NULL;
+}
+
+static int max31785_get_pwm_mode(int id, u8 fan_config, u16 fan_command)
+{
+   enum pmbus_fan_mode mode;
+
+   /* MAX31785 only supports fan 1 on the fan pages */
+   if (WARN_ON(id > 0))
+   return -ENODEV;
+
+   mode = (fan_config & PB_FAN_1_RPM) ? rpm : percent;
+
+   switch (mode) {
+   case percent:
+   if (fan_command >= 0x8000)
+   return 2;
+   else if (fan_command >= 0x2711)
+   return 0;
+   else
+   return 1;
+   case rpm:
+   if (fan_command >= 0x8000)
+   return 2;
+   else
+   return 1;
+   break;
+   }
+
+   return 0;
+}
+
+static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0x };
+
+int max31785_set_pwm_mode(int id, long mode, u8 *fan_config, u16 *fan_command)
+{
+   /* MAX31785 only supports fan 1 on the fan pages */
+   if (WARN_ON(id > 0))
+   return -ENODEV;
+
+   *fan_config &= ~PB_FAN_1_RPM;
+
+   if (mode >= ARRAY_SIZE(max31785_pwm_modes))
+   return -ENOTSUPP;
+
+   *fan_command = max31785_pwm_modes[mode];
+
+   return 0;
+}
+
+static struct pmbus_driver_info max31785_info = {
+   .pages = 23,
+
+   .get_fan_coeffs = max31785_get_fan_coeffs,
+   .get_pwm_mode = max31785_get_pwm_mode,
+   .set_pwm_mode = max31785_set_pwm_mode,
+
+   .format[PSC_FAN] = direct,
+   .func[0] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+   .func[1] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+   .func[2] = PMBUS_HAVE_FAN12 

[RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

I recently sent a patch adding support for the MAX31785 chip[1], but
implemented in terms of hwmon rather than pmbus. The hwmon-based implementation
was driven by a lack of support for the features we needed from the pmbus
subsystem but ideally that shouldn't be a roadblock to a pmbus implementation.

[1] https://lkml.org/lkml/2017/6/6/71

So on that front, I've had a go at modifying the pmbus core to explore the
problem space and provide what we needed for the MAX31785 chip. Namely,
exposing fan interfaces for both RPM and PWM control:

* fan[1-*]_target
* pwm[1-*]
* pwm[1-*]_enable

The interfaces exposed conform to the hwmon sysfs definitions as much as
possible, though there are some warts due to the nature of PMBus. Specifically,
the following details need some discussion:

1. The PMBus spec defines FAN_COMMAND_[1-4] as the fan rate control register
2. Fan rate control can be done either in terms of PWM or RPM
3. Conversion between PWM and RPM is configuration dependent and therefore
   cannot be generic or even necessarily provided by driver callbacks.
4. Attributes for RPM and PWM control need to be exposed to userspace, but only
   one set can be operational at a given time (constrained by the one command
   register).
5. Therefore it is not generally possible to support reading both
   fan[1-*]_target and pwm[1-*] without changing the control method as
   appropriate

To address this last point the implementation currently returns -ENOTSUPP for
reads of fan[1-*]_target and pwm[1-*] if they are not associated with the
current control mode.

Further, pwm[1-*]_enable is effectively invalid whilst in RPM mode. Maybe it
too should return -ENOTSUPP when read in the RPM context, but currently it
provides a value.

Further I've implemented the change of control mode as a last-write-wins
strategy, where a write to fan[1-*]_target configures the fan for RPM mode,
whilst a write to pwm[1-*] or pwm[1-*]_target configures PWM mode. It's not
clear to me that there's any other good strategy whilst being confined to the
existing hwmon sysfs interface.

Given the fan control implementation is driven by wanting to support the
MAX31785 device, the patches add some new callbacks to struct pmbus_driver_info:

int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
u16 *fan_command);

These callbacks allow the driver to use the fan configuration and command
values to interpret or return the correct value for pwm[1-*]_enable. The
MAX31785 chip defines several value ranges for FAN_COMMAND_[1-4] that put the
chip in manual, automatic or full-speed modes.

A third callback was also added:

const struct pmbus_coeffs *(*get_fan_coeffs)(
const struct pmbus_driver_info *info, int index,
enum pmbus_fan_mode mode, int command);

This was a strategy to cope with the MAX31785 chip changing its fan
coefficients when moving between RPM and PWM control modes: The current
coefficients array mechanism isn't capable of describing this behaviour.
Further, different subsets of fan-related registers are affected by the control
mode change. Thus, the simplest solution with the least impact on existing
drivers appeared to be to provide an optional callback member. To go back on
that a little, I've changed the array structure anyway to make the callback
prototype and pmbus_core implementation a little easier. This justifies the
first patch in the RFC, which simply removes the ability to build any of the
other pmbus drivers. Clearly it's a junk patch that won't exist in a non-RFC
series, as if the idea is sensible then all existing drivers will be converted.

I'm hoping this is a reasonable start to getting the support we need for the
MAX31785 chip into the PMBus subsystem, and that we can evolve it into
something acceptable.

Still on the TODO list is adding support for variants of the MAX31785 that
can perform dual-rotor measurements. I haven't yet had a good think about how
to do it given the hardware behaviour, but given the design of pmbus core it
could be a bit intrusive.  Having sent this series out I'll have a think about
the problem, and will look to send a follow-up RFC hashing out an
implementation.

Cheers,

Andrew

Andrew Jeffery (4):
  hwmon: pmbus: Disable build of non-max31785 drivers
  pmbus: Add fan configuration support
  pmbus: Allow dynamic fan coefficient values
  pmbus: Add MAX31785 driver

 drivers/hwmon/pmbus/Kconfig  |  10 +
 drivers/hwmon/pmbus/Makefile |  25 +--
 drivers/hwmon/pmbus/max31785.c   | 201 ++
 drivers/hwmon/pmbus/pmbus.h  |  25 ++-
 drivers/hwmon/pmbus/pmbus_core.c | 439 +--
 5 files changed, 670 insertions(+), 30 deletions(-)
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" i

[RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values

[Andrew Jeffery]

Some PMBus chips, such as the MAX31785, use different coefficients for
FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
or RPM mode. Add a callback to allow the driver to provide the
applicable coefficients to avoid imposing on devices that don't have
this requirement.

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h  |  18 +--
 drivers/hwmon/pmbus/pmbus_core.c | 112 ---
 2 files changed, 108 insertions(+), 22 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 927eabc1b273..338ecc8b25a4 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
 
+struct pmbus_coeffs {
+   int m; /* mantissa for direct data format */
+   int b; /* offset */
+   int R; /* exponent */
+};
+
 struct pmbus_driver_info {
int pages;  /* Total number of pages */
enum pmbus_data_format format[PSC_NUM_CLASSES];
@@ -353,9 +359,7 @@ struct pmbus_driver_info {
 * Support one set of coefficients for each sensor type
 * Used for chips providing data in direct mode.
 */
-   int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
-   int b[PSC_NUM_CLASSES]; /* offset */
-   int R[PSC_NUM_CLASSES]; /* exponent */
+   struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
 
u32 func[PMBUS_PAGES];  /* Functionality, per page */
/*
@@ -382,6 +386,14 @@ struct pmbus_driver_info {
int (*identify)(struct i2c_client *client,
struct pmbus_driver_info *info);
 
+   /*
+* If a fan's coefficents change over time (e.g. between RPM and PWM
+* mode), then the driver can provide a function for retrieving the
+* currently applicable coefficients.
+*/
+   const struct pmbus_coeffs *(*get_fan_coeffs)(
+   const struct pmbus_driver_info *info, int index,
+   enum pmbus_fan_mode mode, int command);
/* Allow the driver to interpret the fan command value */
int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 3b0a55bbbd2c..4ff6a1fd5cce 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -58,10 +58,11 @@
 struct pmbus_sensor {
struct pmbus_sensor *next;
char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
-   struct device_attribute attribute;
+   struct sensor_device_attribute attribute;
u8 page;/* page number */
u16 reg;/* register */
enum pmbus_sensor_classes class;/* sensor class */
+   const struct pmbus_coeffs *coeffs;
bool update;/* runtime sensor update needed */
int data;   /* Sensor data.
   Negative if there was a read error */
@@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
u8 id;
u8 config;
u16 command;
+   const struct pmbus_coeffs *coeffs;
 };
 #define to_pmbus_fan_ctrl_attr(_attr) \
container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
@@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
long val = (s16) sensor->data;
long m, b, R;
 
-   m = data->info->m[sensor->class];
-   b = data->info->b[sensor->class];
-   R = data->info->R[sensor->class];
+   if (sensor->coeffs) {
+   m = sensor->coeffs->m;
+   b = sensor->coeffs->b;
+   R = sensor->coeffs->R;
+   } else {
+   m = data->info->coeffs[sensor->class].m;
+   b = data->info->coeffs[sensor->class].b;
+   R = data->info->coeffs[sensor->class].R;
+   }
 
if (m == 0)
return 0;
@@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
 {
long m, b, R;
 
-   m = data->info->m[sensor->class];
-   b = data->info->b[sensor->class];
-   R = data->info->R[sensor->class];
+   if (sensor->coeffs) {
+   m = sensor->coeffs->m;
+   b = sensor->coeffs->b;
+   R = sensor->coeffs->R;
+   } else {
+   m = data->info->coeffs[sensor->class].m;
+   b = data->info->coeffs[sensor->class].b;
+   R = data->info->coeffs[sensor->class].R;
+   }
 
/* Power is in uW. Adjust R and b. */
if (sensor->class == PSC_POWER) {
@@ -796,7 +810,9 @@ static ssize_t pmbus_show_sen

[RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <and...@aj.id.au>
---
Later patches make incompatible changes to struct pmbus_driver_info, so this
serves to keep the following patches buildable under otherwise incompatible
configurations. It is a junk patch that won't be present in a non-RFC series.

 drivers/hwmon/pmbus/Makefile | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 562132054aaf..24ff7ee7f8bb 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -4,15 +4,15 @@
 
 obj-$(CONFIG_PMBUS)+= pmbus_core.o
 obj-$(CONFIG_SENSORS_PMBUS)+= pmbus.o
-obj-$(CONFIG_SENSORS_ADM1275)  += adm1275.o
-obj-$(CONFIG_SENSORS_LM25066)  += lm25066.o
-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_MAX34440) += max34440.o
-obj-$(CONFIG_SENSORS_MAX8688)  += max8688.o
-obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
-obj-$(CONFIG_SENSORS_UCD9000)  += ucd9000.o
-obj-$(CONFIG_SENSORS_UCD9200)  += ucd9200.o
-obj-$(CONFIG_SENSORS_ZL6100)   += zl6100.o
+# obj-$(CONFIG_SENSORS_ADM1275)+= adm1275.o
+# obj-$(CONFIG_SENSORS_LM25066)+= lm25066.o
+# 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_MAX34440)   += max34440.o
+# obj-$(CONFIG_SENSORS_MAX8688)+= max8688.o
+# obj-$(CONFIG_SENSORS_TPS40422)   += tps40422.o
+# obj-$(CONFIG_SENSORS_UCD9000)+= ucd9000.o
+# obj-$(CONFIG_SENSORS_UCD9200)+= ucd9200.o
+# obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller

[Andrew Jeffery]

On Thu, 2017-06-08 at 05:37 -0700, Guenter Roeck wrote:
> On 06/08/2017 12:53 AM, Andrew Jeffery wrote:
> > On Wed, 2017-06-07 at 08:55 -0700, Guenter Roeck wrote:
> > > On Tue, Jun 06, 2017 at 04:32:30PM +0930, Andrew Jeffery wrote:
> > > > Add a basic driver for the MAX31785, focusing on the fan control
> > > > features but ignoring the temperature and voltage monitoring
> > > > features of the device.
> > > > 
> > > > This driver supports all fan control modes and tachometer / PWM
> > > > readback where applicable.
> > > > 
> > > > > > Signed-off-by: Timothy Pearson <tpear...@raptorengineering.com>
> > > > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > > > 
> > > > ---
> > > > Hello,
> > > > 
> > > > This is a rework of Timothy Pearson's original patch:
> > > > 
> > > >  
> > > > https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html
> > > > 
> > > > I've labelled it as v3 to differentiate from Timothy's postings.
> > > > 
> > > > The original thread had some discussion about the MAX31785 being a 
> > > > PMBus device
> > > > and that it should thus be a PMBus driver. The implementation still 
> > > > makes use
> > > 
> > > After thinking about it, that is what it should be. If I accept it as 
> > > non-PMBus
> > > driver, it will be all but impossible to convert it to a PMBus driver 
> > > later on,
> > > and that just doesn't make any sense.
> > 
> > Hopefully not being too ignorant here, but can you expand on why it
> > would be all but impossible to convert?
> > 
> 
> I've got a lot of noise recently just for converting a driver from the old to 
> the
> new API (which changes the attribute location). Changing the driver from 
> non-PMBus
> to PMBus would very quite likely change some attributes as well.

Okay.

> 
> Besides that, I think it is a bad idea to bypass an infrastructure just 
> because
> it may require a few tweaks. That generates a bad precedent, and people 
> _would_
> use that to argue that the next PMBus chip driver should not use the 
> infrastructure
> either.

I understand not wanting to set a precedent. Thanks for your response.

Andrew

> 
> Guenter
> 
> > > 
> > > With no one interested in writing that driver, I'll try to give it some 
> > > more
> > > priority myself. I do have an evaluation board somewhere, which should 
> > > help.
> > > 
> > > Note that the second fan reading should be implemented as just that, not 
> > > with
> > > a non-standard attribute.
> > 
> > Agreed.
> > 
> > Andrew
> > 
> 
> 

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

Re: [PATCH v3] hwmon: Add support for MAX31785 intelligent fan controller

[Andrew Jeffery]

On Tue, 2017-06-06 at 06:33 -0700, Guenter Roeck wrote:
> On 06/06/2017 12:02 AM, Andrew Jeffery wrote:
> > Add a basic driver for the MAX31785, focusing on the fan control
> > features but ignoring the temperature and voltage monitoring
> > features of the device.
> > 
> > This driver supports all fan control modes and tachometer / PWM
> > readback where applicable.
> > 
> > > > Signed-off-by: Timothy Pearson <tpear...@raptorengineering.com>
> > > > Signed-off-by: Andrew Jeffery <and...@aj.id.au>
> > ---
> > Hello,
> > 
> > This is a rework of Timothy Pearson's original patch:
> > 
> >  https://www.mail-archive.com/linux-hwmon@vger.kernel.org/msg00868.html
> > 
> > I've labelled it as v3 to differentiate from Timothy's postings.
> > 
> > The original thread had some discussion about the MAX31785 being a PMBus 
> > device
> > and that it should thus be a PMBus driver. The implementation still makes 
> > use
> > of features not available in the pmbus core, so I've taken up the earlier
> > suggestion and ported it to the devm_hwmon_device_register_with_info()
> > interface. This gave a modest reduction in lines-of-code and at least to me 
> > is
> > more aesthetically pleasing.
> > 
> > Over and above the features of the original patch is support for a secondary
> > rotor measurement value that is provided by MAX31785 chips with a revised
> > firmware. The feature(s) of the firmware are determined at probe time and 
> > extra
> > attributes exposed accordingly. Specifically, the MFR_REVISION 0x3040 of the
> > firmware supports 'slow' and 'fast' rotor reads. The feature is implemented 
> > by
> > command 0x90 (READ_FAN_SPEED_1) providing a 4-byte response (with the 'fast'
> > measurement in the second word) rather than the 2-bytes response in the
> > original firmware (MFR_REVISION 0x3030).
> > 
> 
> Taking the pmbus driver question out, why would this warrant another 
> non-standard
> attribute outside the ABI ? I could see the desire to replace the 'slow' 
> access
> with the 'fast' one, but provide two attributes ? No, I don't see the point, 
> sorry,
> even more so without detailed explanation why the second attribute in addition
> to the first one would add any value.

At the least I'll update the documentation in line with Matt Barth's
comment, as it was vague. Whether or not we keep the questionable
attribute I hope will be resolved down further in the thread.

> 
> > This feature is not documented in the public datasheet[1].
> > 
> > [1] https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> > 
> > The need to read a 4-byte value drives the addition of a helper that is a
> > cut-down version of i2c_smbus_xfer_emulated(), as 4-byte transactions 
> > aren't a
> > defined transaction type in the PMBus spec. This seemed more tasteful than
> > hacking the PMBus core to support the quirks of a single device.
> > 
> 
> That is why we have PMBus helper drivers.

Right - I meant to say SMBus in the last sentence, not PMBus, as it was
with respect to hacking i2c_smbus_xfer_emulated(). So I understand that
the device-specific PMBus drivers exist to help with quirks, just that
it's not (yet) a PMBus driver.

Thanks for the feedback,

Andrew

> 
> Guenter
> 
> > Also changed from Timothy's original posting is I've massaged the locking a 
> > bit
> > and removed what seemed to be a copy/paste bug around 
> > max31785_fan_set_pulses()
> > setting the fan_command member.
> > 
> > Tested on an IBM Witherspoon machine.
> > 
> > Cheers,
> > 
> > Andrew
> > 
> >   Documentation/hwmon/max31785 |  44 +++
> >   drivers/hwmon/Kconfig|  10 +
> >   drivers/hwmon/Makefile   |   1 +
> >   drivers/hwmon/max31785.c | 824 
> > +++
> >   4 files changed, 879 insertions(+)
> >   create mode 100644 Documentation/hwmon/max31785
> >   create mode 100644 drivers/hwmon/max31785.c
> > 
> > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> > new file mode 100644
> > index ..dd891c06401e
> > --- /dev/null
> > +++ b/Documentation/hwmon/max31785
> > @@ -0,0 +1,44 @@
> > +Kernel driver max31785
> > +==
> > +
> > +Supported chips:
> > +  * Maxim MAX31785
> > +Prefix: 'max31785'
> > +Addresses scanned: 0x52 0x53 0x54 0x55
> > +Datasheet: http://pdfserv.maximintegrated.com/en/ds/MAX31785.pdf
> > +
> > > > +Author: Timothy Pearson

Re: [PATCH linux v7 3/6] hwmon: occ: Add I2C transport implementation for SCOM operations

[Andrew Jeffery]

On Fri, 2017-02-10 at 15:05 -0600, Eddie James wrote:
> On 02/09/2017 11:31 PM, Joel Stanley wrote:
> > > On Wed, Feb 8, 2017 at 9:40 AM,  wrote:
> > >> From: "Edward A. James"
> >>
> >> Add functions to send SCOM operations over I2C bus. The BMC can
> >> communicate with the Power8 host processor over I2C, but needs to use SCOM
> >> operations in order to access the OCC register space.
> > This doesn't need to be separate from the p8_occ_i2c.c file. You can
> > remove a layer of function calls by merging this in and having these
> > be your getscom putscom bus_ops callbacks.
> 
> The purpose of having this separate was so that we could do the scom 
> address shift for p8 separately.

Separation was my suggestion. It makes the I2C transport implementation
independent of any P8 details, and having it separate* is no more
abstract than the core performing SCOMs through the FSI interface when
that's available. I feel like it's muddying the waters conceptually to
bury P8 details in the implementation of a SCOM transport layer.

However, in our less abstract world the P8 will be the only system that
uses the I2C transport, so while I don't think merging the functions is
a good idea from an abstraction perspective it won't have a big impact.

Andrew

* Maybe the I2C SCOM transport even deserves to live outside
drivers/hwmon/occ?

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

Re: [PATCH linux 2/6] hwmon: occ: Add sysfs interface

[Andrew Jeffery]

On Sat, 2017-01-07 at 09:15 -0800, Guenter Roeck wrote:
> On 01/06/2017 02:17 PM, Edward James wrote:
> 
> [ ... ]
> 
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR(online, S_IWUSR | S_IRUGO, show_occ_online,
> > > > + store_occ_online);
> > > > +
> > > > +struct occ_sysfs *occ_sysfs_start(struct device *dev, struct occ *occ,
> > > > +  struct occ_sysfs_config *config)
> > > > +{
> > > > +   struct occ_sysfs *hwmon = devm_kzalloc(dev, sizeof(struct 
> > > > occ_sysfs),
> > > > +  GFP_KERNEL);
> > > > +   int rc;
> > > > +
> > > > +   if (!hwmon)
> > > > +  return ERR_PTR(-ENOMEM);
> > > > +
> > > > +   hwmon->occ = occ;
> > > > +   hwmon->num_caps_fields = config->num_caps_fields;
> > > > +   hwmon->caps_names = config->caps_names;
> > > > +
> > > > +   dev_set_drvdata(dev, hwmon);
> > > > +
> > > > +   rc = device_create_file(dev, _attr_online);
> > > > +   if (rc)
> > > > +  return ERR_PTR(rc);
> > > > +
> > > > +   return hwmon;
> > > > +}
> > > > +EXPORT_SYMBOL(occ_sysfs_start);
> > > > +
> > > > +int occ_sysfs_stop(struct device *dev, struct occ_sysfs *driver)
> > > > +{
> > > > +   if (driver->dev) {
> > > > +  occ_remove_hwmon_attrs(driver);
> > > > +  hwmon_device_unregister(driver->dev);
> > > > +   }
> > > > +
> > > > +   device_remove_file(driver->dev, _attr_online);
> > > > +
> > > > +   devm_kfree(dev, driver);
> > > 
> > > Thw point of using devm_ functions is not to require remove/free 
> > > functions.
> > > Something is completely wrong here if you need that call.
> > > 
> > > Overall, this is architectually completely wrong. One does not register
> > > or instantiate drivers based on writing into sysfs attributes. Please
> > > reconsider your approach.
> > 
> > We had some trouble designing this driver because the BMC only has
> > access to the OCC once the processor is powered on. This will happen
> > sometime after the BMC boots (this driver runs only on the BMC). With
> > no access to the OCC, we don't know what sensors are present on the
> > system without a large static enumeration. Also any sysfs files created
> > before we have OCC access won't be able to return any data.
> > 
> > Instead of the "online" attribute, what do you think about using the
> > "bind"/"unbind" API to probe the device from user space once the system
> > is powered on? All the hwmon registration would take place in the probe
> > function, it would just occur some time after boot.
> > 
> 
> A more common approach would be to have a platform driver. That platform
> driver would need a means to detect if the OCC is up and running, and
> instantiate everything else once it is.
> 
> A trigger from user space is problematic because there is no guarantee
> that the OCC is really up (or that it even exists).

This is true in general, but for the BMC case we have more information:
The host CPU power supply is controlled by several GPIOs from
userspace. Once we receive the "power-good" signal for the host CPU we
can bind the OCC driver and trigger the probe.

Alternatively, in the style of your first para, we could push the host
CPU state management into the kernel and expose a boot/reboot/power-off 
API to userspace. That would give us a place to hook calls for
configuring and cleaning up any host-dependent drivers on the BMC.

The solution to the host-power-state problem is also applicable to the
OpenFSI patches that were recently sent out:

https://lkml.org/lkml/2016/12/6/732

The OpenFSI infra needs to re-scan for CFAMs when the host is powered
up.

> 
> An alternative might be to have the hwmon driver poll for the OCC,
> but that would be a bit more difficult and might require a kernel thread
> or maybe asynchronous probing.

This was our thought as a fallback solution.

Andrew

> 
> Guenter
> 

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