i2c-ocores changes (version 2)

i2c-ocores changes (version 2)

[Jonas Bonn]

Version 2 of patch series incorporating initial feedback from review.

/Jonas

[PATCH 1/3] i2c-ocores: Adapt for device tree

[Jonas Bonn]

This patch adapts the i2c-ocores driver for being defined and configured via
a device tree description.

The device tree bits need to be protected by CONFIG_OF guards as this is
still an optional feature.

Signed-off-by: Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>
---
 drivers/i2c/busses/i2c-ocores.c |   64 ++++++++++++++++++++++++++++++++++-----
 1 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index 0070371..c05e8c4 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -210,6 +210,32 @@ static struct i2c_adapter ocores_adapter = {
 	.algo		= &ocores_algorithm,
 };
 
+#ifdef CONFIG_OF
+static int ocores_i2c_of_probe(struct platform_device* pdev,
+				struct ocores_i2c* i2c)
+{
+	__be32* val;
+
+	val = of_get_property(pdev->dev.of_node, "regstep", NULL);
+	if (!val) {
+		dev_err(&pdev->dev, "Missing required parameter 'regstep'");
+		return -ENODEV;
+	}
+	i2c->regstep = be32_to_cpup(val);
+
+	val = of_get_property(pdev->dev.of_node, "clock-frequency", NULL);
+	if (!val) {
+		dev_err(&pdev->dev,
+			"Missing required parameter 'clock-frequency'");
+		return -ENODEV;
+	}
+	i2c->clock_khz = be32_to_cpup(val) / 1000;
+
+	return 0;
+}
+#else
+#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
+#endif
 
 static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 {
@@ -227,10 +253,6 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	if (!res2)
 		return -ENODEV;
 
-	pdata = (struct ocores_i2c_platform_data*) pdev->dev.platform_data;
-	if (!pdata)
-		return -ENODEV;
-
 	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
@@ -249,8 +271,16 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 		goto map_failed;
 	}
 
-	i2c->regstep = pdata->regstep;
-	i2c->clock_khz = pdata->clock_khz;
+	pdata = pdev->dev.platform_data;
+	if (pdata) {
+		i2c->regstep = pdata->regstep;
+		i2c->clock_khz = pdata->clock_khz;
+	} else {
+		ret = ocores_i2c_of_probe(pdev, i2c);
+		if (ret)
+			return ret;
+	}
+
 	ocores_init(i2c);
 
 	init_waitqueue_head(&i2c->wait);
@@ -265,6 +295,9 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	i2c->adap = ocores_adapter;
 	i2c_set_adapdata(&i2c->adap, i2c);
 	i2c->adap.dev.parent = &pdev->dev;
+#ifdef CONFIG_OF
+	i2c->adap.dev.of_node = pdev->dev.of_node;
+#endif
 
 	/* add i2c adapter to i2c tree */
 	ret = i2c_add_adapter(&i2c->adap);
@@ -274,8 +307,10 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	}
 
 	/* add in known devices to the bus */
-	for (i = 0; i < pdata->num_devices; i++)
-		i2c_new_device(&i2c->adap, pdata->devices + i);
+	if (pdata) {
+		for (i = 0; i < pdata->num_devices; i++)
+			i2c_new_device(&i2c->adap, pdata->devices + i);
+	}
 
 	return 0;
 
@@ -344,6 +379,16 @@ static int ocores_i2c_resume(struct platform_device *pdev)
 #define ocores_i2c_resume	NULL
 #endif
 
+#ifdef CONFIG_OF
+static struct of_device_id ocores_i2c_match[] = {
+        {
+                .compatible = "opencores,i2c-ocores",
+        },
+        {},
+};
+MODULE_DEVICE_TABLE(of, ocores_i2c_match);
+#endif
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:ocores-i2c");
 
@@ -355,6 +400,9 @@ static struct platform_driver ocores_i2c_driver = {
 	.driver  = {
 		.owner = THIS_MODULE,
 		.name = "ocores-i2c",
+#ifdef CONFIG_OF
+                .of_match_table = ocores_i2c_match,
+#endif
 	},
 };
 
-- 
1.7.1

Re: [PATCH 1/3] i2c-ocores: Adapt for device tree

[Walter Goossens]

