On 07/05/12 18:08, Mark Brown wrote:

On Fri, May 04, 2012 at 07:23:24PM +0100, Lee Jones wrote:

Once again, please try to make sure your changelog matches the
subsystem.  This also isn't consistent with the other regulator change
further up the series :(

You've also not included any binding documenation here, binding
documentation should be included for new bindings.

quoted

+static __devinit int
+ab8500_regulator_of_probe(struct platform_device *pdev, struct device_node *np)
+{
+	struct regulator_init_data *ab8500_regulator;
+	struct device_node *child;
+	int err, value, i, id = 0;
+
+	/* Initialise regulator registers to platform specific values. */
+	for (i = 0; i<  ARRAY_SIZE(ab8500_reg_init); i++) {
+		err = of_property_read_u32(np, ab8500_reg_init[i].of_name,&value);
+		if (err<  0)
+			return err;
+
+		err = ab8500_regulator_init_registers(pdev, i, value);
+		if (err<  0)
+			return err;

You should be using of_regulator_match() for this (I think it's supposed
to do an equivalent job...) rather than open coding.

of_regulator_match() didn't exist when I wrote this. In fact, it only 
made it into -next a couple of days ago. Besides, it doesn't satisfy the 
needs of this code segment. of_regulator_match() is a(nother) wrapper 
around of_get_regulation_constraints(), which is only used to populate 
'struct regulation_constraints constraints' after being provided with a 
selection of .compatible strings.

I'd be happy to use an API for what this is trying to achieve, however 
there aren't any as far as I know.

quoted

+	/* Register each ab8500 regulator found in the Device Tree. */
+	for_each_child_of_node(np, child) {
+		ab8500_regulator = of_get_regulator_init_data(&pdev->dev, child);

Definitely don't do this - you should unconditionally register all the
regulators that physically exist, this allows users to inspect their
state even if they aren't used.

No problem. Thanks for the information. I'll change that and re-submit.

It's possible the original driver has this bug (I didn't check but

quoted

+		if (strcmp(ab8500_regulator->constraints.name, "dummy"))
+			ab8500_regulator_register(pdev, ab8500_regulator, id, child);

This is really broken - the whole purpose of allowing users to specify a
name in the constraints is to allow them to assign a name that's
meaningful for their board so they can tie the software in with the
schematic which we will display in diagnostics.  Forcing them to specify
magic strings as the supply name defeats this and makes diagnostics from
the kernel more obscure.

Noted. I'll have that changed to. Thanks.

quoted

  	pdata = dev_get_platdata(ab8500->dev);
-	if (!pdata) {
-		dev_err(&pdev->dev, "null pdata\n");
+	if (!pdata&&  !np) {
+		dev_err(&pdev->dev, "null pdata and no device tree found\n");
  		return -EINVAL;
  	}

Neither should be mandatory.

Okay.

Thanks for the review Mark. I'll get it fixed up and supply early next week.

Kind regards,
Lee

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
M: +44 77 88 633 515
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog