Re: [PATCH v5 2/2] regulator: Add driver for cros-ec-regulator
On Thu, Jun 11, 2020 at 01:44:42PM +0200, Enric Balletbo i Serra wrote: > On 11/6/20 10:25, Pi-Hsun Shih wrote: > > 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. Please delete unneeded context from mails when replying. Doing this makes it much easier to find your reply in the message, helping ensure it won't be missed by people scrolling through the irrelevant quoted material. signature.asc Description: PGP signature
Re: [PATCH v5 2/2] regulator: Add driver for cros-ec-regulator
Hi Pi-Hsun, Thank you for your patch, one last change, sorry to not request it before. On 11/6/20 10:25, Pi-Hsun Shih wrote: > 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 > --- > Changes from v4: > * Change compatible name from regulator-cros-ec to cros-ec-regulator. > > Changes from v3: > * Remove check around CONFIG_OF. > * Add new host commands to cros_ec_trace. > * Use devm_kstrdup for duping regulator name. > * Change license header and add MODULE_AUTHOR. > * Address review comments. > > Changes from v2: > * Add 'depends on OF' to Kconfig. > * Add Kconfig description about compiling as module. > > Changes from v1: > * Change compatible string to google,regulator-cros-ec. > * Use reg property in device tree. > * Address comments on code styles. > > 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/platform/chrome/cros_ec_trace.c | 5 + Could you split this patch in two, one for chrome-platform that introduces the new commands (cros_ec_commands and cros_ec_trace) and another one with the regulator driver. > drivers/regulator/Kconfig | 10 + > drivers/regulator/Makefile| 1 + > drivers/regulator/cros-ec-regulator.c | 256 ++ > .../linux/platform_data/cros_ec_commands.h| 82 ++ > 5 files changed, 354 insertions(+) > create mode 100644 drivers/regulator/cros-ec-regulator.c > > diff --git a/drivers/platform/chrome/cros_ec_trace.c > b/drivers/platform/chrome/cros_ec_trace.c > index 523a39bd0ff6..425e9441b7ca 100644 > --- a/drivers/platform/chrome/cros_ec_trace.c > +++ b/drivers/platform/chrome/cros_ec_trace.c > @@ -161,6 +161,11 @@ > TRACE_SYMBOL(EC_CMD_ADC_READ), \ > TRACE_SYMBOL(EC_CMD_ROLLBACK_INFO), \ > TRACE_SYMBOL(EC_CMD_AP_RESET), \ > + TRACE_SYMBOL(EC_CMD_REGULATOR_GET_INFO), \ > + TRACE_SYMBOL(EC_CMD_REGULATOR_ENABLE), \ > + TRACE_SYMBOL(EC_CMD_REGULATOR_IS_ENABLED), \ > + TRACE_SYMBOL(EC_CMD_REGULATOR_SET_VOLTAGE), \ > + TRACE_SYMBOL(EC_CMD_REGULATOR_GET_VOLTAGE), \ Also make sure this is following the order generated by the command sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' \ include/linux/platform_data/cros_ec_commands.h after introduce the EC commands. With that fixed you can add: Reviewed-by: Enric Balletbo i Serra on both patches. Thanks, Enric > TRACE_SYMBOL(EC_CMD_CR51_BASE), \ > TRACE_SYMBOL(EC_CMD_CR51_LAST), \ > TRACE_SYMBOL(EC_CMD_FP_PASSTHRU), \ > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index 8f677f5d79b4..c398e90e0e73 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -238,6 +238,16 @@ 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 && OF > + help > + This driver supports voltage regulators that is connected to ChromeOS > + EC and controlled through EC host commands. > + > + This driver can also be built as a module. If so, the module > + will be called cros-ec-regulator. > + > 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 ..830ceefc704a > --- /dev/null > +++ b/drivers/regulator/cros-ec-regulator.c > @@ -0,0 +1,256 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +// > +// Copyright 2020 Google LLC. > + > +#include > +#include > +#include > +#include > +#include >
[PATCH v5 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
---
Changes from v4:
* Change compatible name from regulator-cros-ec to cros-ec-regulator.
Changes from v3:
* Remove check around CONFIG_OF.
* Add new host commands to cros_ec_trace.
* Use devm_kstrdup for duping regulator name.
* Change license header and add MODULE_AUTHOR.
* Address review comments.
Changes from v2:
* Add 'depends on OF' to Kconfig.
* Add Kconfig description about compiling as module.
Changes from v1:
* Change compatible string to google,regulator-cros-ec.
* Use reg property in device tree.
* Address comments on code styles.
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/platform/chrome/cros_ec_trace.c | 5 +
drivers/regulator/Kconfig | 10 +
drivers/regulator/Makefile| 1 +
drivers/regulator/cros-ec-regulator.c | 256 ++
.../linux/platform_data/cros_ec_commands.h| 82 ++
5 files changed, 354 insertions(+)
create mode 100644 drivers/regulator/cros-ec-regulator.c
diff --git a/drivers/platform/chrome/cros_ec_trace.c
b/drivers/platform/chrome/cros_ec_trace.c
index 523a39bd0ff6..425e9441b7ca 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -161,6 +161,11 @@
TRACE_SYMBOL(EC_CMD_ADC_READ), \
TRACE_SYMBOL(EC_CMD_ROLLBACK_INFO), \
TRACE_SYMBOL(EC_CMD_AP_RESET), \
+ TRACE_SYMBOL(EC_CMD_REGULATOR_GET_INFO), \
+ TRACE_SYMBOL(EC_CMD_REGULATOR_ENABLE), \
+ TRACE_SYMBOL(EC_CMD_REGULATOR_IS_ENABLED), \
+ TRACE_SYMBOL(EC_CMD_REGULATOR_SET_VOLTAGE), \
+ TRACE_SYMBOL(EC_CMD_REGULATOR_GET_VOLTAGE), \
TRACE_SYMBOL(EC_CMD_CR51_BASE), \
TRACE_SYMBOL(EC_CMD_CR51_LAST), \
TRACE_SYMBOL(EC_CMD_FP_PASSTHRU), \
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 8f677f5d79b4..c398e90e0e73 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -238,6 +238,16 @@ 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 && OF
+ help
+ This driver supports voltage regulators that is connected to ChromeOS
+ EC and controlled through EC host commands.
+
+ This driver can also be built as a module. If so, the module
+ will be called cros-ec-regulator.
+
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 ..830ceefc704a
--- /dev/null
+++ b/drivers/regulator/cros-ec-regulator.c
@@ -0,0 +1,256 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// 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 =
