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
