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

[Sam Protsenko <semen.protsenko@xxxxxxxxxx>]

On Fri, 21 Jan 2022 at 19:26, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
>
> On Fri, Jan 21, 2022 at 1:52 PM Sam Protsenko
> <semen.protsenko@xxxxxxxxxx> wrote:
> > On Wed, 19 Jan 2022 at 01:11, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> > >
> > > Convert the S3C64xx SPI host to use GPIO descriptors.
> > >
> > > Provide GPIO descriptor tables for the one user with CS
> > > 0 and 1.
>
> ...
>
> > >         /* Configure feedback delay */
> > > -       writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
> > > +       if (!cs)
> > > +               /* No delay if not defined */
> > > +               writel(0, sdd->regs + S3C64XX_SPI_FB_CLK);
> > > +       else
> > > +               writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
> >
> > Looks good to me. I'd add {} braces and change "if (!cs)" to "if
> > (cs)", but that's hair splitting and not worth v4, it's fine as it is.
>
> If you are going to change code, then why not use positive conditions?
>
>   if (cs)
>     ...
>   else {
>     ...
>   }
>

It was already pointed out earlier AFAIR. Personally I don't think
it's too major to send v4. But theoretically speaking, maybe it would
be even better this way:

    u32 fb_delay = cs ? cs->fb_delay & 0x3 : 0;

    /* Configure feedback delay */
    writel(fb_delay, sdd->regs + S3C64XX_SPI_FB_CLK);

Again, I think it's not worth another submission, the same code will
be generated, and style is ok as it is.

> --
> With Best Regards,
> Andy Shevchenko