[PATCH] i2c-gpio: Add support for deferred probing

[PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

GPIOs may not be available immediately when i2c-gpio looks for them.
Implement support for deferred probing so that probing can be
attempted again later when GPIO pins are finally available.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
A little more changes were needed than I initially thought, because we
want to check the pins before we allocate memory. Otherwise we would
have a possibly large number of memory allocation and freeing cycles
until GPIO pins are finally available.

Note: I wrote this patch for someone else, I can't test it, so I would
appreciate if someone could at least test and confirm that I did not
break anything in the standard (non-deferred) case. Thanks!

 drivers/i2c/busses/i2c-gpio.c |   76 +++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 25 deletions(-)

--- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-gpio.c	2012-06-05 16:22:59.000000000 +0200
+++ linux-3.5-rc5/drivers/i2c/busses/i2c-gpio.c	2012-07-07 10:53:40.911407245 +0200
@@ -85,23 +85,30 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
-static int __devinit of_i2c_gpio_probe(struct device_node *np,
-			     struct i2c_gpio_platform_data *pdata)
+static int __devinit of_i2c_gpio_get_pins(struct device_node *np,
+					  unsigned int *sda_pin,
+					  unsigned int *scl_pin)
 {
-	u32 reg;
-
 	if (of_gpio_count(np) < 2)
 		return -ENODEV;
 
-	pdata->sda_pin = of_get_gpio(np, 0);
-	pdata->scl_pin = of_get_gpio(np, 1);
+	*sda_pin = of_get_gpio(np, 0);
+	*scl_pin = of_get_gpio(np, 1);
 
-	if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) {
+	if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) {
 		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
-		       np->full_name, pdata->sda_pin, pdata->scl_pin);
+		       np->full_name, *sda_pin, *scl_pin);
 		return -ENODEV;
 	}
 
