Date:   Sun, 7 Mar 2021 12:40:02 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Hector Martin <marcan@...can.st>,
        Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>, Rob Herring <robh@...nel.org>,
        Arnd Bergmann <arnd@...nel.org>,
        Olof Johansson <olof@...om.net>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Tony Lindgren <tony@...mide.com>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Stan Skowronek <stan@...ellium.com>,
        Alexander Graf <graf@...zon.com>,
        Will Deacon <will@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        Catalin Marinas <catalin.marinas@....com>,
        Christoph Hellwig <hch@...radead.org>,
        "David S. Miller" <davem@...emloft.net>,
        devicetree <devicetree@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        Linux Samsung SOC <linux-samsung-soc@...r.kernel.org>,
        Linux-Arch <linux-arch@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for
 Apple UARTs

On 05/03/2021 18:04, Hector Martin wrote:
> On 06/03/2021 00.28, Andy Shevchenko wrote:
>>> +       case TYPE_APPLE_S5L:
>>> +               WARN_ON(1); // No DMA
>>
>> Oh, no, please use the ONCE variant.
> 
> Thanks, changing this for v4.
> 
>>
>> ...
>>
>>> +       /* 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).

> 
>>> +       irqreturn_t ret = IRQ_NONE;
>>
>> Redundant. You may return directly.
> 
> What if both interrupts are pending?
> 
>> 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.
> 
>> Extra blank line (check your entire series for a such)
> 
> Thanks, noted. I'll check the declaration blocks in other patches.
> 
>>> +       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.
> 
>>
>> ...
>>
>>> +               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.
> 
>>> +#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

Powered by blists - more mailing lists