Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
On Wed, Jul 12, 2017 at 04:31:09PM +0930, Andrew Jeffery wrote: > > Indeed. Hence RFC in case I had overlooked something :) Clearly I have. > Not surprising. It isn't exceptionally well documented :-) > > > > > 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. > > > > > > > Many pmbus commands have side effects. I don't see how an explicit callback > > would be different to overloading a standard register or to providing a > > virtual > > register/command, whichever is more convenient. > > I'm going to experiment with the virtual registers. From your > description above and looking at the comments in pmbus.h I think I can > make something work (and drop the callbacks). > Excellent. > > Sure. FWIW I plan on sending a follow-up RFC based on the feedback > you've given here, and I'll look to chop out pmbus_fan_ctrl. I was > suspicious of needing it as well, but was after your input on the > general approach and figured sending the patches was better than > guessing at your potential feedback. > > If a follow-up isn't of interest and you'd definitely rather take on > the work up yourself, let me know. > By all means, please go ahead. I got way too much on my plate already. Thanks, Guenter
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
On Wed, Jul 12, 2017 at 04:31:09PM +0930, Andrew Jeffery wrote: > > Indeed. Hence RFC in case I had overlooked something :) Clearly I have. > Not surprising. It isn't exceptionally well documented :-) > > > > > 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. > > > > > > > Many pmbus commands have side effects. I don't see how an explicit callback > > would be different to overloading a standard register or to providing a > > virtual > > register/command, whichever is more convenient. > > I'm going to experiment with the virtual registers. From your > description above and looking at the comments in pmbus.h I think I can > make something work (and drop the callbacks). > Excellent. > > Sure. FWIW I plan on sending a follow-up RFC based on the feedback > you've given here, and I'll look to chop out pmbus_fan_ctrl. I was > suspicious of needing it as well, but was after your input on the > general approach and figured sending the patches was better than > guessing at your potential feedback. > > If a follow-up isn't of interest and you'd definitely rather take on > the work up yourself, let me know. > By all means, please go ahead. I got way too much on my plate already. Thanks, Guenter
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
> > > >
> > > > ---
> > > > 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 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}?
> >
>
> Every register/command can be implemented in the front end driver in its
> read/write
> functions. For example, see max34440_read_byte_data(), which replaces some of
> the
> status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
> PMBUS_FAN_CONFIG_34.
Right; In the cover letter I mentioned I hadn't thought about how to
implement the dual tach feature of the MAX31785 at the time. After
sending the RFC series I had go at that as well, and ended up
implementing support purely in terms of the read/write callbacks with
no modifications to the core, so I've become familiar with them.
>
> > 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.
> >
>
> Virtual registers/commands are meant to "standardize" non-standard PMBus
> commands.
>
> For
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
> > > >
> > > > ---
> > > > 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 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}?
> >
>
> Every register/command can be implemented in the front end driver in its
> read/write
> functions. For example, see max34440_read_byte_data(), which replaces some of
> the
> status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
> PMBUS_FAN_CONFIG_34.
Right; In the cover letter I mentioned I hadn't thought about how to
implement the dual tach feature of the MAX31785 at the time. After
sending the RFC series I had go at that as well, and ended up
implementing support purely in terms of the read/write callbacks with
no modifications to the core, so I've become familiar with them.
>
> > 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.
> >
>
> Virtual registers/commands are meant to "standardize" non-standard PMBus
> commands.
>
> For example, PMBus
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
---
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 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}?
Every register/command can be implemented in the front end driver in its
read/write
functions. For example, see max34440_read_byte_data(), which replaces some of
the
status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
PMBUS_FAN_CONFIG_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.
Virtual registers/commands are meant to "standardize" non-standard PMBus
commands.
For example, PMBus provides no means to read the average/minimum/maximum
temperature.
Modeling the respective attributes using PMBUS_VIRT_READ_TEMP_AVG,
PMBUS_VIRT_READ_TEMP_MIN,
and PMBUS_VIRT_READ_TEMP_MAX, and providing driver-specific read/write functions
which map the virtual commands to real chip registers makes the code much
simpler
than per-attribute callbacks. With virtual commands, the core only needs
entries such as
}, {
.reg = PMBUS_VIRT_READ_TEMP_MIN,
.attr = "lowest",
}, {
.reg = PMBUS_VIRT_READ_TEMP_AVG,
.attr = "average",
}, {
.reg = PMBUS_VIRT_READ_TEMP_MAX,
.attr = "highest",
to support such non-standard attributes. Imagine how that would look like
if each of the supported virtual commands would be implemented as callback.
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
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
---
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 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}?
Every register/command can be implemented in the front end driver in its
read/write
functions. For example, see max34440_read_byte_data(), which replaces some of
the
status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
PMBUS_FAN_CONFIG_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.
Virtual registers/commands are meant to "standardize" non-standard PMBus
commands.
For example, PMBus provides no means to read the average/minimum/maximum
temperature.
Modeling the respective attributes using PMBUS_VIRT_READ_TEMP_AVG,
PMBUS_VIRT_READ_TEMP_MIN,
and PMBUS_VIRT_READ_TEMP_MAX, and providing driver-specific read/write functions
which map the virtual commands to real chip registers makes the code much
simpler
than per-attribute callbacks. With virtual commands, the core only needs
entries such as
}, {
.reg = PMBUS_VIRT_READ_TEMP_MIN,
.attr = "lowest",
}, {
.reg = PMBUS_VIRT_READ_TEMP_AVG,
.attr = "average",
}, {
.reg = PMBUS_VIRT_READ_TEMP_MAX,
.attr = "highest",
to support such non-standard attributes. Imagine how that would look like
if each of the supported virtual commands would be implemented as callback.
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
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
> > ---
> > 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/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_INSTALLEDBUS_VIRT_
>
> Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
Yes, that's busted. Not sure what went wrong, but I'll clean it up.
>
> > > > +#define PB_FAN_CONFIG_MASK(i)
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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
> > ---
> > 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/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_INSTALLEDBUS_VIRT_
>
> Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
Yes, that's busted. Not sure what went wrong, but I'll clean it up.
>
> > > > +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 *
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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. Couple of comments below.
Guenter
Signed-off-by: Andrew Jeffery
---
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
?
/* 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_INSTALLEDBUS_VIRT_
Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
+#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)))
+
Aren't there standard bit manipulation macros for that ? Either case, this is
just to avoid
having to use the existing defines. Ok, but then I think it would make more
sense to
make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
but PB_FAN_RPM(index) everywhere.
+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)
+
Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
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. Couple of comments below.
Guenter
Signed-off-by: Andrew Jeffery
---
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
?
/* 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_INSTALLEDBUS_VIRT_
Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
+#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)))
+
Aren't there standard bit manipulation macros for that ? Either case, this is
just to avoid
having to use the existing defines. Ok, but then I think it would make more
sense to
make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
but PB_FAN_RPM(index) everywhere.
+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
[RFC PATCH 2/4] pmbus: Add fan configuration support
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
---
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);
+
+ if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
+
[RFC PATCH 2/4] pmbus: Add fan configuration support
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
---
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);
+
+ if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
+ (mode == rpm &&