+	return 0;
+}
+
+static void __devinit of_i2c_gpio_get_props(struct device_node *np,
+					struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+
 	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
 
 	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
@@ -113,8 +120,6 @@ static int __devinit of_i2c_gpio_probe(s
 		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
 	pdata->scl_is_output_only =
 		of_property_read_bool(np, "i2c-gpio,scl-output-only");
-
-	return 0;
 }
 
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
@@ -123,31 +128,52 @@ static int __devinit i2c_gpio_probe(stru
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
+	unsigned int sda_pin, scl_pin;
 	int ret;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	adap = &priv->adap;
-	bit_data = &priv->bit_data;
-	pdata = &priv->pdata;
-
+	/* First get the GPIO pins; if it fails, we'll defer the probe. */
 	if (pdev->dev.of_node) {
-		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
+		ret = of_i2c_gpio_get_pins(pdev->dev.of_node,
+					   &sda_pin, &scl_pin);
 		if (ret)
 			return ret;
 	} else {
 		if (!pdev->dev.platform_data)
 			return -ENXIO;
-		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+		pdata = pdev->dev.platform_data;
+		sda_pin = pdata->sda_pin;
+		scl_pin = pdata->scl_pin;
 	}
 
-	ret = gpio_request(pdata->sda_pin, "sda");
-	if (ret)
+	ret = gpio_request(sda_pin, "sda");
+	if (ret) {
+		if (ret == -EINVAL)
+			ret = -EPROBE_DEFER;	/* Try again later */
 		goto err_request_sda;
-	ret = gpio_request(pdata->scl_pin, "scl");
-	if (ret)
+	}
+	ret = gpio_request(scl_pin, "scl");
+	if (ret) {
+		if (ret == -EINVAL)
+			ret = -EPROBE_DEFER;	/* Try again later */
 		goto err_request_scl;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err_add_bus;
+	}
+	adap = &priv->adap;
+	bit_data = &priv->bit_data;
+	pdata = &priv->pdata;
+
+	if (pdev->dev.of_node) {
+		pdata->sda_pin = sda_pin;
+		pdata->scl_pin = scl_pin;
+		of_i2c_gpio_get_props(pdev->dev.of_node, pdata);
+	} else {
+		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+	}
 
 	if (pdata->sda_is_open_drain) {
 		gpio_direction_output(pdata->sda_pin, 1);
@@ -207,9 +233,9 @@ static int __devinit i2c_gpio_probe(stru
 	return 0;
 
 err_add_bus:
-	gpio_free(pdata->scl_pin);
+	gpio_free(scl_pin);
 err_request_scl:
-	gpio_free(pdata->sda_pin);
+	gpio_free(sda_pin);
 err_request_sda:
 	return ret;
 }


-- 
Jean Delvare

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

On Thu, 19 Jul 2012 13:19:24 +0200, Jean Delvare wrote:
> GPIOs may not be available immediately when i2c-gpio looks for them.
> Implement support for deferred probing so that probing can be
> attempted again later when GPIO pins are finally available.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> A little more changes were needed than I initially thought, because we
> want to check the pins before we allocate memory. Otherwise we would
> have a possibly large number of memory allocation and freeing cycles
> until GPIO pins are finally available.
> 
> Note: I wrote this patch for someone else, I can't test it, so I would
> appreciate if someone could at least test and confirm that I did not
> break anything in the standard (non-deferred) case. Thanks!

Anyone, please? Anyone using i2c-gpio, please test this patch and
report how it goes. Thanks.

>  drivers/i2c/busses/i2c-gpio.c |   76 +++++++++++++++++++++++++++--------------
>  1 file changed, 51 insertions(+), 25 deletions(-)
> 
> --- linux-3.5-rc5.orig/drivers/i2c/busses/i2c-gpio.c	2012-06-05 16:22:59.000000000 +0200
> +++ linux-3.5-rc5/drivers/i2c/busses/i2c-gpio.c	2012-07-07 10:53:40.911407245 +0200
> @@ -85,23 +85,30 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> -static int __devinit of_i2c_gpio_probe(struct device_node *np,
> -			     struct i2c_gpio_platform_data *pdata)
> +static int __devinit of_i2c_gpio_get_pins(struct device_node *np,
> +					  unsigned int *sda_pin,
> +					  unsigned int *scl_pin)
>  {
> -	u32 reg;
> -
>  	if (of_gpio_count(np) < 2)
>  		return -ENODEV;
>  
> -	pdata->sda_pin = of_get_gpio(np, 0);
> -	pdata->scl_pin = of_get_gpio(np, 1);
> +	*sda_pin = of_get_gpio(np, 0);
> +	*scl_pin = of_get_gpio(np, 1);
>  
> -	if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) {
> +	if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) {
>  		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> -		       np->full_name, pdata->sda_pin, pdata->scl_pin);
> +		       np->full_name, *sda_pin, *scl_pin);
>  		return -ENODEV;
>  	}
>  
> +	return 0;
> +}
> +
> +static void __devinit of_i2c_gpio_get_props(struct device_node *np,
> +					struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
>  	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
>  
>  	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
> @@ -113,8 +120,6 @@ static int __devinit of_i2c_gpio_probe(s
>  		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
>  	pdata->scl_is_output_only =
>  		of_property_read_bool(np, "i2c-gpio,scl-output-only");
> -
> -	return 0;
>  }
>  
>  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> @@ -123,31 +128,52 @@ static int __devinit i2c_gpio_probe(stru
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_algo_bit_data *bit_data;
>  	struct i2c_adapter *adap;
> +	unsigned int sda_pin, scl_pin;
>  	int ret;
>  
> -	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -	adap = &priv->adap;
> -	bit_data = &priv->bit_data;
> -	pdata = &priv->pdata;
> -
> +	/* First get the GPIO pins; if it fails, we'll defer the probe. */
>  	if (pdev->dev.of_node) {
> -		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
> +		ret = of_i2c_gpio_get_pins(pdev->dev.of_node,
> +					   &sda_pin, &scl_pin);
>  		if (ret)
>  			return ret;
>  	} else {
>  		if (!pdev->dev.platform_data)
>  			return -ENXIO;
> -		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
> +		pdata = pdev->dev.platform_data;
> +		sda_pin = pdata->sda_pin;
> +		scl_pin = pdata->scl_pin;
>  	}
>  
> -	ret = gpio_request(pdata->sda_pin, "sda");
> -	if (ret)
> +	ret = gpio_request(sda_pin, "sda");
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			ret = -EPROBE_DEFER;	/* Try again later */
>  		goto err_request_sda;
> -	ret = gpio_request(pdata->scl_pin, "scl");
> -	if (ret)
> +	}
> +	ret = gpio_request(scl_pin, "scl");
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			ret = -EPROBE_DEFER;	/* Try again later */
>  		goto err_request_scl;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err_add_bus;
> +	}
> +	adap = &priv->adap;
> +	bit_data = &priv->bit_data;
> +	pdata = &priv->pdata;
> +
> +	if (pdev->dev.of_node) {
> +		pdata->sda_pin = sda_pin;
> +		pdata->scl_pin = scl_pin;
> +		of_i2c_gpio_get_props(pdev->dev.of_node, pdata);
> +	} else {
> +		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
> +	}
>  
>  	if (pdata->sda_is_open_drain) {
>  		gpio_direction_output(pdata->sda_pin, 1);
> @@ -207,9 +233,9 @@ static int __devinit i2c_gpio_probe(stru
>  	return 0;
>  
>  err_add_bus:
> -	gpio_free(pdata->scl_pin);
> +	gpio_free(scl_pin);
>  err_request_scl:
> -	gpio_free(pdata->sda_pin);
> +	gpio_free(sda_pin);
>  err_request_sda:
>  	return ret;
>  }
> 
> 


-- 
Jean Delvare

[PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

GPIOs may not be available immediately when i2c-gpio looks for them.
Implement support for deferred probing so that probing can be
attempted again later when GPIO pins are finally available.

Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
A little more changes were needed than I initially thought, because we
want to check the pins before we allocate memory. Otherwise we would
have a possibly large number of memory allocation and freeing cycles
until GPIO pins are finally available.

I wrote this quite some time ago already, but I do not have any system
to test it. I would appreciate if one or more users of the i2c-gpio
driver could give it a try and confirm it works as intended, or at
least doesn't introduce any regression. Thanks.

 drivers/i2c/busses/i2c-gpio.c |   75 ++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 25 deletions(-)

--- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:26:30.031060312 +0100
+++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:32:59.950308645 +0100
@@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
-static int of_i2c_gpio_probe(struct device_node *np,
-			     struct i2c_gpio_platform_data *pdata)
+static int of_i2c_gpio_get_pins(struct device_node *np,
+				unsigned int *sda_pin, unsigned int *scl_pin)
 {
-	u32 reg;
-
 	if (of_gpio_count(np) < 2)
 		return -ENODEV;
 
-	pdata->sda_pin = of_get_gpio(np, 0);
-	pdata->scl_pin = of_get_gpio(np, 1);
+	*sda_pin = of_get_gpio(np, 0);
+	*scl_pin = of_get_gpio(np, 1);
 
-	if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) {
+	if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) {
 		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
-		       np->full_name, pdata->sda_pin, pdata->scl_pin);
+		       np->full_name, *sda_pin, *scl_pin);
 		return -ENODEV;
 	}
 