Hi Jonas,

Seems to work for me on Nios2. (Compile tested. Ran the kernel devices 
were found, did not test functionality)
I know Nios2 is not (yet) a supported platform but we're busy trying to 
get fdt working...
So an ack from my side!

Walter

On 11/24/10 5:26 PM, Jonas Bonn wrote:
> This patch adapts the i2c-ocores driver for being defined and configured via
> a device tree description.
>
> The device tree bits need to be protected by CONFIG_OF guards as this is
> still an optional feature.
>
> Signed-off-by: Jonas Bonn<jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-ocores.c |   64 ++++++++++++++++++++++++++++++++++-----
>   1 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index 0070371..c05e8c4 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -210,6 +210,32 @@ static struct i2c_adapter ocores_adapter = {
>   	.algo		=&ocores_algorithm,
>   };
>
> +#ifdef CONFIG_OF
> +static int ocores_i2c_of_probe(struct platform_device* pdev,
> +				struct ocores_i2c* i2c)
> +{
> +	__be32* val;
> +
> +	val = of_get_property(pdev->dev.of_node, "regstep", NULL);
> +	if (!val) {
> +		dev_err(&pdev->dev, "Missing required parameter 'regstep'");
> +		return -ENODEV;
> +	}
> +	i2c->regstep = be32_to_cpup(val);
> +
> +	val = of_get_property(pdev->dev.of_node, "clock-frequency", NULL);
> +	if (!val) {
> +		dev_err(&pdev->dev,
> +			"Missing required parameter 'clock-frequency'");
> +		return -ENODEV;
> +	}
> +	i2c->clock_khz = be32_to_cpup(val) / 1000;
> +
> +	return 0;
> +}
> +#else
> +#define ocores_i2c_of_probe(pdev,i2c) -ENODEV
> +#endif
>
>   static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>   {
> @@ -227,10 +253,6 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>   	if (!res2)
>   		return -ENODEV;
>
> -	pdata = (struct ocores_i2c_platform_data*) pdev->dev.platform_data;
> -	if (!pdata)
> -		return -ENODEV;
> -
>   	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
>   	if (!i2c)
>   		return -ENOMEM;
> @@ -249,8 +271,16 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>   		goto map_failed;
>   	}
>
> -	i2c->regstep = pdata->regstep;
> -	i2c->clock_khz = pdata->clock_khz;
> +	pdata = pdev->dev.platform_data;
> +	if (pdata) {
> +		i2c->regstep = pdata->regstep;
> +		i2c->clock_khz = pdata->clock_khz;
> +	} else {
> +		ret = ocores_i2c_of_probe(pdev, i2c);
> +		if (ret)
> +			return ret;
> +	}
> +
>   	ocores_init(i2c);
>
>   	init_waitqueue_head(&i2c->wait);
> @@ -265,6 +295,9 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>   	i2c->adap = ocores_adapter;
>   	i2c_set_adapdata(&i2c->adap, i2c);
>   	i2c->adap.dev.parent =&pdev->dev;
> +#ifdef CONFIG_OF
> +	i2c->adap.dev.of_node = pdev->dev.of_node;
> +#endif
>
>   	/* add i2c adapter to i2c tree */
>   	ret = i2c_add_adapter(&i2c->adap);
> @@ -274,8 +307,10 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
>   	}
>
>   	/* add in known devices to the bus */
> -	for (i = 0; i<  pdata->num_devices; i++)
> -		i2c_new_device(&i2c->adap, pdata->devices + i);
> +	if (pdata) {
> +		for (i = 0; i<  pdata->num_devices; i++)
> +			i2c_new_device(&i2c->adap, pdata->devices + i);
> +	}
>
>   	return 0;
>
> @@ -344,6 +379,16 @@ static int ocores_i2c_resume(struct platform_device *pdev)
>   #define ocores_i2c_resume	NULL
>   #endif
>
> +#ifdef CONFIG_OF
> +static struct of_device_id ocores_i2c_match[] = {
> +        {
> +                .compatible = "opencores,i2c-ocores",
> +        },
> +        {},
> +};
> +MODULE_DEVICE_TABLE(of, ocores_i2c_match);
> +#endif
> +
>   /* work with hotplug and coldplug */
>   MODULE_ALIAS("platform:ocores-i2c");
>
> @@ -355,6 +400,9 @@ static struct platform_driver ocores_i2c_driver = {
>   	.driver  = {
>   		.owner = THIS_MODULE,
>   		.name = "ocores-i2c",
> +#ifdef CONFIG_OF
> +                .of_match_table = ocores_i2c_match,
> +#endif
>   	},
>   };
>

