[PATCH RESEND] rtc: ds1511: add device-tree bindings

[PATCH RESEND] rtc: ds1511: add device-tree bindings

[Marcus Folkesson]

Resending patch.

[PATCH] rtc: ds1511: add device-tree bindings

[Marcus Folkesson]

Compatible with ds1500, ds1501 and ds1511

Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
---
 .../devicetree/bindings/rtc/maxim-ds1511.txt       |   18 +++++
 drivers/rtc/rtc-ds1511.c                           |   85 +++++++++++++++++---
 2 files changed, 90 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/rtc/maxim-ds1511.txt

diff --git a/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt b/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt
new file mode 100644
index 0000000..17fa356
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt
@@ -0,0 +1,18 @@
+* Maxim Real Time Clock
+
+
+Support for Maxim DS1500, DS1501 and DS1511 RTC.
+
+Required properties:
+- compatible : Should be "maxim,ds1500-rtc", "maxim,ds1501-rtc" or "maxim,ds1511-rtc".
+- reg: physical base address of the controller and length of memory mapped
+  region.
+- interrupts: IRQ line for the RTC.
+
+Example:
+
+rtc@10300 {
+        compatible = "maxim,ds1511-rtc";
+        reg = <0xd0010300 0x20>;
+        interrupts = <50>;
+};
diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c
index 6a3fcfe..3778a8e 100644
--- a/drivers/rtc/rtc-ds1511.c
+++ b/drivers/rtc/rtc-ds1511.c
@@ -24,6 +24,9 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
 
 #define DRV_VERSION "0.6"
 
@@ -476,6 +479,52 @@ static struct bin_attribute ds1511_nvram_attr = {
 	.write = ds1511_nvram_write,
 };
 
