From: "Andreas Färber" <afaerber@suse.de> To: Mark Brown <broonie@kernel.org> 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: Mon, 2 Jul 2018 19:34:21 +0200 [thread overview] Message-ID: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de> (raw) In-Reply-To: <20180702161258.GA18744@sirena.org.uk> [-- Attachment #1.1: Type: text/plain, Size: 5074 bytes --] 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 The reason you're in CC on this RFC is two-fold: 1) You applied Ben's patch to associate "semtech,sx1301" with spidev, whereas I am now preparing a new driver for the same compatible. 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? More inline ... 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; >> + >> + ret = sx1301_radio_set_cs(spi->controller, enable); >> + if (ret) >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); >> +} > > So we never disable chip select? Not here, I instead did that in transfer_one below. Unfortunately there seems to be no documentation, only reference code: https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 It sets CS to 0 before writing to address and data registers, then immediately sets CS to 1 and back to 0 before reading or ending the write transaction. I've tried to force the same behavior in this driver. My guess was that CS is high-active during the short 1-0 cycle, because if it's low-active during the register writes then why the heck is it set to 0 again in the end instead of keeping at 1... confusing. Maybe the Semtech folks CC'ed can comment how these registers work? >> + if (tx_buf) { >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > > This looks confused. We're in an if (tx_buf) block but there's a use of > the ternery operator that appears to be checking if we have a tx_buf? Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl will complain about lines too long, and TODOs are sprinkled all over or not even mentioned. It's a Proof of Concept that a net_device could work for a wide range of spi and serdev based drivers, and on top this device has more than one channel, which may influence network-level design discussions. That said, I'll happily drop the second check. Thanks for spotting! >> + 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? 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.). You will find a datasheet with some diagrams mentioning "SPI" at: https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301 >> + if (rx_buf) { >> + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); >> + if (ret) { >> + dev_err(&spi->dev, "SPI radio data read failed\n"); >> + return ret; >> + } >> + } > > For a read we never set an address? To read, you first write the address via tx_buf, then either in the same transfer in the third byte or in a subsequent one-byte transfer as first byte you get the data. If you have better ideas how to structure this, do let me know. >> +static void sx1301_radio_setup(struct spi_controller *ctrl) >> +{ >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; > > This controller has no chip select but we provided a set_cs operation? Oops, I played around with those two options and was hoping SPI_NO_CS would avoid the undesired set_cs invocations, but it didn't work as expected and so I added the "if (enabled)" check above. Thanks for your review, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: afaerber@suse.de (Andreas Färber) To: linux-arm-kernel@lists.infradead.org Subject: [RFC net-next 15/15] net: lora: Add Semtech SX1301 Date: Mon, 2 Jul 2018 19:34:21 +0200 [thread overview] Message-ID: <4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de> (raw) In-Reply-To: <20180702161258.GA18744@sirena.org.uk> 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 The reason you're in CC on this RFC is two-fold: 1) You applied Ben's patch to associate "semtech,sx1301" with spidev, whereas I am now preparing a new driver for the same compatible. 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? More inline ... 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; >> + >> + ret = sx1301_radio_set_cs(spi->controller, enable); >> + if (ret) >> + dev_warn(&spi->dev, "failed to write CS (%d)\n", ret); >> +} > > So we never disable chip select? Not here, I instead did that in transfer_one below. Unfortunately there seems to be no documentation, only reference code: https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L121 https://github.com/Lora-net/lora_gateway/blob/master/libloragw/src/loragw_radio.c#L165 It sets CS to 0 before writing to address and data registers, then immediately sets CS to 1 and back to 0 before reading or ending the write transaction. I've tried to force the same behavior in this driver. My guess was that CS is high-active during the short 1-0 cycle, because if it's low-active during the register writes then why the heck is it set to 0 again in the end instead of keeping at 1... confusing. Maybe the Semtech folks CC'ed can comment how these registers work? >> + if (tx_buf) { >> + ret = sx1301_write(ssx->parent, ssx->regs + REG_RADIO_X_ADDR, tx_buf ? tx_buf[0] : 0); > > This looks confused. We're in an if (tx_buf) block but there's a use of > the ternery operator that appears to be checking if we have a tx_buf? Yeah, as mentioned this RFC is not ready for merging - checkpatch.pl will complain about lines too long, and TODOs are sprinkled all over or not even mentioned. It's a Proof of Concept that a net_device could work for a wide range of spi and serdev based drivers, and on top this device has more than one channel, which may influence network-level design discussions. That said, I'll happily drop the second check. Thanks for spotting! >> + 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? 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.). You will find a datasheet with some diagrams mentioning "SPI" at: https://www.semtech.com/products/wireless-rf/lora-gateways/SX1301 >> + if (rx_buf) { >> + ret = sx1301_read(ssx->parent, ssx->regs + REG_RADIO_X_DATA_READBACK, &rx_buf[xfr->len - 1]); >> + if (ret) { >> + dev_err(&spi->dev, "SPI radio data read failed\n"); >> + return ret; >> + } >> + } > > For a read we never set an address? To read, you first write the address via tx_buf, then either in the same transfer in the third byte or in a subsequent one-byte transfer as first byte you get the data. If you have better ideas how to structure this, do let me know. >> +static void sx1301_radio_setup(struct spi_controller *ctrl) >> +{ >> + ctrl->mode_bits = SPI_CS_HIGH | SPI_NO_CS; > > This controller has no chip select but we provided a set_cs operation? Oops, I played around with those two options and was hoping SPI_NO_CS would avoid the undesired set_cs invocations, but it didn't work as expected and so I added the "if (enabled)" check above. Thanks for your review, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: OpenPGP digital signature URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180702/74036afd/attachment-0001.sig>
next prev parent reply other threads:[~2018-07-02 17:34 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 [this message] 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 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=4e06cc72-2092-70f3-c801-bf6e4c3cbec2@suse.de \ --to=afaerber@suse.de \ --cc=LoRa_Community_Support@semtech.com \ --cc=ben.whitten@lairdtech.com \ --cc=broonie@kernel.org \ --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.