Re: [PATCH 2/3] soc: amazon: al-pos: Introduce Amazon's Annapurna Labs POS driver

[Shenhar, Talel]

On 9/9/2019 12:44 PM, Arnd Bergmann wrote:
> On Mon, Sep 9, 2019 at 11:14 AM Talel Shenhar <talel@xxxxxxxxxx> wrote:
> > The Amazon's Annapurna Labs SoCs includes Point Of Serialization error
> > logging unit that reports an error in case write error (e.g. attempt to
> > write to a read only register).
> > This patch introduces the support for this unit.
> >
> > Signed-off-by: Talel Shenhar <talel@xxxxxxxxxx>
> Looks ok overall, juts a few minor comments:
Thanks.
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Talel Shenhar");
> > +MODULE_DESCRIPTION("Amazon's Annapurna Labs POS driver");
> These usually go to the end of the file.
Ack, Will move them as part of v2.
> > +       log1 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_1);
> > +       if (!FIELD_GET(AL_POS_ERROR_LOG_1_VALID, log1))
> > +               return IRQ_NONE;
> > +
> > +       log0 = readl_relaxed(pos->mmio_base + AL_POS_ERROR_LOG_0);
> > +       writel_relaxed(0, pos->mmio_base + AL_POS_ERROR_LOG_1);
> Why do you require _relaxed() accessors here? Please add a comment
> explaining that, or use the regular readl()/writel().

I don't think commenting is needed here as there is nothing special in 
this type of access.

I don't see this is common to comment the use of the _relaxed accessors.

This driver is for SoC using arm64 cpu.

If one uses the non-relaxed version of readl while running on arm64, he 
shall cause read barrier, which is then doing dsm(ld).. This barrier is 
not needed here, so we spare the use of the more heavy readl in favor of 
the less "harmful" one.

Let me know what you think.

> > +       resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       pos->mmio_base = devm_ioremap_resource(&pdev->dev, resource);
> This can be simplified to devm_platform_ioremap_resource().
Ack, Will simplify them in v2.
> > +       pos->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> And this is usually written as platform_get_irq()
Ack, Will replace them with platform_get_irq() in v2.
>         Arnd