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

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

Next message: Linus Torvalds: "Re: [PATCH RESEND v4] MAINTAINERS: fix lots of alphabetic ordering"
Previous message: Ed Swierk: "[PATCH] thermal: intel_pch_thermal: Read large temp values correctly"
In reply to: Joel Stanley: "Re: [RFC PATCH v2 1/3] hwmon: pmbus: Add fan control support"
Next in thread: Andrew Jeffery: "[RFC PATCH v2 3/3] pmbus: Add MAX31785 driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 2017-07-20 at 00:35 +0930, Joel Stanley wrote:
> > On Tue, Jul 18, 2017 at 1:06 PM, Andrew Jeffery <andrew@xxxxxxxx> 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 <andrew@xxxxxxxx>
> > ---
> >
> > 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_FAN_1_RPM,
> > +ÂÂÂÂÂÂÂPB_FAN_2_RPM,
> > +};
> > +
> > +static const int pmbus_fan_config_registers[] = {
>
> u8?
>
> > +ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_12,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_12,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_34,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_34
> > +};
> > +
> > +static const int pmbus_fan_command_registers[] = {
>
> u8?
>
> > +ÂÂÂÂÂÂÂPMBUS_FAN_COMMAND_1,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_COMMAND_2,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_COMMAND_3,
> > +ÂÂÂÂÂÂÂPMBUS_FAN_COMMAND_4,
> > +};
> > +
> > Âvoid pmbus_clear_cache(struct i2c_client *client)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct pmbus_data *data = i2c_get_clientdata(client);
> > @@ -188,6 +210,29 @@ int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word)
> > Â}
> > ÂEXPORT_SYMBOL_GPL(pmbus_write_word_data);
> >
> > +int pmbus_update_fan(struct i2c_client *client, int page, int id,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint config, int mask, int command)
> > +{
> > +ÂÂÂÂÂÂÂint from, to;
> > +ÂÂÂÂÂÂÂint rv;
> > +
> > +ÂÂÂÂÂÂÂfrom = pmbus_read_byte_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_config_registers[id]);
> > +ÂÂÂÂÂÂÂif (from < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn from;
> > +
> > +ÂÂÂÂÂÂÂto = (from & ~mask) | (config & mask);
> > +
> > +ÂÂÂÂÂÂÂrv = pmbus_write_byte_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_config_registers[id], to);
>
> to is a u8. Perhaps define it as such?
>
> > +ÂÂÂÂÂÂÂif (rv < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > +
> > +ÂÂÂÂÂÂÂreturn pmbus_write_word_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_command_registers[id], command);
>
> Similar with command - it's a u16. This would help the definition of
> pmbus_update_fan match the others in the pmbus header, which mostly
> deal with explicitly sized types. mask and config could be u8 as well
> I think.
>
> > +}
> > +EXPORT_SYMBOL_GPL(pmbus_update_fan);
> > +
> > Â/*
> > Â * _pmbus_write_word_data() is similar to pmbus_write_word_data(), but checks if
> > Â * a device specific mapping function exists and calls it if necessary.
> > @@ -197,15 +242,47 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg,
> > Â{
> > ÂÂÂÂÂÂÂÂstruct pmbus_data *data = i2c_get_clientdata(client);
> > ÂÂÂÂÂÂÂÂconst struct pmbus_driver_info *info = data->info;
> > -ÂÂÂÂÂÂÂint status;
> > +ÂÂÂÂÂÂÂint status = -ENODATA;
>
> It looks like you modify this value in all of the code paths (except
> the one I suggest you remove below).
>
> >
> > ÂÂÂÂÂÂÂÂif (info->write_word_data) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = info->write_word_data(client, page, reg, word);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (status != -ENODATA)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn status;
> > ÂÂÂÂÂÂÂÂ}
> > -ÂÂÂÂÂÂÂif (reg >= PMBUS_VIRT_BASE)
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
> > +ÂÂÂÂÂÂÂif (status == -ENODATA && reg >= PMBUS_VIRT_BASE) {
>
> status will always be -ENODATA if we get down here. I don't think you
> need to make this change.

I agree.

>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint id, bit;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂswitch (reg) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_1:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_2:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_3:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_4:
>
> I haven't read the pmbus spec (feel free to point me to a page in the
> PDF if you think it will help). Can you educate me why we have 1..4?
> for all of these virtual registers? Why not 5, or 3?

Current revisions of the PMBus spec are only available to members in
good standing. Older revisions of the PMBus spec can be found at:

http://pmbus.org/Specifications/OlderSpecifications

The current old revision is 1.2, however the MAX31785 datasheet (page
19, PMBus Protocol Support) claims it is implemented in compliance with
revision 1.1. You want PMBus Specification Part II (Command Language):

http://pmbus.org/Assets/PDFS/Public/PMBus_Specification_Part_II_Rev_1-1_20070205.pdf

Sections 14.10, 14.11 and 14.12 (pages 58-60) outline the reason for 1
- 4 here. But to summarise: PMBus commands are pagedÂ(each page exposes
an independent set of the PMBus registers), so devices aren't limited
to four fans. Rather, each page can support up to four fans, and the
PMBus spec allows up to 32 pages to be defined by a device, for a
maximum of 128 fans.

>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂid = reg - PMBUS_VIRT_FAN_TARGET_1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbit = pmbus_fan_rpm_mask[id];
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = pmbus_update_fan(client, page, id, bit, bit,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂword);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_1:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_2:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_3:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_4:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlong command = word;
>
> long seems a bit... long. And we don't need the sign. Perhaps u32?
>
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂid = reg - PMBUS_VIRT_PWM_1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbit = pmbus_fan_rpm_mask[id];
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcommand *= 100;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcommand /= 255;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = pmbus_update_fan(client, page, id, 0, bit,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcommand);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdefault:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = -ENXIO;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn status;
> > +ÂÂÂÂÂÂÂ}
> > ÂÂÂÂÂÂÂÂreturn pmbus_write_word_data(client, page, reg, word);
> > Â}
> >
> > @@ -221,6 +298,9 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)
> > Â}
> > ÂEXPORT_SYMBOL_GPL(pmbus_read_word_data);
> >
> > +static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂenum pmbus_fan_mode mode);
> > +
> > Â/*
> > Â * _pmbus_read_word_data() is similar to pmbus_read_word_data(), but checks if
> > Â * a device specific mapping function exists and calls it if necessary.
> > @@ -229,15 +309,49 @@ static int _pmbus_read_word_data(struct i2c_client *client, int page, int reg)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct pmbus_data *data = i2c_get_clientdata(client);
> > ÂÂÂÂÂÂÂÂconst struct pmbus_driver_info *info = data->info;
> > -ÂÂÂÂÂÂÂint status;
> > +ÂÂÂÂÂÂÂint status = -ENODATA;
> >
> > ÂÂÂÂÂÂÂÂif (info->read_word_data) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = info->read_word_data(client, page, reg);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (status != -ENODATA)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn status;
> > ÂÂÂÂÂÂÂÂ}
> > -ÂÂÂÂÂÂÂif (reg >= PMBUS_VIRT_BASE)
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
> > +ÂÂÂÂÂÂÂif (status == -ENODATA && reg >= PMBUS_VIRT_BASE) {
>
> Same comments as the write case.
>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂint id;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂswitch (reg) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_1:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_2:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_3:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_FAN_TARGET_4:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂid = reg - PMBUS_VIRT_FAN_TARGET_1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = pmbus_get_fan_command(client, page, id, rpm);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_1:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_2:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_3:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcase PMBUS_VIRT_PWM_4:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂlong rv;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂid = reg - PMBUS_VIRT_PWM_1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv = pmbus_get_fan_command(client, page, id, percent);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (rv < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv *= 255;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrv /= 100;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = rv;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdefault:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstatus = -ENXIO;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn status;
> > +ÂÂÂÂÂÂÂ}
> > ÂÂÂÂÂÂÂÂreturn pmbus_read_word_data(client, page, reg);
> > Â}
> >
> > @@ -304,6 +418,23 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
> > ÂÂÂÂÂÂÂÂreturn pmbus_read_byte_data(client, page, reg);
> > Â}
> >
> > +static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂenum pmbus_fan_mode mode)
> > +{
> > +ÂÂÂÂÂÂÂint config;
> > +
> > +ÂÂÂÂÂÂÂconfig = _pmbus_read_byte_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_config_registers[id]);
> > +ÂÂÂÂÂÂÂif (config < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn config;
> > +
> > +ÂÂÂÂÂÂÂif ((mode == rpm) != (!!(config & pmbus_fan_rpm_mask[id])))
>
> What's (config & mask[id]) testing? Perhaps give it a variable so it's
> obvious. Avoids the ((())) too.

I feel like that's rhetorical, however it's testing whether the
accessed attribute's mode (fanX_target: mode == fan, pwmX: mode == pwm,
captured in the 'mode' variable) matches the RPM/PWM configuration of
the hardware. PMBus only specifies a nibble's-worth of configuration
for each fan, and so packs two fan configurations into one byte-wide
register (again see sections 14.10, 14.11). Thus the mask for the
configuration register is dependent on the fan index in the page (as
opposed to accessing a different configuration register). If the
attribute mode doesn't match the hardware configuration then we can't
sensibly interpret the value in FAN_COMMAND_x.

>
> This looks like it's testing for an error case. Should you be
> returning a negative value instead of zero?

Yes it is an error case, but no - see Guenter's reply about breaking
`sensors` on the previous RFC. The commit message outlines this
behaviour in the attribute read/write tables.

>
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂÂÂreturn _pmbus_read_word_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_command_registers[id]);
> > +}
> > +
> > Âstatic void pmbus_clear_fault_page(struct i2c_client *client, int page)
> > Â{
> > ÂÂÂÂÂÂÂÂ_pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
> > @@ -489,7 +620,7 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> > ÂÂÂÂÂÂÂÂ/* X = 1/m * (Y * 10^-R - b) */
> > ÂÂÂÂÂÂÂÂR = -R;
> > ÂÂÂÂÂÂÂÂ/* scale result to milli-units for everything but fans */
>
> Does this comment need updating?

