On Thu, Aug 08, 2013 at 02:56:29PM +0800, Wei Ni wrote: > + mutex_lock(&data->update_lock); > + > + if (is_enable) > + ret = regulator_enable(data->lm90_reg); > + else > + ret = regulator_disable(data->lm90_reg); > + > + if (ret < 0) > + dev_err(&client->dev, > + "Error in %s rail vdd, error %d\n", > + (is_enable) ? "enabling" : "disabling", ret); > + else > + dev_info(&client->dev, "success in %s rail vdd\n", > + (is_enable) ? "enabling" : "disabling"); > + > + mutex_unlock(&data->update_lock); Two things here. One is that it's not clear what this lokc is protecting since the only thing in the locked region is the regulator operation and that is thread safe. The other thing is that I'm not seeing anthing that ensures that enables and disables are matched - regulators are reference counted so two enables need two disables. > + data->lm90_reg = regulator_get(&client->dev, "vdd"); > + if (IS_ERR_OR_NULL(data->lm90_reg)) { NULL is a valid regulator, use IS_ERR(). > + if (PTR_ERR(data->lm90_reg) == -ENODEV) > + dev_info(&client->dev, > + "No regulator found for vdd. Assuming vdd is always powered."); > + else > + dev_warn(&client->dev, > + "Error [%ld] in getting the regulator handle for vdd.\n", > + PTR_ERR(data->lm90_reg)); You shouldn't just be ignoring errors here, though there are deployment difficulties with making sure a stub regulator is provided. These should be getting easier after the next merge window, the stubs will be being tweaked slightly to have an "assume it's there" option even when regulators are used. Especially in cases with device tree you should be paying attention to -EPROBE_DEFER, that will accurately reflect if a regulator is present but not loaded yet. That said if you *are* going to do this you should request the regulator using devm_regulator_get_optional(), this is intended to support things that don't need regulators (though that's not the case here).
Attachment:
signature.asc
Description: Digital signature