Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Tue, Nov 23, 2021 at 4:03 PM Mark Brown <broonie@kernel.org> wrote:
> On Tue, Nov 23, 2021 at 12:09:54AM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 22, 2021 at 4:34 AM LH.Kuo <lhjeff911@gmail.com> wrote:
>
> > > +// slave only. usually called on driver remove
>
> > Why is it so?
> > Also find use of proper English grammar (capitalization, periods, etc.
> > Ditto for all your comments.
>
> Please don't go overboard on changes you're requesting, the important
> thing with comments is that they're intelligible.  People have different
> levels of skill with English and that's totally fine, it's much better
> that people feel able to write comments than that they stop doing so
> because they are concerned about issues with their foreign language
> skills.

Shame on me! I'm also bad at English (okay, sometimes).

...

> > > +       unsigned long flags;
> > > +       struct sp7021_spi_ctlr *pspim = dev;
> > > +       u32 fd_status = 0;
> > > +       unsigned int tx_len, rx_cnt, tx_cnt, total_len;
> > > +       bool isrdone = false;
>
> > Reversed xmas tree order here and everywhere else.
>
> Again, please don't go overboard - this isn't a general requirement for
> kernel code, some parts of the kernel do want it but outside of those
> areas it's not something that we should be asking for (and TBH even when
> it is required you should explain what it is, it's not as easy to search
> for as it could be).  I certainly don't care.

Here it is. The "reversed xmas tree order" implies that longer lines
in the definition block, where one puts all variables that are being
used locally in the given function, are going first followed by
shorter ones. This makes it a bit easier to read the code. There are
also additional rules that may be applied, such as defines with
assignments _usually_ go before anything else, error code variable
separately and last. That said, the above might look better in the
following form:

       struct sp7021_spi_ctlr *pspim = dev;
       unsigned int tx_len, total_len;
       unsigned int rx_cnt, tx_cnt;
       unsigned long flags;
       bool isrdone = false;
       u32 fd_status = 0;

...

> > > +               if (of_property_read_bool(pdev->dev.of_node, "spi-slave"))
> > > +                       mode = SP7021_SLAVE_MODE;
>
> > There is no need to check of_node for this call.
>
> OTOH if we are checking it anyway it doesn't hurt to have all the
> property reads in the check for of_node.

I assumed that previous comment against SPI ID potentially will be
addressed by removing that code which in the result gives unnecessary
use of the of_node check above. So that's why I pointed this out.

>  Either way it doesn't
> fundamentally matter.

True!

-- 
With Best Regards,
Andy Shevchenko