Re: [PATCH 2/2] regulator: Add driver for cros-ec-regulator
On Wed, Jun 10, 2020 at 12:31:53PM +0800, Pi-Hsun Shih wrote: > On Tue, Jun 9, 2020 at 7:19 PM Mark Brown wrote: > > On Tue, Jun 09, 2020 at 03:59:55PM +0800, Pi-Hsun Shih wrote: > > > + 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. This isn't clear from the code - I'd add a comment or move the division to be next to the check. signature.asc Description: PGP signature
Re: [PATCH 2/2] regulator: Add driver for cros-ec-regulator
Thanks for the review, some inline reply: On Tue, Jun 9, 2020 at 7:19 PM Mark Brown 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", > > +>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.
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", > +>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. signature.asc Description: PGP signature
[PATCH 2/2] regulator: Add driver for cros-ec-regulator
Add driver for cros-ec-regulator, representing a voltage regulator that is connected and controlled by ChromeOS EC, and is controlled by kernel with EC host commands. Signed-off-by: Pi-Hsun Shih --- This patch contains function cros_ec_cmd that is copied from the series: https://lore.kernel.org/patchwork/project/lkml/list/?series=428457. I can't find the first patch in that v2 series, so the function is modified from v1 of that series according to reviewers comment: https://lore.kernel.org/patchwork/patch/1188110/ I copied the function instead of depending on that series since I feel the function is small enough, and the series has stalled for some time. --- drivers/regulator/Kconfig | 7 + drivers/regulator/Makefile| 1 + drivers/regulator/cros-ec-regulator.c | 261 ++ .../linux/platform_data/cros_ec_commands.h| 82 ++ 4 files changed, 351 insertions(+) create mode 100644 drivers/regulator/cros-ec-regulator.c diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 8f677f5d79b4..3543e0fc54ca 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -238,6 +238,13 @@ config REGULATOR_CPCAP Say y here for CPCAP regulator found on some Motorola phones and tablets such as Droid 4. +config REGULATOR_CROS_EC + tristate "ChromeOS EC regulators" + depends on CROS_EC + help + This driver supports voltage regulators that is connected to ChromeOS + EC and controlled through EC host commands. + config REGULATOR_DA903X tristate "Dialog Semiconductor DA9030/DA9034 regulators" depends on PMIC_DA903X diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index e8f163371071..46592c160d22 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o obj-$(CONFIG_REGULATOR_88PG86X) += 88pg86x.o obj-$(CONFIG_REGULATOR_88PM800) += 88pm800-regulator.o obj-$(CONFIG_REGULATOR_88PM8607) += 88pm8607.o +obj-$(CONFIG_REGULATOR_CROS_EC) += cros-ec-regulator.o obj-$(CONFIG_REGULATOR_CPCAP) += cpcap-regulator.o obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o obj-$(CONFIG_REGULATOR_AB3100) += ab3100.o diff --git a/drivers/regulator/cros-ec-regulator.c b/drivers/regulator/cros-ec-regulator.c new file mode 100644 index ..495006f6381c --- /dev/null +++ b/drivers/regulator/cros-ec-regulator.c @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Copyright 2020 Google LLC. + +#include +#include +#include +#include +#include +#include +#include +#include + +struct cros_ec_regulator_data { + struct regulator_desc desc; + struct regulator_dev *dev; + struct cros_ec_device *ec_dev; + + u32 index; + + u16 *voltages_mV; + u16 num_voltages; +}; + +static int cros_ec_cmd(struct cros_ec_device *ec, u32 version, u32 command, + void *outdata, u32 outsize, void *indata, u32 insize) +{ + struct cros_ec_command *msg; + int ret; + + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL); + if (!msg) + return -ENOMEM; + + msg->version = version; + msg->command = command; + msg->outsize = outsize; + msg->insize = insize; + + if (outdata && outsize > 0) + memcpy(msg->data, outdata, outsize); + + ret = cros_ec_cmd_xfer_status(ec, msg); + if (ret < 0) { + dev_warn(ec->dev, "Command failed: %d\n", msg->result); + goto cleanup; + } + + if (insize) + memcpy(indata, msg->data, insize); + +cleanup: + kfree(msg); + return ret; +} + +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, + }; + + return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, , + sizeof(cmd), NULL, 0); +} + +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); +} + +static int cros_ec_regulator_is_enabled(struct regulator_dev *dev) +{ + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); + struct ec_params_regulator_is_enabled cmd = { + .index = data->index, + }; + struct ec_response_regulator_is_enabled resp; + int ret; + + ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_IS_ENABLED, , + sizeof(cmd), , sizeof(resp)); + if (ret < 0) + return ret; + return resp.enabled;