Re: [PATCHv2 2/5] regulator: omap smps regulator driver

[Todd Poynor <toddpoynor@google.com>]

On Wed, Jul 13, 2011 at 05:00:35PM +0300, Tero Kristo wrote:
> OMAP SMPS regulator driver provides access to OMAP voltage processor
> controlled regulators. These include VDD1 and VDD2 for OMAP3 and additionally
> VDD3 for OMAP4. SMPS regulators use the OMAP voltage layer for the actual
> voltage regulation operations.
> 
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> ---
...
> +	for (i = 0; i < pdata->num_regulators; i++) {
> +		initdata = pdata->regulators[i];
> +
> +		c = &initdata->constraints;
> +		c->valid_modes_mask &= REGULATOR_MODE_NORMAL;
> +		c->valid_ops_mask &= REGULATOR_CHANGE_VOLTAGE;
> +		c->always_on = true;
> +
> +		voltdm = omap_voltage_domain_lookup(
> +			initdata->consumer_supplies[0].dev_name);
> +
> +		if (IS_ERR(voltdm)) {
> +			dev_err(&pdev->dev, "can't find voltdm %s, %ld\n",
> +				initdata->consumer_supplies[0].dev_name,
> +				PTR_ERR(voltdm));
> +			return PTR_ERR(voltdm);


Or maybe continue to next loop iteration.

> +		}
> +		info = kzalloc(sizeof(struct omap_smps_reg_info), GFP_KERNEL);
> +
> +		info->voltdm = voltdm;


Check kzalloc NULL return.

> +		info->desc.ops = &omap_smps_ops;
> +		info->desc.name = c->name;
> +		info->desc.type = REGULATOR_VOLTAGE;
> +		info->desc.n_voltages = 0;
> +		rdev = regulator_register(&info->desc, &pdev->dev, initdata,
> +			info);
> +		if (IS_ERR(rdev)) {
> +			dev_err(&pdev->dev, "can't register %s, %ld\n",
> +				info->desc.name, PTR_ERR(rdev));
> +			return PTR_ERR(rdev);

Or suggest continue to next loop iteration.

> +		}
> +		platform_set_drvdata(pdev, rdev);

Performed in a loop, but only last iteration's rdev is set as the
driver data for pdev.  Platform driver data should be pdata?

> +	}
> +
> +	return 0;
> +}
> +
> +static int omap_smps_reg_remove(struct platform_device *pdev)
> +{
> +	regulator_unregister(platform_get_drvdata(pdev));


Should kfree() the struct omap_smps_reg_info data structure(s) and
platform_set_drvdata(pdev, NULL) ?


Todd