[PATCH 0/2] DT bindings for rtc-v3020

[PATCH 0/2] DT bindings for rtc-v3020

[Brandon Martin]

The V3020 driver didn't have DT bindings which is a problem if you want to
use it on a DT-only platform.  Let's fix that.

Tested on a CompuLab CM-T3517 which uses the GPIO attachment/access method.
I don't have anything that hooks it up using MMIO, so I can't test that
access mode.

Long-ish time kernel hacker, first time patch submitter.  Be gentle, please.
Comments welcome, of course.

Brandon Martin (2):
  rtc: v3020: Add documentation for DT bindings
  rtc: v3020: DT bindings

 .../devicetree/bindings/rtc/rtc-v3020.txt          |  38 ++++++
 drivers/rtc/rtc-v3020.c                            | 152 +++++++++++----------
 2 files changed, 119 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-v3020.txt

-- 
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] rtc: v3020: Add documentation for DT bindings

[Brandon Martin]

Document the DT bindings for the rtc-v3020 driver.  MMIO and GPIO
attachment modes are both supported via DT.

Signed-off-by: Brandon Martin <martinbv-cvVvTOR6QHAqDJ6do+/SaQ@public.gmane.org>
---
 .../devicetree/bindings/rtc/rtc-v3020.txt          | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-v3020.txt

diff --git a/Documentation/devicetree/bindings/rtc/rtc-v3020.txt b/Documentation/devicetree/bindings/rtc/rtc-v3020.txt
new file mode 100644
index 000000000000..8b32e6a79960
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-v3020.txt
@@ -0,0 +1,38 @@
+ EM Micro v3020 RTC
+~~~~~~~~~~~~~~~~~~~~
+
+Required properties:
+  - compatible : "emmicro,v3020"
+
+Required for MMIO connection:
+  - reg : should contain registers location and length.
+
+Required for GPIO connection:
+- emmicro,use-gpio
+- cs-gpios, wr-gpios, rd-gpios, io-gpios : specify gpios connected to
+  corresponding pins of the RTC
+
+Optional properties:
+- emmicro,mmio-left-shift : data bit to which IO line is connected for MMIO
+  connection (defaults to 0)
+
+MMIO Example:
+
+       rtc@70 {
+               ompatible = "emmicro,v3020";
+               reg = <1 0x70 1>;
+               emmicro,mmio-left-shift = <16>; /* IO connected to D16 */
+       };
+
+
+GPIO Example:
+
+       rtc {
+               compatible = "emmicro,v3020";
+
+               emmicro,use-gpio;
+               cs-gpios = <&gpio2 3 0>;
+               wr-gpios = <&gpio2 4 0>;
+               rd-gpios = <&gpio2 5 0>;
+               io-gpios = <&gpio2 6 0>;
+       };
-- 
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rtc: v3020: Add documentation for DT bindings

[Rob Herring]

On Tue, Aug 22, 2017 at 05:05:47PM -0400, Brandon Martin wrote:
> Document the DT bindings for the rtc-v3020 driver.  MMIO and GPIO
> attachment modes are both supported via DT.
> 
> Signed-off-by: Brandon Martin <martinbv-cvVvTOR6QHAqDJ6do+/SaQ@public.gmane.org>
> ---
>  .../devicetree/bindings/rtc/rtc-v3020.txt          | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-v3020.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-v3020.txt b/Documentation/devicetree/bindings/rtc/rtc-v3020.txt
> new file mode 100644
> index 000000000000..8b32e6a79960
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-v3020.txt
> @@ -0,0 +1,38 @@
> + EM Micro v3020 RTC
> +~~~~~~~~~~~~~~~~~~~~
> +
> +Required properties:
> +  - compatible : "emmicro,v3020"
> +
> +Required for MMIO connection:
> +  - reg : should contain registers location and length.

Looking at the datasheet, there's really no such thing. You'd have to 
have some specialized h/w to generate the serial waveform.

> +
> +Required for GPIO connection:
> +- emmicro,use-gpio
> +- cs-gpios, wr-gpios, rd-gpios, io-gpios : specify gpios connected to
> +  corresponding pins of the RTC
> +
> +Optional properties:
> +- emmicro,mmio-left-shift : data bit to which IO line is connected for MMIO
> +  connection (defaults to 0)

This has come up several times on RTCs (LP8841, DS1302). This really 
looks like SPI and could probably use the spi-gpio bitbang driver. 

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rtc: v3020: Add documentation for DT bindings

