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

[Tero Kristo <t-kristo@ti.com>]

On Thu, 2011-07-14 at 08:29 +0200, Todd Poynor wrote:
> 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.

Not sure about this. If one of the regulators fails, it is probably
better to fail/disable them all, otherwise we end up in partly
functioning situation. I think I'll just change this into a setup where
all the SMPS regulators are disabled if init of any of them fails.

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

True.

> 
> > +		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.

Same as above continue comment.

> 
> > +		}
> > +		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?

I'll check what the cleanup part needs and fix it.

> 
> > +	}
> > +
> > +	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) ?

Same as above.

> 
> 
> Todd



Texas Instruments Oy, Tekniikantie 12, 02150 Espoo. Y-tunnus: 0115040-6. Kotipaikka: Helsinki