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

From: Mark Brown <broonie@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "LH.Kuo" <lhjeff911@gmail.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	dvorkin@tibbo.com, qinjian@cqplus1.com, wells.lu@sunplus.com,
	"LH.Kuo" <lh.kuo@sunplus.com>
Subject: Re: [PATCH v3 1/2] SPI: Add SPI driver for Sunplus SP7021
Date: Tue, 23 Nov 2021 14:03:11 +0000	[thread overview]
Message-ID: <YZz0n6Mpjl3tKmMe@sirena.org.uk> (raw)
In-Reply-To: <CAHp75Vd2=OHbrpGtsU8AMXdtNfvSPhpc7vhzkWnahaV48XbfUQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

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.  

> > +       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.

> > +               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.  Either way it doesn't
fundamentally matter.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]