| Message ID | 1467026362-29446-2-git-send-email-akinobu.mita@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
On Mon, 2016-06-27 at 20:19 +0900, Akinobu Mita wrote: > The rtc-ds1302 driver now implemented using SPI 3wire mode. > But I would like to access it with using three wires connected to > GPIO > lines. > > This adds abstraction layer for DS1302 register access in order to > prepare to support for using GPIO lines. This enables to share > common > code between SPI driver and GPIO driver. I don't think this is the right way. DS-1302 is an SPI device, not a GPIO one. It can be connected to a hardware SPI controller or a software one (on top of GPIO or memory). Your patch re-adds Microwire SPI control logic to RTC subsystem, which was cleared by my rewrite of drivers/rtc/rtc-ds1302.c. The logic is already present in bitbang_txrx_be_cpha0_lsb() in drivers/spi/spi- lp8841-rtc.c. I still think you need to implement spi-gpio-3wire with LSB-first support in SPI subsystem instead. It wasn't done when I was adding LP8841 support, because LP8841 was the only use case of Microwire SPI control logic. If you add it, drivers/spi/spi-lp8841-rtc.c can be removed and replaced by a GPIO driver to host a new spi-gpio-3wire device.
On 27/06/2016 at 14:50:49 +0300, Sergei Ianovich wrote : > On Mon, 2016-06-27 at 20:19 +0900, Akinobu Mita wrote: > > The rtc-ds1302 driver now implemented using SPI 3wire mode. > > But I would like to access it with using three wires connected to > > GPIO > > lines. > > > > This adds abstraction layer for DS1302 register access in order to > > prepare to support for using GPIO lines. This enables to share > > common > > code between SPI driver and GPIO driver. > > I don't think this is the right way. DS-1302 is an SPI device, not a > GPIO one. It can be connected to a hardware SPI controller or a > software one (on top of GPIO or memory). > > Your patch re-adds Microwire SPI control logic to RTC subsystem, which > was cleared by my rewrite of drivers/rtc/rtc-ds1302.c. The logic is > already present in bitbang_txrx_be_cpha0_lsb() in drivers/spi/spi- > lp8841-rtc.c. > > I still think you need to implement spi-gpio-3wire with LSB-first > support in SPI subsystem instead. It wasn't done when I was adding > LP8841 support, because LP8841 was the only use case of Microwire SPI > control logic. If you add it, drivers/spi/spi-lp8841-rtc.c can be > removed and replaced by a GPIO driver to host a new spi-gpio-3wire > device. Well, back in April, we concluded it was not easily doable after discussing with Mark and there was still issues after implementing it in spi-gpio. My understanding is that while microwire seems compatible with SPI mode 0, it actually isn't and this should be treated as a different mode. If we want to do something generic, I think we should have a microwire-gpio driver. Maybe in the SPI subsystem? How do yo currently select microwire mode for PX270?
Forgot to add Mark in Cc: On 27/06/2016 at 14:44:32 +0200, Alexandre Belloni wrote : > On 27/06/2016 at 14:50:49 +0300, Sergei Ianovich wrote : > > On Mon, 2016-06-27 at 20:19 +0900, Akinobu Mita wrote: > > > The rtc-ds1302 driver now implemented using SPI 3wire mode. > > > But I would like to access it with using three wires connected to > > > GPIO > > > lines. > > > > > > This adds abstraction layer for DS1302 register access in order to > > > prepare to support for using GPIO lines. This enables to share > > > common > > > code between SPI driver and GPIO driver. > > > > I don't think this is the right way. DS-1302 is an SPI device, not a > > GPIO one. It can be connected to a hardware SPI controller or a > > software one (on top of GPIO or memory). > > > > Your patch re-adds Microwire SPI control logic to RTC subsystem, which > > was cleared by my rewrite of drivers/rtc/rtc-ds1302.c. The logic is > > already present in bitbang_txrx_be_cpha0_lsb() in drivers/spi/spi- > > lp8841-rtc.c. > > > > I still think you need to implement spi-gpio-3wire with LSB-first > > support in SPI subsystem instead. It wasn't done when I was adding > > LP8841 support, because LP8841 was the only use case of Microwire SPI > > control logic. If you add it, drivers/spi/spi-lp8841-rtc.c can be > > removed and replaced by a GPIO driver to host a new spi-gpio-3wire > > device. > > Well, back in April, we concluded it was not easily doable after > discussing with Mark and there was still issues after implementing it in > spi-gpio. > > My understanding is that while microwire seems compatible with SPI mode > 0, it actually isn't and this should be treated as a different mode. > If we want to do something generic, I think we should have a > microwire-gpio driver. Maybe in the SPI subsystem? > > How do yo currently select microwire mode for PX270? > > -- > Alexandre Belloni, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
On Mon, 2016-06-27 at 14:44 +0200, Alexandre Belloni wrote: > On 27/06/2016 at 14:50:49 +0300, Sergei Ianovich wrote : > > I don't think this is the right way. DS-1302 is an SPI device, not > > a > > GPIO one. It can be connected to a hardware SPI controller or a > > software one (on top of GPIO or memory). > > > > Your patch re-adds Microwire SPI control logic to RTC subsystem, which > > was cleared by my rewrite of drivers/rtc/rtc-ds1302.c. The logic is > > already present in bitbang_txrx_be_cpha0_lsb() in drivers/spi/spi- > > lp8841-rtc.c. > > > > I still think you need to implement spi-gpio-3wire with LSB-first > > support in SPI subsystem instead. It wasn't done when I was adding > > LP8841 support, because LP8841 was the only use case of Microwire SPI > > control logic. If you add it, drivers/spi/spi-lp8841-rtc.c can be > > removed and replaced by a GPIO driver to host a new spi-gpio-3wire > > device. > > Well, back in April, we concluded it was not easily doable after > discussing with Mark and there was still issues after implementing it in > spi-gpio. > My understanding is that while microwire seems compatible with SPI mode > 0, it actually isn't and this should be treated as a different mode. > If we want to do something generic, I think we should have a > microwire-gpio driver. Maybe in the SPI subsystem? I've seen Akinobu Mita report that he added support for 3wire to spi- gpio, and it didn't work. That's not a big difference from where we are now, when there is just no support for 3wire. Adding a working support for 3wire to spi-gpio needs 2^4 - 2^2 = 12 new bitbang functions to handle 3wire and lsb-first modes. It is a bit difficult to test all of them. I don't have enough hardware for example. In addition, it is unlikely that a 3wire GPIO SPI master would host more than a single device. That's why I think it is easier to add a new spi-gpio-3wire (or spi-gpio-microwire) driver. > How do yo currently select microwire mode for PX270? I didn't say I use PXA270 to drive this RTC. I just say it is possible. The driver needs to set bits 5:4 of SSCR0_1/2/3 register to 0b10. Microwire mode will be selected for a built-in SPI port. All bitbanging will be done by the chip, the driver will just need to set up DMA transfer. This is an example of a pretty sophisticated hardware controller. I actually use a simple software controller in LP8841. The driver is in drivers/spi/spi-lp8841-rtc.c.
On 27/06/2016 at 18:15:17 +0300, Sergei Ianovich wrote : > > How do yo currently select microwire mode for PX270? > > I didn't say I use PXA270 to drive this RTC. I just say it is possible. > The driver needs to set bits 5:4 of SSCR0_1/2/3 register to 0b10. > Microwire mode will be selected for a built-in SPI port. All bitbanging > will be done by the chip, the driver will just need to set up DMA > transfer. This is an example of a pretty sophisticated hardware > controller. > > I actually use a simple software controller in LP8841. The driver is > in drivers/spi/spi-lp8841-rtc.c. Ok, seeing that, now I'm thinking that switching the ds1302 driver to spi was a mistake and bitbanging in the driver was the right thing to do. What you introduced are two drivers instead of one and the abstraction is actually getting worse. So I'm thinking the best solution is to write a proper driver bitbanging microwire (maybe in the SPI subsystem) that everybody could use, including old architectures. Else, I may as well revert to the previous driver.
On Tue, Jul 19, 2016 at 05:13:14PM +0200, Alexandre Belloni wrote: > Ok, seeing that, now I'm thinking that switching the ds1302 driver to > spi was a mistake and bitbanging in the driver was the right thing to > do. > What you introduced are two drivers instead of one and the abstraction > is actually getting worse. > So I'm thinking the best solution is to write a proper driver bitbanging > microwire (maybe in the SPI subsystem) that everybody could use, > including old architectures. That seems to make sense to me. We could probably accomodate in SPI, the programming model should be very similar so the microwire bit could be hidden in the driver.
diff --git a/drivers/rtc/rtc-ds1302.c b/drivers/rtc/rtc-ds1302.c index f5dd09f..635288d 100644 --- a/drivers/rtc/rtc-ds1302.c +++ b/drivers/rtc/rtc-ds1302.c @@ -7,6 +7,8 @@ * This file is subject to the terms and conditions of the GNU General Public * License version 2. See the file "COPYING" in the main directory of * this archive for more details. + * + * Datasheet: https://datasheets.maximintegrated.com/en/ds/DS1302.pdf */ #include <linux/bcd.h> @@ -39,27 +41,61 @@ #define RTC_ADDR_MIN 0x01 /* Address of minute register */ #define RTC_ADDR_SEC 0x00 /* Address of second register */ +struct ds1302 { + const struct ds1302_ops *ops; + struct device *dev; +}; + +struct ds1302_ops { + int (*read)(struct ds1302 *, u8, u8 *, int); + int (*write)(struct ds1302 *, u8, const u8 *, int); +}; + +static int ds1302_readbyte(struct ds1302 *ds1302, u8 addr) +{ + u8 val; + int err; + + err = ds1302->ops->read(ds1302, addr, &val, 1); + + return err ? err : val; +} + +static int ds1302_writebyte(struct ds1302 *ds1302, u8 addr, u8 val) +{ + return ds1302->ops->write(ds1302, addr, &val, 1); +} + +static int ds1302_readburst(struct ds1302 *ds1302, u8 addr, u8 *buf, int size) +{ + if (addr != RTC_CLCK_BURST) + return -EINVAL; + + return ds1302->ops->read(ds1302, addr, buf, size); +} + +static int ds1302_writeburst(struct ds1302 *ds1302, u8 addr, const u8 *buf, + int size) +{ + if (addr != RTC_CLCK_BURST) + return -EINVAL; + + return ds1302->ops->write(ds1302, addr, buf, size); +} + static int ds1302_rtc_set_time(struct device *dev, struct rtc_time *time) { - struct spi_device *spi = dev_get_drvdata(dev); - u8 buf[1 + RTC_CLCK_LEN]; + struct ds1302 *ds1302 = dev_get_drvdata(dev); + u8 buf[RTC_CLCK_LEN]; u8 *bp = buf; int status; /* Enable writing */ - bp = buf; - *bp++ = RTC_ADDR_CTRL << 1 | RTC_CMD_WRITE; - *bp++ = RTC_CMD_WRITE_ENABLE; - - status = spi_write_then_read(spi, buf, 2, - NULL, 0); + status = ds1302_writebyte(ds1302, RTC_ADDR_CTRL, RTC_CMD_WRITE_ENABLE); if (status) return status; /* Write registers starting at the first time/date address. */ - bp = buf; - *bp++ = RTC_CLCK_BURST << 1 | RTC_CMD_WRITE; - *bp++ = bin2bcd(time->tm_sec); *bp++ = bin2bcd(time->tm_min); *bp++ = bin2bcd(time->tm_hour); @@ -69,23 +105,16 @@ static int ds1302_rtc_set_time(struct device *dev, struct rtc_time *time) *bp++ = bin2bcd(time->tm_year % 100); *bp++ = RTC_CMD_WRITE_DISABLE; - /* use write-then-read since dma from stack is nonportable */ - return spi_write_then_read(spi, buf, sizeof(buf), - NULL, 0); + return ds1302_writeburst(ds1302, RTC_CLCK_BURST, buf, sizeof(buf)); } static int ds1302_rtc_get_time(struct device *dev, struct rtc_time *time) { - struct spi_device *spi = dev_get_drvdata(dev); - u8 addr = RTC_CLCK_BURST << 1 | RTC_CMD_READ; + struct ds1302 *ds1302 = dev_get_drvdata(dev); u8 buf[RTC_CLCK_LEN - 1]; int status; - /* Use write-then-read to get all the date/time registers - * since dma from stack is nonportable - */ - status = spi_write_then_read(spi, &addr, sizeof(addr), - buf, sizeof(buf)); + status = ds1302_readburst(ds1302, RTC_CLCK_BURST, buf, sizeof(buf)); if (status < 0) return status; @@ -107,94 +136,127 @@ static struct rtc_class_ops ds1302_rtc_ops = { .set_time = ds1302_rtc_set_time, }; -static int ds1302_probe(struct spi_device *spi) +static int ds1302_probe(struct ds1302 *ds1302) { struct rtc_device *rtc; - u8 addr; - u8 buf[4]; - u8 *bp = buf; int status; - /* Sanity check board setup data. This may be hooked up - * in 3wire mode, but we don't care. Note that unless - * there's an inverter in place, this needs SPI_CS_HIGH! - */ - if (spi->bits_per_word && (spi->bits_per_word != 8)) { - dev_err(&spi->dev, "bad word length\n"); - return -EINVAL; - } else if (spi->max_speed_hz > 2000000) { - dev_err(&spi->dev, "speed is too high\n"); - return -EINVAL; - } else if (spi->mode & SPI_CPHA) { - dev_err(&spi->dev, "bad mode\n"); - return -EINVAL; - } + dev_set_drvdata(ds1302->dev, ds1302); - addr = RTC_ADDR_CTRL << 1 | RTC_CMD_READ; - status = spi_write_then_read(spi, &addr, sizeof(addr), buf, 1); + status = ds1302_readbyte(ds1302, RTC_ADDR_CTRL); if (status < 0) { - dev_err(&spi->dev, "control register read error %d\n", + dev_err(ds1302->dev, "control register read error %d\n", status); return status; } - if ((buf[0] & ~RTC_CMD_WRITE_DISABLE) != 0) { - status = spi_write_then_read(spi, &addr, sizeof(addr), buf, 1); + if (status & ~RTC_CMD_WRITE_DISABLE) { + status = ds1302_readbyte(ds1302, RTC_ADDR_CTRL); if (status < 0) { - dev_err(&spi->dev, "control register read error %d\n", + dev_err(ds1302->dev, "control register read error %d\n", status); return status; } - if ((buf[0] & ~RTC_CMD_WRITE_DISABLE) != 0) { - dev_err(&spi->dev, "junk in control register\n"); + if (status & ~RTC_CMD_WRITE_DISABLE) { + dev_err(ds1302->dev, "junk in control register\n"); return -ENODEV; } } - if (buf[0] == 0) { - bp = buf; - *bp++ = RTC_ADDR_CTRL << 1 | RTC_CMD_WRITE; - *bp++ = RTC_CMD_WRITE_DISABLE; - - status = spi_write_then_read(spi, buf, 2, NULL, 0); + if (status == 0) { + status = ds1302_writebyte(ds1302, RTC_ADDR_CTRL, + RTC_CMD_WRITE_DISABLE); if (status < 0) { - dev_err(&spi->dev, "control register write error %d\n", - status); + dev_err(ds1302->dev, + "control register write error %d\n", status); return status; } - addr = RTC_ADDR_CTRL << 1 | RTC_CMD_READ; - status = spi_write_then_read(spi, &addr, sizeof(addr), buf, 1); + status = ds1302_readbyte(ds1302, RTC_ADDR_CTRL); if (status < 0) { - dev_err(&spi->dev, + dev_err(ds1302->dev, "error %d reading control register\n", status); return status; } - if (buf[0] != RTC_CMD_WRITE_DISABLE) { - dev_err(&spi->dev, "failed to detect chip\n"); + if (status != RTC_CMD_WRITE_DISABLE) { + dev_err(ds1302->dev, "failed to detect chip\n"); return -ENODEV; } } - spi_set_drvdata(spi, spi); - - rtc = devm_rtc_device_register(&spi->dev, "ds1302", + rtc = devm_rtc_device_register(ds1302->dev, "ds1302", &ds1302_rtc_ops, THIS_MODULE); if (IS_ERR(rtc)) { status = PTR_ERR(rtc); - dev_err(&spi->dev, "error %d registering rtc\n", status); + dev_err(ds1302->dev, "error %d registering rtc\n", status); return status; } return 0; } -static int ds1302_remove(struct spi_device *spi) +static int ds1302_spi_read(struct ds1302 *ds1302, u8 addr, u8 *buf, int size) { - spi_set_drvdata(spi, NULL); - return 0; + struct spi_device *spi = to_spi_device(ds1302->dev); + + addr = addr << 1 | RTC_CMD_READ; + + /* Use write-then-read to get all the date/time registers + * since dma from stack is nonportable + */ + return spi_write_then_read(spi, &addr, sizeof(addr), buf, size); +} + +static int ds1302_spi_write(struct ds1302 *ds1302, u8 addr, const u8 *buf, + int size) +{ + struct spi_device *spi = to_spi_device(ds1302->dev); + u8 write_buf[RTC_CLCK_LEN + 1]; + + if (size + 1 > sizeof(write_buf)) + return -EINVAL; + + write_buf[0] = addr << 1 | RTC_CMD_WRITE; + memcpy(write_buf + 1, buf, size); + + /* use write-then-read since dma from stack is nonportable */ + return spi_write_then_read(spi, write_buf, size + 1, NULL, 0); +} + +static const struct ds1302_ops ds1302_spi_ops = { + .read = ds1302_spi_read, + .write = ds1302_spi_write, +}; + +static int ds1302_spi_probe(struct spi_device *spi) +{ + struct ds1302 *ds1302; + + /* Sanity check board setup data. This may be hooked up + * in 3wire mode, but we don't care. Note that unless + * there's an inverter in place, this needs SPI_CS_HIGH! + */ + if (spi->bits_per_word && (spi->bits_per_word != 8)) { + dev_err(&spi->dev, "bad word length\n"); + return -EINVAL; + } else if (spi->max_speed_hz > 2000000) { + dev_err(&spi->dev, "speed is too high\n"); + return -EINVAL; + } else if (spi->mode & SPI_CPHA) { + dev_err(&spi->dev, "bad mode\n"); + return -EINVAL; + } + + ds1302 = devm_kzalloc(&spi->dev, sizeof(*ds1302), GFP_KERNEL); + if (!ds1302) + return -ENOMEM; + + ds1302->ops = &ds1302_spi_ops; + ds1302->dev = &spi->dev; + + return ds1302_probe(ds1302); } #ifdef CONFIG_OF @@ -208,8 +270,7 @@ MODULE_DEVICE_TABLE(of, ds1302_dt_ids); static struct spi_driver ds1302_driver = { .driver.name = "rtc-ds1302", .driver.of_match_table = of_match_ptr(ds1302_dt_ids), - .probe = ds1302_probe, - .remove = ds1302_remove, + .probe = ds1302_spi_probe, }; module_spi_driver(ds1302_driver);
The rtc-ds1302 driver now implemented using SPI 3wire mode. But I would like to access it with using three wires connected to GPIO lines. This adds abstraction layer for DS1302 register access in order to prepare to support for using GPIO lines. This enables to share common code between SPI driver and GPIO driver. Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com> Cc: Sergey Yanovich <ynvich@gmail.com> Cc: Alessandro Zummo <a.zummo@towertech.it> Cc: Alexandre Belloni <alexandre.belloni@free-electrons.com> --- drivers/rtc/rtc-ds1302.c | 199 +++++++++++++++++++++++++++++++---------------- 1 file changed, 130 insertions(+), 69 deletions(-)