+
+
+#ifdef CONFIG_OF
+static int ds1511_of_probe(struct platform_device *pdev,
+	 struct rtc_plat_data *pdata)
+{
+	struct device *dev = &pdev->dev;
+	u32 val;
+	int ret;
+	struct resource res;
+
+	ret = of_address_to_resource(&pdev->dev.of_node, 0, &res);
+	if (ret)
+		return -ENODEV;
+
+	pdata->size = resource_size(&res);
+
+	if (!devm_request_mem_region(&pdev->dev, res.start, pdata->size,
+			pdev->name))
+		return -EBUSY;
+
+	ds1511_base = devm_ioremap(&pdev->dev, res.start, pdata->size);
+	if (!ds1511_base)
+		return -ENOMEM;
+
+	pdata->ioaddr = ds1511_base;
+	pdata->irq = irq_of_parse_and_map(&pdev->dev.of_node, 0);
+
+	return 0;
+}
+
+static const struct of_device_id rtc_of_match[] = {
+	{ .compatible = "maxim,ds1500-rtc" },
+	{ .compatible = "maxim,ds1501-rtc" },
+	{ .compatible = "maxim,ds1511-rtc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rtc_of_match);
+
+#else
+static int ds1511_of_probe(struct platform_device *pdev, struct rtc_plat_data *pdata)
+{
+	return -ENODEV;
+}
+#endif
+
 static int ds1511_rtc_probe(struct platform_device *pdev)
 {
 	struct rtc_device *rtc;
@@ -483,22 +532,31 @@ static int ds1511_rtc_probe(struct platform_device *pdev)
 	struct rtc_plat_data *pdata;
 	int ret = 0;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		return -ENODEV;
-	}
 	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
-	pdata->size = resource_size(res);
-	if (!devm_request_mem_region(&pdev->dev, res->start, pdata->size,
-			pdev->name))
-		return -EBUSY;
-	ds1511_base = devm_ioremap(&pdev->dev, res->start, pdata->size);
-	if (!ds1511_base)
-		return -ENOMEM;
-	pdata->ioaddr = ds1511_base;
-	pdata->irq = platform_get_irq(pdev, 0);
+
+	if (pdev->dev.of_node) {
+		ret = ds1511_of_probe(pdev, pdata);
+		if (ret < 0)
+			return ret;
+	} else if (!pdev) {
+
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		if (!res) {
+			return -ENODEV;
+		}
+		pdata->size = resource_size(res);
+		if (!devm_request_mem_region(&pdev->dev, res->start, pdata->size,
+				pdev->name))
+			return -EBUSY;
+		ds1511_base = devm_ioremap(&pdev->dev, res->start, pdata->size);
+		if (!ds1511_base)
+			return -ENOMEM;
+		pdata->ioaddr = ds1511_base;
+		pdata->irq = platform_get_irq(pdev, 0);
+	} else
+		return -ENODEV;
 
 	/*
 	 * turn on the clock and the crystal, etc.
@@ -575,6 +633,7 @@ static struct platform_driver ds1511_rtc_driver = {
 	.driver		= {
 		.name	= "ds1511",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(rtc_of_match),
 	},
 };
 
-- 
1.7.10.4

Re: [PATCH] rtc: ds1511: add device-tree bindings

[Mark Rutland]

On Fri, Nov 22, 2013 at 07:16:39AM +0000, Marcus Folkesson wrote:
> Compatible with ds1500, ds1501 and ds1511
> 
> Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com>
> ---
>  .../devicetree/bindings/rtc/maxim-ds1511.txt       |   18 +++++
>  drivers/rtc/rtc-ds1511.c                           |   85 +++++++++++++++++---
>  2 files changed, 90 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/rtc/maxim-ds1511.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt b/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt
> new file mode 100644
> index 0000000..17fa356
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/maxim-ds1511.txt
> @@ -0,0 +1,18 @@
> +* Maxim Real Time Clock
> +
> +
> +Support for Maxim DS1500, DS1501 and DS1511 RTC.
> +
> +Required properties:
> +- compatible : Should be "maxim,ds1500-rtc", "maxim,ds1501-rtc" or "maxim,ds1511-rtc".

I'd prefer each of these on an individial line:

- compatible : Should contain one of:
   * "maxim,ds1500-rtc"
   * "maxim,ds1501-rtc"
   * "maxim,ds1511-rtc"

It would also be nice to have a description of how these differ, if
possible.

> +- reg: physical base address of the controller and length of memory mapped
> +  region.
> +- interrupts: IRQ line for the RTC.
> +
> +Example:
> +
> +rtc@10300 {
> +        compatible = "maxim,ds1511-rtc";
> +        reg = <0xd0010300 0x20>;
> +        interrupts = <50>;
> +};

Otherwise, the binding looks fine to me.

> diff --git a/drivers/rtc/rtc-ds1511.c b/drivers/rtc/rtc-ds1511.c
> index 6a3fcfe..3778a8e 100644
> --- a/drivers/rtc/rtc-ds1511.c
> +++ b/drivers/rtc/rtc-ds1511.c
> @@ -24,6 +24,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
>  
>  #define DRV_VERSION "0.6"
>  
> @@ -476,6 +479,52 @@ static struct bin_attribute ds1511_nvram_attr = {
>  	.write = ds1511_nvram_write,
>  };
>  
> +
> +
> +#ifdef CONFIG_OF
> +static int ds1511_of_probe(struct platform_device *pdev,
> +	 struct rtc_plat_data *pdata)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 val;
> +	int ret;
> +	struct resource res;
> +
> +	ret = of_address_to_resource(&pdev->dev.of_node, 0, &res);
> +	if (ret)
> +		return -ENODEV;
> +

You can use platform_get_resource(pdev, IORESOURCE_MEM, 0) here as the
platform bus OF code will have already parsed this out of the dt for
you.

> +	pdata->size = resource_size(&res);
> +
> +	if (!devm_request_mem_region(&pdev->dev, res.start, pdata->size,
> +			pdev->name))
> +		return -EBUSY;
> +
> +	ds1511_base = devm_ioremap(&pdev->dev, res.start, pdata->size);
> +	if (!ds1511_base)
> +		return -ENOMEM;
> +
> +	pdata->ioaddr = ds1511_base;
> +	pdata->irq = irq_of_parse_and_map(&pdev->dev.of_node, 0);

Similarly, platform_get_resource(pdev, IORESOURCE_IRQ, 0).

There's no need to parse the dt twice.

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rtc_of_match[] = {
> +	{ .compatible = "maxim,ds1500-rtc" },
> +	{ .compatible = "maxim,ds1501-rtc" },
> +	{ .compatible = "maxim,ds1511-rtc" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rtc_of_match);
> +
> +#else
> +static int ds1511_of_probe(struct platform_device *pdev, struct rtc_plat_data *pdata)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int ds1511_rtc_probe(struct platform_device *pdev)
>  {
>  	struct rtc_device *rtc;
> @@ -483,22 +532,31 @@ static int ds1511_rtc_probe(struct platform_device *pdev)
>  	struct rtc_plat_data *pdata;
>  	int ret = 0;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		return -ENODEV;
> -	}
>  	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>  	if (!pdata)
>  		return -ENOMEM;
> -	pdata->size = resource_size(res);
> -	if (!devm_request_mem_region(&pdev->dev, res->start, pdata->size,
> -			pdev->name))
> -		return -EBUSY;
> -	ds1511_base = devm_ioremap(&pdev->dev, res->start, pdata->size);
> -	if (!ds1511_base)
> -		return -ENOMEM;
> -	pdata->ioaddr = ds1511_base;
> -	pdata->irq = platform_get_irq(pdev, 0);

As the platform bus OF code parses everything for you, this should have
been sufficient (with the rtc_of_match table wired up).

> +
> +	if (pdev->dev.of_node) {
> +		ret = ds1511_of_probe(pdev, pdata);
> +		if (ret < 0)
> +			return ret;
> +	} else if (!pdev) {

You've already dereferenced pdev above, if this is necessary it's
broken.

How would this get called without a pdev? I believe it can't.

> +
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		if (!res) {
> +			return -ENODEV;
> +		}
> +		pdata->size = resource_size(res);
> +		if (!devm_request_mem_region(&pdev->dev, res->start, pdata->size,
> +				pdev->name))
> +			return -EBUSY;
> +		ds1511_base = devm_ioremap(&pdev->dev, res->start, pdata->size);
> +		if (!ds1511_base)
> +			return -ENOMEM;
> +		pdata->ioaddr = ds1511_base;
> +		pdata->irq = platform_get_irq(pdev, 0);

This all relies on pdev despite only being called when pdev is NULL, so
it's broken.

Thanks,
Mark.