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

From: Andrew Jeffery
Date: Wed Jul 12 2017 - 03:01:41 EST

Next message: Daniel Vetter: "Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths"
Previous message: Geert Uytterhoeven: "[PATCH] mm: Mark create_huge_pmd() inline to prevent build failure"
In reply to: Guenter Roeck: "Re: [RFC PATCH 2/4] pmbus: Add fan configuration support"
Next in thread: Guenter Roeck: "Re: [RFC PATCH 2/4] pmbus: Add fan configuration support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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):
> > > > ÂÂÂÂÂÂÂÂÂÂÂ0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> > > > ÂÂÂÂÂÂÂÂÂÂÂ0x2711 - 0x7fff: 100% fan PWM duty cycle
> > > > ÂÂÂÂÂÂÂÂÂÂÂ0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
> > > >
> > > > RPM (m = 1, b = 0, R = 0):
> > > > ÂÂÂÂÂÂÂÂÂÂÂ0x0000 - 0x7FFF: 0 - 32,767 RPM
> > > > ÂÂÂÂÂÂÂÂÂÂÂ0x8000 - 0xFFFF: 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 <andrew@xxxxxxxx>
> > > >
> > > > ---
> > > > ÂÂÂ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 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.

Indeed. Hence RFC in case I had overlooked something :) Clearly I have.

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