Re: [PATCH 1/3] i2c-ocores: Adapt for device tree

[Ben Dooks]

On 24/11/10 16:26, Jonas Bonn wrote:

OK, applied this series to my -next tree.

[PATCH 2/3] i2c-ocores: Use devres for resource allocation

[Jonas Bonn]

This patch converts the i2c-cores driver to use devres routines for
resource allocation.

Signed-off-by: Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>
---
 drivers/i2c/busses/i2c-ocores.c |   46 +++++++++-----------------------------
 1 files changed, 11 insertions(+), 35 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index c05e8c4..dee0352 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -253,22 +253,21 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	if (!res2)
 		return -ENODEV;
 
-	i2c = kzalloc(sizeof(*i2c), GFP_KERNEL);
+	i2c = devm_kzalloc(&pdev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return -ENOMEM;
 
-	if (!request_mem_region(res->start, resource_size(res),
-				pdev->name)) {
+	if (!devm_request_mem_region(&pdev->dev, res->start,
+				     resource_size(res), pdev->name)) {
 		dev_err(&pdev->dev, "Memory region busy\n");
-		ret = -EBUSY;
-		goto request_mem_failed;
+		return -EBUSY;
 	}
 
-	i2c->base = ioremap(res->start, resource_size(res));
+	i2c->base = devm_ioremap_nocache(&pdev->dev, res->start,
+					 resource_size(res));
 	if (!i2c->base) {
 		dev_err(&pdev->dev, "Unable to map registers\n");
-		ret = -EIO;
-		goto map_failed;
+		return -EIO;
 	}
 
 	pdata = pdev->dev.platform_data;
@@ -284,10 +283,11 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	ocores_init(i2c);
 
 	init_waitqueue_head(&i2c->wait);
-	ret = request_irq(res2->start, ocores_isr, 0, pdev->name, i2c);
+	ret = devm_request_irq(&pdev->dev, res2->start, ocores_isr, 0,
+			       pdev->name, i2c);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot claim IRQ\n");
-		goto request_irq_failed;
+		return ret;
 	}
 
 	/* hook up driver to tree */
@@ -303,7 +303,7 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	ret = i2c_add_adapter(&i2c->adap);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add adapter\n");
-		goto add_adapter_failed;
+		return ret;
 	}
 
 	/* add in known devices to the bus */
@@ -313,23 +313,11 @@ static int __devinit ocores_i2c_probe(struct platform_device *pdev)
 	}
 
 	return 0;
