Messages in this thread | | | Date | Tue, 17 Nov 2015 18:50:30 +0000 | From | Mark Brown <> | Subject | Re: [PATCH V1] regulator: pv88060: new regulator driver |
| |
On Tue, Nov 17, 2015 at 11:25:21AM +0900, James Ban wrote:
A couple of minor points but overall this looks good:
> + node = of_get_child_by_name(dev->of_node, "regulators"); > + if (!node) { > + dev_err(dev, "regulators node not found\n"); > + return ERR_PTR(-ENODEV); > + } > + > + num = of_regulator_match(dev, node, pv88060_matches, > + ARRAY_SIZE(pv88060_matches));
Don't manually parse the DT, use the core support and set of_match and regulators_node in the regulator_desc.
> + if (i == 0) { > + pv88060_matches[i].init_data->constraints > + .valid_modes_mask > + |= REGULATOR_MODE_FAST > + | REGULATOR_MODE_NORMAL > + | REGULATOR_MODE_STANDBY; > + pv88060_matches[i].init_data->constraints.valid_ops_mask > + |= REGULATOR_CHANGE_MODE; > + }
The driver should never modify the constraints that are passed in - the whole point with constraints is to allow the system to set up what is safe on that particular system.
> + if (!chip->pdata) > + chip->pdata = pv88060_parse_regulators_dt(chip->dev); > + > + if (IS_ERR(chip->pdata)) { > + dev_err(chip->dev, "No regulators defined for the platform\n"); > + return PTR_ERR(chip->pdata); > + }
This should not be an error, the driver should just instantiate all regulators the device has and at least let people read back the current state. [unhandled content-type:application/pgp-signature] | |