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 10:23:17 +0000 [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 --]
WARNING: multiple messages have this Message-ID (diff)
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: 107+ 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-20 2:53 ` Rich Felker [not found] ` <f3c89e4834665790ff13478c571cc9aaa9de5559.1463708766.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-05-23 20:48 ` Rob Herring 2016-05-23 20:48 ` Rob Herring 2016-05-23 20:48 ` Rob Herring 2016-05-23 21:03 ` Rich Felker 2016-05-23 21:03 ` Rich Felker 2016-05-23 23:29 ` Rob Herring 2016-05-23 23:29 ` Rob Herring 2016-05-24 2:39 ` Rich Felker 2016-05-24 2:39 ` Rich Felker 2016-05-24 21:30 ` Rob Landley 2016-05-24 21:30 ` Rob Landley 2016-05-25 1:13 ` Rob Herring 2016-05-25 1:13 ` Rob Herring 2016-05-25 2:33 ` Rich Felker 2016-05-25 2:33 ` Rich Felker 2016-05-25 13:13 ` Rob Herring 2016-05-25 13:13 ` Rob Herring 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 8:04 ` Geert Uytterhoeven [not found] ` <CAMuHMdVs9=8BGwmCBuYch2abJBnCLKoEx7i3EMghW3UUjXc7dA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-05-20 22:34 ` Rich Felker 2016-05-20 22:34 ` Rich Felker 2016-05-20 22:34 ` Rich Felker 2016-05-21 18:07 ` Geert Uytterhoeven 2016-05-21 18:07 ` Geert Uytterhoeven 2016-05-21 19:17 ` Rich Felker 2016-05-21 19:17 ` Rich Felker 2016-05-23 20:53 ` Rob Herring 2016-05-23 20:53 ` Rob Herring 2016-05-23 21:13 ` Rich Felker 2016-05-23 21:13 ` Rich Felker 2016-05-23 21:13 ` Rich Felker [not found] ` <20160523211342.GG21636-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org> 2016-05-24 8:09 ` Marc Zyngier 2016-05-24 8:09 ` Marc Zyngier 2016-05-24 8:09 ` Marc Zyngier [not found] ` <57440C45.5040105-5wv7dgnIgG8@public.gmane.org> 2016-05-25 2:25 ` Rich Felker 2016-05-25 2:25 ` Rich Felker 2016-05-25 2:25 ` Rich Felker 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:08 ` Geert Uytterhoeven 2016-05-20 8:15 ` Marc Zyngier 2016-05-20 8:15 ` Marc Zyngier 2016-05-25 4:29 ` Rich Felker 2016-05-25 4:29 ` Rich Felker [not found] ` <cover.1463708766.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-05-20 2:53 ` [PATCH v2 01/12] of: add vendor prefix for J-Core Rich Felker 2016-05-20 2:53 ` Rich Felker 2016-05-20 2:53 ` Rich Felker 2016-05-23 20:49 ` Rob Herring 2016-05-23 20:49 ` Rob Herring 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 2:53 ` Rich Felker 2016-05-20 2:53 ` Rich Felker [not found] ` <9960d6526523727d1bee3d11c6704e09e600c6fe.1463708766.git.dalias-8zAoT0mYgF4@public.gmane.org> 2016-05-20 8:17 ` Geert Uytterhoeven 2016-05-20 8:17 ` Geert Uytterhoeven 2016-05-20 8:17 ` Geert Uytterhoeven 2016-05-20 22:42 ` Rich Felker 2016-05-20 22:42 ` 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 ` 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 04/12] of: add J-Core timer bindings Rich Felker 2016-05-20 8:03 ` Geert Uytterhoeven 2016-05-20 8:03 ` Geert Uytterhoeven 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-20 14:01 ` Daniel Lezcano 2016-05-21 3:15 ` Rich Felker 2016-05-21 3:15 ` Rich Felker 2016-05-21 15:55 ` Rob Landley 2016-05-21 15:55 ` Rob Landley 2016-05-23 20:32 ` Daniel Lezcano 2016-05-23 20:32 ` Daniel Lezcano 2016-05-24 2:25 ` Rich Felker 2016-05-24 2:25 ` Rich Felker 2016-05-20 2:53 ` [PATCH v2 10/12] spi: add driver for J-Core SPI controller Rich Felker 2016-05-20 2:53 ` Rich Felker 2016-05-20 8:15 ` Geert Uytterhoeven 2016-05-20 8:15 ` Geert Uytterhoeven 2016-05-20 22:50 ` Rich Felker 2016-05-20 22:50 ` Rich Felker 2016-05-20 10:23 ` Mark Brown [this message] 2016-05-20 10:23 ` Mark Brown 2016-05-20 23:24 ` Rich Felker 2016-05-20 23:24 ` Rich Felker 2016-05-23 15:30 ` Mark Brown 2016-05-23 15:30 ` Mark Brown 2016-05-23 20:29 ` Rich Felker 2016-05-23 20:29 ` Rich Felker [not found] ` <20160523202938.GD21636-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org> 2016-05-23 22:11 ` Mark Brown 2016-05-23 22:11 ` Mark Brown 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-20 8:05 ` Geert Uytterhoeven 2016-05-20 8:05 ` Geert Uytterhoeven 2016-05-23 21:00 ` Rob Herring 2016-05-23 21:00 ` Rob Herring 2016-05-23 21:06 ` Rich Felker 2016-05-23 21:06 ` Rich Felker [not found] ` <20160523210618.GF21636-C3MtFaGISjmo6RMmaWD+6Sb1p8zYI1N1@public.gmane.org> 2016-05-23 23:16 ` Rob Herring 2016-05-23 23:16 ` Rob Herring 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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.