On 10/7/19 6:52 AM, Sa, Nuno wrote:
[ ... ]
+static long axi_fan_control_get_pwm_duty(const struct
axi_fan_control_data *ctl)
+{
+ u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
+ u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
ctl);
+
+ return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
pwm_period);
Is pwm_period guaranteed to be != 0 ?
Yes, It is a RO register and it is set by the core with the default of
0x4e20.
Trusting the hardware doesn't make me too comfortable. Are we sure at all
times that the HW isn't messed up ? If so, please at least add a comment
stating that the HW will never return 0. We can then fix it after we get
the first crash report from the field ;-).
[ ... ]
+ if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
+ /* hardware requested a new pwm */
+ ctl->hw_pwm_req = true;
+
I don't really understand the logic here. If
ADI_IRQ_SRC_TEMP_INCREASE means
that hardware wants a new pwm, how is userspace informed about that
request ?
It isn't. Userspace would have to read the pwm attribute and figure
that changed. Should I use something like sysfs_notify() on the pwm
attribute?
That might make sense.
And why are the tacho paramaters _not_ updated in this case later on
(unless
ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
It might be useful to describe the expected sequence of events.
The core can change the PWM by itself (which is when we receive
ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
values to evaluate the tacho signal (so it won't use the values on
TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can request a
new PWM by writing the pwm attribute. In this case the CORE is
expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise it
won't evaluate the tacho signal. Note that when is the user which
requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +
if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
when do I have to update the tacho parameters.
Wondering ... if setting the pwm requires an update of period and tolerance,
why not set update_tacho_params to true when the pwm value is written, or
update the registers directly instead of waiting for an interrupt ?
Either case, I think the above sequence of events should be explained
in the driver for future developers to understand why the code is written
the way it is.
)
+ if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
+ ctl->fan_fault = 1;
Is it on purpose that this bit is never reset ?
Yes, and it is wrong. I though that the core would never clear this
alarm but it does clear it in the next temperature reading cycle (and
set it again if needed). Then, would a clear on read be a correct
approach?
Not sure if there is a "correct", but I think it would make sense.
+
+ /* clear all interrupts */
+ clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
+ axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
+
+ return IRQ_HANDLED;
+}
+
+static int axi_fan_control_init(struct axi_fan_control_data *ctl,
+ const struct device_node *np)
+{
+ int ret;
+
+ /* get fan pulses per revolution */
+ ret = of_property_read_u32(np, "adi,pulses-per-revolution",
&ctl->ppr);
+ if (ret)
+ return ret;
So all random values are acceptable, including 0 and 0xffffffff ?
Yes, I'm aware that 1 and 2 are typical values but I'm not sure what is
the maximum that typically exists so I didn't want to put limits here
without knowing. Though at least 0 must not be accepted since then we
are always dividing by 0 when reading the FAN rpm.
The only values I am aware of are 2 and 4. I don't recall seeing any fans
with 1 pulse per revolution. Overall, I don't think values other than 1, 2,
and 4 make sense.
Guenter