I don't think so, because the PWM class is still controlling a fan. At
least, in theory. I replied to the previous RFC about PSC_FAN, RPM and
PWM mode with respect to PSC_FAN vs PSC_PWM being a slight oddity,
which is what you've picked on here.

>
> > -ÂÂÂÂÂÂÂif (sensor->class != PSC_FAN) {
> > +ÂÂÂÂÂÂÂif (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂR += 3;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂb *= 1000;
> > ÂÂÂÂÂÂÂÂ}
> > @@ -539,6 +670,9 @@ static long pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor)
> > Â{
> > ÂÂÂÂÂÂÂÂlong val;
> >
> > +ÂÂÂÂÂÂÂif (!sensor->convert)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn sensor->data;
> > +
> > ÂÂÂÂÂÂÂÂswitch (data->info->format[sensor->class]) {
> > ÂÂÂÂÂÂÂÂcase direct:
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂval = pmbus_reg2data_direct(data, sensor);
> > @@ -642,7 +776,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> > ÂÂÂÂÂÂÂÂ}
> >
> > ÂÂÂÂÂÂÂÂ/* Calculate Y = (m * X + b) * 10^R */
> > -ÂÂÂÂÂÂÂif (sensor->class != PSC_FAN) {
> > +ÂÂÂÂÂÂÂif (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂR -= 3;ÂÂÂÂÂÂÂÂÂ/* Adjust R and b for data in milli-units */
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂb *= 1000;
> > ÂÂÂÂÂÂÂÂ}
> > @@ -673,6 +807,9 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
> > Â{
> > ÂÂÂÂÂÂÂÂu16 regval;
> >
> > +ÂÂÂÂÂÂÂif (!sensor->convert)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn val;
> > +
> > ÂÂÂÂÂÂÂÂswitch (data->info->format[sensor->class]) {
> > ÂÂÂÂÂÂÂÂcase direct:
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂregval = pmbus_data2reg_direct(data, sensor, val);
> > @@ -895,12 +1032,13 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn NULL;
> > ÂÂÂÂÂÂÂÂa = &sensor->attribute;
> >
> > -ÂÂÂÂÂÂÂsnprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂname, seq, type);
> > +ÂÂÂÂÂÂÂsnprintf(sensor->name, sizeof(sensor->name), "%s%d%s%s",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂname, seq, type ? "_" : "", type ? type : "");
>
> Perhaps something like this would be more readable:
>
> if (type)
> ÂÂÂÂÂÂÂsnprintf(sensor->name, sizeof(sensor->name), "%s%d_%s,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂname, seq, type);
> else
> ÂÂÂÂÂÂÂsnprintf(sensor->name, sizeof(sensor->name), "%s%d,
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂname, seq);
>

That was a quick hack, and I intended to polish it a bit for the non-
RFC series. Your suggestion is what I was intending to go with.

Cheers,

Andrew

>
>
> > ÂÂÂÂÂÂÂÂsensor->page = page;
> > ÂÂÂÂÂÂÂÂsensor->reg = reg;
> > ÂÂÂÂÂÂÂÂsensor->class = class;
> > ÂÂÂÂÂÂÂÂsensor->update = update;
> > +ÂÂÂÂÂÂÂsensor->convert = true;
> > ÂÂÂÂÂÂÂÂpmbus_dev_attr_init(a, sensor->name,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreadonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_show_sensor, pmbus_set_sensor);
> > @@ -1558,13 +1696,6 @@ static const int pmbus_fan_registers[] = {
> > ÂÂÂÂÂÂÂÂPMBUS_READ_FAN_SPEED_4
> > Â};
> >
> > -static const int pmbus_fan_config_registers[] = {
> > -ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_12,
> > -ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_12,
> > -ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_34,
> > -ÂÂÂÂÂÂÂPMBUS_FAN_CONFIG_34
> > -};
> > -
> > Âstatic const int pmbus_fan_status_registers[] = {
> > ÂÂÂÂÂÂÂÂPMBUS_STATUS_FAN_12,
> > ÂÂÂÂÂÂÂÂPMBUS_STATUS_FAN_12,
> > @@ -1587,6 +1718,47 @@ static const u32 pmbus_fan_status_flags[] = {
> > Â};
> >
> > Â/* Fans */
> > +static int pmbus_add_fan_ctrl(struct i2c_client *client,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct pmbus_data *data, int index, int page, int id,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂu8 config)
> > +{
> > +ÂÂÂÂÂÂÂstruct pmbus_sensor *sensor;
> > +ÂÂÂÂÂÂÂint rv;
> > +
> > +ÂÂÂÂÂÂÂrv = _pmbus_read_word_data(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_command_registers[id]);
> > +ÂÂÂÂÂÂÂif (rv < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn rv;
> > +
> > +ÂÂÂÂÂÂÂsensor = pmbus_add_sensor(data, "fan", "target", index, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂPMBUS_VIRT_FAN_TARGET_1 + id, PSC_FAN,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtrue, false);
> > +
> > +ÂÂÂÂÂÂÂif (!sensor)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂif (!data->info->m[PSC_PWM])
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂÂÂsensor = pmbus_add_sensor(data, "pwm", NULL, index, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂPMBUS_VIRT_PWM_1 + id, PSC_PWM,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtrue, false);
> > +
> > +ÂÂÂÂÂÂÂif (!sensor)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂsensor = pmbus_add_sensor(data, "pwm", "enable", index, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂPMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂtrue, false);
> > +
> > +ÂÂÂÂÂÂÂif (!sensor)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> > +
> > +ÂÂÂÂÂÂÂsensor->convert = false;
> > +
> > +ÂÂÂÂÂÂÂreturn 0;
> > +}
> > +
> > Âstatic int pmbus_add_fan_attributes(struct i2c_client *client,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct pmbus_data *data)
> > Â{
> > @@ -1624,6 +1796,15 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂPSC_FAN, true, true) == NULL)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENOMEM;
> >
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* Fan control */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (pmbus_check_word_register(client, page,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpmbus_fan_command_registers[f])) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = pmbus_add_fan_ctrl(client, data, index,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpage, f, regval);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Each fan status register covers multiple fans,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* so we have to do some magic.
> > --
> > 2.11.0
> >

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

Next message: Linus Torvalds: "Re: [PATCH RESEND v4] MAINTAINERS: fix lots of alphabetic ordering"
Previous message: Ed Swierk: "[PATCH] thermal: intel_pch_thermal: Read large temp values correctly"
In reply to: Joel Stanley: "Re: [RFC PATCH v2 1/3] hwmon: pmbus: Add fan control support"
Next in thread: Andrew Jeffery: "[RFC PATCH v2 3/3] pmbus: Add MAX31785 driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]