Re: [PATCH 3/3 v2] spi: s3c64xx: Convert to use GPIO descriptors

[Sam Protsenko <semen.protsenko@xxxxxxxxxx>]

On Tue, 18 Jan 2022 at 18:08, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Tue, Jan 18, 2022 at 4:27 PM Sam Protsenko
> <semen.protsenko@xxxxxxxxxx> wrote:
>
> > > +       if (!cs)
> > > +               return 0;
> >
> > Shouldn't some error code be returned in that case? Or is it normal case?
>
> No, because it just means the platform didn't define any fb_delay.
>
> > >         /* Configure feedback delay */
> > >         writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
>
> But maybe I should always write 0 into this then.
>

Yeah, probably makes sense, as some other slave node may have
configured fb_delay on the same bus / driver instance. And of course 0
is correct value, just checked in Exynos850 TRM, means "do not use
this feedback clock".

> > > - * This is per SPI-Slave Chipselect information.
> > > + * This is per SPI-Slave FB delay information.
> >
> > Not sure if this change is needed: this comments will be incorrect if
> > some new fields are added in future. For example, downstream trees
> > like [1] have more fields there, not only feedback delay one.
>
> OK I fix
>
> > Other than minor comments above:
> >
> > Reviewed-by: Sam Protsenko <semen.protsenko@xxxxxxxxxx>
> >
> > Btw, this SPI driver is used in Exynos850 SoC, which I'm working on
> > right now. It's the shame I didn't enable it yet on my board, so I
> > can't test the code. But I'm happy to see it's being modernized, kudos
> > for doing that!
>
> Yeah I saw it being used in the Tesla SoC and also Exynos850 so as
> I now know this is not going away any time soon we might as well just
> fix it up.
>
> Yours,
> Linus Walleij