-
-add_adapter_failed:
-	free_irq(res2->start, i2c);
-request_irq_failed:
-	iounmap(i2c->base);
-map_failed:
-	release_mem_region(res->start, resource_size(res));
-request_mem_failed:
-	kfree(i2c);
-
-	return ret;
 }
 
 static int __devexit ocores_i2c_remove(struct platform_device* pdev)
 {
 	struct ocores_i2c *i2c = platform_get_drvdata(pdev);
-	struct resource *res;
 
 	/* disable i2c logic */
 	oc_setreg(i2c, OCI2C_CONTROL, oc_getreg(i2c, OCI2C_CONTROL)
@@ -339,18 +327,6 @@ static int __devexit ocores_i2c_remove(struct platform_device* pdev)
 	i2c_del_adapter(&i2c->adap);
 	platform_set_drvdata(pdev, NULL);
 
-	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (res)
-		free_irq(res->start, i2c);
-
-	iounmap(i2c->base);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (res)
-		release_mem_region(res->start, resource_size(res));
-
-	kfree(i2c);
-
 	return 0;
 }
 
-- 
1.7.1

[PATCH 3/3] i2c-ocores: add some device tree documentation

[Jonas Bonn]

This puts some documentation for the device tree configuration at the head
of the driver file.  Hopefully this can get moved to a common area for this
type of documentation at a later date; unfortunately, there isn't really
such a place in the kernel tree at this time.

Furthermore, the regstep and clock-frequency parameters are really bus
parameters and should probably be passed to the driver in a better way.
Consider that a TODO.

Signed-off-by: Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>
---
 drivers/i2c/busses/i2c-ocores.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
index dee0352..ef3bcb1 100644
--- a/drivers/i2c/busses/i2c-ocores.c
+++ b/drivers/i2c/busses/i2c-ocores.c
@@ -9,6 +9,41 @@
  * kind, whether express or implied.
  */
 
+/*
+ * Device tree configuration:
+ *
+ * Required properties:
+ * - compatible      : "opencores,i2c-ocores"
+ * - reg             : bus address start and address range size of device
+ * - interrupts      : interrupt number
+ * - regstep         : size of device registers in bytes
+ * - clock-frequency : frequency of bus clock in Hz
+ * 
+ * Example:
+ *
+ *  i2c0: ocores@a0000000 {
+ *              compatible = "opencores,i2c-ocores";
+ *              reg = <0xa0000000 0x8>;
+ *              interrupts = <10>;
+ *
+ *              regstep = <1>;
+ *              clock-frequency = <20000000>;
+ *
+ * -- Devices connected on this I2C bus get
+ * -- defined here; address- and size-cells
+ * -- apply to these child devices
+ *
+ *              #address-cells = <1>;
+ *              #size-cells = <0>;
+ *
+ *              dummy@60 {
+ *                     compatible = "dummy";
+ *                     reg = <60>;
+ *              };
+ *  };
+ *
+ */
+
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
-- 
1.7.1

Re: [PATCH 3/3] i2c-ocores: add some device tree documentation

[Grant Likely]

On Wed, Nov 24, 2010 at 05:26:22PM +0100, Jonas Bonn wrote:
> This puts some documentation for the device tree configuration at the head
> of the driver file.  Hopefully this can get moved to a common area for this
> type of documentation at a later date; unfortunately, there isn't really
> such a place in the kernel tree at this time.
> 
> Furthermore, the regstep and clock-frequency parameters are really bus
> parameters and should probably be passed to the driver in a better way.
> Consider that a TODO.
> 
> Signed-off-by: Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-ocores.c |   35 +++++++++++++++++++++++++++++++++++
>  1 files changed, 35 insertions(+), 0 deletions(-)

Binding looks good, except for a comment on the compatible value
below, but it should be kept with the other bindings.  Please move
into a file in Documentation/powerpc/dts-bindings (and, yes, this
directory does need to be moved out of Documentation/powerpc), or
document it on the devicetree.org wiki.

g.

> 
> diff --git a/drivers/i2c/busses/i2c-ocores.c b/drivers/i2c/busses/i2c-ocores.c
> index dee0352..ef3bcb1 100644
> --- a/drivers/i2c/busses/i2c-ocores.c
> +++ b/drivers/i2c/busses/i2c-ocores.c
> @@ -9,6 +9,41 @@
>   * kind, whether express or implied.
>   */
>  
> +/*
> + * Device tree configuration:
> + *
> + * Required properties:
> + * - compatible      : "opencores,i2c-ocores"

I assume the i2c-ocore interface could end up changing in the future.
This compatible value should have some form of version embedded into
it.

> + * - reg             : bus address start and address range size of device
> + * - interrupts      : interrupt number
> + * - regstep         : size of device registers in bytes
> + * - clock-frequency : frequency of bus clock in Hz
> + * 
> + * Example:
> + *
> + *  i2c0: ocores@a0000000 {
> + *              compatible = "opencores,i2c-ocores";
> + *              reg = <0xa0000000 0x8>;
> + *              interrupts = <10>;
> + *
> + *              regstep = <1>;
> + *              clock-frequency = <20000000>;
> + *
> + * -- Devices connected on this I2C bus get
> + * -- defined here; address- and size-cells
> + * -- apply to these child devices
> + *
> + *              #address-cells = <1>;
> + *              #size-cells = <0>;
> + *
> + *              dummy@60 {
> + *                     compatible = "dummy";
> + *                     reg = <60>;
> + *              };
> + *  };
> + *
> + */
> +
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> -- 
> 1.7.1
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/3] i2c-ocores: add some device tree documentation

[Jonas Bonn]

