Re: [PATCH v8 3/3] hwmon: (aspeed-g6-pwm-tacho): Support for ASPEED g6 PWM/Fan tach

[Billy Tsai]

On 8/30/23 20:32, Billy Tsai wrote:
>> +static int aspeed_tach_hwmon_write(struct device *dev,
>> +                                enum hwmon_sensor_types type, u32 attr,
>> +                                int channel, long val)
>> +{
>> +     struct aspeed_pwm_tach_data *priv = dev_get_drvdata(dev);
>> +     u32 reg_val;
>> +
>> +     switch (attr) {
>> +     case hwmon_fan_div:
>> +             if (!is_power_of_2(val) || (ilog2(val) % 2) ||
>> +                 DIV_TO_REG(val) > 0xb)
>> +                     return -EINVAL;
>> +             priv->tach_divisor = val;
>> +             reg_val = readl(priv->base + TACH_ASPEED_CTRL(channel));
>> +             reg_val &= ~TACH_ASPEED_CLK_DIV_T_MASK;
>> +             reg_val |= FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK,
>> +                                  DIV_TO_REG(priv->tach_divisor));

> Hi Billy,
> I notice the fanX_div is always shows 1 after I set 1024.
> I think FIELD_GET() needs to replaced with FIELD_PREP().

>> +             writel(reg_val, priv->base + TACH_ASPEED_CTRL(channel));
>> +             break;
>> +     default:
>> +             return -EOPNOTSUPP;
>> +     }
>> +
>> +     return 0;
>> +}


>> +static void aspeed_present_fan_tach(struct aspeed_pwm_tach_data *priv, u32 tach_ch)
>> +{
>> +     u32 val;
>> +
>> +     priv->tach_present[tach_ch] = true;
>> +     priv->tach_divisor = DEFAULT_TACH_DIV;
>> +
>> +     val = readl(priv->base + TACH_ASPEED_CTRL(tach_ch));
>> +     val &= ~(TACH_ASPEED_INVERS_LIMIT | TACH_ASPEED_DEBOUNCE_MASK |
>> +              TACH_ASPEED_IO_EDGE_MASK | TACH_ASPEED_CLK_DIV_T_MASK |
>> +              TACH_ASPEED_THRESHOLD_MASK);
>> +     val |= (DEBOUNCE_3_CLK << TACH_ASPEED_DEBOUNCE_BIT) | F2F_EDGES |
>> +            FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK,
>> +                      DIV_TO_REG(priv->tach_divisor));

> And here as well.

>> +     writel(val, priv->base + TACH_ASPEED_CTRL(tach_ch));
>> +
>> +     aspeed_tach_ch_enable(priv, tach_ch, true);
>> +}
>> +
>>

Hi Potin,

I will fix it in next verison of the patch.

Thanks for reviewing.