+	return 0;
+}
+
+static void of_i2c_gpio_get_props(struct device_node *np,
+				  struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+
 	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
 
 	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
@@ -113,8 +119,6 @@ static int of_i2c_gpio_probe(struct devi
 		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
 	pdata->scl_is_output_only =
 		of_property_read_bool(np, "i2c-gpio,scl-output-only");
-
-	return 0;
 }
 
 static int i2c_gpio_probe(struct platform_device *pdev)
@@ -123,31 +127,52 @@ static int i2c_gpio_probe(struct platfor
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
+	unsigned int sda_pin, scl_pin;
 	int ret;
 
-	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-	adap = &priv->adap;
-	bit_data = &priv->bit_data;
-	pdata = &priv->pdata;
-
+	/* First get the GPIO pins; if it fails, we'll defer the probe. */
 	if (pdev->dev.of_node) {
-		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
+		ret = of_i2c_gpio_get_pins(pdev->dev.of_node,
+					   &sda_pin, &scl_pin);
 		if (ret)
 			return ret;
 	} else {
 		if (!pdev->dev.platform_data)
 			return -ENXIO;
-		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+		pdata = pdev->dev.platform_data;
+		sda_pin = pdata->sda_pin;
+		scl_pin = pdata->scl_pin;
 	}
 
-	ret = gpio_request(pdata->sda_pin, "sda");
-	if (ret)
+	ret = gpio_request(sda_pin, "sda");
+	if (ret) {
+		if (ret == -EINVAL)
+			ret = -EPROBE_DEFER;	/* Try again later */
 		goto err_request_sda;
-	ret = gpio_request(pdata->scl_pin, "scl");
-	if (ret)
+	}
+	ret = gpio_request(scl_pin, "scl");
+	if (ret) {
+		if (ret == -EINVAL)
+			ret = -EPROBE_DEFER;	/* Try again later */
 		goto err_request_scl;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err_add_bus;
+	}
+	adap = &priv->adap;
+	bit_data = &priv->bit_data;
+	pdata = &priv->pdata;
+
+	if (pdev->dev.of_node) {
+		pdata->sda_pin = sda_pin;
+		pdata->scl_pin = scl_pin;
+		of_i2c_gpio_get_props(pdev->dev.of_node, pdata);
+	} else {
+		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+	}
 
 	if (pdata->sda_is_open_drain) {
 		gpio_direction_output(pdata->sda_pin, 1);
@@ -211,9 +236,9 @@ static int i2c_gpio_probe(struct platfor
 	return 0;
 
 err_add_bus:
-	gpio_free(pdata->scl_pin);
+	gpio_free(scl_pin);
 err_request_scl:
-	gpio_free(pdata->sda_pin);
+	gpio_free(sda_pin);
 err_request_sda:
 	return ret;
 }


-- 
Jean Delvare

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Bo Shen]

Hi Jean,

On 2/28/2013 19:01, Jean Delvare wrote:
> GPIOs may not be available immediately when i2c-gpio looks for them.
> Implement support for deferred probing so that probing can be
> attempted again later when GPIO pins are finally available.
>
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> A little more changes were needed than I initially thought, because we
> want to check the pins before we allocate memory. Otherwise we would
> have a possibly large number of memory allocation and freeing cycles
> until GPIO pins are finally available.
>
> I wrote this quite some time ago already, but I do not have any system
> to test it. I would appreciate if one or more users of the i2c-gpio
> driver could give it a try and confirm it works as intended, or at
> least doesn't introduce any regression. Thanks.
>
>   drivers/i2c/busses/i2c-gpio.c |   75 ++++++++++++++++++++++++++++--------------
>   1 file changed, 50 insertions(+), 25 deletions(-)

Test on at91sam9g20ek_2mmc board, booting with non-dt kernel and 
dt-kernel base on Linux-3.8. Both are OK.

Tested-by: Bo Shen <voice.shen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Best Regards,
Bo Shen

> --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:26:30.031060312 +0100
> +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:32:59.950308645 +0100
> @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data)
>   	return gpio_get_value(pdata->scl_pin);
>   }
>
> -static int of_i2c_gpio_probe(struct device_node *np,
> -			     struct i2c_gpio_platform_data *pdata)
> +static int of_i2c_gpio_get_pins(struct device_node *np,
> +				unsigned int *sda_pin, unsigned int *scl_pin)
>   {
> -	u32 reg;
> -
>   	if (of_gpio_count(np) < 2)
>   		return -ENODEV;
>
> -	pdata->sda_pin = of_get_gpio(np, 0);
> -	pdata->scl_pin = of_get_gpio(np, 1);
> +	*sda_pin = of_get_gpio(np, 0);
> +	*scl_pin = of_get_gpio(np, 1);
>
> -	if (!gpio_is_valid(pdata->sda_pin) || !gpio_is_valid(pdata->scl_pin)) {
> +	if (!gpio_is_valid(*sda_pin) || !gpio_is_valid(*scl_pin)) {
>   		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> -		       np->full_name, pdata->sda_pin, pdata->scl_pin);
> +		       np->full_name, *sda_pin, *scl_pin);
>   		return -ENODEV;
>   	}
>
> +	return 0;
> +}
> +
> +static void of_i2c_gpio_get_props(struct device_node *np,
> +				  struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
>   	of_property_read_u32(np, "i2c-gpio,delay-us", &pdata->udelay);
>
>   	if (!of_property_read_u32(np, "i2c-gpio,timeout-ms", &reg))
> @@ -113,8 +119,6 @@ static int of_i2c_gpio_probe(struct devi
>   		of_property_read_bool(np, "i2c-gpio,scl-open-drain");
>   	pdata->scl_is_output_only =
>   		of_property_read_bool(np, "i2c-gpio,scl-output-only");
> -
> -	return 0;
>   }
>
>   static int i2c_gpio_probe(struct platform_device *pdev)
> @@ -123,31 +127,52 @@ static int i2c_gpio_probe(struct platfor
>   	struct i2c_gpio_platform_data *pdata;
>   	struct i2c_algo_bit_data *bit_data;
>   	struct i2c_adapter *adap;
> +	unsigned int sda_pin, scl_pin;
>   	int ret;
>
> -	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -	adap = &priv->adap;
> -	bit_data = &priv->bit_data;
> -	pdata = &priv->pdata;
> -
> +	/* First get the GPIO pins; if it fails, we'll defer the probe. */
>   	if (pdev->dev.of_node) {
> -		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
> +		ret = of_i2c_gpio_get_pins(pdev->dev.of_node,
> +					   &sda_pin, &scl_pin);
>   		if (ret)
>   			return ret;
>   	} else {
>   		if (!pdev->dev.platform_data)
>   			return -ENXIO;
> -		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
> +		pdata = pdev->dev.platform_data;
> +		sda_pin = pdata->sda_pin;
> +		scl_pin = pdata->scl_pin;
>   	}
>
> -	ret = gpio_request(pdata->sda_pin, "sda");
> -	if (ret)
> +	ret = gpio_request(sda_pin, "sda");
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			ret = -EPROBE_DEFER;	/* Try again later */
>   		goto err_request_sda;
> -	ret = gpio_request(pdata->scl_pin, "scl");
> -	if (ret)
> +	}
> +	ret = gpio_request(scl_pin, "scl");
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			ret = -EPROBE_DEFER;	/* Try again later */
>   		goto err_request_scl;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err_add_bus;
> +	}
> +	adap = &priv->adap;
> +	bit_data = &priv->bit_data;
> +	pdata = &priv->pdata;
> +
> +	if (pdev->dev.of_node) {
> +		pdata->sda_pin = sda_pin;
> +		pdata->scl_pin = scl_pin;
> +		of_i2c_gpio_get_props(pdev->dev.of_node, pdata);
> +	} else {
> +		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
> +	}
>
>   	if (pdata->sda_is_open_drain) {
>   		gpio_direction_output(pdata->sda_pin, 1);
> @@ -211,9 +236,9 @@ static int i2c_gpio_probe(struct platfor
>   	return 0;
>
>   err_add_bus:
> -	gpio_free(pdata->scl_pin);
> +	gpio_free(scl_pin);
>   err_request_scl:
> -	gpio_free(pdata->sda_pin);
> +	gpio_free(sda_pin);
>   err_request_sda:
>   	return ret;
>   }
>
>

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

On Fri, 01 Mar 2013 11:58:29 +0800, Bo Shen wrote:
> Hi Jean,
> 
> On 2/28/2013 19:01, Jean Delvare wrote:
> > GPIOs may not be available immediately when i2c-gpio looks for them.
> > Implement support for deferred probing so that probing can be
> > attempted again later when GPIO pins are finally available.
> >
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > A little more changes were needed than I initially thought, because we
> > want to check the pins before we allocate memory. Otherwise we would
> > have a possibly large number of memory allocation and freeing cycles
> > until GPIO pins are finally available.
> >
> > I wrote this quite some time ago already, but I do not have any system
> > to test it. I would appreciate if one or more users of the i2c-gpio
> > driver could give it a try and confirm it works as intended, or at
> > least doesn't introduce any regression. Thanks.
> >
> >   drivers/i2c/busses/i2c-gpio.c |   75 ++++++++++++++++++++++++++++--------------
> >   1 file changed, 50 insertions(+), 25 deletions(-)
> 
> Test on at91sam9g20ek_2mmc board, booting with non-dt kernel and 
> dt-kernel base on Linux-3.8. Both are OK.
> 
> Tested-by: Bo Shen <voice.shen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

Thanks Bo, this is very appreciated!

-- 
Jean Delvare

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Wolfram Sang]

