Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp669184ybg; Wed, 10 Jun 2020 10:27:44 -0700 (PDT) X-Google-Smtp-Source: ABdhPJytDbpNewY2C3RExwuLzohpov4DqJW2y2N3gSM2epjaMVAHw066FSrN0pZPvEs0EhU9J/xU X-Received: by 2002:a17:906:1c8e:: with SMTP id g14mr4437375ejh.136.1591810063834; Wed, 10 Jun 2020 10:27:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591810063; cv=none; d=google.com; s=arc-20160816; b=NJ3fJiKHtgLwOK54iEVclFWinNa837Jzrfhu5C4q5P/FibjI/tEg0lufItOc/dpy55 5OtC1Kj6sUwaCOOm8g+EoxjJ+LQsTNrt/GWjluByXA4V2XRWlXXlOa5lHS5ifuIOSRud +64RC8j8fPMm9hRZY/Rtj9YSWoOVikiCix3YVAP9IBUf5DIzOgi7N8juX3f5ZXqJP0Kg PREyBD1U7Ji53TT8V1ol2ryCJunW6XY41ybAUvzTVwRVwQc84BeF0QuU6xR1RQ4YM21C oiHVMjrvmSO3k4dl3hAfsES/xGJb8qxcUf6UCnXH7TfLvKjLDXmofoDA2zWcmqEDRKX1 E0tA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=LOhoGD6yaFmvVg5W8Py+roDfEdePfgVM4bD9D4xZG2Q=; b=Hysb1KhclXvreroqYpDlLVfd3bvNYl2cL9+D34pFTfAR/148naXLambNjAnj/QyrZ1 7Ssa0I2BkUOJ9hrgX6pGCUZXL/x3q5WNMc7CxDeCw6gdBVNXltRamNgFWtBBVfehS0my IpA1nCxAeupD0LyHT+hJTQWXRELRPADXmmZkb89VePv+M7GQVW10fEZ4Regkz9UCscQy fWauSEG8/rayRzN4ccqNm777sQ+3r0xmhAsA8gsUKh3iIC5187cv5JoVMUZjW9TmBDmA AAVdXdc0B0W6CGPaYfB3u4mJ863Vq18slLlM4BOWSKzerQBL5iQctAqJFIcgdszM1bPb mx8A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d13si115904edo.382.2020.06.10.10.27.20; Wed, 10 Jun 2020 10:27:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=collabora.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726857AbgFJQrH (ORCPT + 99 others); Wed, 10 Jun 2020 12:47:07 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:38320 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726358AbgFJQrG (ORCPT ); Wed, 10 Jun 2020 12:47:06 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: eballetbo) with ESMTPSA id 948192A0DB7 Subject: Re: [PATCH v3 2/2] regulator: Add driver for cros-ec-regulator To: Pi-Hsun Shih Cc: Nicolas Boichat , Liam Girdwood , Mark Brown , Benson Leung , Guenter Roeck , Tzung-Bi Shih , Lee Jones , Yicheng Li , open list References: <20200610090748.45908-1-pihsun@chromium.org> <20200610090748.45908-3-pihsun@chromium.org> From: Enric Balletbo i Serra Message-ID: <3776237e-a6d5-ccda-79e4-39545b818e34@collabora.com> Date: Wed, 10 Jun 2020 18:47:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1 MIME-Version: 1.0 In-Reply-To: <20200610090748.45908-3-pihsun@chromium.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pi-Hsun, Thank you for your patch. On 10/6/20 11:07, 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 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/regulator/Kconfig | 10 + > drivers/regulator/Makefile | 1 + > drivers/regulator/cros-ec-regulator.c | 262 ++++++++++++++++++ > .../linux/platform_data/cros_ec_commands.h | 82 ++++++ > 4 files changed, 355 insertions(+) > create mode 100644 drivers/regulator/cros-ec-regulator.c > > 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 000000000000..0af3a397781c > --- /dev/null > +++ b/drivers/regulator/cros-ec-regulator.c > @@ -0,0 +1,262 @@ > +// SPDX-License-Identifier: GPL-2.0 You probably want to specify that this is GPL-2.0-only license. > +// > +// 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); The cros_ec_cmd_xfer_status() will already print an error if fails, don't need to do this as is redundant. > + goto cleanup; > + } > + > + if (insize) > + memcpy(indata, msg->data, insize); > + > +cleanup: > + kfree(msg); > + return ret; > +} > + > +static int cros_ec_regulator_enable(struct regulator_dev *dev) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + struct ec_params_regulator_enable cmd = { > + .index = data->index, > + .enable = 1, > + }; > + > + return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd, > + sizeof(cmd), NULL, 0); > +} > + > +static int cros_ec_regulator_disable(struct regulator_dev *dev) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + struct ec_params_regulator_enable cmd = { > + .index = data->index, > + .enable = 0, > + }; > + > + return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_ENABLE, &cmd, > + sizeof(cmd), NULL, 0); > +} > + > +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, &cmd, > + sizeof(cmd), &resp, sizeof(resp)); > + if (ret < 0) > + return ret; > + return resp.enabled; > +} > + > +static int cros_ec_regulator_list_voltage(struct regulator_dev *dev, > + unsigned int selector) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + > + if (selector >= data->num_voltages) > + return -EINVAL; > + > + return data->voltages_mV[selector] * 1000; > +} > + > +static int cros_ec_regulator_get_voltage(struct regulator_dev *dev) > +{ > + struct cros_ec_regulator_data *data = rdev_get_drvdata(dev); > + struct ec_params_regulator_get_voltage cmd = { > + .index = data->index, > + }; > + struct ec_response_regulator_get_voltage resp; > + int ret; > + > + ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_VOLTAGE, &cmd, > + sizeof(cmd), &resp, sizeof(resp)); > + if (ret < 0) > + return ret; > + return resp.voltage_mv * 1000; > +} > + > +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; > + return cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_SET_VOLTAGE, &cmd, > + sizeof(cmd), NULL, 0); > +} > + > +static struct regulator_ops cros_ec_regulator_voltage_ops = { > + .enable = cros_ec_regulator_enable, > + .disable = cros_ec_regulator_disable, > + .is_enabled = cros_ec_regulator_is_enabled, > + .list_voltage = cros_ec_regulator_list_voltage, > + .get_voltage = cros_ec_regulator_get_voltage, > + .set_voltage = cros_ec_regulator_set_voltage, > +}; > + > +static int cros_ec_regulator_init_info(struct device *dev, > + struct cros_ec_regulator_data *data) > +{ > + struct ec_params_regulator_get_info cmd = { > + .index = data->index, > + }; > + struct ec_response_regulator_get_info resp; > + int ret; > + > + ret = cros_ec_cmd(data->ec_dev, 0, EC_CMD_REGULATOR_GET_INFO, &cmd, > + sizeof(cmd), &resp, sizeof(resp)); > + if (ret < 0) > + return ret; > + > + data->num_voltages = > + min_t(u16, ARRAY_SIZE(resp.voltages_mv), resp.num_voltages); > + data->voltages_mV = > + devm_kmemdup(dev, resp.voltages_mv, > + sizeof(u16) * data->num_voltages, GFP_KERNEL); > + data->desc.n_voltages = data->num_voltages; > + data->desc.name = kstrndup(resp.name, sizeof(resp.name), GFP_KERNEL); Can you also use a device managed allocation here, so we can get rid of the cros_ec_regulator_remove() function? > + if (!data->desc.name) > + return -ENOMEM; > + > + return 0; > +} > + > +static int cros_ec_regulator_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct cros_ec_regulator_data *drvdata; > + struct regulator_init_data *init_data; > + struct regulator_config cfg = {}; > + struct regulator_desc *desc; > + int ret; > + > + drvdata = devm_kzalloc( > + &pdev->dev, sizeof(struct cros_ec_regulator_data), GFP_KERNEL); > + if (!drvdata) > + return -ENOMEM; > + > + drvdata->ec_dev = dev_get_drvdata(dev->parent); > + desc = &drvdata->desc; > + > + init_data = of_get_regulator_init_data(dev, np, desc); > + if (!init_data) > + return -EINVAL; > + > + ret = of_property_read_u32(np, "reg", &drvdata->index); > + if (ret < 0) > + return ret; > + > + desc->owner = THIS_MODULE; > + desc->type = REGULATOR_VOLTAGE; > + desc->ops = &cros_ec_regulator_voltage_ops; > + > + ret = cros_ec_regulator_init_info(dev, drvdata); > + if (ret < 0) > + return ret; > + > + cfg.dev = &pdev->dev; > + cfg.init_data = init_data; > + cfg.driver_data = drvdata; > + cfg.of_node = np; > + > + drvdata->dev = regulator_register(&drvdata->desc, &cfg); > + if (IS_ERR(drvdata->dev)) { > + ret = PTR_ERR(drvdata->dev); > + dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret); > + goto free_name; > + } > + > + platform_set_drvdata(pdev, drvdata); > + > + return 0; > + > +free_name: > + kfree(desc->name); > + return ret; > +} > + > +static int cros_ec_regulator_remove(struct platform_device *pdev) > +{ > + struct cros_ec_regulator_data *drvdata = platform_get_drvdata(pdev); > + > + kfree(drvdata->desc.name); > + > + return 0; > +} > + > +#if defined(CONFIG_OF) This is not really needed as this driver is OF-only. > +static const struct of_device_id regulator_cros_ec_of_match[] = { > + { .compatible = "google,regulator-cros-ec", }, > + {}, The sentinel indicates the last item on structure/arrays so no need to add a comma at the end. > +}; > +MODULE_DEVICE_TABLE(of, regulator_cros_ec_of_match); > +#endif > + > +static struct platform_driver cros_ec_regulator_driver = { > + .probe = cros_ec_regulator_probe, > + .remove = cros_ec_regulator_remove, > + .driver = { > + .name = "cros-ec-regulator", > + .of_match_table = of_match_ptr(regulator_cros_ec_of_match), Also you can just do .of_match_table = regulator_cros_ec_of_match as this driver is OF-only. > + }, > +}; > + > +module_platform_driver(cros_ec_regulator_driver); > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("ChromeOS EC controlled regulator"); MODULE_AUTHOR? :-) > diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h > index 69210881ebac..a417b51b5764 100644 > --- a/include/linux/platform_data/cros_ec_commands.h > +++ b/include/linux/platform_data/cros_ec_commands.h > @@ -5430,6 +5430,88 @@ struct ec_response_rollback_info { > /* Issue AP reset */ > #define EC_CMD_AP_RESET 0x0125 > > +/*****************************************************************************/ > +/* Voltage regulator controls */ > + > +/* > + * Get basic info of voltage regulator for given index. > + * > + * Returns the regulator name and supported voltage list in mV. > + */ > +#define EC_CMD_REGULATOR_GET_INFO 0x012B This introduces a new EC command, while you are here, please also add the command in drivers/platform/chrome/cros_ec_trace.c, so we can trace properly the command. Also can you point me to the commit that introduces this command in the EC firmware? > + > +/* Maximum length of regulator name */ > +#define EC_REGULATOR_NAME_MAX_LEN 16 > + > +/* Maximum length of the supported voltage list. */ > +#define EC_REGULATOR_VOLTAGE_MAX_COUNT 16 > + > +struct ec_params_regulator_get_info { > + uint32_t index; > +} __ec_align4; > + > +struct ec_response_regulator_get_info { > + char name[EC_REGULATOR_NAME_MAX_LEN]; > + uint16_t num_voltages; > + uint16_t voltages_mv[EC_REGULATOR_VOLTAGE_MAX_COUNT]; > +} __ec_align1; > + > +/* > + * Configure the regulator as enabled / disabled. > + */ > +#define EC_CMD_REGULATOR_ENABLE 0x012C > + Same comment here and for all the new commands introduced. > +struct ec_params_regulator_enable { > + uint32_t index; > + uint8_t enable; > +} __ec_align4; > + > +/* > + * Query if the regulator is enabled. > + * > + * Returns 1 if the regulator is enabled, 0 if not. > + */ > +#define EC_CMD_REGULATOR_IS_ENABLED 0x012D > + > +struct ec_params_regulator_is_enabled { > + uint32_t index; > +} __ec_align4; > + > +struct ec_response_regulator_is_enabled { > + uint8_t enabled; > +} __ec_align1; > + > +/* > + * Set voltage for the voltage regulator within the range specified. > + * > + * The driver should select the voltage in range closest to min_mv. > + * > + * Also note that this might be called before the regulator is enabled, and the > + * setting should be in effect after the regulator is enabled. > + */ > +#define EC_CMD_REGULATOR_SET_VOLTAGE 0x012E > + > +struct ec_params_regulator_set_voltage { > + uint32_t index; > + uint32_t min_mv; > + uint32_t max_mv; > +} __ec_align4; > + > +/* > + * Get the currently configured voltage for the voltage regulator. > + * > + * Note that this might be called before the regulator is enabled. > + */ > +#define EC_CMD_REGULATOR_GET_VOLTAGE 0x012F > + > +struct ec_params_regulator_get_voltage { > + uint32_t index; > +} __ec_align4; > + > +struct ec_response_regulator_get_voltage { > + uint32_t voltage_mv; > +} __ec_align4; > + > /*****************************************************************************/ > /* The command range 0x200-0x2FF is reserved for Rotor. */ > >