From: Mark Brown <broonie@kernel.org>
To: Rich Felker <dalias@libc.org>
Cc: linux-kernel@vger.kernel.org, linux-sh@vger.kernel.org,
linux-spi@vger.kernel.org
Subject: Re: [PATCH v2 10/12] spi: add driver for J-Core SPI controller
Date: Fri, 20 May 2016 11:23:17 +0100 [thread overview]
Message-ID: <20160520102317.GH8206@sirena.org.uk> (raw)
In-Reply-To: <2e287ca758002621ef8eed3db9df37678e26af5e.1463708766.git.dalias@libc.org>
[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]
On Fri, May 20, 2016 at 02:53:04AM +0000, Rich Felker wrote:
> Signed-off-by: Rich Felker <dalias@libc.org>
> ---
> My previous post of the patch series accidentally omitted omitted
> Cc'ing of subsystem maintainers for the necessary clocksource,
> irqchip, and spi drivers. Please ack if this looks ok because I want
> to get it merged as part of the arch/sh pull request for 4.7.
This is *extremely* late for a first posting of a driver for v4.7 (you
missed the list as well as the maintainers).
> +static void jcore_spi_chipsel(struct spi_device *spi, bool value)
> +{
> + struct jcore_spi *hw = spi_master_get_devdata(spi->master);
> +
> + pr_debug("%s: CS=%d\n", __func__, value);
dev_dbg()
> +
> + hw->csReg = ( JCORE_SPI_CTRL_ACS | JCORE_SPI_CTRL_CCS | JCORE_SPI_CTRL_DCS )
> + ^ (!value << 2*spi->chip_select);
Why the xor here and not an or? The coding style is also weird, a mix
of extra spaces around the () and missing ones around *. I'm finding
the intent of the code confusing here.
> +static int jcore_spi_txrx(struct spi_master *master, struct spi_device *spi, struct spi_transfer *t)
Coding style, please keep lines under 80 columns unless there's a good
reason.
> +#if !USE_MESSAGE_MODE
> + spi_finalize_current_transfer(master);
> +#endif
I'm not sure what the if is about but it doesn't belong upstream, you
shouldn't be open coding bits of the framework.
> + /* register our spi controller */
> + err = spi_register_master(master);
devm_
> +static int jcore_spi_remove(struct platform_device *dev)
> +{
> + struct jcore_spi *hw = platform_get_drvdata(dev);
> + struct spi_master *master = hw->master;
> +
> + platform_set_drvdata(dev, NULL);
> + spi_master_put(master);
> + return 0;
> +}
This can be removed entirely.
> +static const struct of_device_id jcore_spi_of_match[] = {
> + { .compatible = "jcore,spi2" },
> + {},
> +};
This is adding a DT binding with no binding document. All new DT
bindings need to be documented.
> + .owner = THIS_MODULE,
> + .pm = NULL,
No need to set either of these.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
next prev parent reply other threads:[~2016-05-20 10:23 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-20 2:53 [PATCH v2 00/12] J-core J2 cpu and SoC peripherals support Rich Felker
2016-05-20 2:53 ` [PATCH v2 02/12] of: add J-Core cpu bindings Rich Felker
2016-05-23 20:48 ` Rob Herring
2016-05-23 21:03 ` Rich Felker
2016-05-23 23:29 ` Rob Herring
2016-05-24 2:39 ` Rich Felker
2016-05-24 21:30 ` Rob Landley
2016-05-25 1:13 ` Rob Herring
2016-05-25 2:33 ` Rich Felker
2016-05-25 13:13 ` Rob Herring
2016-05-20 2:53 ` [PATCH v2 01/12] of: add vendor prefix for J-Core Rich Felker
2016-05-23 20:49 ` Rob Herring
2016-05-20 2:53 ` [PATCH v2 08/12] irqchip: add J-Core AIC driver Rich Felker
2016-05-20 8:08 ` Geert Uytterhoeven
2016-05-20 8:15 ` Marc Zyngier
2016-05-25 4:29 ` Rich Felker
2016-05-20 2:53 ` [PATCH v2 03/12] of: add J-Core interrupt controller bindings Rich Felker
2016-05-20 8:04 ` Geert Uytterhoeven
2016-05-20 22:34 ` Rich Felker
2016-05-21 18:07 ` Geert Uytterhoeven
2016-05-21 19:17 ` Rich Felker
2016-05-23 20:53 ` Rob Herring
2016-05-23 21:13 ` Rich Felker
2016-05-24 8:09 ` Marc Zyngier
2016-05-25 2:25 ` Rich Felker
2016-05-20 2:53 ` [PATCH v2 06/12] sh: add support for J-Core J2 processor Rich Felker
2016-05-20 2:53 ` [PATCH v2 11/12] sh: add defconfig for J-Core J2 Rich Felker
2016-05-20 2:53 ` [PATCH v2 09/12] clocksource: add J-Core PIT/RTC driver Rich Felker
2016-05-20 14:01 ` Daniel Lezcano
2016-05-21 3:15 ` Rich Felker
2016-05-21 15:55 ` Rob Landley
2016-05-23 20:32 ` Daniel Lezcano
2016-05-24 2:25 ` Rich Felker
2016-05-20 2:53 ` [PATCH v2 04/12] of: add J-Core timer bindings Rich Felker
2016-05-20 8:03 ` Geert Uytterhoeven
2016-05-20 2:53 ` [PATCH v2 12/12] sh: add device tree source for J2 FPGA on Mimas v2 board Rich Felker
2016-05-20 8:17 ` Geert Uytterhoeven
2016-05-20 22:42 ` Rich Felker
2016-05-20 2:53 ` [PATCH v2 10/12] spi: add driver for J-Core SPI controller Rich Felker
2016-05-20 8:15 ` Geert Uytterhoeven
2016-05-20 22:50 ` Rich Felker
2016-05-20 10:23 ` Mark Brown [this message]
2016-05-20 23:24 ` Rich Felker
2016-05-23 15:30 ` Mark Brown
2016-05-23 20:29 ` Rich Felker
2016-05-23 22:11 ` Mark Brown
2016-05-20 2:53 ` [PATCH v2 07/12] sh: add AT_HWCAP flag for J-Core cas.l instruction Rich Felker
2016-05-20 2:53 ` [PATCH v2 05/12] of: add J-Core SPI master bindings Rich Felker
2016-05-20 8:05 ` Geert Uytterhoeven
2016-05-23 21:00 ` Rob Herring
2016-05-23 21:06 ` Rich Felker
2016-05-23 23:16 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160520102317.GH8206@sirena.org.uk \
--to=broonie@kernel.org \
--cc=dalias@libc.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sh@vger.kernel.org \
--cc=linux-spi@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).