Hi Jean,

On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote:
> GPIOs may not be available immediately when i2c-gpio looks for them.
> Implement support for deferred probing so that probing can be
> attempted again later when GPIO pins are finally available.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> A little more changes were needed than I initially thought, because we
> want to check the pins before we allocate memory. Otherwise we would
> have a possibly large number of memory allocation and freeing cycles
> until GPIO pins are finally available.
> 
> I wrote this quite some time ago already, but I do not have any system
> to test it. I would appreciate if one or more users of the i2c-gpio
> driver could give it a try and confirm it works as intended, or at
> least doesn't introduce any regression. Thanks.
> 
>  drivers/i2c/busses/i2c-gpio.c |   75 ++++++++++++++++++++++++++++--------------
>  1 file changed, 50 insertions(+), 25 deletions(-)
> 
> --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:26:30.031060312 +0100
> +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:32:59.950308645 +0100
> @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> -static int of_i2c_gpio_probe(struct device_node *np,
> -			     struct i2c_gpio_platform_data *pdata)
> +static int of_i2c_gpio_get_pins(struct device_node *np,
> +				unsigned int *sda_pin, unsigned int *scl_pin)

GPIOs are expressed via int.

> +	ret = gpio_request(sda_pin, "sda");
> +	if (ret) {
> +		if (ret == -EINVAL)
> +			ret = -EPROBE_DEFER;	/* Try again later */

Would gpio_request_array() make the code simpler?

Thanks,

   Wolfram

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

Hi Wolfram,

Thanks for the review.

On Fri, 22 Mar 2013 12:56:21 +0100, Wolfram Sang wrote:
> On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote:
> > GPIOs may not be available immediately when i2c-gpio looks for them.
> > Implement support for deferred probing so that probing can be
> > attempted again later when GPIO pins are finally available.
> > 
> > Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> > Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > ---
> > A little more changes were needed than I initially thought, because we
> > want to check the pins before we allocate memory. Otherwise we would
> > have a possibly large number of memory allocation and freeing cycles
> > until GPIO pins are finally available.
> > 
> > I wrote this quite some time ago already, but I do not have any system
> > to test it. I would appreciate if one or more users of the i2c-gpio
> > driver could give it a try and confirm it works as intended, or at
> > least doesn't introduce any regression. Thanks.
> > 
> >  drivers/i2c/busses/i2c-gpio.c |   75 ++++++++++++++++++++++++++++--------------
> >  1 file changed, 50 insertions(+), 25 deletions(-)
> > 
> > --- linux-3.8-rc2.orig/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:26:30.031060312 +0100
> > +++ linux-3.8-rc2/drivers/i2c/busses/i2c-gpio.c	2013-01-09 22:32:59.950308645 +0100
> > @@ -85,23 +85,29 @@ static int i2c_gpio_getscl(void *data)
> >  	return gpio_get_value(pdata->scl_pin);
> >  }
> >  
> > -static int of_i2c_gpio_probe(struct device_node *np,
> > -			     struct i2c_gpio_platform_data *pdata)
> > +static int of_i2c_gpio_get_pins(struct device_node *np,
> > +				unsigned int *sda_pin, unsigned int *scl_pin)
> 
> GPIOs are expressed via int.

