On 13. 06. 25 9:13 odp., Vadim Fedorenko wrote:
+ synth->enabled = FIELD_GET(ZL_SYNTH_CTRL_EN, synth_ctrl);
+ synth->dpll = FIELD_GET(ZL_SYNTH_CTRL_DPLL_SEL, synth_ctrl);
+
+ dev_dbg(zldev->dev, "SYNTH%u is %s and driven by DPLL%u\n", index,
+ synth->enabled ? "enabled" : "disabled", synth->dpll);
+
+ guard(mutex)(&zldev->multiop_lock);
Not a strong suggestion, but it would be good to follow netdev style
(same for some previous functions):
Hi Vadim,
I'm using guard() on places (functions) where it is necessary to hold
the lock from that place to the end of the function. Due to this
scoped_guard() does not give any advantage. Using classic mutex_lock()
and mutex_unlock() would only increases the risks of locking-related
bugs. Also manual locking enforces to use mutex_unlock() or goto in
all error paths after taking lock.
https://docs.kernel.org/process/maintainer-netdev.html#using-device-
managed-and-cleanup-h-constructs
"Use of guard() is discouraged within any function longer than 20 lines,
scoped_guard() is considered more readable. Using normal lock/unlock is
still (weakly) preferred."
+
+ /* Read synth configuration */
+ rc = zl3073x_mb_op(zldev, ZL_REG_SYNTH_MB_SEM, ZL_SYNTH_MB_SEM_RD,
+ ZL_REG_SYNTH_MB_MASK, BIT(index));
+ if (rc)
+ return rc;
+
+ /* The output frequency is determined by the following formula:
+ * base * multiplier * numerator / denominator
+ *
+ * Read registers with these values
+ */
+ rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_BASE, &base);
+ if (rc)
+ return rc;
+
+ rc = zl3073x_read_u32(zldev, ZL_REG_SYNTH_FREQ_MULT, &mult);
+ if (rc)
+ return rc;
+
+ rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_M, &m);
+ if (rc)
+ return rc;
+
+ rc = zl3073x_read_u16(zldev, ZL_REG_SYNTH_FREQ_N, &n);
+ if (rc)
+ return rc;
+
---> You have to keep the lock to here.
+ /* Check denominator for zero to avoid div by 0 */
+ if (!n) {
+ dev_err(zldev->dev,
+ "Zero divisor for SYNTH%u retrieved from device\n",
+ index);
+ return -EINVAL;
+ }
+
+ /* Compute and store synth frequency */
+ zldev->synth[index].freq = div_u64(mul_u32_u32(base * m, mult), n);
+
+ dev_dbg(zldev->dev, "SYNTH%u frequency: %u Hz\n", index,
+ zldev->synth[index].freq);
+
+ return rc;
+}
This kind of function (above) is mailbox-read:
1. Take lock
2. Ask firmware to fill mailbox latch registers
3. Read latch1
4. ...
5. Unlock
But in later commits there are mailbox-write functions that:
1. Take lock
2. Ask firmware to fill mailbox latch registers
3. Write or read-update-write latch registers
4. ...
5. Ask firmware to update HW from the latch registers (commit)
6. Unlock
Step 5 here is usually represented by:
return zl3073x_mb_op(zldev, ZL_REG_*_MB_SEM, ZL_*_MB_SEM_RD,
ZL_REG_*_MB_MASK, BIT(index));
and here is an advantage of guard() that unlocks the mutex automatically
after zl3073x_mb_op() and prior returning its return value.
Thanks,
Ivan
