[patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Minor cleanup to twl4030-core: define a helper function to populate
a single child node, and use it to replace six inconsistent versions
of the same logic.  Both object and source code shrink.

As part of this, some devices now have more IRQ resources:  battery
charger, keypad, ADC, and USB transceiver.  That will help to remove
some irq #defines that prevent this code from compiling on non-OMAP
platforms.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
Boots fine on two boards that use GPIO, RTC, and USB.  Which means
charger, keypad, and ADC deserve sanity testing.  (The diff looks
nasty, but that's just inability to see "add a function and replace
the interior of six 'if' blocks" as the relevant change.)

 drivers/mfd/twl4030-core.c |  297 +++++++++++--------------------------------
 1 file changed, 82 insertions(+), 215 deletions(-)

--- a/drivers/mfd/twl4030-core.c
+++ b/drivers/mfd/twl4030-core.c
@@ -360,261 +360,128 @@ EXPORT_SYMBOL(twl4030_i2c_read_u8);
 
 /*----------------------------------------------------------------------*/
 
-/*
- * NOTE:  We know the first 8 IRQs after pdata->base_irq are
- * for the PIH, and the next are for the PWR_INT SIH, since
- * that's how twl_init_irq() sets things up.
- */
-
-static int add_children(struct twl4030_platform_data *pdata)
+static int add_child(unsigned chip, const char *name,
+		void *pdata, unsigned pdata_len,
+		bool can_wakeup, int irq0, int irq1)
 {
-	struct platform_device	*pdev = NULL;
-	struct twl4030_client	*twl = NULL;
-	int			status = 0;
-
-	if (twl_has_bci() && pdata->bci) {
-		twl = &twl4030_modules[3];
-
-		pdev = platform_device_alloc("twl4030_bci", -1);
-		if (!pdev) {
-			pr_debug("%s: can't alloc bci dev\n", DRIVER_NAME);
-			status = -ENOMEM;
-			goto err;
-		}
-
-		if (status == 0) {
-			pdev->dev.parent = &twl->client->dev;
-			status = platform_device_add_data(pdev, pdata->bci,
-					sizeof(*pdata->bci));
-			if (status < 0) {
-				dev_dbg(&twl->client->dev,
-					"can't add bci data, %d\n",
-					status);
-				goto err;
-			}
-		}
-
-		if (status == 0) {
-			struct resource r = {
-				.start = pdata->irq_base + 8 + 1,
-				.flags = IORESOURCE_IRQ,
-			};
+	struct platform_device	*pdev;
+	struct twl4030_client	*twl = &twl4030_modules[chip];
+	int			status;
 
-			status = platform_device_add_resources(pdev, &r, 1);
-		}
+	pdev = platform_device_alloc(name, -1);
+	if (!pdev) {
+		dev_dbg(&twl->client->dev, "can't alloc dev\n");
+		status = -ENOMEM;
+		goto err;
+	}
 
-		if (status == 0)
-			status = platform_device_add(pdev);
+	device_init_wakeup(&pdev->dev, can_wakeup);
+	pdev->dev.parent = &twl->client->dev;
 
+	if (pdata) {
+		status = platform_device_add_data(pdev, pdata, pdata_len);
 		if (status < 0) {
-			platform_device_put(pdev);
-			dev_dbg(&twl->client->dev,
-					"can't create bci dev, %d\n",
-					status);
+			dev_dbg(&pdev->dev, "can't add platform_data\n");
 			goto err;
 		}
 	}
 
-	if (twl_has_gpio() && pdata->gpio) {
-		twl = &twl4030_modules[1];
+	if (irq0) {
+		struct resource r[2] = {
+			{ .start = irq0, .flags = IORESOURCE_IRQ, },
+			{ .start = irq1, .flags = IORESOURCE_IRQ, },
+		};
 
-		pdev = platform_device_alloc("twl4030_gpio", -1);
-		if (!pdev) {
-			pr_debug("%s: can't alloc gpio dev\n", DRIVER_NAME);
-			status = -ENOMEM;
+		status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);
+		if (status < 0) {
+			dev_dbg(&pdev->dev, "can't add irqs\n");
 			goto err;
 		}
+	}
 
-		/* more driver model init */
-		if (status == 0) {
-			pdev->dev.parent = &twl->client->dev;
-			/* device_init_wakeup(&pdev->dev, 1); */
+	status = platform_device_add(pdev);
 
-			status = platform_device_add_data(pdev, pdata->gpio,
-					sizeof(*pdata->gpio));
-			if (status < 0) {
-				dev_dbg(&twl->client->dev,
-					"can't add gpio data, %d\n",
-					status);
-				goto err;
-			}
-		}
+err:
+	if (status < 0) {
+		platform_device_put(pdev);
+		dev_err(&twl->client->dev, "can't add %s dev\n", name);
+	}
+	return status;
+}
 
-		/* GPIO module IRQ */
-		if (status == 0) {
-			struct resource	r = {
-				.start = pdata->irq_base + 0,
-				.flags = IORESOURCE_IRQ,
-			};
+/*
+ * NOTE:  We know the first 8 IRQs after pdata->base_irq are
+ * for the PIH, and the next are for the PWR_INT SIH, since
+ * that's how twl_init_irq() sets things up.
+ */
 
-			status = platform_device_add_resources(pdev, &r, 1);
-		}
+static int add_children(struct twl4030_platform_data *pdata)
+{
+	int			status;
 
-		if (status == 0)
-			status = platform_device_add(pdev);
+	if (twl_has_bci() && pdata->bci) {
+		status = add_child(3, "twl4030_bci",
+				pdata->bci, sizeof(*pdata->bci),
+				false,
+				/* irq0 = CHG_PRES, irq1 = BCI */
+				pdata->irq_base + 8 + 1, pdata->irq_base + 2);
+		if (status < 0)
+			return status;
+	}
 
-		if (status < 0) {
-			platform_device_put(pdev);
-			dev_dbg(&twl->client->dev,
-					"can't create gpio dev, %d\n",
-					status);
-			goto err;
-		}
+	if (twl_has_gpio() && pdata->gpio) {
+		status = add_child(1, "twl4030_gpio",
+				pdata->gpio, sizeof(*pdata->gpio),
+				false, pdata->irq_base + 0, 0);
+		if (status < 0)
+			return status;
 	}
 
 	if (twl_has_keypad() && pdata->keypad) {
-		pdev = platform_device_alloc("twl4030_keypad", -1);
-		if (pdev) {
-			twl = &twl4030_modules[2];
-			pdev->dev.parent = &twl->client->dev;
-			device_init_wakeup(&pdev->dev, 1);
-			status = platform_device_add_data(pdev, pdata->keypad,
-					sizeof(*pdata->keypad));
-			if (status < 0) {
-				dev_dbg(&twl->client->dev,
-					"can't add keypad data, %d\n",
-					status);
-				platform_device_put(pdev);
-				goto err;
-			}
-			status = platform_device_add(pdev);
-			if (status < 0) {
-				platform_device_put(pdev);
-				dev_dbg(&twl->client->dev,
-						"can't create keypad dev, %d\n",
-						status);
-				goto err;
-			}
-		} else {
-			pr_debug("%s: can't alloc keypad dev\n", DRIVER_NAME);
-			status = -ENOMEM;
-			goto err;
-		}
+		status = add_child(2, "twl4030_keypad",
+				pdata->keypad, sizeof(*pdata->keypad),
+				true, pdata->irq_base + 1, 0);
+		if (status < 0)
+			return status;
 	}
 
 	if (twl_has_madc() && pdata->madc) {
-		pdev = platform_device_alloc("twl4030_madc", -1);
-		if (pdev) {
-			twl = &twl4030_modules[2];
-			pdev->dev.parent = &twl->client->dev;
-			device_init_wakeup(&pdev->dev, 1);
-			status = platform_device_add_data(pdev, pdata->madc,
-					sizeof(*pdata->madc));
-			if (status < 0) {
-				platform_device_put(pdev);
-				dev_dbg(&twl->client->dev,
-					"can't add madc data, %d\n",
-					status);
-				goto err;
-			}
-			status = platform_device_add(pdev);
-			if (status < 0) {
-				platform_device_put(pdev);
-				dev_dbg(&twl->client->dev,
-						"can't create madc dev, %d\n",
-						status);
-				goto err;
-			}
-		} else {
-			pr_debug("%s: can't alloc madc dev\n", DRIVER_NAME);
-			status = -ENOMEM;
-			goto err;
-		}
+		status = add_child(2, "twl4030_madc",
+				pdata->madc, sizeof(*pdata->madc),
+				true, pdata->irq_base + 3, 0);
+		if (status < 0)
+			return status;
 	}
 
 	if (twl_has_power() && pdata->power)
 		twl4030_power_init(pdata->power);
 
 	if (twl_has_rtc()) {
-		twl = &twl4030_modules[3];
-
-		pdev = platform_device_alloc("twl4030_rtc", -1);
-		if (!pdev) {
-			pr_debug("%s: can't alloc rtc dev\n", DRIVER_NAME);
-			status = -ENOMEM;
-		} else {
-			pdev->dev.parent = &twl->client->dev;
-			device_init_wakeup(&pdev->dev, 1);
-		}
-
 		/*
-		 * REVISIT platform_data here currently might use of
+		 * REVISIT platform_data here currently might expose the
 		 * "msecure" line ... but for now we just expect board
-		 * setup to tell the chip "we are secure" at all times.
+		 * setup to tell the chip "it's always ok to SET_TIME".
 		 * Eventually, Linux might become more aware of such
 		 * HW security concerns, and "least privilege".
 		 */
-
-		/* RTC module IRQ */
-		if (status == 0) {
-			struct resource	r = {
-				.start = pdata->irq_base + 8 + 3,
-				.flags = IORESOURCE_IRQ,
-			};
-
-			status = platform_device_add_resources(pdev, &r, 1);
-		}
-
-		if (status == 0)
-			status = platform_device_add(pdev);
-
-		if (status < 0) {
-			platform_device_put(pdev);
-			dev_dbg(&twl->client->dev,
-					"can't create rtc dev, %d\n",
-					status);
-			goto err;
-		}
+		status = add_child(3, "twl4030_rtc",
+				NULL, 0,
+				true, pdata->irq_base + 8 + 3, 0);
+		if (status < 0)
+			return status;
 	}
 
 	if (twl_has_usb() && pdata->usb) {
-		twl = &twl4030_modules[0];
-
-		pdev = platform_device_alloc("twl4030_usb", -1);
-		if (!pdev) {
-			pr_debug("%s: can't alloc usb dev\n", DRIVER_NAME);
-			status = -ENOMEM;
-			goto err;
-		}
-
-		if (status == 0) {
-			pdev->dev.parent = &twl->client->dev;
-			device_init_wakeup(&pdev->dev, 1);
-			status = platform_device_add_data(pdev, pdata->usb,
-					sizeof(*pdata->usb));
-			if (status < 0) {
-				platform_device_put(pdev);
-				dev_dbg(&twl->client->dev,
-					"can't add usb data, %d\n",
-					status);
-				goto err;
-			}
-		}
-
-		if (status == 0) {
-			struct resource r = {
-				.start = pdata->irq_base + 8 + 2,
-				.flags = IORESOURCE_IRQ,
-			};
-
-			status = platform_device_add_resources(pdev, &r, 1);
-		}
-
-		if (status == 0)
-			status = platform_device_add(pdev);
-
-		if (status < 0) {
-			platform_device_put(pdev);
-			dev_dbg(&twl->client->dev,
-					"can't create usb dev, %d\n",
-					status);
-		}
+		status = add_child(0, "twl4030_usb",
+				pdata->usb, sizeof(*pdata->usb),
+				true,
+				/* irq0 = USB_PRES, irq1 = USB */
+				pdata->irq_base + 8 + 2, pdata->irq_base + 4);
+		if (status < 0)
+			return status;
 	}
 
-err:
-	if (status)
-		pr_err("failed to add twl4030's children (status %d)\n", status);
-	return status;
+	return 0;
 }
 
 /*----------------------------------------------------------------------*/

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [081107 16:45]:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Minor cleanup to twl4030-core: define a helper function to populate
> a single child node, and use it to replace six inconsistent versions
> of the same logic.  Both object and source code shrink.
> 
> As part of this, some devices now have more IRQ resources:  battery
> charger, keypad, ADC, and USB transceiver.  That will help to remove
> some irq #defines that prevent this code from compiling on non-OMAP
> platforms.

Pushing to linux-omap tree while waiting for this to fall down from
mainline tree via Samuel's queue.

Tony

> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
> Boots fine on two boards that use GPIO, RTC, and USB.  Which means
> charger, keypad, and ADC deserve sanity testing.  (The diff looks
> nasty, but that's just inability to see "add a function and replace
> the interior of six 'if' blocks" as the relevant change.)
> 
>  drivers/mfd/twl4030-core.c |  297 +++++++++++--------------------------------
>  1 file changed, 82 insertions(+), 215 deletions(-)
> 
> --- a/drivers/mfd/twl4030-core.c
> +++ b/drivers/mfd/twl4030-core.c
> @@ -360,261 +360,128 @@ EXPORT_SYMBOL(twl4030_i2c_read_u8);
>  
>  /*----------------------------------------------------------------------*/
>  
> -/*
> - * NOTE:  We know the first 8 IRQs after pdata->base_irq are
> - * for the PIH, and the next are for the PWR_INT SIH, since
> - * that's how twl_init_irq() sets things up.
> - */
> -
> -static int add_children(struct twl4030_platform_data *pdata)
> +static int add_child(unsigned chip, const char *name,
> +		void *pdata, unsigned pdata_len,
> +		bool can_wakeup, int irq0, int irq1)
>  {
> -	struct platform_device	*pdev = NULL;
> -	struct twl4030_client	*twl = NULL;
> -	int			status = 0;
> -
> -	if (twl_has_bci() && pdata->bci) {
> -		twl = &twl4030_modules[3];
> -
> -		pdev = platform_device_alloc("twl4030_bci", -1);
> -		if (!pdev) {
> -			pr_debug("%s: can't alloc bci dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> -			goto err;
> -		}
> -
> -		if (status == 0) {
> -			pdev->dev.parent = &twl->client->dev;
> -			status = platform_device_add_data(pdev, pdata->bci,
> -					sizeof(*pdata->bci));
> -			if (status < 0) {
> -				dev_dbg(&twl->client->dev,
> -					"can't add bci data, %d\n",
> -					status);
> -				goto err;
> -			}
> -		}
> -
> -		if (status == 0) {
> -			struct resource r = {
> -				.start = pdata->irq_base + 8 + 1,
> -				.flags = IORESOURCE_IRQ,
> -			};
> +	struct platform_device	*pdev;
> +	struct twl4030_client	*twl = &twl4030_modules[chip];
> +	int			status;
>  
> -			status = platform_device_add_resources(pdev, &r, 1);
> -		}
> +	pdev = platform_device_alloc(name, -1);
> +	if (!pdev) {
> +		dev_dbg(&twl->client->dev, "can't alloc dev\n");
> +		status = -ENOMEM;
> +		goto err;
> +	}
>  
> -		if (status == 0)
> -			status = platform_device_add(pdev);
> +	device_init_wakeup(&pdev->dev, can_wakeup);
> +	pdev->dev.parent = &twl->client->dev;
>  
> +	if (pdata) {
> +		status = platform_device_add_data(pdev, pdata, pdata_len);
>  		if (status < 0) {
> -			platform_device_put(pdev);
> -			dev_dbg(&twl->client->dev,
> -					"can't create bci dev, %d\n",
> -					status);
> +			dev_dbg(&pdev->dev, "can't add platform_data\n");
>  			goto err;
>  		}
>  	}
>  
> -	if (twl_has_gpio() && pdata->gpio) {
> -		twl = &twl4030_modules[1];
> +	if (irq0) {
> +		struct resource r[2] = {
> +			{ .start = irq0, .flags = IORESOURCE_IRQ, },
> +			{ .start = irq1, .flags = IORESOURCE_IRQ, },
> +		};
>  
> -		pdev = platform_device_alloc("twl4030_gpio", -1);
> -		if (!pdev) {
> -			pr_debug("%s: can't alloc gpio dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> +		status = platform_device_add_resources(pdev, r, irq1 ? 2 : 1);
> +		if (status < 0) {
> +			dev_dbg(&pdev->dev, "can't add irqs\n");
>  			goto err;
>  		}
> +	}
>  
> -		/* more driver model init */
> -		if (status == 0) {
> -			pdev->dev.parent = &twl->client->dev;
> -			/* device_init_wakeup(&pdev->dev, 1); */
> +	status = platform_device_add(pdev);
>  
> -			status = platform_device_add_data(pdev, pdata->gpio,
> -					sizeof(*pdata->gpio));
> -			if (status < 0) {
> -				dev_dbg(&twl->client->dev,
> -					"can't add gpio data, %d\n",
> -					status);
> -				goto err;
> -			}
> -		}
> +err:
> +	if (status < 0) {
> +		platform_device_put(pdev);
> +		dev_err(&twl->client->dev, "can't add %s dev\n", name);
> +	}
> +	return status;
> +}
>  
> -		/* GPIO module IRQ */
> -		if (status == 0) {
> -			struct resource	r = {
> -				.start = pdata->irq_base + 0,
> -				.flags = IORESOURCE_IRQ,
> -			};
> +/*
> + * NOTE:  We know the first 8 IRQs after pdata->base_irq are
> + * for the PIH, and the next are for the PWR_INT SIH, since
> + * that's how twl_init_irq() sets things up.
> + */
>  
> -			status = platform_device_add_resources(pdev, &r, 1);
> -		}
> +static int add_children(struct twl4030_platform_data *pdata)
> +{
> +	int			status;
>  
> -		if (status == 0)
> -			status = platform_device_add(pdev);
> +	if (twl_has_bci() && pdata->bci) {
> +		status = add_child(3, "twl4030_bci",
> +				pdata->bci, sizeof(*pdata->bci),
> +				false,
> +				/* irq0 = CHG_PRES, irq1 = BCI */
> +				pdata->irq_base + 8 + 1, pdata->irq_base + 2);
> +		if (status < 0)
> +			return status;
> +	}
>  
> -		if (status < 0) {
> -			platform_device_put(pdev);
> -			dev_dbg(&twl->client->dev,
> -					"can't create gpio dev, %d\n",
> -					status);
> -			goto err;
> -		}
> +	if (twl_has_gpio() && pdata->gpio) {
> +		status = add_child(1, "twl4030_gpio",
> +				pdata->gpio, sizeof(*pdata->gpio),
> +				false, pdata->irq_base + 0, 0);
> +		if (status < 0)
> +			return status;
>  	}
>  
>  	if (twl_has_keypad() && pdata->keypad) {
> -		pdev = platform_device_alloc("twl4030_keypad", -1);
> -		if (pdev) {
> -			twl = &twl4030_modules[2];
> -			pdev->dev.parent = &twl->client->dev;
> -			device_init_wakeup(&pdev->dev, 1);
> -			status = platform_device_add_data(pdev, pdata->keypad,
> -					sizeof(*pdata->keypad));
> -			if (status < 0) {
> -				dev_dbg(&twl->client->dev,
> -					"can't add keypad data, %d\n",
> -					status);
> -				platform_device_put(pdev);
> -				goto err;
> -			}
> -			status = platform_device_add(pdev);
> -			if (status < 0) {
> -				platform_device_put(pdev);
> -				dev_dbg(&twl->client->dev,
> -						"can't create keypad dev, %d\n",
> -						status);
> -				goto err;
> -			}
> -		} else {
> -			pr_debug("%s: can't alloc keypad dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> -			goto err;
> -		}
> +		status = add_child(2, "twl4030_keypad",
> +				pdata->keypad, sizeof(*pdata->keypad),
> +				true, pdata->irq_base + 1, 0);
> +		if (status < 0)
> +			return status;
>  	}
>  
>  	if (twl_has_madc() && pdata->madc) {
> -		pdev = platform_device_alloc("twl4030_madc", -1);
> -		if (pdev) {
> -			twl = &twl4030_modules[2];
> -			pdev->dev.parent = &twl->client->dev;
> -			device_init_wakeup(&pdev->dev, 1);
> -			status = platform_device_add_data(pdev, pdata->madc,
> -					sizeof(*pdata->madc));
> -			if (status < 0) {
> -				platform_device_put(pdev);
> -				dev_dbg(&twl->client->dev,
> -					"can't add madc data, %d\n",
> -					status);
> -				goto err;
> -			}
> -			status = platform_device_add(pdev);
> -			if (status < 0) {
> -				platform_device_put(pdev);
> -				dev_dbg(&twl->client->dev,
> -						"can't create madc dev, %d\n",
> -						status);
> -				goto err;
> -			}
> -		} else {
> -			pr_debug("%s: can't alloc madc dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> -			goto err;
> -		}
> +		status = add_child(2, "twl4030_madc",
> +				pdata->madc, sizeof(*pdata->madc),
> +				true, pdata->irq_base + 3, 0);
> +		if (status < 0)
> +			return status;
>  	}
>  
>  	if (twl_has_power() && pdata->power)
>  		twl4030_power_init(pdata->power);
>  
>  	if (twl_has_rtc()) {
> -		twl = &twl4030_modules[3];
> -
> -		pdev = platform_device_alloc("twl4030_rtc", -1);
> -		if (!pdev) {
> -			pr_debug("%s: can't alloc rtc dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> -		} else {
> -			pdev->dev.parent = &twl->client->dev;
> -			device_init_wakeup(&pdev->dev, 1);
> -		}
> -
>  		/*
> -		 * REVISIT platform_data here currently might use of
> +		 * REVISIT platform_data here currently might expose the
>  		 * "msecure" line ... but for now we just expect board
> -		 * setup to tell the chip "we are secure" at all times.
> +		 * setup to tell the chip "it's always ok to SET_TIME".
>  		 * Eventually, Linux might become more aware of such
>  		 * HW security concerns, and "least privilege".
>  		 */
> -
> -		/* RTC module IRQ */
> -		if (status == 0) {
> -			struct resource	r = {
> -				.start = pdata->irq_base + 8 + 3,
> -				.flags = IORESOURCE_IRQ,
> -			};
> -
> -			status = platform_device_add_resources(pdev, &r, 1);
> -		}
> -
> -		if (status == 0)
> -			status = platform_device_add(pdev);
> -
> -		if (status < 0) {
> -			platform_device_put(pdev);
> -			dev_dbg(&twl->client->dev,
> -					"can't create rtc dev, %d\n",
> -					status);
> -			goto err;
> -		}
> +		status = add_child(3, "twl4030_rtc",
> +				NULL, 0,
> +				true, pdata->irq_base + 8 + 3, 0);
> +		if (status < 0)
> +			return status;
>  	}
>  
>  	if (twl_has_usb() && pdata->usb) {
> -		twl = &twl4030_modules[0];
> -
> -		pdev = platform_device_alloc("twl4030_usb", -1);
> -		if (!pdev) {
> -			pr_debug("%s: can't alloc usb dev\n", DRIVER_NAME);
> -			status = -ENOMEM;
> -			goto err;
> -		}
> -
> -		if (status == 0) {
> -			pdev->dev.parent = &twl->client->dev;
> -			device_init_wakeup(&pdev->dev, 1);
> -			status = platform_device_add_data(pdev, pdata->usb,
> -					sizeof(*pdata->usb));
> -			if (status < 0) {
> -				platform_device_put(pdev);
> -				dev_dbg(&twl->client->dev,
> -					"can't add usb data, %d\n",
> -					status);
> -				goto err;
> -			}
> -		}
> -
> -		if (status == 0) {
> -			struct resource r = {
> -				.start = pdata->irq_base + 8 + 2,
> -				.flags = IORESOURCE_IRQ,
> -			};
> -
> -			status = platform_device_add_resources(pdev, &r, 1);
> -		}
> -
> -		if (status == 0)
> -			status = platform_device_add(pdev);
> -
> -		if (status < 0) {
> -			platform_device_put(pdev);
> -			dev_dbg(&twl->client->dev,
> -					"can't create usb dev, %d\n",
> -					status);
> -		}
> +		status = add_child(0, "twl4030_usb",
> +				pdata->usb, sizeof(*pdata->usb),
> +				true,
> +				/* irq0 = USB_PRES, irq1 = USB */
> +				pdata->irq_base + 8 + 2, pdata->irq_base + 4);
> +		if (status < 0)
> +			return status;
>  	}
>  
> -err:
> -	if (status)
> -		pr_err("failed to add twl4030's children (status %d)\n", status);
> -	return status;
> +	return 0;
>  }
>  
>  /*----------------------------------------------------------------------*/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[David Brownell]

On Thursday 13 November 2008, Tony Lindgren wrote:
> * David Brownell <david-b@pacbell.net> [081107 16:45]:
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Minor cleanup to twl4030-core: define a helper function to populate
> > a single child node, and use it to replace six inconsistent versions
> > of the same logic.  Both object and source code shrink.
> > 
> > As part of this, some devices now have more IRQ resources:  battery
> > charger, keypad, ADC, and USB transceiver.  That will help to remove
> > some irq #defines that prevent this code from compiling on non-OMAP
> > platforms.
> 
> Pushing to linux-omap tree while waiting for this to fall down from
> mainline tree via Samuel's queue.

Hmm, better to revert this and send the updated version I sent
as part of the regulator series... that returns the platform
device, which is useful in setting up consumers of the regulator
devices.

I hadn't gotten any test results other than knowing Felipe was
using this.  So I'll hold off a bit on sending the updated
patch to Samuel, probably till end-of-week, on the grounds that
the keypad, battery, and ADC devices didn't get tested yet.

- Dave

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Felipe Balbi]

On Thu, Nov 13, 2008 at 04:15:09PM -0800, David Brownell wrote:
> On Thursday 13 November 2008, Tony Lindgren wrote:
> > * David Brownell <david-b@pacbell.net> [081107 16:45]:
> > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > 
> > > Minor cleanup to twl4030-core: define a helper function to populate
> > > a single child node, and use it to replace six inconsistent versions
> > > of the same logic.  Both object and source code shrink.
> > > 
> > > As part of this, some devices now have more IRQ resources:  battery
> > > charger, keypad, ADC, and USB transceiver.  That will help to remove
> > > some irq #defines that prevent this code from compiling on non-OMAP
> > > platforms.
> > 
> > Pushing to linux-omap tree while waiting for this to fall down from
> > mainline tree via Samuel's queue.
> 
> Hmm, better to revert this and send the updated version I sent
> as part of the regulator series... that returns the platform
> device, which is useful in setting up consumers of the regulator
> devices.
> 
> I hadn't gotten any test results other than knowing Felipe was
> using this.  So I'll hold off a bit on sending the updated
> patch to Samuel, probably till end-of-week, on the grounds that
> the keypad, battery, and ADC devices didn't get tested yet.

The new add_child() works quite ok and simplifies a lot the code. But
the regulator driver, I think we can hold a bit as that needs careful
testing.

-- 
balbi

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Tony Lindgren]

* Felipe Balbi <me@felipebalbi.com> [081113 16:36]:
> On Thu, Nov 13, 2008 at 04:15:09PM -0800, David Brownell wrote:
> > On Thursday 13 November 2008, Tony Lindgren wrote:
> > > * David Brownell <david-b@pacbell.net> [081107 16:45]:
> > > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > > 
> > > > Minor cleanup to twl4030-core: define a helper function to populate
> > > > a single child node, and use it to replace six inconsistent versions
> > > > of the same logic.  Both object and source code shrink.
> > > > 
> > > > As part of this, some devices now have more IRQ resources:  battery
> > > > charger, keypad, ADC, and USB transceiver.  That will help to remove
> > > > some irq #defines that prevent this code from compiling on non-OMAP
> > > > platforms.
> > > 
> > > Pushing to linux-omap tree while waiting for this to fall down from
> > > mainline tree via Samuel's queue.
> > 
> > Hmm, better to revert this and send the updated version I sent
> > as part of the regulator series... that returns the platform
> > device, which is useful in setting up consumers of the regulator
> > devices.

OK, will do.

> > I hadn't gotten any test results other than knowing Felipe was
> > using this.  So I'll hold off a bit on sending the updated
> > patch to Samuel, probably till end-of-week, on the grounds that
> > the keypad, battery, and ADC devices didn't get tested yet.
> 
> The new add_child() works quite ok and simplifies a lot the code. But
> the regulator driver, I think we can hold a bit as that needs careful
> testing.

That's to make USB use the twl regulator code, right?

Tony

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Felipe Balbi]

On Thu, Nov 13, 2008 at 04:42:44PM -0800, Tony Lindgren wrote:
> That's to make USB use the twl regulator code, right?

Starting with usb, but there are other uses :-)

-- 
balbi

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [081113 16:43]:
> * Felipe Balbi <me@felipebalbi.com> [081113 16:36]:
> > On Thu, Nov 13, 2008 at 04:15:09PM -0800, David Brownell wrote:
> > > On Thursday 13 November 2008, Tony Lindgren wrote:
> > > > * David Brownell <david-b@pacbell.net> [081107 16:45]:
> > > > > From: David Brownell <dbrownell@users.sourceforge.net>
> > > > > 
> > > > > Minor cleanup to twl4030-core: define a helper function to populate
> > > > > a single child node, and use it to replace six inconsistent versions
> > > > > of the same logic.  Both object and source code shrink.
> > > > > 
> > > > > As part of this, some devices now have more IRQ resources:  battery
> > > > > charger, keypad, ADC, and USB transceiver.  That will help to remove
> > > > > some irq #defines that prevent this code from compiling on non-OMAP
> > > > > platforms.
> > > > 
> > > > Pushing to linux-omap tree while waiting for this to fall down from
> > > > mainline tree via Samuel's queue.
> > > 
> > > Hmm, better to revert this and send the updated version I sent
> > > as part of the regulator series... that returns the platform
> > > device, which is useful in setting up consumers of the regulator
> > > devices.
> 
> OK, will do.

If CONFIG_REGULATOR is not set, I get:

drivers/mfd/twl4030-core.c: In function 'add_children':
drivers/mfd/twl4030-core.c:592: error: 'REGULATOR_MODE_OFF' undeclared (first use in this function)

Where do you want to have that for now? Add it also into
twl4030-core.c like in twl4030-regulator.c?

Tony

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[David Brownell]

On Thursday 13 November 2008, Tony Lindgren wrote:
> > > I hadn't gotten any test results other than knowing Felipe was
> > > using this.  So I'll hold off a bit on sending the updated
> > > patch to Samuel, probably till end-of-week, on the grounds that
> > > the keypad, battery, and ADC devices didn't get tested yet.
> > 
> > The new add_child() works quite ok and simplifies a lot the code. But
> > the regulator driver, I think we can hold a bit as that needs careful
> > testing.
> 
> That's to make USB use the twl regulator code, right?

Looks for now like twl4030_usb might be the first thing that's
able to use the regulator code "in all its glory".  Felipe can
gain the glory of being the Official First Guinea Pig.  ;)

No rush on that, IMO.

Next two obvious candidates to use "struct regulator" are the
HSMMC support, and the LCD/touchscreen power domains on some of
the SDPs.

I think providing the board-specific init data for anything
else is likely to be a big PITA, since it presumes the actual
device node is known when the regulator device is created.
That's not always going to be practical.

- Dave

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

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[David Brownell]

On Thursday 13 November 2008, Tony Lindgren wrote:
> If CONFIG_REGULATOR is not set, I get:
> 
> drivers/mfd/twl4030-core.c: In function 'add_children':
> drivers/mfd/twl4030-core.c:592: error: 'REGULATOR_MODE_OFF' undeclared (first use in this function)
> 
> Where do you want to have that for now? Add it also into
> twl4030-core.c like in twl4030-regulator.c?

#ifndef REGULATOR_MODE_OFF
#define REGULATOR_MODE_OFF 0
#endif

for now... but the fact that you're getting that at all seems to
mean you're applying the third regulator patch (register regulators)
without the fourth (debug oriented) which has that #ifndef.

Don't merge regulator patches #3 or #4 yet ... #2 should be OK,
but it's a NOP without #3 (which has the is ifdef issue).

- Dave

Re: [patch/rft 2.6.28-rc3-omap] twl4030-core simplification

[Tony Lindgren]

* David Brownell <david-b@pacbell.net> [081113 17:05]:
> On Thursday 13 November 2008, Tony Lindgren wrote:
> > If CONFIG_REGULATOR is not set, I get:
> > 
> > drivers/mfd/twl4030-core.c: In function 'add_children':
> > drivers/mfd/twl4030-core.c:592: error: 'REGULATOR_MODE_OFF' undeclared (first use in this function)
> > 
> > Where do you want to have that for now? Add it also into
> > twl4030-core.c like in twl4030-regulator.c?
> 
> #ifndef REGULATOR_MODE_OFF
> #define REGULATOR_MODE_OFF 0
> #endif
> 
> for now... but the fact that you're getting that at all seems to
> mean you're applying the third regulator patch (register regulators)
> without the fourth (debug oriented) which has that #ifndef.
> 
> Don't merge regulator patches #3 or #4 yet ... #2 should be OK,
> but it's a NOP without #3 (which has the is ifdef issue).

OK, thanks. Pushed the first two only.

Tony