[PATCH v2 0/2] support control with using GPIO lines

[PATCH v2 0/2] support control with using GPIO lines

[Akinobu Mita]

This series adds support access to DS1302 with GPIO lines.

This series first introduces the abstraction layer for the register
access which enables to share the most code between existing spi driver
and new platform driver, and then adds the platform driver using GPIO.

Changes from v1
* Remove two fixes which have already been merged
* Remove readbyte and writebyte ops from ds1302_ops because they were
  just special cases for readburst and writeburst ops
* Place Kconfig entry under Platform RTC drivers

Akinobu Mita (2):
  rtc: ds1302: add register access abstraction layer
  rtc: ds1302: support control with using GPIO lines

 .../devicetree/bindings/rtc/maxim-ds1302.txt       |  13 +
 drivers/rtc/Kconfig                                |  17 +-
 drivers/rtc/rtc-ds1302.c                           | 509 ++++++++++++++++++---
 3 files changed, 456 insertions(+), 83 deletions(-)

Cc: Sergey Yanovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
-- 
2.7.4

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Akinobu Mita]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Sergey Yanovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/rtc/rtc-ds1302.c | 199 +++++++++++++++++++++++++++++++----------------
 1 file changed, 130 insertions(+), 69 deletions(-)

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);
-- 
2.7.4

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Sergei Ianovich]

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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Alexandre Belloni]

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

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Alexandre Belloni]

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

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Sergei Ianovich]

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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Alexandre Belloni]

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.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 1/2] rtc: ds1302: add register access abstraction layer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

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.

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v2 2/2] rtc: ds1302: support control with using GPIO lines

[Akinobu Mita]

This adds support control with GPIO lines connected to the DS1302
which can communicate with three wires.

Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Sergey Yanovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 .../devicetree/bindings/rtc/maxim-ds1302.txt       |  13 +
 drivers/rtc/Kconfig                                |  17 +-
 drivers/rtc/rtc-ds1302.c                           | 320 ++++++++++++++++++++-
 3 files changed, 331 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
index ba470c5..d489753 100644
--- a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
+++ b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
@@ -27,6 +27,12 @@ Required SPI properties:
 - spi-cs-high: DS-1302 has active high chip select line. This is
   required unless inverted in hardware.
 
+Required properties when using GPIO lines:
+
+- gpio-ce: GPIO connected to CE pin
+- gpio-io: GPIO connected to I/O pin
+- gpio-reset: GPIO connected to SCLK pin
+
 Example:
 
 spi@901c {
@@ -44,3 +50,10 @@ spi@901c {
 		spi-cs-high;
 	};
 };
+
+	rtc: ds1302 {
+		compatible = "maxim,ds1302";
+		gpio-ce = <&gpio2 6 GPIO_ACTIVE_HIGH>;
+		gpio-io = <&gpio2 7 GPIO_ACTIVE_HIGH>;
+		gpio-sclk = <&gpio2 8 GPIO_ACTIVE_HIGH>;
+	};
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 18639e0..618f644 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -616,15 +616,6 @@ config RTC_DRV_M41T94
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-m41t94.
 
-config RTC_DRV_DS1302
-	tristate "Dallas/Maxim DS1302"
-	depends on SPI
-	help
-	  If you say yes here you get support for the Dallas DS1302 RTC chips.
-
-	  This driver can also be built as a module. If so, the module
-	  will be called rtc-ds1302.
-
 config RTC_DRV_DS1305
 	tristate "Dallas/Maxim DS1305/DS1306"
 	help
@@ -844,6 +835,14 @@ config RTC_DRV_DS1286
 	help
 	  If you say yes here you get support for the Dallas DS1286 RTC chips.
 
+config RTC_DRV_DS1302
+	tristate "Dallas/Maxim DS1302"
+	help
+	  If you say yes here you get support for the Dallas DS1302 RTC chips.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called rtc-ds1302.
+
 config RTC_DRV_DS1511
 	tristate "Dallas DS1511"
 	depends on HAS_IOMEM
diff --git a/drivers/rtc/rtc-ds1302.c b/drivers/rtc/rtc-ds1302.c
index 635288d..5a21785 100644
--- a/drivers/rtc/rtc-ds1302.c
+++ b/drivers/rtc/rtc-ds1302.c
@@ -19,6 +19,10 @@
 #include <linux/of.h>
 #include <linux/rtc.h>
 #include <linux/spi/spi.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
 
 #define DRV_NAME	"rtc-ds1302"
 