>
> > >
> > > > > > > > > > > > ÂÂÂ /* 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_INSTALLED PB_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 * !((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.
> >
> > As I store the configuration for each fan in a struct pmbus_fan_ctrl
> > dedicated to the fan, I reasoned that intermediate code should not have
>
> I rather wonder if pmbus_fan_ctrl is needed in the first place. The notion of
> local 'struct pmbus_sensor' variables seems really messy. I think I'll really
> have to spend some time on this to see if and how it can be simplified;
> it just should not be that complex.

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.

Thanks again for taking a look.

Andrew

>
> Thanks,
> Guenter
>
> > to deal with the details of which nibble to access with respect to the
> > fan's (per-page) ID. Rather, code reading or writing
> > PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
> > are provided.
> >
> > > 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.
> >
> > I'll make the change throughout pmbus core.
> >
> > >
> > > > +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(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> > > > > > > > > > > > + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
> > >
> > > Not create the attribute in question in the first place, or return 0. The above
> > > messes up the 'sensors' command.
> >
> > I think returning 0 is the only valid option of the two, given that we
> > can dynamically switch between RPM and PWM modes.
> >
> > Thanks for the feedback.
> >
> > Andrew
> >
> > >
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + sensor.class = PSC_FAN;
> > > > > > > > > > > > + if (mode == percent)
> > > > > > > > > > > > + sensor.data = fan->command * 255 / 100;
> > > > > > > > > > > > + else
> > > > > > + sensor.data = fan->command;
> > > >
> > > > +
> > > > > > + val = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_fan_target(struct device *dev,
> > > > > > + ÂÂÂÂÂstruct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, rpm,
> > > > > > + ÂÂÂÂÂÂfan_target_to_pmbus_fan_ctrl(da), buf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm(struct device *dev,
> > > > > > + ÂÂÂÂÂÂstruct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + ÂÂÂÂÂÂbuf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > > > > > > > > > + ÂÂÂÂÂenum pmbus_fan_mode mode,
> > > > > > > > > > > > + ÂÂÂÂÂstruct pmbus_fan_ctrl *fan,
> > > > > > + ÂÂÂÂÂconst char *buf, ssize_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv;
> > > > > > + long val;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &val) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > + val = pmbus_data2reg(data, &sensor, val);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == percent)
> > > > > > + val = val * 100 / 255;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == rpm)
> > > > > > > > > > > > + fan->config |= PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + else
> > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + ÂÂÂÂPB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + ÂÂÂÂPB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->command = val;
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + ÂÂÂfan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_target(struct device *dev,
> > > > > > > > > > > > + ÂÂÂÂstruct device_attribute *da,
> > > > > > + ÂÂÂÂconst char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, rpm,
> > > > > > > > > > > > + ÂÂÂÂÂfan_target_to_pmbus_fan_ctrl(da), buf,
> > > > > > + ÂÂÂÂÂcount);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> > > > > > + ÂÂÂÂÂconst char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + ÂÂÂÂÂbuf, count);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > > > + ÂÂÂÂÂstruct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > +
> > > > > > > > > > > > + if (data->info->get_pwm_mode) {
> > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > >
> > > > +
> > > > > > > > > > > > + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + struct pmbus_sensor sensor = {
> > > > > > > > > > > > + .class = PSC_FAN,
> > > > > > > > > > > > + .data = fan->command,
> > > > > > > > > > > > + };
> > > > > > + long command;
> > > >
> > > > +
> > > > > > + command = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > > > > > > > + /* XXX: Need to do something sensible */
> > > > > > > > > > > > + if (fan->config & PB_FAN_CONFIG_RPM)
> > > > > > > > > > > > + mode = 2;
> > > > > > > > > > > > + else
> > > > > > > > > > > > + mode = (command >= 0 && command < 100);
> > > > > > + }
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > > > > > > > > > + ÂÂÂÂstruct device_attribute *da,
> > > > > > + ÂÂÂÂconst char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv = count;
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &mode) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (data->info->set_pwm_mode) {
> > > > > > > > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > > > + u16 command = fan->command;
> > > >
> > > > +
> > > > > > > > > > > > + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> > > > > > > > > > > > + fan->command = command;
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + switch (mode) {
> > > > > > > > > > > > + case 0:
> > > > > > > > > > > > + case 1:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = pmbus_data2reg(data, &sensor, 100);
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + case 2:
> > > > > > > > > > > > + default:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = 0xffff;
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + }
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + ÂÂÂÂPB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + ÂÂÂÂPB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + ÂÂÂfan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > ÂÂÂstatic int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> > > > ÂÂÂ{
> > > > > > ÂÂÂ if (data->num_attributes >= data->max_attributes - 1) {
> > > >
> > > > @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> > > > > > ÂÂÂ return 0;
> > > >
> > > > ÂÂÂ}
> > > > ÂÂÂ
> > > > +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> > > > > > > > > > > > + ÂÂÂstruct pmbus_fan_ctrl_attr *fa,
> > > > > > > > > > > > + ÂÂÂconst char *name_fmt,
> > > > > > > > > > > > + ÂÂÂssize_t (*show)(struct device *dev,
> > > > > > > > > > > > + ÂÂÂstruct device_attribute *attr,
> > > > > > > > > > > > + ÂÂÂchar *buf),
> > > > > > > > > > > > + ÂÂÂssize_t (*store)(struct device *dev,
> > > > > > > > > > > > + ÂÂÂstruct device_attribute *attr,
> > > > > > > > > > > > + ÂÂÂconst char *buf, size_t count),
> > > > > > + ÂÂÂint idx)
> > > >
> > > > +{
> > > > > > + struct device_attribute *da;
> > > >
> > > > +
> > > > > > + da = &fa->attribute;
> > > >
> > > > +
> > > > > > > > > > > > + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> > > > > > + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> > > >
> > > > +
> > > > > > + return pmbus_add_attribute(data, &da->attr);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> > > > > > + ÂÂÂÂstruct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> > > > > > > > > > > > + ÂÂÂÂÂÂÂpmbus_show_fan_target,
> > > > > > + ÂÂÂÂÂÂÂpmbus_set_fan_target, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> > > > > > + ÂÂÂÂÂstruct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > +
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> > > > > > + ÂÂÂÂÂÂÂpmbus_set_pwm, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> > > > > > + ÂÂÂÂstruct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> > > > > > > > > > > > + ÂÂÂÂÂÂÂpmbus_show_pwm_enable,
> > > > > > + ÂÂÂÂÂÂÂpmbus_set_pwm_enable, fan->index);
> > > >
> > > > +}
> > > > +
> > > > ÂÂÂstatic const struct pmbus_limit_attr vin_limit_attrs[] = {
> > > > > > > > > > > > ÂÂÂ {
> > > > > > ÂÂÂ .reg = PMBUS_VIN_UV_WARN_LIMIT,
> > > >
> > > > @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> > > > > > ÂÂÂ PMBUS_FAN_CONFIG_34
> > > >
> > > > ÂÂÂ};
> > > > ÂÂÂ
> > > > +static const int pmbus_fan_command_registers[] = {
> > > > > > > > > > > > + PMBUS_FAN_COMMAND_1,
> > > > > > > > > > > > + PMBUS_FAN_COMMAND_2,
> > > > > > > > > > > > + PMBUS_FAN_COMMAND_3,
> > > > > > + PMBUS_FAN_COMMAND_4,
> > > >
> > > > +};
> > > > +
> > > > ÂÂÂstatic const int pmbus_fan_status_registers[] = {
> > > > > > > > > > > > ÂÂÂ PMBUS_STATUS_FAN_12,
> > > > > > ÂÂÂ PMBUS_STATUS_FAN_12,
> > > >
> > > > @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> > > > > > + int rv;
> > > >
> > > > +
> > > > > > > > > > > > + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> > > > > > > > > > > > + if (!fan)
> > > > > > + return -ENOMEM;
> > > >
> > > > +
> > > > > > > > > > > > + fan->index = index;
> > > > > > > > > > > > + fan->page = page;
> > > > > > > > > > > > + fan->id = id;
> > > > > > + fan->config = config;
> > > >
> > > > +
> > > > > > > > > > > > + rv = _pmbus_read_word_data(client, page,
> > > > > > > > > > > > + pmbus_fan_command_registers[id]);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > > > > > > > + return rv;
> > > > > > + fan->command = rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_fan_target_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_pwm_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return pmbus_add_pwm_enable_attr(data, fan);
> > > >
> > > > +}
> > > > +
> > > > ÂÂÂstatic int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > ÂÂÂ ÂÂÂÂstruct pmbus_data *data)
> > > >
> > > > ÂÂÂ{
> > > > @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > > > > > > > ÂÂÂ ÂÂÂÂÂPSC_FAN, true, true) == NULL)
> > > > > > ÂÂÂ return -ENOMEM;
> > > >
> > > > ÂÂÂ
> > > > > > > > > > > > + 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.
>
>

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

Next message: Daniel Vetter: "Re: [PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths"
Previous message: Geert Uytterhoeven: "[PATCH] mm: Mark create_huge_pmd() inline to prevent build failure"
In reply to: Guenter Roeck: "Re: [RFC PATCH 2/4] pmbus: Add fan configuration support"
Next in thread: Guenter Roeck: "Re: [RFC PATCH 2/4] pmbus: Add fan configuration support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]