[Brandon Martin]

On 08/25/2017 05:54 PM, Rob Herring wrote:
>> +
>> +Required for MMIO connection:
>> +  - reg : should contain registers location and length.
>
> Looking at the datasheet, there's really no such thing. You'd have to
> have some specialized h/w to generate the serial waveform.

If it's MMIO attached i.e. hooked up to a single data bit on an 
otherwise multi-drop, flat addressed external bus, you do have to 
specify the memory address at which one would access the device.  Length 
is irrelevant, yes, as it has but one externally-accessible "register" 
in its memory map.

Is there a better way to handle this sort of thing than the typical 
"reg" binding?  It certainly seems to map nicely to ioremap().

>
>> +
>> +Required for GPIO connection:
>> +- emmicro,use-gpio
>> +- cs-gpios, wr-gpios, rd-gpios, io-gpios : specify gpios connected to
>> +  corresponding pins of the RTC
>> +
>> +Optional properties:
>> +- emmicro,mmio-left-shift : data bit to which IO line is connected for MMIO
>> +  connection (defaults to 0)
>
> This has come up several times on RTCs (LP8841, DS1302). This really
> looks like SPI and could probably use the spi-gpio bitbang driver.

Indeed it is perhaps like that.

This was a comparatively blind pass at DT-izing the existing driver 
without other changes.  It does work and has immediate application on a 
mainline'd DT board: Compulab CM-T3517 which is in fact what I'm porting to.

I don't think you could use spi-gpio bitbang if you had it 
memory-mapped, and there are actual platforms that do this.  Some of 
Compulab's older PXA based boards appear to do it, and they are in fact 
the boards still using pdata rather than DT that caused me to keep that 
around when doing this.

-- 
Brandon Martin
Mothic Technologies
317-565-1357 x7000
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rtc: v3020: Add documentation for DT bindings

[Rob Herring]

On Fri, Aug 25, 2017 at 5:36 PM, Brandon Martin <martinbv-cvVvTOR6QHAqDJ6do+/SaQ@public.gmane.org> wrote:
> On 08/25/2017 05:54 PM, Rob Herring wrote:
>>>
>>> +
>>> +Required for MMIO connection:
>>> +  - reg : should contain registers location and length.
>>
>>
>> Looking at the datasheet, there's really no such thing. You'd have to
>> have some specialized h/w to generate the serial waveform.
>
>
> If it's MMIO attached i.e. hooked up to a single data bit on an otherwise
> multi-drop, flat addressed external bus, you do have to specify the memory
> address at which one would access the device.  Length is irrelevant, yes, as
> it has but one externally-accessible "register" in its memory map.
>
> Is there a better way to handle this sort of thing than the typical "reg"
> binding?  It certainly seems to map nicely to ioremap().

Okay, I guess you should keep that.

>>> +
>>> +Required for GPIO connection:
>>> +- emmicro,use-gpio
>>> +- cs-gpios, wr-gpios, rd-gpios, io-gpios : specify gpios connected to
>>> +  corresponding pins of the RTC
>>> +
>>> +Optional properties:
>>> +- emmicro,mmio-left-shift : data bit to which IO line is connected for
>>> MMIO
>>> +  connection (defaults to 0)
>>
>>
>> This has come up several times on RTCs (LP8841, DS1302). This really
>> looks like SPI and could probably use the spi-gpio bitbang driver.
>
>
> Indeed it is perhaps like that.
>
> This was a comparatively blind pass at DT-izing the existing driver without
> other changes.  It does work and has immediate application on a mainline'd
> DT board: Compulab CM-T3517 which is in fact what I'm porting to.
>
> I don't think you could use spi-gpio bitbang if you had it memory-mapped,
> and there are actual platforms that do this.  Some of Compulab's older PXA
> based boards appear to do it, and they are in fact the boards still using
> pdata rather than DT that caused me to keep that around when doing this.

I only meant for the GPIO based connection, that it looks like SPI bitbanging.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] rtc: v3020: Add documentation for DT bindings

[Brandon Martin]

On 08/28/2017 02:36 PM, Rob Herring wrote:
> I only meant for the GPIO based connection, that it looks like SPI bitbanging.

I guess the question then becomes if that's an appropriate thing to do 
as part of a change set intended only to add DT bindings to an existing 
driver.  It wouldn't remove much code since much of the bit-banging type 
code still needs to remain to support the MMIO mode.