@@ -197,6 +201,16 @@ static int ds1302_probe(struct ds1302 *ds1302)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id ds1302_dt_ids[] = {
+	{ .compatible = "maxim,ds1302", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ds1302_dt_ids);
+#endif
+
+#if IS_ENABLED(CONFIG_SPI_MASTER)
+
 static int ds1302_spi_read(struct ds1302 *ds1302, u8 addr, u8 *buf, int size)
 {
 	struct spi_device *spi = to_spi_device(ds1302->dev);
@@ -259,21 +273,307 @@ static int ds1302_spi_probe(struct spi_device *spi)
 	return ds1302_probe(ds1302);
 }
 
-#ifdef CONFIG_OF
-static const struct of_device_id ds1302_dt_ids[] = {
-	{ .compatible = "maxim,ds1302", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, ds1302_dt_ids);
-#endif
-
-static struct spi_driver ds1302_driver = {
+static struct spi_driver ds1302_spi_driver = {
 	.driver.name	= "rtc-ds1302",
 	.driver.of_match_table = of_match_ptr(ds1302_dt_ids),
 	.probe		= ds1302_spi_probe,
 };
 
-module_spi_driver(ds1302_driver);
+static int ds1302_spi_register_driver(void)
+{
+	return spi_register_driver(&ds1302_spi_driver);
+}
+
+static void ds1302_spi_unregister_driver(void)
+{
+	spi_unregister_driver(&ds1302_spi_driver);
+}
+
+#else
+
+static int ds1302_spi_register_driver(void)
+{
+	return 0;
+}
+
+static void ds1302_spi_unregister_driver(void)
+{
+}
+
+#endif
+
+/*
+ * ds1302 driver using three GPIO lines
+ *
+ * The information to implement this is gleaned from
+ * http://playground.arduino.cc/Main/DS1302
+ */
+struct ds1302_gpio {
+	struct gpio_desc *gpiod_ce;
+	struct gpio_desc *gpiod_io;
+	struct gpio_desc *gpiod_sclk;
+
+	struct ds1302 ds1302;
+};
+
+static struct ds1302_gpio *to_ds1302_gpio(struct ds1302 *ds1302)
+{
+	return container_of(ds1302, struct ds1302_gpio, ds1302);
+}
+
+static int ds1302_gpio_reset(struct ds1302 *ds1302)
+{
+	struct ds1302_gpio *gpio = to_ds1302_gpio(ds1302);
+	int ret;
+
+	ret = gpiod_direction_output(gpio->gpiod_ce, 0);
+	if (ret)
+		return ret;
+
+	ret = gpiod_direction_output(gpio->gpiod_io, 0);
+	if (ret)
+		return ret;
+
+	return gpiod_direction_output(gpio->gpiod_sclk, 0);
+}
+
+static void ds1302_gpio_chip_enable(struct ds1302 *ds1302, int enable)
+{
+	struct ds1302_gpio *gpio = to_ds1302_gpio(ds1302);
+
+	gpiod_set_value(gpio->gpiod_ce, enable);
+	if (enable) {
+		/*
+		 * tCC (CE to CLK Setup): 4us
+		 */
+		udelay(4);
+	} else {
+		/*
+		 * tCWH (CE Inactive Time): 4us
+		 */
+		udelay(4);
+	}
+}
+
+static int ds1302_gpio_sendbits(struct ds1302 *ds1302, u8 val)
+{
+	struct ds1302_gpio *gpio = to_ds1302_gpio(ds1302);
+	int i;
+
+	for (i = 0; i < 8; i++, val >>= 1) {
+		gpiod_set_value(gpio->gpiod_sclk, 0);
+		/* tCL (CLK Low Time): 1000ns */
+		udelay(1);
+
+		gpiod_set_value(gpio->gpiod_io, val & 0x1);
+		/* tDC (Data to CLK Setup): 200ns */
+		udelay(1);
+
+		gpiod_set_value(gpio->gpiod_sclk, 1);
+		/*
+		 * tCH (CLK High Time): 1000ns
+		 * tCDH (CLK to Data Hold): 800ns
+		 */
+		udelay(1);
+	}
+
+	return 0;
+}
+
+static unsigned int ds1302_gpio_recvbits(struct ds1302 *ds1302)
+{
+	unsigned int val;
+	int i;
+	struct ds1302_gpio *gpio = to_ds1302_gpio(ds1302);
+
+	for (i = 0, val = 0; i < 8; i++) {
+		int bit;
+
+		gpiod_set_value(gpio->gpiod_sclk, 0);
+		/*
+		 * tCL (CLK Low Time): 1000ns
+		 * tCDD (CLK to Data Delay): 800ns
+		 */
+		udelay(1);
+
+		bit = gpiod_get_value(gpio->gpiod_io);
+		if (bit < 0)
+			return bit;
+		val |= bit << i;
+
+		gpiod_set_value(gpio->gpiod_sclk, 1);
+		/* tCH (CLK High Time): 1000ns */
+		udelay(1);
+	}
+
+	return val;
+}
+
+static int ds1302_gpio_read(struct ds1302 *ds1302, u8 addr, u8 *buf, int size)
+{
+	int i;
+	int ret;
+	struct ds1302_gpio *gpio = to_ds1302_gpio(ds1302);
+
+	ret = ds1302_gpio_reset(ds1302);
+	if (ret)
+		return ret;
+
+	ds1302_gpio_chip_enable(ds1302, true);
+
+	ds1302_gpio_sendbits(ds1302, ((addr & 0x3f) << 1) | RTC_CMD_READ);
+
+	ret = gpiod_direction_input(gpio->gpiod_io);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < size; i++) {
+		ret = ds1302_gpio_recvbits(ds1302);
+		if (ret < 0)
+			break;
+		 buf[i] = ret;
+	}
+
+	ds1302_gpio_chip_enable(ds1302, false);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ds1302_gpio_write(struct ds1302 *ds1302, u8 addr, const u8 *buf,
+			     int size)
+{
+	int err;
+	int i;
+
+	err = ds1302_gpio_reset(ds1302);
+	if (err)
+		return err;
+
+	ds1302_gpio_chip_enable(ds1302, true);
+
+	ds1302_gpio_sendbits(ds1302, ((addr & 0x3f) << 1) | RTC_CMD_WRITE);
+
+	for (i = 0; i < size; i++) {
+		err = ds1302_gpio_sendbits(ds1302, buf[i]);
+		if (err)
+			break;
+	}
+
+	ds1302_gpio_chip_enable(ds1302, false);
+
+	return err;
+}
+
+static const struct ds1302_ops ds1302_gpio_ops = {
+	.read = ds1302_gpio_read,
+	.write = ds1302_gpio_write,
+};
+
+static struct gpio_desc *ds1302_gpiod_request(struct device *dev,
+						const char *name)
+{
+	int gpio;
+	int ret;
+
+	if (!dev->of_node)
+		return ERR_PTR(-ENODEV);
+
+	gpio = of_get_named_gpio(dev->of_node, name, 0);
+	if (!gpio_is_valid(gpio))
+		return ERR_PTR(gpio);
+
+	ret = devm_gpio_request_one(dev, gpio, 0, name);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return gpio_to_desc(gpio);
+}
+
+static struct ds1302 *ds1302_gpio_init(struct device *dev)
+{
+	struct ds1302_gpio *ds1302_gpio;
+	struct gpio_desc *gpiod;
+
+	ds1302_gpio = devm_kzalloc(dev, sizeof(*ds1302_gpio), GFP_KERNEL);
+	if (!ds1302_gpio)
+		return ERR_PTR(-ENOMEM);
+
+	ds1302_gpio->ds1302.ops = &ds1302_gpio_ops;
+	ds1302_gpio->ds1302.dev = dev;
+
+	gpiod = ds1302_gpiod_request(dev, "gpio-ce");
+	if (IS_ERR(gpiod))
+		return ERR_CAST(gpiod);
+	ds1302_gpio->gpiod_ce = gpiod;
+
+	gpiod = ds1302_gpiod_request(dev, "gpio-io");
+	if (IS_ERR(gpiod))
+		return ERR_CAST(gpiod);
+	ds1302_gpio->gpiod_io = gpiod;
+
+	gpiod = ds1302_gpiod_request(dev, "gpio-sclk");
+	if (IS_ERR(gpiod))
+		return ERR_CAST(gpiod);
+	ds1302_gpio->gpiod_sclk = gpiod;
+
+	return &ds1302_gpio->ds1302;
+}
+
+static int ds1302_gpio_probe(struct platform_device *pdev)
+{
+	struct ds1302 *ds1302;
+
+	ds1302 = ds1302_gpio_init(&pdev->dev);
+	if (IS_ERR(ds1302))
+		return PTR_ERR(ds1302);
+
+	return ds1302_probe(ds1302);
+}
+
+static struct platform_driver ds1302_gpio_driver = {
+	.probe = ds1302_gpio_probe,
+	.driver		= {
+		.name	= "rtc-ds1302",
+		.of_match_table = of_match_ptr(ds1302_dt_ids),
+	},
+};
+
+static int ds1302_gpio_register_driver(void)
+{
+	return platform_driver_register(&ds1302_gpio_driver);
+}
+
+static void ds1302_gpio_unregister_driver(void)
+{
+	return platform_driver_unregister(&ds1302_gpio_driver);
+}
+
+static int __init ds1302_init(void)
+{
+	int ret;
+
+	ret = ds1302_spi_register_driver();
+	if (ret) {
+		pr_err("Failed to register ds1302 spi driver: %d\n", ret);
+		return ret;
+	}
+
+	ret = ds1302_gpio_register_driver();
+	if (ret) {
+		pr_err("Failed to register ds1302 gpio driver: %d\n", ret);
+		ds1302_spi_unregister_driver();
+	}
+
+	return ret;
+}
+module_init(ds1302_init)
+
+static void __exit ds1302_exit(void)
+{
+	ds1302_spi_unregister_driver();
+	ds1302_gpio_unregister_driver();
+}
+module_exit(ds1302_exit)
 
 MODULE_DESCRIPTION("Dallas DS1302 RTC driver");
 MODULE_AUTHOR("Paul Mundt, David McCullough");
-- 
2.7.4

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 2/2] rtc: ds1302: support control with using GPIO lines

[Rob Herring]

On Mon, Jun 27, 2016 at 08:19:22PM +0900, Akinobu Mita wrote:
> This adds support control with GPIO lines connected to the DS1302
> which can communicate with three wires.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Sergey Yanovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
> Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  .../devicetree/bindings/rtc/maxim-ds1302.txt       |  13 +
>  drivers/rtc/Kconfig                                |  17 +-
>  drivers/rtc/rtc-ds1302.c                           | 320 ++++++++++++++++++++-
>  3 files changed, 331 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> index ba470c5..d489753 100644
> --- a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> +++ b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> @@ -27,6 +27,12 @@ Required SPI properties:
>  - spi-cs-high: DS-1302 has active high chip select line. This is
>    required unless inverted in hardware.
>  
> +Required properties when using GPIO lines:
> +
> +- gpio-ce: GPIO connected to CE pin
> +- gpio-io: GPIO connected to I/O pin
> +- gpio-reset: GPIO connected to SCLK pin
> +

IIRC, we got rid of the direct gpio bit-banging in this driver (or a 
similar one) and replaced it with SPI GPIO driver. We should do that 
here.

Rob

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH v2 2/2] rtc: ds1302: support control with using GPIO lines

[Alexandre Belloni]

On 28/06/2016 at 15:57:04 -0500, Rob Herring wrote :
> On Mon, Jun 27, 2016 at 08:19:22PM +0900, Akinobu Mita wrote:
> > This adds support control with GPIO lines connected to the DS1302
> > which can communicate with three wires.
> > 
> > Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Sergey Yanovich <ynvich-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Cc: Alessandro Zummo <a.zummo-BfzFCNDTiLLj+vYz1yj4TQ@public.gmane.org>
> > Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> >  .../devicetree/bindings/rtc/maxim-ds1302.txt       |  13 +
> >  drivers/rtc/Kconfig                                |  17 +-
> >  drivers/rtc/rtc-ds1302.c                           | 320 ++++++++++++++++++++-
> >  3 files changed, 331 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> > index ba470c5..d489753 100644
> > --- a/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> > +++ b/Documentation/devicetree/bindings/rtc/maxim-ds1302.txt
> > @@ -27,6 +27,12 @@ Required SPI properties:
> >  - spi-cs-high: DS-1302 has active high chip select line. This is
> >    required unless inverted in hardware.
> >  
> > +Required properties when using GPIO lines:
> > +
> > +- gpio-ce: GPIO connected to CE pin
> > +- gpio-io: GPIO connected to I/O pin
> > +- gpio-reset: GPIO connected to SCLK pin
> > +
> 
> IIRC, we got rid of the direct gpio bit-banging in this driver (or a 
> similar one) and replaced it with SPI GPIO driver. We should do that 
> here.
> 

Well, the fact is that it is not actually SPI but microwire which has
different timings and we didn't reach a conclusion yet with Mark.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.