On Fri, Apr 20, 2012 at 12:38:41AM +0800, Ying-Chun Liu (PaulLiu) wrote:
+static const int mc34708_sw1A[] = {
- 650000, 662500, 675000, 687500, 700000, 712500,
Replace these by direct calculations, using tables is both less efficient and less clear.
- mc34708_lock(priv->mc34708);
- ret = mc34708_reg_rmw(priv->mc34708, mc34708_regulators[id].reg,
mc34708_regulators[id].enable_bit,mc34708_regulators[id].enable_bit);- mc34708_unlock(priv->mc34708);
Having to open code this locking in every single driver is a bit painful; just have the default register I/O operations do the locking and introduce additional unlocked versions if needed.
All this stuff could be factored out if you were using regmap.
+EXPORT_SYMBOL_GPL(mc34708_regulator_list_voltage);
No, this stuff should only be accessed via the ops. Why are you doing this?
+int +mc34708_get_best_voltage_index(struct regulator_dev *rdev,
int min_uV, int max_uV)+{
You're reimplementing core functionality here, or it'd be even better to use calculations.
+static int mc34708_regulator_get_voltage(struct regulator_dev *rdev) +{
Why is this not get_voltage_sel?
+static struct regulator_ops mc34708_regulator_ops = {
- .enable = mc34708_regulator_enable,
- .disable = mc34708_regulator_disable,
- .is_enabled = mc34708_regulator_is_enabled,
- .list_voltage = mc34708_regulator_list_voltage,
- .set_voltage = mc34708_regulator_set_voltage,
- .get_voltage = mc34708_regulator_get_voltage,
+}; +EXPORT_SYMBOL_GPL(mc34708_regulator_ops);
No. What are you doing this for?
+int +mc34708_fixed_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
int max_uV, unsigned *selector)
This function makes no sense...
+int mc34708_sw_regulator_is_enabled(struct regulator_dev *rdev) +{
- return 1;
+}
Why are you doing this - this function is redundant.
- ret = mc34708_reg_rmw(mc34708, MC34708_SW12OP,
MC34708_SW12OP_SW1AMODE_M |MC34708_SW12OP_SW2MODE_M,MC34708_SW12OP_SW1AMODE_VALUE |MC34708_SW12OP_SW2MODE_VALUE);- if (ret)
goto err_free;- ret = mc34708_reg_rmw(mc34708, MC34708_SW345OP,
MC34708_SW345OP_SW3MODE_M |MC34708_SW345OP_SW4AMODE_M |MC34708_SW345OP_SW4BMODE_M |MC34708_SW345OP_SW5MODE_M,MC34708_SW345OP_SW3MODE_VALUE |MC34708_SW345OP_SW4AMODE_VALUE |MC34708_SW345OP_SW4BMODE_VALUE |MC34708_SW345OP_SW5MODE_VALUE);- if (ret)
goto err_free;
If this needs to be done unconditionally shouldn't it be being donei in the MFD core driver?