The chip does explicitly support a SPI type connection, but I'm unaware 
of any boards using it with a real SPI controller.
-- 
Brandon Martin
Mothic Technologies
317-565-1357 x7000
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] rtc: v3020: DT bindings

[Brandon Martin]

Add DT bindings for rtc-v3020 and remove platform_data configuration
for GPIO attachment as it is no longer used by anything in-tree.
platform_data for MMIO attachment is retained as some in-tree boards
still use it.

Convert to devm managed resources for GPIO and IOMEM.  Kernel memory
was already using devm.

Signed-off-by: Brandon Martin <martinbv-cvVvTOR6QHAqDJ6do+/SaQ@public.gmane.org>
---
 drivers/rtc/rtc-v3020.c | 152 ++++++++++++++++++++++++++----------------------
 1 file changed, 81 insertions(+), 71 deletions(-)

diff --git a/drivers/rtc/rtc-v3020.c b/drivers/rtc/rtc-v3020.c
index 1f3117b5a83c..ce82b4cf3a3c 100644
--- a/drivers/rtc/rtc-v3020.c
+++ b/drivers/rtc/rtc-v3020.c
@@ -1,5 +1,6 @@
 /* drivers/rtc/rtc-v3020.c
  *
+ * Copyright (C) 2017 Mothic Technologies LLC
  * Copyright (C) 2006 8D Technologies inc.
  * Copyright (C) 2004 Compulab Ltd.
  *
@@ -11,6 +12,10 @@
  *
  * Changelog:
  *
+ *  17-Aug-2017: Brandon Martin <martinbv-cvVvTOR6QHAqDJ6do+/SaQ@public.gmane.org>
+ *                             - Add DT support/gpio now DT-only
+ *                             - Use devm_* for gpio/iomem
+ *
  *  10-May-2006: Raphael Assenat <raph-04oEwi3R4qQ@public.gmane.org>
  *                             - Converted to platform driver
  *                             - Use the generic rtc class
@@ -29,6 +34,7 @@
 #include <linux/delay.h>
 #include <linux/gpio.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 
 #include <linux/io.h>
 
@@ -39,15 +45,18 @@ struct v3020;
 struct v3020_chip_ops {
        int (*map_io)(struct v3020 *chip, struct platform_device *pdev,
                      struct v3020_platform_data *pdata);
-       void (*unmap_io)(struct v3020 *chip);
        unsigned char (*read_bit)(struct v3020 *chip);
        void (*write_bit)(struct v3020 *chip, unsigned char bit);
 };
 
-#define V3020_CS       0
-#define V3020_WR       1
-#define V3020_RD       2
-#define V3020_IO       3
+enum v3020_gpios {
+       V3020_CS,
+       V3020_WR,
+       V3020_RD,
+       V3020_IO,
+
+       V3020_GPIO_MAX
+};
 
 struct v3020 {
        /* MMIO access */
