Thanks for the review, some inline reply: On Tue, Jun 9, 2020 at 7:19 PM Mark Brown <broo...@kernel.org> wrote: > > 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. Ack, would fix in v2.
> > > +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. Ack, would fix in v2. > > > +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. Since I'm doing DIV_ROUND_UP for the min_mV, so this may happen if the min_uV~max_uV range given by the core doesn't contain any value that can be represented exactly in mV. > > > + 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. Ok I'll change this to reg property in v2. > > > +#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. Ack, would fix in v2.