-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi Simon,
Thank you for awesome review!
I fully agree with most of the suggestions (for this and following patches), so to keep it short will reply only to questions (or where I disagree)
On 15.12.2015 19:58, Simon Glass wrote:
Hi Mateusz,
On 10 December 2015 at 14:41, Mateusz Kulikowski mateusz.kulikowski@gmail.com wrote:
[...]
+static int msm_serial_getc(struct udevice *dev) +{
struct msm_serial_data *p = dev_get_priv(dev);unsigned sr;char c;/* There was something buffered */if (p->chars_cnt) {c = p->chars_buf & 0xFF;p->chars_buf >>= 8;p->chars_cnt--;return c;}/* Clear error in case of buffer overrun */if (readl(p->base + UARTDM_SR) & UARTDM_SR_UART_OVERRUN)writel(UARTDM_CR_CMD_RESET_ERR, p->base + UARTDM_CR);/* We need to fetch new character */sr = readl(p->base + UARTDM_SR);/* There are at least 4 bytes in fifo */if (sr & UARTDM_SR_RX_READY) {p->chars_buf = readl(p->base + UARTDM_RF);c = p->chars_buf & 0xFF;p->chars_cnt = 3; /* 4 - one read */p->chars_buf >>= 8;return c;}/* Check if there is anything in fifo */p->chars_cnt = readl(p->base + UARTDM_RXFS);/* Extract number of characters in UART packing buffer */p->chars_cnt = (p->chars_cnt >> UARTDM_RXFS_BUF_SHIFT) &UARTDM_RXFS_BUF_MASK;if (!p->chars_cnt)return -EAGAIN;/* There is at least one charcter, move it to fifo */writel(UARTDM_CR_CMD_FORCE_STALE, p->base + UARTDM_CR);p->chars_buf = readl(p->base + UARTDM_RF);writel(UARTDM_CR_CMD_RESET_STALE_INT, p->base + UARTDM_CR);writel(0xFFFFFF, p->base + UARTDM_DMRX);c = p->chars_buf & 0xFF;p->chars_buf >>= 8;p->chars_cnt--;Can you not rationalise this code a bit? E.g.
if (no chars in fifo) { try to get some } if (no chars in fifo) return -EAGAIN extract char from fifo return ch;
You seem to have three copies of the same code.
That is good idea. Just be warned that "try to get some" will have most of the current code inside. Reason: Characters from FIFO are packed into 32-bit register. There are different ways for extracting >=4 chars, and <4 chars.
uclass_get_device_by_of_offset(UCLASS_CLK, clkd[0], &clk);Check return value. -ENODEV means there is no clk. Is it OK to have no clock?
if (clk)clk_set_periph_rate(clk, clkd[1], clk_rate);If is OK to
Of course will check ret value;
For now (at least when it comes to default UART) it's OK not to have clock, as U-Boot is chain-loaded from fastboot, but this will change once I get rid of fastboot dependency.
[...]
.ofdata_to_platdata = msm_serial_ofdata_to_platdata,.priv_auto_alloc_size = sizeof(struct msm_serial_data),.probe = msm_serial_probe,.ops = &msm_serial_ops,.flags = DM_FLAG_PRE_RELOC,Do you need this? You can specify this with u-boot,dm-pre-reloc in the device tree.
Probably not, as I do it in dts :)
Regards, Mateusz