On 05/03/2021 18:04, Hector Martin wrote:

On 06/03/2021 00.28, Andy Shevchenko wrote:

quoted

quoted

+       case TYPE_APPLE_S5L:
+               WARN_ON(1); // No DMA

Oh, no, please use the ONCE variant.

Thanks, changing this for v4.

quoted

...

quoted

+       /* Apple types use these bits for IRQ masks */
+       if (ourport->info->type != TYPE_APPLE_S5L) {
+               ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK |
+                               S3C64XX_UCON_EMPTYINT_EN |
+                               S3C64XX_UCON_DMASUS_EN |
+                               S3C64XX_UCON_TIMEOUT_EN);
+               ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT |

Can you spell 0xf with named constant(s), please?

In case they are repetitive via the code, introduce either a temporary
variable (in case it scoped to one function only), or define it as a
constant.

I'm just moving this code; as far as I can tell this is a timeout value 
(so just an integer), but I don't know if there is any special meaning 
to 0xf here. Note that this codepath is for *non-Apple* chips, as the 
Apple ones don't even have this field (at least not here).

I agree here with Hector. Andi, you propose here unrelated change (which
without documentation might not be doable by Hector).

quoted

quoted

+       irqreturn_t ret = IRQ_NONE;

Redundant. You may return directly.

What if both interrupts are pending?

quoted

No IO serialization?

There is no DMA on the Apple variants (as far as I know; it's not 
implemented anyway), so there is no need for serializing IO with DMA. In 
any case, dealing with that is the DMA code's job, the interrupt handler 
shouldn't need to care.

If you mean serializing IO with the IRQ: CPU-wise, I would hope that's 
the irqchip's job (AIC does this with a readl on the event). If you mean 
ensuring all writes are complete (i.e. posted write issue), on the Apple 
chips everything is non-posted as explained in the previous patches.

quoted

Extra blank line (check your entire series for a such)

Thanks, noted. I'll check the declaration blocks in other patches.

quoted

quoted

+       ourport->rx_enabled = 1;
+       ourport->tx_enabled = 0;

How are these protected against race?

The serial core should be holding the port mutex for pretty much every 
call into the driver, as far as I can tell.

quoted

...

quoted

+               case TYPE_APPLE_S5L: {
+                       unsigned int ucon;
+                       int ret;
+
+                       ret = clk_prepare_enable(ourport->clk);
+                       if (ret) {
+                               dev_err(dev, "clk_enable clk failed: %d\n", ret);
+                               return ret;
+                       }
+                       if (!IS_ERR(ourport->baudclk)) {
+                               ret = clk_prepare_enable(ourport->baudclk);
+                               if (ret) {
+                                       dev_err(dev, "clk_enable baudclk failed: %d\n", ret);
+                                       clk_disable_unprepare(ourport->clk);
+                                       return ret;
+                               }
+                       }

Wouldn't it be better to use CLK bulk API?

Ah, I guess that could save a line or two of code here, even though it 
requires setting up the array. I'll give it a shot.

quoted

quoted

+#ifdef CONFIG_ARCH_APPLE

Why? Wouldn't you like the one kernel to work on many SoCs?

This *adds* Apple support, it is not mutually exclusive with all the 
other SoCs. You can enable all of those options and get a driver that 
works on all of them. This is the same pattern used throughout the 
driver for all the other Samsung variants. There is no reason to have 
Apple SoC support in the samsung driver if the rest of the kernel 
doesn't have Apple SoC support either, of course.

How ifdef on ARCH_APLLE makes it non-working on many SoCs? All new
platforms are multi... The true question is - do the ifdefs in the code
make it more difficult to read/review?

Best regards,
Krzysztof