On 19-08-16 04:54, Guenter Roeck wrote:
> On 08/16/2016 12:53 AM, Mike Looijmans wrote:
>> The fan = can be stopped by writing "0" to fan1_target in sysfs.
>>
&= gt;> Writing non-zero values to fan1_target or pwm1 in sysfs automatical= ly
>> selects the corresponding control mode (closed or open loo= p).
>>
>> This allows userspace applications to contr= ol the fan speed without
>> the need to know specific details of= the controller (like the fact
>> that fan1_target does not take= effect when pwm1_enable is set to
>> anything but "2"). Early i= nitialization of the fan controller prevents
>> overheating, for= example when resetting the board while the fan was
>> completel= y turned off.
>>
>
> But this is the normal hwmo= n ABI. It should not be a surprise.
> On the other side, changing t= he mode automatically as side effect
> _is_ a surprise.

= I was under the impression that the pwm*_enable=3D2 mode was intended for <= br />temperature controlled fans. Reading the docs for a bunch of drivers r= eveals
that apart from "0=3Dfull on" and "1=3Dmanual pwm" there is no= standard whatsoever
for the meaning of this value, each driver assig= ns its own, forcing userspace
to know all the details of a particular= model. So indeed, I'll have to change
that. I'll have to wri= te software specific for this fan then, it cannot be
generic with the= current ABI.

I was actually aiming for supporting "off" mode, s= o I could later (when the
new hardware arrives) also control a relate= d power supply from this driver.

Guess I should just add a mode = "3" to implement "off".

>
>> Signed-off-by: Mike L= ooijmans <mike.looijmans@topic.nl>
>> ---
>> Th= is patch requires the devicetree support patch for the max6650.
>&g= t;
>> Changes the functionality and interface of the driver, to = be able to
>> initialize the chip at boot, and allows userspace = control without
>> requiring hardware knowledge.
>>>> .../devicetree/bindings/hwmon/max6650.txt   &n= bsp;      |   5 +
>> &nb= sp;Documentation/hwmon/max6650        &n= bsp;            = ;   |   5 +-
>> drivers/hwmon/max= 6650.c            &n= bsp;            = ;   | 153 +++++++++++++--------
>> 3 files = changed, 106 insertions(+), 57 deletions(-)
>>
>> dif= f --git a/Documentation/devicetree/bindings/hwmon/max6650.txt
>>= b/Documentation/devicetree/bindings/hwmon/max6650.txt
>> index = 2e46e69..724ab3a 100644
>> --- a/Documentation/devicetree/bindin= gs/hwmon/max6650.txt
>> +++ b/Documentation/devicetree/bindings/= hwmon/max6650.txt
>> @@ -13,6 +13,10 @@ Optional properties, def= ault is to retain the chip's
>> current setting:
>&g= t; - maxim,fan-prescale : Pre-scaling value, as per datasheet [= 1]. Lower values
>>        &n= bsp;     allow more fine-grained control of slower= fans.
>>          =     Valid: 1, 2, 4, 8, 16.
>> +- maxim,fan-t= arget-rpm: Initial requested fan rotation speed. If specified,
>>= ; the
>> +          = ;  driver selects closed-loop mode and the requested speed.
= >> +           &nbs= p;This ensures the fan is already running before userspace
>> + =            takes ove= r.
>>
>
> Needs to be separate patch and be appr= oved by devicetree maintainers. Overall,
> it would be better to ma= ke have the devicetree changces as single patch.

Okay.

How do they get synchronized then, i.e. what if the documentation gets approved but the actual implementation does not?

>> &n= bsp;Example:
>>      fan-max6650: max66= 50@1b {
>> @@ -20,4 +24,5 @@ Example:
>>   =        compatible =3D "maxim,max6650";>>          maxim,fa= n-microvolt =3D <12000000>;
>>     &nb= sp;    maxim,fan-prescale =3D <4>;
>> = +        maxim,fan-target-rpm =3D <12= 00>;
>>      };
>> diff -= -git a/Documentation/hwmon/max6650 b/Documentation/hwmon/max6650
>&= gt; index 58d9644..53e308b 100644
>> --- a/Documentation/hwmon/m= ax6650
>> +++ b/Documentation/hwmon/max6650
>> @@ -33= ,9 +33,12 @@ fan2_input    ro    "
>&= gt; fan3_input    ro    "
>>= fan4_input    ro    "
>> &n= bsp;fan1_target    rw    desired fan speed in= RPM (closed loop mode only)
>> +      =       Set to 0 to stop the fan. Writing any o= ther value sets
>> +        &= nbsp;   the regulator mode to "closed loop".
>> &= nbsp;pwm1_enable    rw    regulator mode, 0= =3Dfull on, 1=3Dopen loop, 2=3Dclosed loop
>> pwm1 &= nbsp;     rw    relative speed (0-2= 55), 255=3Dmax. speed.
>> -       =      Used in open loop mode only.
>> + =            Set to 0 = to stop the fan. Writing any other value sets
>> +   &= nbsp;        the regulator mode to = "open loop".
>
> The mode should really be selected using p= wm1_enable. I don't like the idea
> of automatically changing i= t as side effect.