What do you mean? In <linux/gpio.h> I see:

struct gpio {
	unsigned	gpio;
(...)

static inline int gpio_get_value(unsigned int gpio)
{
	return __gpio_get_value(gpio);
}

And in <linux/i2c-gpio.h>:

struct i2c_gpio_platform_data {
	unsigned int    sda_pin;
	unsigned int    scl_pin;
(...)

So unsigned int seems to be the right type for gpios. The OF layer
indeed uses int as the return type of of_get_gpio(), presumably to be
able to report errors, but the original code did not handle errors from
these calls so I assumed they couldn't fail and did not bother adding
error handling.

If you still have a concern about the types used, please clarify and
let me know what change you expect.

> > +	ret = gpio_request(sda_pin, "sda");
> > +	if (ret) {
> > +		if (ret == -EINVAL)
> > +			ret = -EPROBE_DEFER;	/* Try again later */
> 
> Would gpio_request_array() make the code simpler?

I gave it a try, this indeed makes the code slightly simpler (-4 lines)
but the resulting binary slightly larger (+40 bytes on x86-64.) I'd say
it's not worth it?

Note that my patch doesn't introduce the gpio_request() calls, they
were there before, so this decision is actually independent from my
patch, and even if we decide to switch to using gpio_request_array(),
I'd rather do it in a separate patch for clarity.

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Wolfram Sang]

> What do you mean? In <linux/gpio.h> I see:
> 
> struct gpio {
> 	unsigned	gpio;
> (...)
> 
> static inline int gpio_get_value(unsigned int gpio)
> {
> 	return __gpio_get_value(gpio);
> }
> 
> And in <linux/i2c-gpio.h>:
> 
> struct i2c_gpio_platform_data {
> 	unsigned int    sda_pin;
> 	unsigned int    scl_pin;
> (...)

I remembered this paragraph from Documentation/gpio.txt:

===

If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL"); that will never be valid.  To
test if such number from such a structure could reference a GPIO, you
may use this predicate:

        int gpio_is_valid(int number);
...

===

Confusingly, I know found that the chapter starts with

===

GPIOs are identified by unsigned integers in the range 0..MAX_INT.  That
reserves "negative" numbers for other purposes like marking signals as
"not available on this board", or indicating faults.

===

Meh.

> If you still have a concern about the types used, please clarify and
> let me know what change you expect.

Leave it. I think the fragile part is gpio_is_valid() but this is truly
outside the scope of this patch.

> > > +	ret = gpio_request(sda_pin, "sda");
> > > +	if (ret) {
> > > +		if (ret == -EINVAL)
> > > +			ret = -EPROBE_DEFER;	/* Try again later */
> > 
> > Would gpio_request_array() make the code simpler?
> 
> I gave it a try, this indeed makes the code slightly simpler (-4 lines)
> but the resulting binary slightly larger (+40 bytes on x86-64.) I'd say
> it's not worth it?

OK. Then leave it.

> Note that my patch doesn't introduce the gpio_request() calls, they
> were there before, so this decision is actually independent from my
> patch, and even if we decide to switch to using gpio_request_array(),
> I'd rather do it in a separate patch for clarity.

I don't fully get it. Do you want to appl gpio_request() to this patch?
Otherwise, I'd take it as is.

Thanks,

   Wolfram

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Jean Delvare]

Hi Wolfram,

On Tue, 26 Mar 2013 12:09:08 +0100, Wolfram Sang wrote:
> > If you still have a concern about the types used, please clarify and
> > let me know what change you expect.
> 
> Leave it. I think the fragile part is gpio_is_valid() but this is truly
> outside the scope of this patch.
> (...)
> > Note that my patch doesn't introduce the gpio_request() calls, they
> > were there before, so this decision is actually independent from my
> > patch, and even if we decide to switch to using gpio_request_array(),
> > I'd rather do it in a separate patch for clarity.
> 
> I don't fully get it. Do you want to appl gpio_request() to this patch?
> Otherwise, I'd take it as is.

As I do not understand your question, I'd say you take my patch as is :)

Thanks,
-- 
Jean Delvare

Re: [PATCH] i2c-gpio: Add support for deferred probing

[Wolfram Sang]

On Thu, Feb 28, 2013 at 12:01:40PM +0100, Jean Delvare wrote:
> GPIOs may not be available immediately when i2c-gpio looks for them.
> Implement support for deferred probing so that probing can be
> attempted again later when GPIO pins are finally available.
> 
> Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Haavard Skinnemoen <hskinnemoen-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applied to for-next, thanks!