> > +/*
> > + * Device tree configuration:
> > + *
> > + * Required properties:
> > + * - compatible      : "opencores,i2c-ocores"
> 
> I assume the i2c-ocore interface could end up changing in the future.
> This compatible value should have some form of version embedded into
> it.
> 

Unfortunately, versioning is currently one of OpenCores weak points.
This driver is based on specification version 0.9 and has implementation
version 1.16 according to source and SVN revision 76.  I've started an
internal discussion to try to standardize on something useful, but
there's no consensus, as of yet.  I could make something up here for the
compatible value, but I'd rather we find a consistent versioning scheme
that's tenable long-term.

I guess I'll have to sit on these patches for another cycle while we
think about this.  Of course, if you have any suggestions, let me know;
looking at other drivers, there doesn't seem to be a canonical way of
doing this...

Perhaps:  opencores,i2c-cores-0.9-76
(i.e. {driver}-{spec revision}-{implementation svn rev})

/Jonas

PS: Honestly not crazy about the name 'i2c-ocores' either as it doesn't
match the name of the upstream Verilog project which is called,
unfortunately, simply 'i2c'.  This driver, however, has been upstream
for a while so I guess we're stuck with it.

Re: [PATCH 3/3] i2c-ocores: add some device tree documentation

[Peter Korsgaard]

>>>>> "Jonas" == Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org> writes:

Hi,

 Jonas> PS: Honestly not crazy about the name 'i2c-ocores' either as it
 Jonas> doesn't match the name of the upstream Verilog project which is
 Jonas> called, unfortunately, simply 'i2c'.  This driver, however, has
 Jonas> been upstream for a while so I guess we're stuck with it.

Yes, it was more or less the best name I could come with. Just calling
it i2c.c wouldn't have been any good either.

-- 
Bye, Peter Korsgaard

Re: [PATCH 3/3] i2c-ocores: add some device tree documentation

[Grant Likely]

On Fri, Dec 24, 2010 at 2:38 AM, Jonas Bonn <jonas-A9uVI2HLR7kOP4wsBPIw7w@public.gmane.org> wrote:
>> > +/*
>> > + * Device tree configuration:
>> > + *
>> > + * Required properties:
>> > + * - compatible      : "opencores,i2c-ocores"
>>
>> I assume the i2c-ocore interface could end up changing in the future.
>> This compatible value should have some form of version embedded into
>> it.
>>
>
> Unfortunately, versioning is currently one of OpenCores weak points.
> This driver is based on specification version 0.9 and has implementation
> version 1.16 according to source and SVN revision 76.  I've started an
> internal discussion to try to standardize on something useful, but
> there's no consensus, as of yet.  I could make something up here for the
> compatible value, but I'd rather we find a consistent versioning scheme
> that's tenable long-term.
>
> I guess I'll have to sit on these patches for another cycle while we
> think about this.

I think your suggestion below is fine.  I don't see any reason not to
merge this series in 2.6.38.

>  Of course, if you have any suggestions, let me know;
> looking at other drivers, there doesn't seem to be a canonical way of
> doing this...
>
> Perhaps:  opencores,i2c-cores-0.9-76
> (i.e. {driver}-{spec revision}-{implementation svn rev})

Yes, that sounds appropriate.  For xilinx cores I've pushed for the
full revision in the compatible values.  Newer cores can and should
claim compatibility with an older version that implements the same
interface.  Typically one well known, working version tends to become
the canonical version that newer cores claim compatibility against.

g.

Re: i2c-ocores changes (version 2)

[Ben Dooks]

On 24/11/10 16:26, Jonas Bonn wrote:
> Version 2 of patch series incorporating initial feedback from review.
> 
> /Jonas

I'm relatively happy with this.
Any comments from the devicetree side?

Re: i2c-ocores changes (version 2)

[Grant Likely]

On Mon, Dec 06, 2010 at 04:33:46AM +0000, Ben Dooks wrote:
> On 24/11/10 16:26, Jonas Bonn wrote:
> > Version 2 of patch series incorporating initial feedback from review.
> > 
> > /Jonas
> 
> I'm relatively happy with this.
> Any comments from the devicetree side?

Patches 1 & 2 look good to me.  Some comments on the 3rd patch.

Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>