Skip to contentSkip to conversation

W1: Support onewire master on i.MX53

PATCH Superseded Martin Fuzzey 4 participants 10 messages Jan 16, 2013

Subsystems (7)
Device tree bindings Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape +3 more subsystems

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Martin Fuzzey

author
#0 lore raw

cover W1: Support onewire master on i.MX53

W1: Support onewire master on i.MX53
* Add device tree and pinctrl support to the MXC master driver
* Add i.MX53 clocks
* Add i.MX53 device tree entries

The final patch converts the driver to use devm_

author
#1 lore raw

PATCH 1/5 W1: Add device tree support to MXC onewire master.

Signed-off-by Martin Fuzzey <[email protected]>
---
 drivers/w1/masters/mxc_w1.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/w1/masters/mxc_w1.c +8 -1
index 708a25f..949e566 100644
@@ -186,9 +186,16 @@ static int mxc_w1_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id mxc_w1_dt_ids[] = {
+	{ .compatible = "fsl,imx21-owire" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxc_w1_dt_ids);
+
 static struct platform_driver mxc_w1_driver = {
 	.driver = {
-		   .name = "mxc_w1",
+		.name = "mxc_w1",
+		.of_match_table = mxc_w1_dt_ids,
 	},
 	.probe = mxc_w1_probe,
 	.remove = mxc_w1_remove,
#2 lore raw

PATCH 1/5 W1: Add device tree support to MXC onewire master.

Hi,

This looks nice and simple, I just have a couple of comment.

Quoted 18 lines · Martin Fuzzey
Signed-off-by Martin Fuzzey <[email protected]>
---
 drivers/w1/masters/mxc_w1.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/w1/masters/mxc_w1.c +4 -0
index 708a25f..949e566 100644
@@ -186,9 +186,16 @@ static int mxc_w1_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id mxc_w1_dt_ids[] = {
+	{ .compatible = "fsl,imx21-owire" },
+	{ /* sentinel */ }
+};

drivers/w1/masters/mxc_w1.c static int mxc_w1_remove(struct platform_device *pdev) · line 201

Quoted 10 lines
drivers/w1/masters/mxc_w1.c +4 -1
+MODULE_DEVICE_TABLE(of, mxc_w1_dt_ids);
+
 static struct platform_driver mxc_w1_driver = {
 	.driver = {
-		   .name = "mxc_w1",
+		.name = "mxc_w1",
+		.of_match_table = mxc_w1_dt_ids,
 	},
 	.probe = mxc_w1_probe,
 	.remove = mxc_w1_remove,

I see there's already a binding for gpio-based w1 -- "w1-gpio". Given this is already using the "w1" naming convention in a binding, I think it'd be worth using the compatible string "fsl,imx21-w1", for consistency (both with the other binding and the driver name).

It's also be worth documenting (in Documentation/devicetree/bindings/w1/fsl-imx21-w1, presumably).

When adding bindings, it's also good to Cc devicetree-discuss.

Thanks,
Mark.

author
#3 lore raw

PATCH 1/5 W1: Add device tree support to MXC onewire master.

Quoted 3 lines · Mark Rutland

Hi,

This looks nice and simple, I just have a couple of comment.

Thank you

Quoted 25 lines · Martin Fuzzey
Quoted 24 lines
Signed-off-by Martin Fuzzey <[email protected]>
---
  drivers/w1/masters/mxc_w1.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)
drivers/w1/masters/mxc_w1.c +8 -1
index 708a25f..949e566 100644
@@ -186,9 +186,16 @@ static int mxc_w1_remove(struct platform_device *pdev)
  	return 0;
  }
 
+static struct of_device_id mxc_w1_dt_ids[] = {
+	{ .compatible = "fsl,imx21-owire" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxc_w1_dt_ids);
+
  static struct platform_driver mxc_w1_driver = {
  	.driver = {
-		   .name = "mxc_w1",
+		.name = "mxc_w1",
+		.of_match_table = mxc_w1_dt_ids,
Quoted 8 lines

drivers/w1/masters/mxc_w1.c static int mxc_w1_remove(struct platform_device *pdev) · line 201

Quoted 4 lines
drivers/w1/masters/mxc_w1.c +0 -0
  	},
  	.probe = mxc_w1_probe,
  	.remove = mxc_w1_remove,

I see there's already a binding for gpio-based w1 -- "w1-gpio". Given this is already using the "w1" naming convention in a binding, I think it'd be worth using the compatible string "fsl,imx21-w1", for consistency (both with the other binding and the driver name).

I don't have any strong opinions on this but the other imx53 bindings use the names used by Freescale in the hardware documentation hence the "owire" rather than "w1".

I'm happy to change it if that is the consensus though.

Quoted 2 lines

It's also be worth documenting (in Documentation/devicetree/bindings/w1/fsl-imx21-w1, presumably).

Ok, I thought that as the driver doesn't define any properties itself that this wasn't required. However l see some other drivers do document in this case.

So I will add a Documentation file.

Quoted 1 line

When adding bindings, it's also good to Cc devicetree-discuss.

Ok, added to -Cc

Martin

author
#4 lore raw

PATCH 2/5 ARM: i.MX53: Add clocks for i.mx53 onewire master.

In reply to Martin Fuzzey
Signed-off-by Martin Fuzzey <[email protected]>
---
 .../devicetree/bindings/clock/imx5-clock.txt       |    1 +
 arch/arm/mach-imx/clk-imx51-imx53.c                |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)
Documentation/devicetree/bindings/clock/imx5-clock.txt +1 -0
index 04ad478..2a0c904 100644
@@ -171,6 +171,7 @@ clocks and IDs.
 	can_sel			156
 	can1_serial_gate	157
 	can1_ipg_gate		158
+	owire_gate		159
 
 Examples (for mx53):
arch/arm/mach-imx/clk-imx51-imx53.c +3 -1
index 579023f..7ff293b 100644
@@ -83,6 +83,7 @@ enum imx5_clks {
 	ssi2_root_gate, ssi3_root_gate, ssi_ext1_gate, ssi_ext2_gate,
 	epit1_ipg_gate, epit1_hf_gate, epit2_ipg_gate, epit2_hf_gate,
 	can_sel, can1_serial_gate, can1_ipg_gate,
+	owire_gate,
 	clk_max
 };
 

@@ -233,12 +234,13 @@ static void __init mx5_clocks_common_init(unsigned long rate_ckil,
 	clk[epit1_hf_gate] = imx_clk_gate2("epit1_hf_gate", "per_root", MXC_CCM_CCGR2, 4);
 	clk[epit2_ipg_gate] = imx_clk_gate2("epit2_ipg_gate", "ipg", MXC_CCM_CCGR2, 6);
 	clk[epit2_hf_gate] = imx_clk_gate2("epit2_hf_gate", "per_root", MXC_CCM_CCGR2, 8);
+	clk[owire_gate] = imx_clk_gate2("owire_gate", "per_root", MXC_CCM_CCGR2, 22);
 
 	for (i = 0; i < ARRAY_SIZE(clk); i++)
 		if (IS_ERR(clk[i]))
 			pr_err("i.MX5 clk %d: register failed with %ld\n",
 				i, PTR_ERR(clk[i]));
-	
+
 	clk_register_clkdev(clk[gpt_hf_gate], "per", "imx-gpt.0");
 	clk_register_clkdev(clk[gpt_ipg_gate], "ipg", "imx-gpt.0");
 	clk_register_clkdev(clk[uart1_per_gate], "per", "imx21-uart.0");
author
#5 lore raw

PATCH 3/5 DTS: Add device tree entry for onewire master on i.MX53

In reply to Martin Fuzzey
Signed-off-by Martin Fuzzey <[email protected]>
---
 arch/arm/boot/dts/imx53.dtsi |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
arch/arm/boot/dts/imx53.dtsi +15 -0
index edc3f1e..00c957c 100644
@@ -387,6 +387,14 @@
 					};
 				};
 
+				owire {
+					pinctrl_owire_1: owiregrp-1 {
+						fsl,pins = <
+								1166 0x80000000 /* MX53_PAD_GPIO_18__OWIRE_LINE */
+						>;
+					};
+				};
+
 				uart1 {
 					pinctrl_uart1_1: uart1grp-1 {
 						fsl,pins = <

@@ -570,6 +578,13 @@
 				status = "disabled";
 			};
 
+			owire: owire@63fa4000 {
+				compatible = "fsl,imx53-owire", "fsl,imx21-owire";
+				reg = <0x63fa4000 0x4000>;
+				clocks = <&clks 159>;
+				status = "disabled";
+			};
+
 			ecspi2: ecspi at 63fac000 {
 				#address-cells = <1>;
 				#size-cells = <0>;
author
#6 lore raw

PATCH 4/5 W1: Add pinctrl support to MXC onewire master.

In reply to Martin Fuzzey
Signed-off-by Martin Fuzzey <[email protected]>
---
 drivers/w1/masters/mxc_w1.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
drivers/w1/masters/mxc_w1.c +10 -0
index 949e566..7454938 100644
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/delay.h>
 #include <linux/io.h>
+#include <linux/pinctrl/consumer.h>
 
 #include "../w1.h"
 #include "../w1_int.h"

@@ -107,6 +108,7 @@ static int mxc_w1_probe(struct platform_device *pdev)
 {
 	struct mxc_w1_device *mdev;
 	struct resource *res;
+	struct pinctrl *pinctrl;
 	int err = 0;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -117,6 +119,13 @@ static int mxc_w1_probe(struct platform_device *pdev)
 	if (!mdev)
 		return -ENOMEM;
 
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		err = PTR_ERR(pinctrl);
+		dev_err(&pdev->dev, "failed to get default pinctrl: %d\n", err);
+		goto failed_pin;
+	}
+
 	mdev->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(mdev->clk)) {
 		err = PTR_ERR(mdev->clk);

@@ -159,6 +168,7 @@ failed_ioremap:
 	release_mem_region(res->start, resource_size(res));
 failed_req:
 	clk_put(mdev->clk);
+failed_pin:
 failed_clk:
 	kfree(mdev);
 	return err;
author
#7 lore raw

PATCH 5/5 W1: Convert MXC onewire master to devm_ functions.

In reply to Martin Fuzzey
Signed-off-by Martin Fuzzey <[email protected]>
---
 drivers/w1/masters/mxc_w1.c |   35 ++++++++---------------------------
 1 files changed, 8 insertions(+), 27 deletions(-)
drivers/w1/masters/mxc_w1.c +8 -27
index 7454938..5b0c700 100644
@@ -123,28 +123,22 @@ static int mxc_w1_probe(struct platform_device *pdev)
 	if (IS_ERR(pinctrl)) {
 		err = PTR_ERR(pinctrl);
 		dev_err(&pdev->dev, "failed to get default pinctrl: %d\n", err);
-		goto failed_pin;
+		goto failed;
 	}
 
-	mdev->clk = clk_get(&pdev->dev, NULL);
+	mdev->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(mdev->clk)) {
 		err = PTR_ERR(mdev->clk);
-		goto failed_clk;
+		goto failed;
 	}
 
 	mdev->clkdiv = (clk_get_rate(mdev->clk) / 1000000) - 1;
 
-	res = request_mem_region(res->start, resource_size(res),
-				"mxc_w1");
-	if (!res) {
-		err = -EBUSY;
-		goto failed_req;
-	}
-
-	mdev->regs = ioremap(res->start, resource_size(res));
+	mdev->regs = devm_request_and_ioremap(&pdev->dev, res);
 	if (!mdev->regs) {
 		dev_err(&pdev->dev, "Cannot map mxc_w1 registers\n");
-		goto failed_ioremap;
+		err = -ENXIO;
+		goto failed;
 	}
 
 	clk_prepare_enable(mdev->clk);

@@ -157,19 +151,12 @@ static int mxc_w1_probe(struct platform_device *pdev)
 	err = w1_add_master_device(&mdev->bus_master);
 
 	if (err)
-		goto failed_add;
+		goto failed;
 
 	platform_set_drvdata(pdev, mdev);
 	return 0;
 
-failed_add:
-	iounmap(mdev->regs);
-failed_ioremap:
-	release_mem_region(res->start, resource_size(res));
-failed_req:
-	clk_put(mdev->clk);
-failed_pin:
-failed_clk:
+failed:
 	kfree(mdev);
 	return err;
 }

@@ -180,16 +167,10 @@ failed_clk:
 static int mxc_w1_remove(struct platform_device *pdev)
 {
 	struct mxc_w1_device *mdev = platform_get_drvdata(pdev);
-	struct resource *res;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	w1_remove_master_device(&mdev->bus_master);
 
-	iounmap(mdev->regs);
-	release_mem_region(res->start, resource_size(res));
 	clk_disable_unprepare(mdev->clk);
-	clk_put(mdev->clk);
 
 	platform_set_drvdata(pdev, NULL);
maintainer
#8 lore raw

cover W1: Support onewire master on i.MX53

In reply to Martin Fuzzey

Hi Martin,

Quoted 6 lines · Martin Fuzzey

W1: Support onewire master on i.MX53
* Add device tree and pinctrl support to the MXC master driver
* Add i.MX53 clocks
* Add i.MX53 device tree entries

The final patch converts the driver to use devm_

Looks mostly good. I think the patch converting to devm_ should come before the one adding pinctrl for the driver, just because the driver should first be switched to devm_ and afterwards the devm_ usage should be extended.

Other than that I just noticed that the clk_prepare_enable/clk_disable_unprepare is unbalanced in the error case of the driver (clk_disable_unprepare missing after the failed_add label). Might be worth fixing this in this series.

Sascha

Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
#9 lore raw

cover W1: Support onewire master on i.MX53

In reply to Martin Fuzzey

Hi

Quoted 6 lines · Martin Fuzzey (mfuzzey at parkeon.com)

W1: Support onewire master on i.MX53
* Add device tree and pinctrl support to the MXC master driver
* Add i.MX53 clocks
* Add i.MX53 device tree entries

The final patch converts the driver to use devm_

Looks good to me, thank you

Acked-by Evgeniy Polyakov <[email protected]>
Signature · 1 line
	Evgeniy Polyakov