Messages in this thread | | | Date | Tue, 9 Jun 2020 12:19:19 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 2/2] regulator: Add driver for cros-ec-regulator |
| |
On Tue, Jun 09, 2020 at 03:59:55PM +0800, Pi-Hsun Shih wrote:
> +static int cros_ec_regulator_set_state(struct regulator_dev *dev, bool enable) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + struct ec_params_regulator_enable cmd = { > + .index = data->index, > + .enable = enable ? 1 : 0,
The ternery operator is totally redundant here.
> +static int cros_ec_regulator_enable(struct regulator_dev *dev) > +{ > + return cros_ec_regulator_set_state(dev, true); > +}
> +static int cros_ec_regulator_disable(struct regulator_dev *dev) > +{ > + return cros_ec_regulator_set_state(dev, false); > +}
I'm not sure that the shared function is really worthwhile though, there's not really enough in it and certainly not anything complicated.
> +static int cros_ec_regulator_set_voltage(struct regulator_dev *dev, int min_uV, > + int max_uV, unsigned int *selector) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + int min_mV = DIV_ROUND_UP(min_uV, 1000); > + int max_mV = max_uV / 1000; > + struct ec_params_regulator_set_voltage cmd = { > + .index = data->index, > + .min_mv = min_mV, > + .max_mv = max_mV, > + }; > + > + if (min_mV > max_mV) > + return -EINVAL;
The core will do this for you.
> + ret = of_property_read_u32(np, "google,remote-regulator", > + &drvdata->index); > + if (ret < 0) > + return ret;
This remote-regulator property is a bit weird, it feels like it should be a reg property on a bus.
> +#if defined(CONFIG_OF) > +static const struct of_device_id regulator_cros_ec_of_match[] = { > + { .compatible = "regulator-cros-ec", }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, regulator_cros_ec_of_match); > +#endif
Your compatible is google,regulator-cros-ec. [unhandled content-type:application/pgp-signature] | |