Hi Caleb,
On Mon, 8 Jul 2024 at 16:13, Caleb Connolly caleb.connolly@linaro.org wrote:
Hi Sumit,
Sorry for the late response.
In general, I'd rather keep this port closer to the Linux original, so that future cherry-picking of new features and fixes is easier. This is quite standard practice in U-Boot (and why the compatibility macro's are included in the first place).
Otherwise, we end up with our own quite different implementation, with the higher maintenance burden that comes with it.
This port doesn't do a perfect job at keeping close to upstream, but still I don't think we should stray further without a good justification.
As per your cover letter:
Quote: "These drivers were originally taken from Linux, however they have been heavily modified and simplified for U-Boot. Since the original Linux drivers contained heavy optimisations related to multithreading and asynchronous probing, as well as support for idle and suspend states which we don't need to deal with here."
This made me think the driver is nowhere close to Linux driver functionality wise. Also, the commit message doesn't say which Linux commit this driver is influenced from. I am not really sure if cherry-picking will be straight forward. IMHO, if we are heavily modifying a driver port for U-Boot then it should be done properly as per U-Boot needs. A middle ground approach which you have taken is only going to lead to confusion and rather negatively impact maintenance.
-Sumit
Kind regards,
+EXPORT_SYMBOL(cmd_db_read_addr);
Export symbols aren't required in U-Boot here and other instances in this patch.
...
/** Wait until we read back the same value. Use a counter rather than* ktime for timeout since this may be called after timekeeping stops.*/for (i = 0; i < USEC_PER_SEC; i++) {if (readl(addr) == data)return;udelay(1);}Can we rather use readx_poll_sleep_timeout() here instead?
...
for (i = 0; i < 5 * USEC_PER_SEC; i++) {if (readl(tcs_reg_addr(drv, reg, tcs_id)) & BIT(tcs_id))break;udelay(1);}Can we rather use readx_poll_sleep_timeout() here instead?
...
spin_lock_irqsave(&drv->lock, flags);Locks aren't needed in U-Boot, can be dropped here and other places.
...
spin_lock_init(&drv->lock);init_waitqueue_head(&drv->tcs_wait);Similarly waitqueue should be dropped too.
-Sumit
-- // Caleb (they/them)