As above, will do.

>> f= an1_div    rw    sets the speed range the inp= uts can handle. Legal
>>       &nb= sp;      values are 1, 2, 4, and 8. Use lower= values for
>>         &= nbsp;    faster fans.
>> diff --git a/driver= s/hwmon/max6650.c b/drivers/hwmon/max6650.c
>> index 6beb62c..d4= 0756a 100644
>> --- a/drivers/hwmon/max6650.c
>> +++ = b/drivers/hwmon/max6650.c
>> @@ -185,6 +185,35 @@ static struct = max6650_data
>> *max6650_update_device(struct device *dev)
= >>      return data;
>> }>>
>> +static bool max6650_is_powerdown(const struct m= ax6650_data *data)
>> +{
>> +    retur= n (data->config & MAX6650_CFG_MODE_MASK) =3D=3D MAX6650_CFG_MODE_OFF= ;
>> +}
>> +
>> +/*
>> + * Chan= ge the operating mode of the chip (if needed).
>> + * mode is on= e of the MAX6650_CFG_MODE_* values.
>> + */
>> +stati= c int max6650_set_operating_mode(struct max6650_data *data, u8 mode)
&= gt;> +{
>> +    int result;
>> + &n= bsp;  u8 config =3D data->config;
>> +
>>= ; +    if (mode =3D=3D (config & MAX6650_CFG_MODE_MASK))=
>> +        return 0;
&= gt;> +
>> +    config =3D (config & ~MAX66= 50_CFG_MODE_MASK) | mode;
>> +
>> +    = ;result =3D i2c_smbus_write_byte_data(data->client, MAX6650_REG_CONFIG,<= br />>> +           = ;            co= nfig);
>> +    if (result < 0)
>> +=        return result;
>> +>> +    data->config =3D config;
>> = +
>> +    return 0;
>> +}
>>= ; +
>> static ssize_t get_fan(struct device *dev, struct d= evice_attribute *devattr,
>>       = ;          char *buf)
>> {
>> @@ -258,26 +287,26 @@ static ssize_t get_t= arget(struct device *dev, struct
>> device_attribute *devattr,>>       *    FanSpe= ed =3D KSCALE x fCLK / [256 x (KTACH + 1)]
>>    =    *
>>       * the= n multiply by 60 to give rpm.
>> +     *
>> +     * When not running, report target to b= e "0".
>>       */
>>>> -    kscale =3D DIV_FROM_REG(data->config);<= br />>> -    ktach =3D data->speed;
>> -=    rpm =3D 60 * kscale * clock / (256 * (ktach + 1));
= >> +    if (max6650_is_powerdown(data))
>> = +        rpm =3D 0;
>> + &nbs= p;  else {
>> +       &n= bsp;kscale =3D DIV_FROM_REG(data->config);
>> +   &= nbsp;    ktach =3D data->speed;
>> + &nbs= p;      rpm =3D 60 * kscale * clock / (256 * = (ktach + 1));
>> +    }
>>   = ;   return sprintf(buf, "%d\n", rpm);
>> }<= br />>>
>> -static ssize_t set_target(struct device *dev, = struct device_attribute
>> *devattr,
>> -   = ;          const char *bu= f, size_t count)
>> +static int max6650_set_target(struct max665= 0_data *data, unsigned long rpm)
>> {
>> - &nbs= p;  struct max6650_data *data =3D dev_get_drvdata(dev);
>= > -    struct i2c_client *client =3D data->client;
>>      int kscale, ktach;
>> -=    unsigned long rpm;
>> -    int= err;
>>
>> -    err =3D kstrtoul(buf,= 10, &rpm);
>> -    if (err)
>> - =        return err;
>> + = ;  if (rpm =3D=3D 0)
>> +     &nbs= p;  return max6650_set_operating_mode(data, MAX6650_CFG_MODE_OFF)= ;
>>
>>      rpm =3D clamp_v= al(rpm, FAN_RPM_MIN, FAN_RPM_MAX);
>>
>> @@ -288,8 +3= 17,6 @@ static ssize_t set_target(struct device *dev, struct
>> = device_attribute *devattr,
>>      &nbs= p;*     KTACH =3D [(fCLK x KSCALE) / (256 x FanSpeed)] = - 1
>>       */
>>
>> -    mutex_lock(&data->update_lock);
&= gt;> -
>>      kscale =3D DIV_FROM_R= EG(data->config);
>>      ktach =3D = ((clock * kscale) / (256 * rpm / 60)) - 1;
>>    =   if (ktach < 0)
>> @@ -298,7 +325,25 @@ static ssi= ze_t set_target(struct device *dev, struct
>> device_attribute *= devattr,
>>         &nbs= p;ktach =3D 255;
>>      data->speed= =3D ktach;
>>
>> -    i2c_smbus_write= _byte_data(client, MAX6650_REG_SPEED, data->speed);
>> + &nbs= p;  i2c_smbus_write_byte_data(data->client, MAX6650_REG_SPEED,= data->speed);
>> +
>> +    return = max6650_set_operating_mode(data, MAX6650_CFG_MODE_CLOSED_LOOP);
>&g= t; +}
>> +
>> +static ssize_t set_target(struct devic= e *dev, struct device_attribute
>> *devattr,
>> + &nb= sp;           const = char *buf, size_t count)
>> +{
>> +    = ;struct max6650_data *data =3D dev_get_drvdata(dev);
>> + =   unsigned long rpm;
>> +    int err;>> +
>> +    err =3D kstrtoul(buf, 10, = &rpm);
>> +    if (err)
>> + = ;      return err;
>> +
>&= gt; +    mutex_lock(&data->update_lock);
>>= ; +
>> +    err =3D max6650_set_target(data, rpm)= ;
>>
>>      mutex_unlock(&a= mp;data->update_lock);
>>
>> @@ -320,17 +365,21 @@= static ssize_t get_pwm(struct device *dev, struct
>> device_att= ribute *devattr,
>>      int pwm;
= >>      struct max6650_data *data =3D max665= 0_update_device(dev);
>>
>> -    /*>> -     * Useful range for dac is 0-180 for 1= 2V fans and 0-76 for 5V fans.
>> -     * Low= er DAC values mean higher speeds.
>> -     *= /
>> -    if (data->config & MAX6650_CFG_V= 12)
>> -        pwm =3D 255 -= (255 * (int)data->dac)/180;
>> -    else
>> -        pwm =3D 255 - (255 * = (int)data->dac)/76;
>> -
>> -    if= (pwm < 0)
>> +    if (max6650_is_powerdown(da= ta))
>>          pw= m =3D 0;
>> +    else {
>> + &nb= sp;     /*
>> +     = ;    * Useful range for dac is 0-180 for 12V fans and 0= -76 for 5V
>> +         = * fans. Lower DAC values mean higher speeds.
>> +   &n= bsp;     */
>> +    &nbs= p;   if (data->config & MAX6650_CFG_V12)
>>= ; +            pwm = =3D 255 - (255 * (int)data->dac)/180;
>> +    =     else
>> +      =       pwm =3D 255 - (255 * (int)data->dac)= /76;
>> +
>> +       &n= bsp;if (pwm < 0)
>> +       &nb= sp;    pwm =3D 0;
>> +    }>>
>>      return sprintf(buf= , "%d\n", pwm);
>> }
>> @@ -351,16 +400,20 @@ s= tatic ssize_t set_pwm(struct device *dev, struct
>> device_attri= bute *devattr,
>>
>>      mu= tex_lock(&data->update_lock);
>>
>> - &n= bsp; if (data->config & MAX6650_CFG_V12)
>> - =       data->dac =3D 180 - (180 * pwm)/255;=
>> -    else
>> -    &= nbsp;   data->dac =3D 76 - (76 * pwm)/255;
>> = -
>> -    i2c_smbus_write_byte_data(client, MAX66= 50_REG_DAC, data->dac);
>> +    if (pwm =3D=3D= 0)
>> +        err =3D max66= 50_set_operating_mode(data, MAX6650_CFG_MODE_OFF);
>> + &n= bsp; else {
>> +        = if (data->config & MAX6650_CFG_V12)
>> +   &nbs= p;        data->dac =3D 180 - (1= 80 * pwm)/255;
>> +        el= se
>> +          &n= bsp; data->dac =3D 76 - (76 * pwm)/255;
>> +   = ;     i2c_smbus_write_byte_data(client, MAX6650_RE= G_DAC, data->dac);
>> +       &= nbsp;err =3D max6650_set_operating_mode(data, MAX6650_CFG_MODE_OPEN_LOOP);<= br />>> +    }
>>
>>   = ;   mutex_unlock(&data->update_lock);
>>>> -    return count;
>> +   = return err < 0 ? err : count;
>> }
>><= br />>> /*
>> @@ -385,25 +438,24 @@ static ssize_t s= et_enable(struct device *dev, struct
>> device_attribute *devatt= r,
>>          &nbs= p;     const char *buf, size_t count)
>>= ; {
>>      struct max6650_data *= data =3D dev_get_drvdata(dev);
>> -    struct i2c= _client *client =3D data->client;
>> -    int = max6650_modes[3] =3D {0, 3, 2};
>>      = ;unsigned long mode;
>>      int err;>> +    const u8 max6650_modes[3] =3D {
>&= gt; +        MAX6650_CFG_MODE_ON,
&= gt;> +        MAX6650_CFG_MODE_OPEN_L= OOP,
>> +        MAX6650_CFG_= MODE_CLOSED_LOOP
>> +        = };
>>
>>      err =3D kstrto= ul(buf, 10, &mode);
>>      if (err= )
>>          retur= n err;
>>
>> -    if (mode > 2)
>> +    if (mode >=3D ARRAY_SIZE(max6650_modes))<= br />>>          return = -EINVAL;
>>
>>      mutex_lo= ck(&data->update_lock);
>>
>> -   &n= bsp;data->config =3D i2c_smbus_read_byte_data(client, MAX6650_REG_CONFIG= );
>> -    data->config =3D (data->config &= amp; ~MAX6650_CFG_MODE_MASK)
>> -      =          | (max6650_modes[mode= ] << 4);
>> -
>> -    i2c_smbus_= write_byte_data(client, MAX6650_REG_CONFIG, data->config);
>>= +    max6650_set_operating_mode(data, max6650_modes[mode]);=
>>
>>      mutex_unlock(&am= p;data->update_lock);
>>
>> @@ -582,6 +634,7 @@ st= atic int max6650_init_client(struct max6650_data *data,
>> = ;    int err =3D -EIO;
>>    =   u32 voltage;
>>      u32 pr= escale;
>> +    u32 target_rpm;
>>
>>      if (of_property_read_u32(dev->o= f_node, "maxim,fan-microvolt",
>>      =             &nb= sp;&voltage))
>> @@ -642,22 +695,6 @@ static int max6650_ini= t_client(struct max6650_data *data,
>>     &= nbsp;     (config & MAX6650_CFG_V12) ? 12 : 5,=
>>           = 1 << (config & MAX6650_CFG_PRESCALER_MASK));
>>
&= gt;> -    /*
>> -     * If= mode is set to "full off", we change it to "open loop" and
>> -=     * set DAC to 255, which has the same effect. We do= this because
>> -     * there's no "ful= l off" mode defined in hwmon specifications.
>> -   &n= bsp; */
>> -
>> -    if ((config = & MAX6650_CFG_MODE_MASK) =3D=3D MAX6650_CFG_MODE_OFF) {
>> -=        dev_dbg(dev, "Change mode to ope= n loop, full off.\n");
>> -       = config =3D (config & ~MAX6650_CFG_MODE_MASK)
>> - &nbs= p;           | MAX66= 50_CFG_MODE_OPEN_LOOP;
>> -       = if (i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, 255)) {
&= gt;> -            = ;dev_err(dev, "DAC write error, aborting.\n");
>> -   =          return err;
>= > -        }
>> - &n= bsp; }
>> -
>>      if = (i2c_smbus_write_byte_data(client, MAX6650_REG_CONFIG, config)) {
>= >          dev_err(dev, "Co= nfig write error, aborting.\n");
>>     &nbs= p;    return err;
>> @@ -666,6 +703,10 @@ st= atic int max6650_init_client(struct max6650_data *data,
>> = ;    data->config =3D config;
>> &n= bsp;   data->count =3D i2c_smbus_read_byte_data(client, M= AX6650_REG_COUNT);
>>
>> +    if (!of_= property_read_u32(client->dev.of_node, "maxim,fan-target-rpm",
>= > +            &n= bsp;    &target_rpm))
>> +   &= nbsp;    max6650_set_target(data, target_rpm);
>= ;> +
>>      return 0;
>>= }
>>
>>
>

TOPIC Products
Mate= riaalweg 4
5681 RJ Best	T:	+31 = (0) 499 33 69 69
Post= bus 440	E:	mike.looijmans@topicproducts.com
5680= AK Best	W:	www.topicproducts.com
The Netherlands