From: Mark Brown <broonie@kernel.org> To: "Andreas Färber" <afaerber@suse.de> Cc: netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, "Jian-Hong Pan" <starnight@g.ncu.edu.tw>, "Jiri Pirko" <jiri@resnulli.us>, "Marcel Holtmann" <marcel@holtmann.org>, "David S . Miller" <davem@davemloft.net>, "Matthias Brugger" <mbrugger@suse.com>, "Janus Piwek" <jpiwek@arroweurope.com>, "Michael Röder" <michael.roeder@avnet.eu>, "Dollar Chen" <dollar.chen@wtmec.com>, "Ken Yu" <ken.yu@rakwireless.com>, "Ben Whitten" <ben.whitten@lairdtech.com>, "Steve deRosier" <derosier@gmail.com>, linux-spi@vger.kernel.org, LoRa_Community_Support@semtech.com Subject: Re: [RFC net-next 15/15] net: lora: Add Semtech SX1301 Date: Tue, 3 Jul 2018 15:50:22 +0100 [thread overview] Message-ID: <20180703145022.GA13744@sirena.org.uk> (raw) In-Reply-To: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de> [-- Attachment #1: Type: text/plain, Size: 3135 bytes --] On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas Färber wrote: > Hi Mark, > > This driver is still evolving, there's newer code on my lora-next branch > already: https://github.com/afaerber/linux/commits/lora-next Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > 2) This SPI device is in turn exposing the two SPI masters that you > already found below, and I didn't see a sane way to split that code out > into drivers/spi/, so it's in drivers/net/lora/ here - has there been > any precedence either way? A MFD? > Am 02.07.2018 um 18:12 schrieb Mark Brown: > > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas Färber wrote: > >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > >> +{ > >> + int ret; > >> + > >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > >> + > >> + if (enable) > >> + return; > > So we never disable chip select? > Not here, I instead did that in transfer_one below. That's obviously at best going to be fragile, you're implementing half the operation here and half somewhere else which is most likely going to break at some point when the framework changes. > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio address write failed\n"); > >> + return ret; > >> + } > >> + > >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio data write failed\n"); > >> + return ret; > >> + } > > This looks awfully like you're coming in at the wrong abstraction layer > > and the hardware actually implements a register abstraction rather than > > a SPI one so you should be using regmap as the abstraction. > I don't understand. Ben has suggested using regmap for the SPI _device_ > that we're talking to, which may be a good idea. But this SX1301 device > in turn has two SPI _masters_ talking to an SX125x slave each. I don't > see how using regmap instead of my wrappers avoids this spi_controller? It seems obvious from the code that this isn't actually interacting with a SPI controller, you're writing an address and a value to a register map rather than dealing with a byte stream. There may be a SPI bus somewhere behind some other hardware but you don't seem to be interacting with it as such. > The whole point of this spi_controller is to abstract and separate the > SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, > instead of mixing it into the SX1301 driver - to me that looks cleaner > and more extensible. It also has the side-effect that we could configure > the two radios via DT (frequencies, clk output, etc.). A register map would work just as well here, we already have plenty of devices that abstract at this level (most obviously the I2C/SPI devices that use it to offer both interfaces with a single core driver). [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: broonie@kernel.org (Mark Brown) To: linux-arm-kernel@lists.infradead.org Subject: [RFC net-next 15/15] net: lora: Add Semtech SX1301 Date: Tue, 3 Jul 2018 15:50:22 +0100 [thread overview] Message-ID: <20180703145022.GA13744@sirena.org.uk> (raw) In-Reply-To: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de> On Mon, Jul 02, 2018 at 07:34:21PM +0200, Andreas F?rber wrote: > Hi Mark, > > This driver is still evolving, there's newer code on my lora-next branch > already: https://github.com/afaerber/linux/commits/lora-next Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed. > 2) This SPI device is in turn exposing the two SPI masters that you > already found below, and I didn't see a sane way to split that code out > into drivers/spi/, so it's in drivers/net/lora/ here - has there been > any precedence either way? A MFD? > Am 02.07.2018 um 18:12 schrieb Mark Brown: > > On Sun, Jul 01, 2018 at 01:08:04PM +0200, Andreas F?rber wrote: > >> +static void sx1301_radio_spi_set_cs(struct spi_device *spi, bool enable) > >> +{ > >> + int ret; > >> + > >> + dev_dbg(&spi->dev, "setting SPI CS to %s\n", enable ? "1" : "0"); > >> + > >> + if (enable) > >> + return; > > So we never disable chip select? > Not here, I instead did that in transfer_one below. That's obviously at best going to be fragile, you're implementing half the operation here and half somewhere else which is most likely going to break at some point when the framework changes. > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio address write failed\n"); > >> + return ret; > >> + } > >> + > >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_DATA, (tx_buf && xfr->len >= 2) ? tx_buf[1] : 0); > >> + if (ret) { > >> + dev_err(&spi->dev, "SPI radio data write failed\n"); > >> + return ret; > >> + } > > This looks awfully like you're coming in at the wrong abstraction layer > > and the hardware actually implements a register abstraction rather than > > a SPI one so you should be using regmap as the abstraction. > I don't understand. Ben has suggested using regmap for the SPI _device_ > that we're talking to, which may be a good idea. But this SX1301 device > in turn has two SPI _masters_ talking to an SX125x slave each. I don't > see how using regmap instead of my wrappers avoids this spi_controller? It seems obvious from the code that this isn't actually interacting with a SPI controller, you're writing an address and a value to a register map rather than dealing with a byte stream. There may be a SPI bus somewhere behind some other hardware but you don't seem to be interacting with it as such. > The whole point of this spi_controller is to abstract and separate the > SX1255 vs. SX1257 vs. whatever-radio-attached into a separate driver, > instead of mixing it into the SX1301 driver - to me that looks cleaner > and more extensible. It also has the side-effect that we could configure > the two radios via DT (frequencies, clk output, etc.). A register map would work just as well here, we already have plenty of devices that abstract at this level (most obviously the I2C/SPI devices that use it to offer both interfaces with a single core driver). -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180703/3ba28a7e/attachment.sig>
next prev parent reply other threads:[~2018-07-03 14:51 UTC|newest] Thread overview: 173+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-01 11:07 [RFC net-next 00/15] net: A socket API for LoRa Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 01/15] net: Reserve protocol numbers " Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 02/15] net: lora: Define sockaddr_lora Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 03/15] net: lora: Add protocol numbers Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 04/15] net: Add lora subsystem Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 05/15] HACK: net: lora: Deal with .poll_mask in 4.18-rc2 Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-02 16:22 ` Jiri Pirko 2018-07-02 16:22 ` Jiri Pirko 2018-07-02 16:59 ` Andreas Färber 2018-07-02 16:59 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 06/15] net: lora: Prepare for device drivers Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 07/15] net: lora: Add Semtech SX1276 Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 12:02 ` Andreas Färber 2018-07-01 12:02 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 08/15] net: lora: sx1276: Add debugfs Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-02 16:26 ` Jiri Pirko 2018-07-02 16:26 ` Jiri Pirko 2018-07-02 17:57 ` Andreas Färber 2018-07-02 17:57 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 09/15] net: lora: Prepare EUI helpers Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:07 ` [RFC net-next 10/15] net: lora: Add Microchip RN2483 Andreas Färber 2018-07-01 11:07 ` Andreas Färber 2018-07-01 11:08 ` [RFC net-next 11/15] net: lora: Add IMST WiMOD Andreas Färber 2018-07-01 11:08 ` Andreas Färber 2019-01-06 14:57 ` Heinrich Schuchardt 2019-01-06 14:57 ` Heinrich Schuchardt 2019-01-07 11:29 ` Andreas Färber 2019-01-07 11:29 ` Andreas Färber 2018-07-01 11:08 ` [RFC net-next 12/15] net: lora: Add USI WM-SG-SM-42 Andreas Färber 2018-07-01 11:08 ` Andreas Färber 2018-07-01 11:08 ` [RFC net-next 13/15] net: lora: Prepare RAK RAK811 Andreas Färber 2018-07-01 11:08 ` Andreas Färber 2018-07-01 11:08 ` [RFC net-next 14/15] net: lora: Prepare Semtech SX1257 Andreas Färber 2018-07-01 11:08 ` Andreas Färber 2018-07-01 11:08 ` [RFC net-next 15/15] net: lora: Add Semtech SX1301 Andreas Färber 2018-07-01 11:08 ` Andreas Färber 2018-07-02 11:51 ` Ben Whitten 2018-07-02 11:51 ` Ben Whitten 2018-07-03 3:01 ` Andreas Färber 2018-07-03 3:01 ` Andreas Färber 2018-07-05 8:59 ` Ben Whitten 2018-07-05 8:59 ` Ben Whitten 2018-07-05 8:59 ` Ben Whitten 2018-07-02 16:12 ` Mark Brown 2018-07-02 16:12 ` Mark Brown 2018-07-02 16:12 ` Mark Brown 2018-07-02 17:34 ` Andreas Färber 2018-07-02 17:34 ` Andreas Färber 2018-07-02 20:43 ` Ben Whitten 2018-07-02 20:43 ` Ben Whitten 2018-07-03 3:21 ` Andreas Färber 2018-07-03 3:21 ` Andreas Färber 2018-07-03 3:21 ` Andreas Färber 2018-07-05 8:43 ` Ben Whitten 2018-07-05 8:43 ` Ben Whitten 2018-07-05 8:43 ` Ben Whitten 2018-07-03 14:50 ` Mark Brown [this message] 2018-07-03 14:50 ` Mark Brown 2018-07-03 15:09 ` Andreas Färber 2018-07-03 15:09 ` Andreas Färber 2018-07-03 15:09 ` Andreas Färber 2018-07-03 15:31 ` Mark Brown 2018-07-03 15:31 ` Mark Brown 2018-07-03 15:31 ` Mark Brown 2018-07-03 16:40 ` Andreas Färber 2018-07-03 16:40 ` Andreas Färber 2018-07-04 11:43 ` Mark Brown 2018-07-04 11:43 ` Mark Brown 2018-07-04 13:41 ` Ben Whitten 2018-07-04 13:41 ` Ben Whitten 2018-07-04 13:41 ` Ben Whitten 2018-07-04 14:32 ` Mark Brown 2018-07-04 14:32 ` Mark Brown 2018-07-04 14:32 ` Mark Brown 2018-07-03 15:11 ` [RFC net-next 00/15] net: A socket API for LoRa Jian-Hong Pan 2018-07-03 15:11 ` Jian-Hong Pan 2018-07-03 15:11 ` Jian-Hong Pan 2018-08-05 0:11 ` Andreas Färber 2018-08-05 0:11 ` Andreas Färber 2018-08-05 0:11 ` Andreas Färber 2018-08-08 20:36 ` Alan Cox 2018-08-08 20:36 ` Alan Cox 2018-08-08 20:36 ` Alan Cox 2018-08-08 22:42 ` Andreas Färber 2018-08-08 22:42 ` Andreas Färber 2018-08-08 22:42 ` Andreas Färber 2018-08-09 11:59 ` Alan Cox 2018-08-09 11:59 ` Alan Cox 2018-08-09 11:59 ` Alan Cox 2018-08-09 15:02 ` Jian-Hong Pan 2018-08-09 15:02 ` Jian-Hong Pan 2018-08-09 15:02 ` Jian-Hong Pan 2018-08-09 15:21 ` Alexander Aring 2018-08-09 15:21 ` Alexander Aring 2018-08-09 15:21 ` Alexander Aring 2018-08-10 15:57 ` Alan Cox 2018-08-10 15:57 ` Alan Cox 2018-08-10 15:57 ` Alan Cox 2018-08-11 18:30 ` Stefan Schmidt 2018-08-11 18:30 ` Stefan Schmidt 2018-08-11 18:30 ` Stefan Schmidt 2018-08-12 16:49 ` Andreas Färber 2018-08-12 16:49 ` Andreas Färber 2018-08-12 16:49 ` Andreas Färber 2018-08-12 16:37 ` Jian-Hong Pan 2018-08-12 16:37 ` Jian-Hong Pan 2018-08-12 16:37 ` Jian-Hong Pan 2018-08-12 17:59 ` Andreas Färber 2018-08-12 17:59 ` Andreas Färber 2018-08-12 17:59 ` Andreas Färber 2018-08-13 12:36 ` Alan Cox 2018-08-13 12:36 ` Alan Cox 2018-08-13 12:36 ` Alan Cox 2018-08-09 15:12 ` Alexander Aring 2018-08-09 15:12 ` Alexander Aring 2018-08-09 15:12 ` Alexander Aring 2018-08-09 15:12 ` Alexander Aring 2018-08-09 0:50 ` Andreas Färber 2018-08-09 0:50 ` Andreas Färber 2018-08-09 0:50 ` Andreas Färber 2018-07-04 18:26 ` Stefan Schmidt 2018-07-04 18:26 ` Stefan Schmidt 2018-07-04 18:26 ` Stefan Schmidt 2018-07-05 10:43 ` Helmut Tschemernjak 2018-07-05 10:43 ` Helmut Tschemernjak 2018-07-05 10:43 ` Helmut Tschemernjak 2018-07-11 2:07 ` Andreas Färber 2018-07-11 2:07 ` Andreas Färber 2018-07-11 2:07 ` Andreas Färber 2018-07-11 11:45 ` Helmut Tschemernjak 2018-07-11 11:45 ` Helmut Tschemernjak 2018-07-11 11:45 ` Helmut Tschemernjak 2018-07-11 15:21 ` Ben Whitten 2018-07-11 15:21 ` Ben Whitten 2018-07-11 15:21 ` Ben Whitten 2018-07-15 18:13 ` Andreas Färber 2018-07-15 18:13 ` Andreas Färber 2018-07-15 18:13 ` Andreas Färber 2018-07-18 11:28 ` Ben Whitten 2018-07-18 11:28 ` Ben Whitten 2018-07-18 11:28 ` Ben Whitten 2018-07-18 11:28 ` Ben Whitten 2018-08-02 7:52 ` Jian-Hong Pan 2018-08-02 7:52 ` Jian-Hong Pan 2018-08-02 7:52 ` Jian-Hong Pan 2018-08-02 7:52 ` Jian-Hong Pan 2018-08-03 8:44 ` linux-lora.git and LoRaWAN (was: [RFC net-next 00/15] net: A socket API for LoRa) Andreas Färber 2018-08-03 8:44 ` Andreas Färber 2018-08-05 12:49 ` Jian-Hong Pan 2018-08-05 12:49 ` Jian-Hong Pan 2018-08-05 12:49 ` Jian-Hong Pan 2018-08-05 12:49 ` Jian-Hong Pan [not found] ` <20180803150258.791b9942@alans-desktop> 2018-08-05 14:08 ` Jian-Hong Pan 2018-08-05 14:08 ` Jian-Hong Pan 2018-08-05 13:49 ` [RFC net-next 00/15] net: A socket API for LoRa Andreas Färber 2018-08-05 13:49 ` Andreas Färber 2018-08-05 13:49 ` Andreas Färber
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=20180703145022.GA13744@sirena.org.uk \ --to=broonie@kernel.org \ --cc=LoRa_Community_Support@semtech.com \ --cc=afaerber@suse.de \ --cc=ben.whitten@lairdtech.com \ --cc=davem@davemloft.net \ --cc=derosier@gmail.com \ --cc=dollar.chen@wtmec.com \ --cc=jiri@resnulli.us \ --cc=jpiwek@arroweurope.com \ --cc=ken.yu@rakwireless.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-spi@vger.kernel.org \ --cc=marcel@holtmann.org \ --cc=mbrugger@suse.com \ --cc=michael.roeder@avnet.eu \ --cc=netdev@vger.kernel.org \ --cc=starnight@g.ncu.edu.tw \ /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.