@@ -55,7 +64,7 @@ struct v3020 {
        int leftshift;
 
        /* GPIO access */
-       struct gpio *gpio;
+       struct gpio_desc *gpiod[V3020_GPIO_MAX];
 
        const struct v3020_chip_ops *ops;
 
@@ -72,19 +81,23 @@ static int v3020_mmio_map(struct v3020 *chip, struct platform_device *pdev,
        if (pdev->resource[0].flags != IORESOURCE_MEM)
                return -EBUSY;
 
-       chip->leftshift = pdata->leftshift;
-       chip->ioaddress = ioremap(pdev->resource[0].start, 1);
+       if (pdev->dev.of_node) {
+               if (of_property_read_u32(pdev->dev.of_node,
+                                       "emmicro,mmio-left-shift",
+                                       &chip->leftshift) != 0) {
+                       chip->leftshift = 0;
+               }
+       } else {
+               chip->leftshift = pdata->leftshift;
+       }
+
+       chip->ioaddress = devm_ioremap(&pdev->dev, pdev->resource[0].start, 1);
        if (chip->ioaddress == NULL)
                return -EBUSY;
 
        return 0;
 }
 
-static void v3020_mmio_unmap(struct v3020 *chip)
-{
-       iounmap(chip->ioaddress);
-}
-
 static void v3020_mmio_write_bit(struct v3020 *chip, unsigned char bit)
 {
        writel(bit << chip->leftshift, chip->ioaddress);
@@ -97,70 +110,60 @@ static unsigned char v3020_mmio_read_bit(struct v3020 *chip)
 
 static const struct v3020_chip_ops v3020_mmio_ops = {
        .map_io         = v3020_mmio_map,
-       .unmap_io       = v3020_mmio_unmap,
        .read_bit       = v3020_mmio_read_bit,
        .write_bit      = v3020_mmio_write_bit,
 };
 
-static struct gpio v3020_gpio[] = {
-       { 0, GPIOF_OUT_INIT_HIGH, "RTC CS"},
-       { 0, GPIOF_OUT_INIT_HIGH, "RTC WR"},
-       { 0, GPIOF_OUT_INIT_HIGH, "RTC RD"},
-       { 0, GPIOF_OUT_INIT_HIGH, "RTC IO"},
-};
-
 static int v3020_gpio_map(struct v3020 *chip, struct platform_device *pdev,
                          struct v3020_platform_data *pdata)
 {
-       int err;
+       chip->gpiod[V3020_CS] = devm_gpiod_get(&pdev->dev, "cs", GPIOD_OUT_HIGH);
+       if (IS_ERR(chip->gpiod[V3020_CS]))
+               return PTR_ERR(chip->gpiod[V3020_CS]);
 
-       v3020_gpio[V3020_CS].gpio = pdata->gpio_cs;
-       v3020_gpio[V3020_WR].gpio = pdata->gpio_wr;
-       v3020_gpio[V3020_RD].gpio = pdata->gpio_rd;
-       v3020_gpio[V3020_IO].gpio = pdata->gpio_io;
+       chip->gpiod[V3020_WR] = devm_gpiod_get(&pdev->dev, "wr", GPIOD_OUT_HIGH);
+       if (IS_ERR(chip->gpiod[V3020_WR]))
+               return PTR_ERR(chip->gpiod[V3020_WR]);
 
-       err = gpio_request_array(v3020_gpio, ARRAY_SIZE(v3020_gpio));
+       chip->gpiod[V3020_RD] = devm_gpiod_get(&pdev->dev, "rd", GPIOD_OUT_HIGH);
+       if (IS_ERR(chip->gpiod[V3020_RD]))
+               return PTR_ERR(chip->gpiod[V3020_RD]);
 
-       if (!err)
-               chip->gpio = v3020_gpio;
+       chip->gpiod[V3020_IO] = devm_gpiod_get(&pdev->dev, "io", GPIOD_OUT_HIGH);
+       if (IS_ERR(chip->gpiod[V3020_IO]))
+               return PTR_ERR(chip->gpiod[V3020_IO]);
 
-       return err;
-}
-
-static void v3020_gpio_unmap(struct v3020 *chip)
-{
-       gpio_free_array(v3020_gpio, ARRAY_SIZE(v3020_gpio));
+       return 0;
 }
 
 static void v3020_gpio_write_bit(struct v3020 *chip, unsigned char bit)
 {
-       gpio_direction_output(chip->gpio[V3020_IO].gpio, bit);
-       gpio_set_value(chip->gpio[V3020_CS].gpio, 0);
-       gpio_set_value(chip->gpio[V3020_WR].gpio, 0);
+       gpiod_direction_output(chip->gpiod[V3020_IO], bit);
+       gpiod_set_value(chip->gpiod[V3020_CS], 0);
+       gpiod_set_value(chip->gpiod[V3020_WR], 0);
        udelay(1);
-       gpio_set_value(chip->gpio[V3020_WR].gpio, 1);
-       gpio_set_value(chip->gpio[V3020_CS].gpio, 1);
+       gpiod_set_value(chip->gpiod[V3020_WR], 1);
+       gpiod_set_value(chip->gpiod[V3020_CS], 1);
 }
 
 static unsigned char v3020_gpio_read_bit(struct v3020 *chip)
 {
        int bit;
 
-       gpio_direction_input(chip->gpio[V3020_IO].gpio);
-       gpio_set_value(chip->gpio[V3020_CS].gpio, 0);
-       gpio_set_value(chip->gpio[V3020_RD].gpio, 0);
+       gpiod_direction_input(chip->gpiod[V3020_IO]);
+       gpiod_set_value(chip->gpiod[V3020_CS], 0);
+       gpiod_set_value(chip->gpiod[V3020_RD], 0);
        udelay(1);
-       bit = !!gpio_get_value(chip->gpio[V3020_IO].gpio);
+       bit = !!gpiod_get_value(chip->gpiod[V3020_IO]);
        udelay(1);
-       gpio_set_value(chip->gpio[V3020_RD].gpio, 1);
-       gpio_set_value(chip->gpio[V3020_CS].gpio, 1);
+       gpiod_set_value(chip->gpiod[V3020_RD], 1);
+       gpiod_set_value(chip->gpiod[V3020_CS], 1);
 
        return bit;
 }
 
 static const struct v3020_chip_ops v3020_gpio_ops = {
        .map_io         = v3020_gpio_map,
-       .unmap_io       = v3020_gpio_unmap,
        .read_bit       = v3020_gpio_read_bit,
        .write_bit      = v3020_gpio_write_bit,
 };
@@ -289,19 +292,31 @@ static int rtc_probe(struct platform_device *pdev)
        int retval = -EBUSY;
        int i;
        int temp;
+       bool use_gpio;
 
        chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
        if (!chip)
                return -ENOMEM;
 
-       if (pdata->use_gpio)
-               chip->ops = &v3020_gpio_ops;
+       if (pdev->dev.of_node)
+               use_gpio = of_property_read_bool(pdev->dev.of_node,
+                               "emmicro,use-gpio");
        else
+               use_gpio = false;
+
+       if (use_gpio) {
+               chip->ops = &v3020_gpio_ops;
+               dev_dbg(&pdev->dev, "using gpio");
+       } else {
                chip->ops = &v3020_mmio_ops;
+               dev_dbg(&pdev->dev, "using mmio");
+       }
 
        retval = chip->ops->map_io(chip, pdev, pdata);
-       if (retval)
+       if (retval) {
+               dev_err(&pdev->dev, "map_io failed: %d", retval);
                return retval;
+       }
 
        /* Make sure the v3020 expects a communication cycle
         * by reading 8 times */
@@ -312,19 +327,21 @@ static int rtc_probe(struct platform_device *pdev)
         * to the chip ram */
        v3020_set_reg(chip, V3020_SECONDS, 0x33);
        if (v3020_get_reg(chip, V3020_SECONDS) != 0x33) {
-               retval = -ENODEV;
-               goto err_io;
+               dev_err(&pdev->dev, "Chip r/w test failed");
+               return -ENODEV;
        }
 
        /* Make sure frequency measurement mode, test modes, and lock
         * are all disabled */
        v3020_set_reg(chip, V3020_STATUS_0, 0x0);
 
-       if (pdata->use_gpio)
+       if (use_gpio)
                dev_info(&pdev->dev, "Chip available at GPIOs "
                         "%d, %d, %d, %d\n",
-                        chip->gpio[V3020_CS].gpio, chip->gpio[V3020_WR].gpio,
-                        chip->gpio[V3020_RD].gpio, chip->gpio[V3020_IO].gpio);
+                        desc_to_gpio(chip->gpiod[V3020_CS]),
+                        desc_to_gpio(chip->gpiod[V3020_WR]),
+                        desc_to_gpio(chip->gpiod[V3020_RD]),
+                        desc_to_gpio(chip->gpiod[V3020_IO]));
        else
                dev_info(&pdev->dev, "Chip available at "
                         "physical address 0x%llx,"
@@ -337,32 +354,25 @@ static int rtc_probe(struct platform_device *pdev)
        chip->rtc = devm_rtc_device_register(&pdev->dev, "v3020",
                                        &v3020_rtc_ops, THIS_MODULE);
        if (IS_ERR(chip->rtc)) {
-               retval = PTR_ERR(chip->rtc);
-               goto err_io;
+               return PTR_ERR(chip->rtc);
        }
 
        return 0;
-
-err_io:
-       chip->ops->unmap_io(chip);
-
-       return retval;
 }
 
-static int rtc_remove(struct platform_device *dev)
-{
-       struct v3020 *chip = platform_get_drvdata(dev);
-
-       chip->ops->unmap_io(chip);
-
-       return 0;
-}
+#if defined(CONFIG_OF)
+static const struct of_device_id v3020_of_match[] = {
+       { .compatible = "emmicro,v3020" },
+       {},
+};
+MODULE_DEVICE_TABLE(of, v3020_of_match);
+#endif
 
 static struct platform_driver rtc_device_driver = {
        .probe  = rtc_probe,
-       .remove = rtc_remove,
        .driver = {
                .name   = "v3020",
+               .of_match_table = of_match_ptr(v3020_of_match),
        },
 };
 
-- 
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html