* [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC @ 2019-10-24 14:29 Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin ` (2 more replies) 0 siblings, 3 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-24 14:29 UTC (permalink / raw) To: lgirdwood, broonie, andriy.shevchenko, lee.jones, linux-kernel Cc: Andrey Zhizhikin This patchset introduces additional regulator driver for Intel Cherry Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this PMIC, which is used to instantiate this regulator. Regulator support for this PMIC was present in kernel release from Intel targeted Aero platform, but was not entirely ported upstream and has been omitted in mainline kernel releases. Consecutively, absence of regulator caused the SD Card interface not to be provided with Vqcc voltage source needed to operate with UHS-I cards. Following patches are addessing this issue and making sd card interface to be fully operable with UHS-I cards. Regulator driver lists an ACPI id of the SD Card interface in consumers and exposes optional "vqmmc" voltage source, which mmc driver uses to switch signalling voltages between 1.8V and 3.3V. This set contains of 2 patches: one is implementing the regulator driver (based on a non upstreamed version from Intel Aero), and another patch registers this driver as mfd cell in exising Whiskey Cove PMIC driver. Andrey Zhizhikin (2): regulator: add support for Intel Cherry Whiskey Cove regulator mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC drivers/mfd/intel_soc_pmic_chtwc.c | 15 +- drivers/regulator/Kconfig | 10 + drivers/regulator/Makefile | 1 + drivers/regulator/intel-cht-wc-regulator.c | 433 ++++++++++++++++++ .../linux/regulator/intel-cht-wc-regulator.h | 64 +++ 5 files changed, 521 insertions(+), 2 deletions(-) create mode 100644 drivers/regulator/intel-cht-wc-regulator.c create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h -- 2.17.1 ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-24 14:29 [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin @ 2019-10-24 14:29 ` Andrey Zhizhikin 2019-10-25 8:01 ` Andy Shevchenko 2019-10-25 12:17 ` Mark Brown 2019-10-24 14:29 ` [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin 2019-10-25 7:53 ` [PATCH 0/2] add regulator driver and mfd cell for Intel " Andy Shevchenko 2 siblings, 2 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-24 14:29 UTC (permalink / raw) To: lgirdwood, broonie, andriy.shevchenko, lee.jones, linux-kernel Cc: Andrey Zhizhikin Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which supplies various voltage rails. This initial version supports only vsdio, which is required to source vqmmc for sd card interface. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- drivers/regulator/Kconfig | 10 + drivers/regulator/Makefile | 1 + drivers/regulator/intel-cht-wc-regulator.c | 433 ++++++++++++++++++ .../linux/regulator/intel-cht-wc-regulator.h | 64 +++ 4 files changed, 508 insertions(+) create mode 100644 drivers/regulator/intel-cht-wc-regulator.c create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 3ee63531f6d5..ae3d57f572df 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -1077,6 +1077,16 @@ config REGULATOR_VEXPRESS This driver provides support for voltage regulators available on the ARM Ltd's Versatile Express platform. +config REGULATOR_WHISKEY_COVE + tristate "PMIC Whiskey Cove voltage regulator" + depends on INTEL_SOC_PMIC_CHTWC + help + This driver provides support for the voltage regulators of the + Intel Whiskey Cove PMIC. It is provided as an mfd cell of the + Intel Cherry Trail PMIC driver. + Only select this regulator driver if the MFD part is selected + in the Kernel configuration, it is meant to be operable as a cell. + config REGULATOR_WM831X tristate "Wolfson Microelectronics WM831x PMIC regulators" depends on MFD_WM831X diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 2210ba56f9bd..380d536126c6 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -132,6 +132,7 @@ obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress-regulator.o +obj-$(CONFIG_REGULATOR_WHISKEY_COVE) += intel-cht-wc-regulator.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o diff --git a/drivers/regulator/intel-cht-wc-regulator.c b/drivers/regulator/intel-cht-wc-regulator.c new file mode 100644 index 000000000000..5a66a6b37dc6 --- /dev/null +++ b/drivers/regulator/intel-cht-wc-regulator.c @@ -0,0 +1,433 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * intel-cht-wc-regulator.c - CherryTrail regulator driver + * + * Copyright (c) 2019, Leica Geosystems AG + * + * Base on the non-upstreamed version from Intel, + * which was introduced for Intel Aero board. + * Copyright (c) 2014, Intel Corporation. + * + */ + +#include <linux/module.h> +#include <linux/init.h> +#include <linux/err.h> +#include <linux/platform_device.h> +#include <linux/mod_devicetable.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/mfd/intel_soc_pmic.h> +#include <linux/regulator/intel-cht-wc-regulator.h> + + +/* + * Voltage Control regulator offsets + */ + +/* buck boost regulators */ +#define CHT_WC_V3P3A_CTRL 0x6e5e +/* buck regulators */ +#define CHT_WC_V1P8A_CTRL 0x6e56 +#define CHT_WC_V1P05A_CTRL 0x6e3b +#define CHT_WC_V1P15_CTRL 0x6e3c +#define CHT_WC_VDDQ_CTRL 0x6e58 +/* boot regulators */ +/* ldo regulators */ +#define CHT_WC_VPROG1A_CTRL 0x6e90 +#define CHT_WC_VPROG1B_CTRL 0x6e91 +#define CHT_WC_VPROG1F_CTRL 0x6e95 +#define CHT_WC_V1P8SX_CTRL 0x6e57 +#define CHT_WC_V1P2A_CTRL 0x6e59 +#define CHT_WC_V1P2SX_CTRL 0x6e5a +#define CHT_WC_VSDIO_CTRL 0x6e67 +#define CHT_WC_V2P8SX_CTRL 0x6e5d +#define CHT_WC_V3P3SD_CTRL 0x6e5f +#define CHT_WC_VPROG2D_CTRL 0x6e99 +#define CHT_WC_VPROG3A_CTRL 0x6e9a +#define CHT_WC_VPROG3B_CTRL 0x6e9b +#define CHT_WC_VPROG4A_CTRL 0x6e9c +#define CHT_WC_VPROG4B_CTRL 0x6e9d +#define CHT_WC_VPROG4C_CTRL 0x6e9e +#define CHT_WC_VPROG4D_CTRL 0x6e9f +#define CHT_WC_VPROG5A_CTRL 0x6ea0 +#define CHT_WC_VPROG5B_CTRL 0x6ea1 +#define CHT_WC_VPROG6A_CTRL 0x6ea2 +#define CHT_WC_VPROG6B_CTRL 0x6ea3 + + +/* + * Voltage Selector regulator offsets + */ + +/* buck boost regulators */ +#define CHT_WC_V3P3A_VSEL 0x6e68 +/* buck regulators */ +#define CHT_WC_V1P8A_VSEL 0x6e5b +#define CHT_WC_V1P05A_VSEL 0x6e3d +#define CHT_WC_V1P15_VSEL 0x6e3e +#define CHT_WC_VDDQ_VSEL 0x6e5c +/* boot regulators */ +/* ldo regulators */ +#define CHT_WC_VPROG1A_VSEL 0x6ec0 +#define CHT_WC_VPROG1B_VSEL 0x6ec1 +#define CHT_WC_V1P8SX_VSEL 0x6ec2 +#define CHT_WC_V1P2SX_VSEL 0x6ec3 +#define CHT_WC_V1P2A_VSEL 0x6ec4 +#define CHT_WC_VPROG1F_VSEL 0x6ec5 +#define CHT_WC_VSDIO_VSEL 0x6ec6 +#define CHT_WC_V2P8SX_VSEL 0x6ec7 +#define CHT_WC_V3P3SD_VSEL 0x6ec8 +#define CHT_WC_VPROG2D_VSEL 0x6ec9 +#define CHT_WC_VPROG3A_VSEL 0x6eca +#define CHT_WC_VPROG3B_VSEL 0x6ecb +#define CHT_WC_VPROG4A_VSEL 0x6ecc +#define CHT_WC_VPROG4B_VSEL 0x6ecd +#define CHT_WC_VPROG4C_VSEL 0x6ece +#define CHT_WC_VPROG4D_VSEL 0x6ecf +#define CHT_WC_VPROG5A_VSEL 0x6ed0 +#define CHT_WC_VPROG5B_VSEL 0x6ed1 +#define CHT_WC_VPROG6A_VSEL 0x6ed2 +#define CHT_WC_VPROG6B_VSEL 0x6ed3 + + +/* number of voltage variations exposed */ + +/* buck boost regulators */ +#define CHT_WC_V3P3A_VRANGE 8 +/* buck regulators */ +#define CHT_WC_V1P8A_VRANGE 256 +#define CHT_WC_V1P05A_VRANGE 256 +#define CHT_WC_VDDQ_VRANGE 120 +/* boot regulators */ +/* ldo regulators */ +#define CHT_WC_VPROG1A_VRANGE 53 +#define CHT_WC_VPROG1B_VRANGE 53 +#define CHT_WC_VPROG1F_VRANGE 53 +#define CHT_WC_V1P8SX_VRANGE 53 +#define CHT_WC_V1P2SX_VRANGE 53 +#define CHT_WC_V1P2A_VRANGE 53 +#define CHT_WC_VSDIO_VRANGE 53 +#define CHT_WC_V2P8SX_VRANGE 53 +#define CHT_WC_V3P3SD_VRANGE 53 +#define CHT_WC_VPROG2D_VRANGE 53 +#define CHT_WC_VPROG3A_VRANGE 53 +#define CHT_WC_VPROG3B_VRANGE 53 +#define CHT_WC_VPROG4A_VRANGE 53 +#define CHT_WC_VPROG4B_VRANGE 53 +#define CHT_WC_VPROG4C_VRANGE 53 +#define CHT_WC_VPROG4D_VRANGE 53 +#define CHT_WC_VPROG5A_VRANGE 53 +#define CHT_WC_VPROG5B_VRANGE 53 +#define CHT_WC_VPROG6A_VRANGE 53 +#define CHT_WC_VPROG6B_VRANGE 53 + + +/* voltage tables */ +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE], + CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE], + CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE], + CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE], + CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE], + CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE], + CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE], + CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE], + CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE], + CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE], + CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE], + CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE], + CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE], + CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE], + CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE], + CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE], + CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE], + CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE], + CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE], + CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE], + CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE], + CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE], + CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE]; + +/* + * VSDIO regulator description, voltage tables, consumers and constraints + */ +/* + * The VSDIO regulator should only support 1.8V and 3.3V. All other + * voltages are invalid for sd card, so disable them here. + */ +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + 0, 0, 3300000, 0, 0 +}; + +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO + * regulator. This is the only interface that requires this source on Cherry + * Trail to operate with UHS-I cards. + */ +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = { + REGULATOR_SUPPLY("vqmmc", "80860F14:02"), +}; + +static struct regulator_init_data vqmmc_init_data = { + .constraints = { + .min_uV = 1800000, + .max_uV = 3300000, + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE | + REGULATOR_CHANGE_STATUS, + .valid_modes_mask = REGULATOR_MODE_NORMAL, + .settling_time = 20000, + }, + .num_consumer_supplies = ARRAY_SIZE(cht_wc_vqmmc_supply_consumer), + .consumer_supplies = cht_wc_vqmmc_supply_consumer, +}; + +static int cht_wc_regulator_enable(struct regulator_dev *rdev) +{ + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); + + int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg, + pmic_info->vctl_mask, pmic_info->reg_enbl_mask); + if (ret < 0) + dev_err(&rdev->dev, "Failed to enable regulator %s: %d\n", + pmic_info->desc.name, ret); + + return ret; +} + +static int cht_wc_regulator_disable(struct regulator_dev *rdev) +{ + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); + + int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg, + pmic_info->vctl_mask, pmic_info->reg_dsbl_mask); + if (ret < 0) + dev_err(&rdev->dev, "Failed to disable regulator %s: %d\n", + pmic_info->desc.name, ret); + + return ret; +} + +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev) +{ + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); + int rval; + + int ret = regmap_read(rdev->regmap, pmic_info->vctl_reg, &rval); + + if (ret < 0) { + dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n", + pmic_info->vsel_reg, ret); + return ret; + } + + rval &= pmic_info->vctl_mask; + + return rval & pmic_info->reg_enbl_mask; +} + +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev) +{ + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); + int rval; + + int ret = regmap_read(rdev->regmap, pmic_info->vsel_reg, &rval); + + if (ret < 0) { + dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n", + pmic_info->vsel_reg, ret); + return ret; + } + + return (rval & pmic_info->vsel_mask) - pmic_info->start; +} + +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev, + unsigned int selector) +{ + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); + + int ret = regmap_update_bits(rdev->regmap, pmic_info->vsel_reg, + pmic_info->vsel_mask, selector + pmic_info->start); + if (ret < 0) + dev_err(&rdev->dev, "Failed to set voltage for regulator %s: %d\n", + pmic_info->desc.name, ret); + + return ret; +} + +/* regulator ops */ +static struct regulator_ops wcove_regulator_ops = { + .enable = cht_wc_regulator_enable, + .disable = cht_wc_regulator_disable, + .is_enabled = cht_wc_regulator_is_enabled, + .get_voltage_sel = cht_wc_regulator_read_voltage_sel, + .set_voltage_sel = cht_wc_regulator_set_voltage_sel, + .list_voltage = regulator_list_voltage_table, +}; + +#define CHT_WC_REGULATOR(_id, minv, maxv, strt, vselmsk, vscale, \ + vctlmsk, enbl, dsbl, rt_flag, _init_data) \ +{\ + .desc = {\ + .name = ""#_id,\ + .ops = &wcove_regulator_ops,\ + .type = REGULATOR_VOLTAGE,\ + .id = CHT_WC_REGULATOR_##_id,\ + .owner = THIS_MODULE,\ + },\ + .rdev = NULL, \ + .init_data = (_init_data), \ + .vctl_reg = CHT_WC_##_id##_CTRL,\ + .vsel_reg = CHT_WC_##_id##_VSEL,\ + .min_mV = (minv),\ + .max_mV = (maxv),\ + .start = (strt),\ + .vsel_mask = (vselmsk),\ + .scale = (vscale),\ + .nvolts = CHT_WC_##_id##_VRANGE,\ + .vctl_mask = (vctlmsk),\ + .reg_enbl_mask = (enbl),\ + .reg_dsbl_mask = (dsbl),\ + .vtable = CHT_WC_##_id##_VSEL_TABLE,\ + .runtime_table = (rt_flag),\ +} + +/* Regulator descriptions */ +static struct ch_wc_regulator_info regulators_info[] = { + CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07, 50, 0x01, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V1P8A, 250, 2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V1P05A, 250, 2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VDDQ, 250, 1440, 0x00, 0x7f, 10, 0x01, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V1P8SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V1P2A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V1P2SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(V2P8SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VSDIO, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, false, &vqmmc_init_data), + CHT_WC_REGULATOR(V3P3SD, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG1A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG1B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG1F, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG2D, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG3A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG3B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG4A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG4B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG4C, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG4D, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG5A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG5B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG6A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), + CHT_WC_REGULATOR(VPROG6B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), +}; + +static inline struct ch_wc_regulator_info *cht_wc_find_regulator_info(int id) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(regulators_info); i++) { + if (regulators_info[i].desc.id == id) + return ®ulators_info[i]; + } + return NULL; +} + +static void initialize_vtable(struct ch_wc_regulator_info *reg_info) +{ + unsigned int i, volt; + + if (reg_info->runtime_table == true) { + for (i = 0; i < reg_info->nvolts; i++) { + volt = reg_info->min_mV + (i * reg_info->scale); + if (volt < reg_info->min_mV) + volt = reg_info->min_mV; + if (volt > reg_info->max_mV) + volt = reg_info->max_mV; + /* set value in uV */ + reg_info->vtable[i] = volt*1000; + } + } + reg_info->desc.volt_table = reg_info->vtable; + reg_info->desc.n_voltages = reg_info->nvolts; +} + +static int cht_wc_regulator_probe(struct platform_device *pdev) +{ + struct ch_wc_regulator_info *regulator; + struct regulator_config config = { }; + + struct ch_wc_regulator_info *reg_info = + cht_wc_find_regulator_info(pdev->id); + + if (reg_info == NULL) + return -EINVAL; + + regulator = devm_kzalloc(&pdev->dev, sizeof(*regulator), GFP_KERNEL); + if (!regulator) + return -ENOMEM; + + platform_set_drvdata(pdev, regulator); + + initialize_vtable(reg_info); + + config.dev = &pdev->dev; + config.driver_data = reg_info; + config.init_data = reg_info->init_data; + + regulator->rdev = regulator_register(®_info->desc, &config); + + if (IS_ERR(regulator->rdev)) { + dev_err(&pdev->dev, "failed to register regulator as %s\n", + reg_info->desc.name); + return PTR_ERR(regulator->rdev); + } + + dev_dbg(&pdev->dev, "registered Whiskey Cove regulator as %s\n", + dev_name(®ulator->rdev->dev)); + + return 0; +} + +static int cht_wc_regulator_remove(struct platform_device *pdev) +{ + regulator_unregister(platform_get_drvdata(pdev)); + return 0; +} + +static const struct platform_device_id cht_wc_regulator_id_table[] = { + { + .name = "cht_wcove_regulator" + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(platform, wcove_regulator_id_table); + +static struct platform_driver cht_wc_regulator_driver = { + .driver = { + .name = "cht_wcove_regulator", + .owner = THIS_MODULE, + }, + .probe = cht_wc_regulator_probe, + .remove = cht_wc_regulator_remove, + .id_table = cht_wc_regulator_id_table, +}; + +static int __init cht_wc_regulator_init(void) +{ + return platform_driver_register(&cht_wc_regulator_driver); +} +subsys_initcall_sync(cht_wc_regulator_init); + +static void __exit cht_wc_regulator_exit(void) +{ + platform_driver_unregister(&cht_wc_regulator_driver); +} +module_exit(cht_wc_regulator_exit); + +MODULE_ALIAS("platform:cht-wc-regulator"); +MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove Regulator driver"); +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com"); +MODULE_AUTHOR("Nitheesh K L <nitheesh.k.l@intel.com"); diff --git a/include/linux/regulator/intel-cht-wc-regulator.h b/include/linux/regulator/intel-cht-wc-regulator.h new file mode 100644 index 000000000000..0f6b3be11fea --- /dev/null +++ b/include/linux/regulator/intel-cht-wc-regulator.h @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * intel-cht-wc-regulator.h - Cherry Trail Whiskey Cove PMIC regulator + * + * Copyright (c) 2019, Leica Geosystems AG + * + * Base on the non-upstreamed version from Intel, which was + * introduced for Intel Aero board. + * Copyright (c) 2014, Intel Corporation. + */ + +#ifndef __LINUX_REGULATOR_INTEL_CHT_WC_H_ +#define __LINUX_REGULATOR_INTEL_CHT_WC_H_ + +#include <linux/regulator/driver.h> + +enum { + CHT_WC_REGULATOR_V1P8A = 1, + CHT_WC_REGULATOR_V1P05A, + CHT_WC_REGULATOR_V1P15, + CHT_WC_REGULATOR_VDDQ, + CHT_WC_REGULATOR_V3P3A, + CHT_WC_REGULATOR_VPROG1A, + CHT_WC_REGULATOR_VPROG1B, + CHT_WC_REGULATOR_VPROG1F, + CHT_WC_REGULATOR_V1P8SX, + CHT_WC_REGULATOR_V1P2SX, + CHT_WC_REGULATOR_V1P2A, + CHT_WC_REGULATOR_VSDIO, + CHT_WC_REGULATOR_V2P8SX, + CHT_WC_REGULATOR_V3P3SD, + CHT_WC_REGULATOR_VPROG2D, + CHT_WC_REGULATOR_VPROG3A, + CHT_WC_REGULATOR_VPROG3B, + CHT_WC_REGULATOR_VPROG4A, + CHT_WC_REGULATOR_VPROG4B, + CHT_WC_REGULATOR_VPROG4C, + CHT_WC_REGULATOR_VPROG4D, + CHT_WC_REGULATOR_VPROG5A, + CHT_WC_REGULATOR_VPROG5B, + CHT_WC_REGULATOR_VPROG6A, + CHT_WC_REGULATOR_VPROG6B, +}; + +struct ch_wc_regulator_info { + struct regulator_desc desc; + struct regulator_dev *rdev; + struct regulator_init_data *init_data; + unsigned int vctl_reg; + unsigned int vsel_reg; + unsigned int min_mV; + unsigned int max_mV; + unsigned int start; + unsigned int vsel_mask; + unsigned int scale; + unsigned int nvolts; + unsigned int vctl_mask; + unsigned int reg_enbl_mask; + unsigned int reg_dsbl_mask; + unsigned int *vtable; + bool runtime_table; +}; + +#endif /* __LINUX_REGULATOR_INTEL_CHT_WC_H_ */ -- 2.17.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin @ 2019-10-25 8:01 ` Andy Shevchenko 2019-10-25 8:58 ` Andrey Zhizhikin 2019-10-25 12:17 ` Mark Brown 1 sibling, 1 reply; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 8:01 UTC (permalink / raw) To: Andrey Zhizhikin, Hans de Goede, Adrian Hunter Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which > supplies various voltage rails. > > This initial version supports only vsdio, which is required to source > vqmmc for sd card interface. This patch has some style issues. I will wait for Adrian and Hans to comment on the approach as a whole and then we will see how to improve the rest. > Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> > --- > drivers/regulator/Kconfig | 10 + > drivers/regulator/Makefile | 1 + > drivers/regulator/intel-cht-wc-regulator.c | 433 ++++++++++++++++++ > .../linux/regulator/intel-cht-wc-regulator.h | 64 +++ > 4 files changed, 508 insertions(+) > create mode 100644 drivers/regulator/intel-cht-wc-regulator.c > create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h > > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index 3ee63531f6d5..ae3d57f572df 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -1077,6 +1077,16 @@ config REGULATOR_VEXPRESS > This driver provides support for voltage regulators available > on the ARM Ltd's Versatile Express platform. > > +config REGULATOR_WHISKEY_COVE > + tristate "PMIC Whiskey Cove voltage regulator" > + depends on INTEL_SOC_PMIC_CHTWC > + help > + This driver provides support for the voltage regulators of the > + Intel Whiskey Cove PMIC. It is provided as an mfd cell of the > + Intel Cherry Trail PMIC driver. > + Only select this regulator driver if the MFD part is selected > + in the Kernel configuration, it is meant to be operable as a cell. > + > config REGULATOR_WM831X > tristate "Wolfson Microelectronics WM831x PMIC regulators" > depends on MFD_WM831X > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 2210ba56f9bd..380d536126c6 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -132,6 +132,7 @@ obj-$(CONFIG_REGULATOR_TWL4030) += twl-regulator.o twl6030-regulator.o > obj-$(CONFIG_REGULATOR_UNIPHIER) += uniphier-regulator.o > obj-$(CONFIG_REGULATOR_VCTRL) += vctrl-regulator.o > obj-$(CONFIG_REGULATOR_VEXPRESS) += vexpress-regulator.o > +obj-$(CONFIG_REGULATOR_WHISKEY_COVE) += intel-cht-wc-regulator.o > obj-$(CONFIG_REGULATOR_WM831X) += wm831x-dcdc.o > obj-$(CONFIG_REGULATOR_WM831X) += wm831x-isink.o > obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o > diff --git a/drivers/regulator/intel-cht-wc-regulator.c b/drivers/regulator/intel-cht-wc-regulator.c > new file mode 100644 > index 000000000000..5a66a6b37dc6 > --- /dev/null > +++ b/drivers/regulator/intel-cht-wc-regulator.c > @@ -0,0 +1,433 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * intel-cht-wc-regulator.c - CherryTrail regulator driver > + * > + * Copyright (c) 2019, Leica Geosystems AG > + * > + * Base on the non-upstreamed version from Intel, > + * which was introduced for Intel Aero board. > + * Copyright (c) 2014, Intel Corporation. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/platform_device.h> > +#include <linux/mod_devicetable.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > +#include <linux/mfd/intel_soc_pmic.h> > +#include <linux/regulator/intel-cht-wc-regulator.h> > + > + > +/* > + * Voltage Control regulator offsets > + */ > + > +/* buck boost regulators */ > +#define CHT_WC_V3P3A_CTRL 0x6e5e > +/* buck regulators */ > +#define CHT_WC_V1P8A_CTRL 0x6e56 > +#define CHT_WC_V1P05A_CTRL 0x6e3b > +#define CHT_WC_V1P15_CTRL 0x6e3c > +#define CHT_WC_VDDQ_CTRL 0x6e58 > +/* boot regulators */ > +/* ldo regulators */ > +#define CHT_WC_VPROG1A_CTRL 0x6e90 > +#define CHT_WC_VPROG1B_CTRL 0x6e91 > +#define CHT_WC_VPROG1F_CTRL 0x6e95 > +#define CHT_WC_V1P8SX_CTRL 0x6e57 > +#define CHT_WC_V1P2A_CTRL 0x6e59 > +#define CHT_WC_V1P2SX_CTRL 0x6e5a > +#define CHT_WC_VSDIO_CTRL 0x6e67 > +#define CHT_WC_V2P8SX_CTRL 0x6e5d > +#define CHT_WC_V3P3SD_CTRL 0x6e5f > +#define CHT_WC_VPROG2D_CTRL 0x6e99 > +#define CHT_WC_VPROG3A_CTRL 0x6e9a > +#define CHT_WC_VPROG3B_CTRL 0x6e9b > +#define CHT_WC_VPROG4A_CTRL 0x6e9c > +#define CHT_WC_VPROG4B_CTRL 0x6e9d > +#define CHT_WC_VPROG4C_CTRL 0x6e9e > +#define CHT_WC_VPROG4D_CTRL 0x6e9f > +#define CHT_WC_VPROG5A_CTRL 0x6ea0 > +#define CHT_WC_VPROG5B_CTRL 0x6ea1 > +#define CHT_WC_VPROG6A_CTRL 0x6ea2 > +#define CHT_WC_VPROG6B_CTRL 0x6ea3 > + > + > +/* > + * Voltage Selector regulator offsets > + */ > + > +/* buck boost regulators */ > +#define CHT_WC_V3P3A_VSEL 0x6e68 > +/* buck regulators */ > +#define CHT_WC_V1P8A_VSEL 0x6e5b > +#define CHT_WC_V1P05A_VSEL 0x6e3d > +#define CHT_WC_V1P15_VSEL 0x6e3e > +#define CHT_WC_VDDQ_VSEL 0x6e5c > +/* boot regulators */ > +/* ldo regulators */ > +#define CHT_WC_VPROG1A_VSEL 0x6ec0 > +#define CHT_WC_VPROG1B_VSEL 0x6ec1 > +#define CHT_WC_V1P8SX_VSEL 0x6ec2 > +#define CHT_WC_V1P2SX_VSEL 0x6ec3 > +#define CHT_WC_V1P2A_VSEL 0x6ec4 > +#define CHT_WC_VPROG1F_VSEL 0x6ec5 > +#define CHT_WC_VSDIO_VSEL 0x6ec6 > +#define CHT_WC_V2P8SX_VSEL 0x6ec7 > +#define CHT_WC_V3P3SD_VSEL 0x6ec8 > +#define CHT_WC_VPROG2D_VSEL 0x6ec9 > +#define CHT_WC_VPROG3A_VSEL 0x6eca > +#define CHT_WC_VPROG3B_VSEL 0x6ecb > +#define CHT_WC_VPROG4A_VSEL 0x6ecc > +#define CHT_WC_VPROG4B_VSEL 0x6ecd > +#define CHT_WC_VPROG4C_VSEL 0x6ece > +#define CHT_WC_VPROG4D_VSEL 0x6ecf > +#define CHT_WC_VPROG5A_VSEL 0x6ed0 > +#define CHT_WC_VPROG5B_VSEL 0x6ed1 > +#define CHT_WC_VPROG6A_VSEL 0x6ed2 > +#define CHT_WC_VPROG6B_VSEL 0x6ed3 > + > + > +/* number of voltage variations exposed */ > + > +/* buck boost regulators */ > +#define CHT_WC_V3P3A_VRANGE 8 > +/* buck regulators */ > +#define CHT_WC_V1P8A_VRANGE 256 > +#define CHT_WC_V1P05A_VRANGE 256 > +#define CHT_WC_VDDQ_VRANGE 120 > +/* boot regulators */ > +/* ldo regulators */ > +#define CHT_WC_VPROG1A_VRANGE 53 > +#define CHT_WC_VPROG1B_VRANGE 53 > +#define CHT_WC_VPROG1F_VRANGE 53 > +#define CHT_WC_V1P8SX_VRANGE 53 > +#define CHT_WC_V1P2SX_VRANGE 53 > +#define CHT_WC_V1P2A_VRANGE 53 > +#define CHT_WC_VSDIO_VRANGE 53 > +#define CHT_WC_V2P8SX_VRANGE 53 > +#define CHT_WC_V3P3SD_VRANGE 53 > +#define CHT_WC_VPROG2D_VRANGE 53 > +#define CHT_WC_VPROG3A_VRANGE 53 > +#define CHT_WC_VPROG3B_VRANGE 53 > +#define CHT_WC_VPROG4A_VRANGE 53 > +#define CHT_WC_VPROG4B_VRANGE 53 > +#define CHT_WC_VPROG4C_VRANGE 53 > +#define CHT_WC_VPROG4D_VRANGE 53 > +#define CHT_WC_VPROG5A_VRANGE 53 > +#define CHT_WC_VPROG5B_VRANGE 53 > +#define CHT_WC_VPROG6A_VRANGE 53 > +#define CHT_WC_VPROG6B_VRANGE 53 > + > + > +/* voltage tables */ > +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE], > + CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE], > + CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE], > + CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE], > + CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE], > + CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE], > + CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE], > + CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE], > + CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE], > + CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE], > + CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE], > + CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE], > + CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE], > + CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE], > + CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE], > + CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE], > + CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE], > + CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE], > + CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE], > + CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE], > + CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE], > + CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE], > + CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE]; > + > +/* > + * VSDIO regulator description, voltage tables, consumers and constraints > + */ > +/* > + * The VSDIO regulator should only support 1.8V and 3.3V. All other > + * voltages are invalid for sd card, so disable them here. > + */ > +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = { > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 3300000, 0, 0 > +}; > + > +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO > + * regulator. This is the only interface that requires this source on Cherry > + * Trail to operate with UHS-I cards. > + */ > +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = { > + REGULATOR_SUPPLY("vqmmc", "80860F14:02"), > +}; > + > +static struct regulator_init_data vqmmc_init_data = { > + .constraints = { > + .min_uV = 1800000, > + .max_uV = 3300000, > + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE | > + REGULATOR_CHANGE_STATUS, > + .valid_modes_mask = REGULATOR_MODE_NORMAL, > + .settling_time = 20000, > + }, > + .num_consumer_supplies = ARRAY_SIZE(cht_wc_vqmmc_supply_consumer), > + .consumer_supplies = cht_wc_vqmmc_supply_consumer, > +}; > + > +static int cht_wc_regulator_enable(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + > + int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg, > + pmic_info->vctl_mask, pmic_info->reg_enbl_mask); > + if (ret < 0) > + dev_err(&rdev->dev, "Failed to enable regulator %s: %d\n", > + pmic_info->desc.name, ret); > + > + return ret; > +} > + > +static int cht_wc_regulator_disable(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + > + int ret = regmap_update_bits(rdev->regmap, pmic_info->vctl_reg, > + pmic_info->vctl_mask, pmic_info->reg_dsbl_mask); > + if (ret < 0) > + dev_err(&rdev->dev, "Failed to disable regulator %s: %d\n", > + pmic_info->desc.name, ret); > + > + return ret; > +} > + > +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + int rval; > + > + int ret = regmap_read(rdev->regmap, pmic_info->vctl_reg, &rval); > + > + if (ret < 0) { > + dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n", > + pmic_info->vsel_reg, ret); > + return ret; > + } > + > + rval &= pmic_info->vctl_mask; > + > + return rval & pmic_info->reg_enbl_mask; > +} > + > +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + int rval; > + > + int ret = regmap_read(rdev->regmap, pmic_info->vsel_reg, &rval); > + > + if (ret < 0) { > + dev_err(&rdev->dev, "Register 0x%04x read failed: %d\n", > + pmic_info->vsel_reg, ret); > + return ret; > + } > + > + return (rval & pmic_info->vsel_mask) - pmic_info->start; > +} > + > +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev, > + unsigned int selector) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + > + int ret = regmap_update_bits(rdev->regmap, pmic_info->vsel_reg, > + pmic_info->vsel_mask, selector + pmic_info->start); > + if (ret < 0) > + dev_err(&rdev->dev, "Failed to set voltage for regulator %s: %d\n", > + pmic_info->desc.name, ret); > + > + return ret; > +} > + > +/* regulator ops */ > +static struct regulator_ops wcove_regulator_ops = { > + .enable = cht_wc_regulator_enable, > + .disable = cht_wc_regulator_disable, > + .is_enabled = cht_wc_regulator_is_enabled, > + .get_voltage_sel = cht_wc_regulator_read_voltage_sel, > + .set_voltage_sel = cht_wc_regulator_set_voltage_sel, > + .list_voltage = regulator_list_voltage_table, > +}; > + > +#define CHT_WC_REGULATOR(_id, minv, maxv, strt, vselmsk, vscale, \ > + vctlmsk, enbl, dsbl, rt_flag, _init_data) \ > +{\ > + .desc = {\ > + .name = ""#_id,\ > + .ops = &wcove_regulator_ops,\ > + .type = REGULATOR_VOLTAGE,\ > + .id = CHT_WC_REGULATOR_##_id,\ > + .owner = THIS_MODULE,\ > + },\ > + .rdev = NULL, \ > + .init_data = (_init_data), \ > + .vctl_reg = CHT_WC_##_id##_CTRL,\ > + .vsel_reg = CHT_WC_##_id##_VSEL,\ > + .min_mV = (minv),\ > + .max_mV = (maxv),\ > + .start = (strt),\ > + .vsel_mask = (vselmsk),\ > + .scale = (vscale),\ > + .nvolts = CHT_WC_##_id##_VRANGE,\ > + .vctl_mask = (vctlmsk),\ > + .reg_enbl_mask = (enbl),\ > + .reg_dsbl_mask = (dsbl),\ > + .vtable = CHT_WC_##_id##_VSEL_TABLE,\ > + .runtime_table = (rt_flag),\ > +} > + > +/* Regulator descriptions */ > +static struct ch_wc_regulator_info regulators_info[] = { > + CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07, 50, 0x01, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V1P8A, 250, 2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V1P05A, 250, 2100, 0x00, 0xff, 10, 0x01, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VDDQ, 250, 1440, 0x00, 0x7f, 10, 0x01, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V1P8SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V1P2A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V1P2SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(V2P8SX, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VSDIO, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, false, &vqmmc_init_data), > + CHT_WC_REGULATOR(V3P3SD, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG1A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG1B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG1F, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG2D, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG3A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG3B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG4A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG4B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG4C, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG4D, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG5A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG5B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG6A, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > + CHT_WC_REGULATOR(VPROG6B, 800, 3400, 0x0b, 0x3f, 50, 0x07, 0x01, 0x0, true, NULL), > +}; > + > +static inline struct ch_wc_regulator_info *cht_wc_find_regulator_info(int id) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(regulators_info); i++) { > + if (regulators_info[i].desc.id == id) > + return ®ulators_info[i]; > + } > + return NULL; > +} > + > +static void initialize_vtable(struct ch_wc_regulator_info *reg_info) > +{ > + unsigned int i, volt; > + > + if (reg_info->runtime_table == true) { > + for (i = 0; i < reg_info->nvolts; i++) { > + volt = reg_info->min_mV + (i * reg_info->scale); > + if (volt < reg_info->min_mV) > + volt = reg_info->min_mV; > + if (volt > reg_info->max_mV) > + volt = reg_info->max_mV; > + /* set value in uV */ > + reg_info->vtable[i] = volt*1000; > + } > + } > + reg_info->desc.volt_table = reg_info->vtable; > + reg_info->desc.n_voltages = reg_info->nvolts; > +} > + > +static int cht_wc_regulator_probe(struct platform_device *pdev) > +{ > + struct ch_wc_regulator_info *regulator; > + struct regulator_config config = { }; > + > + struct ch_wc_regulator_info *reg_info = > + cht_wc_find_regulator_info(pdev->id); > + > + if (reg_info == NULL) > + return -EINVAL; > + > + regulator = devm_kzalloc(&pdev->dev, sizeof(*regulator), GFP_KERNEL); > + if (!regulator) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, regulator); > + > + initialize_vtable(reg_info); > + > + config.dev = &pdev->dev; > + config.driver_data = reg_info; > + config.init_data = reg_info->init_data; > + > + regulator->rdev = regulator_register(®_info->desc, &config); > + > + if (IS_ERR(regulator->rdev)) { > + dev_err(&pdev->dev, "failed to register regulator as %s\n", > + reg_info->desc.name); > + return PTR_ERR(regulator->rdev); > + } > + > + dev_dbg(&pdev->dev, "registered Whiskey Cove regulator as %s\n", > + dev_name(®ulator->rdev->dev)); > + > + return 0; > +} > + > +static int cht_wc_regulator_remove(struct platform_device *pdev) > +{ > + regulator_unregister(platform_get_drvdata(pdev)); > + return 0; > +} > + > +static const struct platform_device_id cht_wc_regulator_id_table[] = { > + { > + .name = "cht_wcove_regulator" > + }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(platform, wcove_regulator_id_table); > + > +static struct platform_driver cht_wc_regulator_driver = { > + .driver = { > + .name = "cht_wcove_regulator", > + .owner = THIS_MODULE, > + }, > + .probe = cht_wc_regulator_probe, > + .remove = cht_wc_regulator_remove, > + .id_table = cht_wc_regulator_id_table, > +}; > + > +static int __init cht_wc_regulator_init(void) > +{ > + return platform_driver_register(&cht_wc_regulator_driver); > +} > +subsys_initcall_sync(cht_wc_regulator_init); > + > +static void __exit cht_wc_regulator_exit(void) > +{ > + platform_driver_unregister(&cht_wc_regulator_driver); > +} > +module_exit(cht_wc_regulator_exit); > + > +MODULE_ALIAS("platform:cht-wc-regulator"); > +MODULE_DESCRIPTION("Intel Cherry Trail Whiskey Cove Regulator driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_AUTHOR("Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com"); > +MODULE_AUTHOR("Nitheesh K L <nitheesh.k.l@intel.com"); > diff --git a/include/linux/regulator/intel-cht-wc-regulator.h b/include/linux/regulator/intel-cht-wc-regulator.h > new file mode 100644 > index 000000000000..0f6b3be11fea > --- /dev/null > +++ b/include/linux/regulator/intel-cht-wc-regulator.h > @@ -0,0 +1,64 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * intel-cht-wc-regulator.h - Cherry Trail Whiskey Cove PMIC regulator > + * > + * Copyright (c) 2019, Leica Geosystems AG > + * > + * Base on the non-upstreamed version from Intel, which was > + * introduced for Intel Aero board. > + * Copyright (c) 2014, Intel Corporation. > + */ > + > +#ifndef __LINUX_REGULATOR_INTEL_CHT_WC_H_ > +#define __LINUX_REGULATOR_INTEL_CHT_WC_H_ > + > +#include <linux/regulator/driver.h> > + > +enum { > + CHT_WC_REGULATOR_V1P8A = 1, > + CHT_WC_REGULATOR_V1P05A, > + CHT_WC_REGULATOR_V1P15, > + CHT_WC_REGULATOR_VDDQ, > + CHT_WC_REGULATOR_V3P3A, > + CHT_WC_REGULATOR_VPROG1A, > + CHT_WC_REGULATOR_VPROG1B, > + CHT_WC_REGULATOR_VPROG1F, > + CHT_WC_REGULATOR_V1P8SX, > + CHT_WC_REGULATOR_V1P2SX, > + CHT_WC_REGULATOR_V1P2A, > + CHT_WC_REGULATOR_VSDIO, > + CHT_WC_REGULATOR_V2P8SX, > + CHT_WC_REGULATOR_V3P3SD, > + CHT_WC_REGULATOR_VPROG2D, > + CHT_WC_REGULATOR_VPROG3A, > + CHT_WC_REGULATOR_VPROG3B, > + CHT_WC_REGULATOR_VPROG4A, > + CHT_WC_REGULATOR_VPROG4B, > + CHT_WC_REGULATOR_VPROG4C, > + CHT_WC_REGULATOR_VPROG4D, > + CHT_WC_REGULATOR_VPROG5A, > + CHT_WC_REGULATOR_VPROG5B, > + CHT_WC_REGULATOR_VPROG6A, > + CHT_WC_REGULATOR_VPROG6B, > +}; > + > +struct ch_wc_regulator_info { > + struct regulator_desc desc; > + struct regulator_dev *rdev; > + struct regulator_init_data *init_data; > + unsigned int vctl_reg; > + unsigned int vsel_reg; > + unsigned int min_mV; > + unsigned int max_mV; > + unsigned int start; > + unsigned int vsel_mask; > + unsigned int scale; > + unsigned int nvolts; > + unsigned int vctl_mask; > + unsigned int reg_enbl_mask; > + unsigned int reg_dsbl_mask; > + unsigned int *vtable; > + bool runtime_table; > +}; > + > +#endif /* __LINUX_REGULATOR_INTEL_CHT_WC_H_ */ > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 8:01 ` Andy Shevchenko @ 2019-10-25 8:58 ` Andrey Zhizhikin 2019-10-25 9:14 ` Andy Shevchenko 0 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 8:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Adrian Hunter, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which > > supplies various voltage rails. > > > > This initial version supports only vsdio, which is required to source > > vqmmc for sd card interface. > > This patch has some style issues. I will wait for Adrian and Hans to comment on > the approach as a whole and then we will see how to improve the rest. Agreed, styling issues are coming from definition of CHT_WC_REGULATOR elements in regulators_info array, and they are mainly related to 80 characters per line. I decided to leave it like this since it is more readable. But if the 80 characters rule is to be enforced here - I can go with something like this for every element: CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07,\ 50, 0x01, 0x01, 0x0, true, NULL), Let's wait for other people's comments here and I can then come up with v2 of this patch. > > -- > With Best Regards, > Andy Shevchenko > > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 8:58 ` Andrey Zhizhikin @ 2019-10-25 9:14 ` Andy Shevchenko 2019-10-25 9:31 ` Andrey Zhizhikin 0 siblings, 1 reply; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 9:14 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Hans de Goede, Adrian Hunter, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote: > On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which > > > supplies various voltage rails. > > > > > > This initial version supports only vsdio, which is required to source > > > vqmmc for sd card interface. > > > > This patch has some style issues. I will wait for Adrian and Hans to comment on > > the approach as a whole and then we will see how to improve the rest. > > Agreed, styling issues are coming from definition of CHT_WC_REGULATOR > elements in regulators_info array, and they are mainly related to 80 > characters per line. I decided to leave it like this since it is more > readable. But if the 80 characters rule is to be enforced here - I can > go with something like this for every element: > CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07,\ > 50, 0x01, 0x01, 0x0, true, NULL), No, I'm not talking about these. Actually I hate this 80 limit in 21st century. But let's not discuss it right now. > Let's wait for other people's comments here and I can then come up > with v2 of this patch. Exactly! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 9:14 ` Andy Shevchenko @ 2019-10-25 9:31 ` Andrey Zhizhikin 2019-10-25 10:47 ` Andy Shevchenko 0 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 9:31 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Adrian Hunter, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 11:14 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote: > > On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which > > > > supplies various voltage rails. > > > > > > > > This initial version supports only vsdio, which is required to source > > > > vqmmc for sd card interface. > > > > > > This patch has some style issues. I will wait for Adrian and Hans to comment on > > > the approach as a whole and then we will see how to improve the rest. > > > > Agreed, styling issues are coming from definition of CHT_WC_REGULATOR > > elements in regulators_info array, and they are mainly related to 80 > > characters per line. I decided to leave it like this since it is more > > readable. But if the 80 characters rule is to be enforced here - I can > > go with something like this for every element: > > CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07,\ > > 50, 0x01, 0x01, 0x0, true, NULL), > > No, I'm not talking about these. Actually I hate this 80 limit in 21st century. > But let's not discuss it right now. I just ran a checkpatch and I think I know what you're referring to: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? In this case I really have to wait what others have to say, since I do not know who and how the maintainer would be assigned. > > > Let's wait for other people's comments here and I can then come up > > with v2 of this patch. > > Exactly! > > -- > With Best Regards, > Andy Shevchenko > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 9:31 ` Andrey Zhizhikin @ 2019-10-25 10:47 ` Andy Shevchenko 0 siblings, 0 replies; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 10:47 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Hans de Goede, Adrian Hunter, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 11:31:50AM +0200, Andrey Zhizhikin wrote: > On Fri, Oct 25, 2019 at 11:14 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 25, 2019 at 10:58:05AM +0200, Andrey Zhizhikin wrote: > > > On Fri, Oct 25, 2019 at 10:01 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > > > Add regulator driver for Intel Cherry Trail Whiskey Cove PMIC, which > > > > > supplies various voltage rails. > > > > > > > > > > This initial version supports only vsdio, which is required to source > > > > > vqmmc for sd card interface. > > > > > > > > This patch has some style issues. I will wait for Adrian and Hans to comment on > > > > the approach as a whole and then we will see how to improve the rest. > > > > > > Agreed, styling issues are coming from definition of CHT_WC_REGULATOR > > > elements in regulators_info array, and they are mainly related to 80 > > > characters per line. I decided to leave it like this since it is more > > > readable. But if the 80 characters rule is to be enforced here - I can > > > go with something like this for every element: > > > CHT_WC_REGULATOR(V3P3A, 3000, 3350, 0x00, 0x07,\ > > > 50, 0x01, 0x01, 0x0, true, NULL), > > > > No, I'm not talking about these. Actually I hate this 80 limit in 21st century. > > But let's not discuss it right now. > > I just ran a checkpatch and I think I know what you're referring to: > WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? > > In this case I really have to wait what others have to say, since I do > not know who and how the maintainer would be assigned. Neither this :-) Whoever it's an interesting warning from checkpatch. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin 2019-10-25 8:01 ` Andy Shevchenko @ 2019-10-25 12:17 ` Mark Brown 2019-10-25 15:26 ` Andrey Zhizhikin 1 sibling, 1 reply; 35+ messages in thread From: Mark Brown @ 2019-10-25 12:17 UTC (permalink / raw) To: Andrey Zhizhikin Cc: lgirdwood, andriy.shevchenko, lee.jones, linux-kernel, Andrey Zhizhikin [-- Attachment #1: Type: text/plain, Size: 6522 bytes --] On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > + Only select this regulator driver if the MFD part is selected > + in the Kernel configuration, it is meant to be operable as a cell. This i what Kconfig dependencies are for. > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * intel-cht-wc-regulator.c - CherryTrail regulator driver > + * Please use C++ style for the entire comment so things look more consistent. > +#define CHT_WC_VPROG1A_VRANGE 53 > +#define CHT_WC_VPROG1B_VRANGE 53 > +#define CHT_WC_VPROG1F_VRANGE 53 > +#define CHT_WC_V1P8SX_VRANGE 53 > +#define CHT_WC_V1P2SX_VRANGE 53 > +#define CHT_WC_V1P2A_VRANGE 53 > +#define CHT_WC_VSDIO_VRANGE 53 > +#define CHT_WC_V2P8SX_VRANGE 53 > +#define CHT_WC_V3P3SD_VRANGE 53 > +#define CHT_WC_VPROG2D_VRANGE 53 > +#define CHT_WC_VPROG3A_VRANGE 53 > +#define CHT_WC_VPROG3B_VRANGE 53 > +#define CHT_WC_VPROG4A_VRANGE 53 > +#define CHT_WC_VPROG4B_VRANGE 53 > +#define CHT_WC_VPROG4C_VRANGE 53 > +#define CHT_WC_VPROG4D_VRANGE 53 > +#define CHT_WC_VPROG5A_VRANGE 53 > +#define CHT_WC_VPROG5B_VRANGE 53 > +#define CHT_WC_VPROG6A_VRANGE 53 > +#define CHT_WC_VPROG6B_VRANGE 53 These appear to be identical - is this not just a bunch of instantiations of the same IPs > +/* voltage tables */ > +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE], > + CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE], > + CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE], > + CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE], > + CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE], > + CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE], > + CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE], > + CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE], > + CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE], > + CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE], > + CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE], > + CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE], > + CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE], > + CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE], > + CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE], > + CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE], > + CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE], > + CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE], > + CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE], > + CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE], > + CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE], > + CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE], > + CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE]; Please write a series of individual variable declarations, the above way of writing things is very unusual and hence confusing to read. Though like I say it looks like the tables are mostly identical so you probably only need a much smaller number of tables, one per IP. > +/* > + * The VSDIO regulator should only support 1.8V and 3.3V. All other > + * voltages are invalid for sd card, so disable them here. > + */ > +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = { > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > + 0, 0, 3300000, 0, 0 > +}; Is this really a limitation of the *regulator* or is it a limitation of the consumer? The combination of the way this is written and the register layout makes it look like it's a consumer limitation in which case leave it up to the consumer to figure out what constraints it has. > +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO > + * regulator. This is the only interface that requires this source on Cherry > + * Trail to operate with UHS-I cards. > + */ > +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = { > + REGULATOR_SUPPLY("vqmmc", "80860F14:02"), > +}; This looks like board specific configuration so should be defined in the board. > +static struct regulator_init_data vqmmc_init_data = { > + .constraints = { > + .min_uV = 1800000, > + .max_uV = 3300000, > + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE | > + REGULATOR_CHANGE_STATUS, > + .valid_modes_mask = REGULATOR_MODE_NORMAL, > + .settling_time = 20000, > + }, > + .num_consumer_supplies = ARRAY_SIZE(cht_wc_vqmmc_supply_consumer), > + .consumer_supplies = cht_wc_vqmmc_supply_consumer, > +}; This *definitely* appears to be board specific configuration and should be defined for the board. > +static int cht_wc_regulator_enable(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); regulator_enable_regmap() > +static int cht_wc_regulator_disable(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); regulator_disable_regmap() > +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev) > +{ > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > + int rval; This looks like it's a get_status() operation (reading back the actual staus of the regulator rather than if we asked for it to be enabled or disabled). > +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev) > +{ regulator_get_voltage_sel_regmap() > +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev, > + unsigned int selector) regulator_set_voltage_sel_regmap() > +static void initialize_vtable(struct ch_wc_regulator_info *reg_info) > +{ > + unsigned int i, volt; > + > + if (reg_info->runtime_table == true) { > + for (i = 0; i < reg_info->nvolts; i++) { > + volt = reg_info->min_mV + (i * reg_info->scale); > + if (volt < reg_info->min_mV) > + volt = reg_info->min_mV; > + if (volt > reg_info->max_mV) > + volt = reg_info->max_mV; > + /* set value in uV */ > + reg_info->vtable[i] = volt*1000; > + } > + } > + reg_info->desc.volt_table = reg_info->vtable; > + reg_info->desc.n_voltages = reg_info->nvolts; > +} This looks like you've got a linear range so you should be using regulator_map_voltage_linear and regulator_list_voltage_linear. > + regulator->rdev = regulator_register(®_info->desc, &config); devm_regulator_register() > +static int __init cht_wc_regulator_init(void) > +{ > + return platform_driver_register(&cht_wc_regulator_driver); > +} > +subsys_initcall_sync(cht_wc_regulator_init); Why subsys_initcall() and not just module_platform_driver? Deferred probe should work fine for regulators. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 12:17 ` Mark Brown @ 2019-10-25 15:26 ` Andrey Zhizhikin 2019-10-25 16:02 ` Andy Shevchenko 0 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 15:26 UTC (permalink / raw) To: Mark Brown Cc: lgirdwood, Andy Shevchenko, Lee Jones, linux-kernel, Andrey Zhizhikin, Hans de Goede Something went wrong (mainly with me pressing wrong buttons), so I managed to pull the conversation off the list. I'd duplicate my answers here with comments from Mark, first being: On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote: > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > Please don't take things off-list unless there is a really strong reason > to do so. Sending things to the list ensures that everyone gets a > chance to read and comment on things. (From some of the things > in your mail I think this might've been unintentional.) > Sorry for mess, sometimes it happens but I try not to create it... On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > + Only select this regulator driver if the MFD part is selected > > + in the Kernel configuration, it is meant to be operable as a cell. > > This i what Kconfig dependencies are for. True, would re-phrase to description. > > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver > > + * > > Please use C++ style for the entire comment so things look more > consistent. This is what I'm puzzled about - which style to use for the file header since the introduction of SPDX and a rule that it should be "C++ style" commented for source files and "C style" for header files. After this introduction, should the more-or-less standard header be also done in C++ style? I saw different source files are doing different things... But all-in-all I would follow you advise here with converting entire block to C++. [Mark]: The only thing SPDX cares about is the first line, the making the rest of the block a C++ one is mostly a preference I have. Got it, would be done! > > > +#define CHT_WC_VPROG1A_VRANGE 53 > > +#define CHT_WC_VPROG1B_VRANGE 53 > > +#define CHT_WC_VPROG1F_VRANGE 53 > > +#define CHT_WC_V1P8SX_VRANGE 53 > > +#define CHT_WC_V1P2SX_VRANGE 53 > > +#define CHT_WC_V1P2A_VRANGE 53 > > +#define CHT_WC_VSDIO_VRANGE 53 > > +#define CHT_WC_V2P8SX_VRANGE 53 > > +#define CHT_WC_V3P3SD_VRANGE 53 > > +#define CHT_WC_VPROG2D_VRANGE 53 > > +#define CHT_WC_VPROG3A_VRANGE 53 > > +#define CHT_WC_VPROG3B_VRANGE 53 > > +#define CHT_WC_VPROG4A_VRANGE 53 > > +#define CHT_WC_VPROG4B_VRANGE 53 > > +#define CHT_WC_VPROG4C_VRANGE 53 > > +#define CHT_WC_VPROG4D_VRANGE 53 > > +#define CHT_WC_VPROG5A_VRANGE 53 > > +#define CHT_WC_VPROG5B_VRANGE 53 > > +#define CHT_WC_VPROG6A_VRANGE 53 > > +#define CHT_WC_VPROG6B_VRANGE 53 > > These appear to be identical - is this not just a bunch of > instantiations of the same IPs That was done for "macro convenience". I would definitely re-work this part. > > > +/* voltage tables */ > > +static unsigned int CHT_WC_V3P3A_VSEL_TABLE[CHT_WC_V3P3A_VRANGE], > > + CHT_WC_V1P8A_VSEL_TABLE[CHT_WC_V1P8A_VRANGE], > > + CHT_WC_V1P05A_VSEL_TABLE[CHT_WC_V1P05A_VRANGE], > > + CHT_WC_VDDQ_VSEL_TABLE[CHT_WC_VDDQ_VRANGE], > > + CHT_WC_V1P8SX_VSEL_TABLE[CHT_WC_V1P8SX_VRANGE], > > + CHT_WC_V1P2SX_VSEL_TABLE[CHT_WC_V1P2SX_VRANGE], > > + CHT_WC_V1P2A_VSEL_TABLE[CHT_WC_V1P2A_VRANGE], > > + CHT_WC_V2P8SX_VSEL_TABLE[CHT_WC_V2P8SX_VRANGE], > > + CHT_WC_V3P3SD_VSEL_TABLE[CHT_WC_V3P3SD_VRANGE], > > + CHT_WC_VPROG1A_VSEL_TABLE[CHT_WC_VPROG1A_VRANGE], > > + CHT_WC_VPROG1B_VSEL_TABLE[CHT_WC_VPROG1B_VRANGE], > > + CHT_WC_VPROG1F_VSEL_TABLE[CHT_WC_VPROG1F_VRANGE], > > + CHT_WC_VPROG2D_VSEL_TABLE[CHT_WC_VPROG2D_VRANGE], > > + CHT_WC_VPROG3A_VSEL_TABLE[CHT_WC_VPROG3A_VRANGE], > > + CHT_WC_VPROG3B_VSEL_TABLE[CHT_WC_VPROG3B_VRANGE], > > + CHT_WC_VPROG4A_VSEL_TABLE[CHT_WC_VPROG4A_VRANGE], > > + CHT_WC_VPROG4B_VSEL_TABLE[CHT_WC_VPROG4B_VRANGE], > > + CHT_WC_VPROG4C_VSEL_TABLE[CHT_WC_VPROG4C_VRANGE], > > + CHT_WC_VPROG4D_VSEL_TABLE[CHT_WC_VPROG4D_VRANGE], > > + CHT_WC_VPROG5A_VSEL_TABLE[CHT_WC_VPROG5A_VRANGE], > > + CHT_WC_VPROG5B_VSEL_TABLE[CHT_WC_VPROG5B_VRANGE], > > + CHT_WC_VPROG6A_VSEL_TABLE[CHT_WC_VPROG6A_VRANGE], > > + CHT_WC_VPROG6B_VSEL_TABLE[CHT_WC_VPROG6B_VRANGE]; > > Please write a series of individual variable declarations, the > above way of writing things is very unusual and hence confusing > to read. Though like I say it looks like the tables are mostly > identical so you probably only need a much smaller number of > tables, one per IP. Agreed, would be re-worked as well. > > > +/* > > + * The VSDIO regulator should only support 1.8V and 3.3V. All other > > + * voltages are invalid for sd card, so disable them here. > > + */ > > +static unsigned int CHT_WC_VSDIO_VSEL_TABLE[CHT_WC_VSDIO_VRANGE] = { > > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > > + 0, 0, 0, 0, 0, 0, 0, 0, 1800000, 0, 0, 0, > > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > > + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, > > + 0, 0, 3300000, 0, 0 > > +}; > > Is this really a limitation of the *regulator* or is it a > limitation of the consumer? The combination of the way this is > written and the register layout makes it look like it's a > consumer limitation in which case leave it up to the consumer to > figure out what constraints it has. This is a tricky point. Since there is no datasheet available from Intel on this IP - I went with a safe option of taking this part from original Intel patch, which they did for Aero board as the range was described there in exactly this way. I am totally unsure if the regulator does not provide all voltages, or this limit is purely artificial here. Nevertheless, according to SD specification this regulator should only operate with only those two voltage levels: 1v8 and 3v3 and consumers (mmc) should be well aware of this. [Mark]: There's always the possibility that someone builds a board with something else attached to the regulator if they don't want to use SD cards for example. I would re-work this to map all voltage ranges here properly, so there would be no further confusions. [Mark]: OK > > > +/* List the SD card interface as a consumer of vqmmc voltage source from VSDIO > > + * regulator. This is the only interface that requires this source on Cherry > > + * Trail to operate with UHS-I cards. > > + */ > > +static struct regulator_consumer_supply cht_wc_vqmmc_supply_consumer[] = { > > + REGULATOR_SUPPLY("vqmmc", "80860F14:02"), > > +}; > > This looks like board specific configuration so should be defined > in the board. > > > +static struct regulator_init_data vqmmc_init_data = { > > + .constraints = { > > + .min_uV = 1800000, > > + .max_uV = 3300000, > > + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE | > > + REGULATOR_CHANGE_STATUS, > > + .valid_modes_mask = REGULATOR_MODE_NORMAL, > > + .settling_time = 20000, > > + }, > > + .num_consumer_supplies = ARRAY_SIZE(cht_wc_vqmmc_supply_consumer), > > + .consumer_supplies = cht_wc_vqmmc_supply_consumer, > > +}; > > This *definitely* appears to be board specific configuration and > should be defined for the board. Above those two points above: I totally agree this is not the regulator configuration but rather a specific board one. The only thing I was not able to locate is a correct board file to put this into. Maybe you or Hans can guide me here on where to have this code as best? [Mark]: I *think* drivers/platform/x86 might be what you're looking for but I'm not super familiar with x86. There's also arch/x86/platform but I think they're also trying to push things out of arch/. > > > +static int cht_wc_regulator_enable(struct regulator_dev *rdev) > > +{ > > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > > regulator_enable_regmap() Agreed, would re-work this. > > > +static int cht_wc_regulator_disable(struct regulator_dev *rdev) > > +{ > > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > > regulator_disable_regmap() ... and this as well. > > > +static int cht_wc_regulator_is_enabled(struct regulator_dev *rdev) > > +{ > > + struct ch_wc_regulator_info *pmic_info = rdev_get_drvdata(rdev); > > + int rval; > > This looks like it's a get_status() operation (reading back the > actual staus of the regulator rather than if we asked for it to > be enabled or disabled). Yes, this is actually a get_status. Would re-work it. > > > +static int cht_wc_regulator_read_voltage_sel(struct regulator_dev *rdev) > > +{ > > regulator_get_voltage_sel_regmap() > > > +static int cht_wc_regulator_set_voltage_sel(struct regulator_dev *rdev, > > + unsigned int selector) > > regulator_set_voltage_sel_regmap() I'd revise the whole regulator core API usage from comments above, as I see now the driver code could be simplified dramatically. > > > +static void initialize_vtable(struct ch_wc_regulator_info *reg_info) > > +{ > > + unsigned int i, volt; > > + > > + if (reg_info->runtime_table == true) { > > + for (i = 0; i < reg_info->nvolts; i++) { > > + volt = reg_info->min_mV + (i * reg_info->scale); > > + if (volt < reg_info->min_mV) > > + volt = reg_info->min_mV; > > + if (volt > reg_info->max_mV) > > + volt = reg_info->max_mV; > > + /* set value in uV */ > > + reg_info->vtable[i] = volt*1000; > > + } > > + } > > + reg_info->desc.volt_table = reg_info->vtable; > > + reg_info->desc.n_voltages = reg_info->nvolts; > > +} > > This looks like you've got a linear range so you should be using > regulator_map_voltage_linear and regulator_list_voltage_linear. Yes, you're absolutely right, I've missed this one out. Would use a proper API here as well. > > > + regulator->rdev = regulator_register(®_info->desc, &config); > > devm_regulator_register() Yeap, and consecutively devm_regulator_unregister() in cht_wc_regulator_remove call. [Mark]: With devm_ you don't even need to explicitly unregister so the entire remove function can go. > > > +static int __init cht_wc_regulator_init(void) > > +{ > > + return platform_driver_register(&cht_wc_regulator_driver); > > +} > > +subsys_initcall_sync(cht_wc_regulator_init); > > Why subsys_initcall() and not just module_platform_driver? > Deferred probe should work fine for regulators. Got it, would correct this as well. Overall, I would definitely work on a v2 here for both the regulator and mfd cell part. If anyone has additional comment - please share them here so I would gladly take them in! Thanks a lot to all of you for reviewing this set! -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 15:26 ` Andrey Zhizhikin @ 2019-10-25 16:02 ` Andy Shevchenko 2019-10-25 16:21 ` Andrey Zhizhikin 0 siblings, 1 reply; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 16:02 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Mark Brown, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin, Hans de Goede On Fri, Oct 25, 2019 at 05:26:56PM +0200, Andrey Zhizhikin wrote: > On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote: > > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > > Please don't take things off-list unless there is a really strong reason > > to do so. Sending things to the list ensures that everyone gets a > > chance to read and comment on things. (From some of the things > > in your mail I think this might've been unintentional.) > Sorry for mess, sometimes it happens but I try not to create it... Script nodded... :) > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver > > > + * > > > > Please use C++ style for the entire comment so things look more > > consistent. > > This is what I'm puzzled about - which style to use for the file > header since the introduction of SPDX and a rule that it should be > "C++ style" commented for source files and "C style" for header files. > After this introduction, should the more-or-less standard header be > also done in C++ style? I saw different source files are doing > different things... But all-in-all I would follow you advise here with > converting entire block to C++. > > [Mark]: The only thing SPDX cares about is the first line, the making > the rest of the block a C++ one is mostly a preference I have. > > Got it, would be done! Also remove the file name(s) from file(s). If we would ever rename the file, its name will be additional churn inside file (or often being forgotten). > > Is this really a limitation of the *regulator* or is it a > > limitation of the consumer? The combination of the way this is > > written and the register layout makes it look like it's a > > consumer limitation in which case leave it up to the consumer to > > figure out what constraints it has. > > This is a tricky point. Since there is no datasheet available from Key word "publicly". I may look for it some like in November and perhaps be able to answer to (some) questions. > Intel on this IP - I went with a safe option of taking this part from > original Intel patch, which they did for Aero board as the range was > described there in exactly this way. > > This *definitely* appears to be board specific configuration and > > should be defined for the board. > > Above those two points above: I totally agree this is not the > regulator configuration but rather a specific board one. The only > thing I was not able to locate is a correct board file to put this > into. See below. > Maybe you or Hans can guide me here on where to have this code as best? > > [Mark]: I *think* drivers/platform/x86 might be what you're looking > for but I'm not super familiar with x86. There's also > arch/x86/platform but I think they're also trying to push things out > of arch/. It's more likely drivers/acpi/acpi_lpss.c. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator 2019-10-25 16:02 ` Andy Shevchenko @ 2019-10-25 16:21 ` Andrey Zhizhikin 0 siblings, 0 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 16:21 UTC (permalink / raw) To: Andy Shevchenko Cc: Mark Brown, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin, Hans de Goede On Fri, Oct 25, 2019 at 6:02 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 25, 2019 at 05:26:56PM +0200, Andrey Zhizhikin wrote: > > On Fri, Oct 25, 2019 at 4:43 PM Mark Brown <broonie@kernel.org> wrote: > > > On Fri, Oct 25, 2019 at 03:55:17PM +0200, Andrey Zhizhikin wrote: > > > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > > > > Please don't take things off-list unless there is a really strong reason > > > to do so. Sending things to the list ensures that everyone gets a > > > chance to read and comment on things. (From some of the things > > > in your mail I think this might've been unintentional.) > > > Sorry for mess, sometimes it happens but I try not to create it... > > Script nodded... :) Point taken! :) > > > On Fri, Oct 25, 2019 at 2:17 PM Mark Brown <broonie@kernel.org> wrote: > > > On Thu, Oct 24, 2019 at 02:29:38PM +0000, Andrey Zhizhikin wrote: > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * intel-cht-wc-regulator.c - CherryTrail regulator driver > > > > + * > > > > > > Please use C++ style for the entire comment so things look more > > > consistent. > > > > This is what I'm puzzled about - which style to use for the file > > header since the introduction of SPDX and a rule that it should be > > "C++ style" commented for source files and "C style" for header files. > > After this introduction, should the more-or-less standard header be > > also done in C++ style? I saw different source files are doing > > different things... But all-in-all I would follow you advise here with > > converting entire block to C++. > > > > [Mark]: The only thing SPDX cares about is the first line, the making > > the rest of the block a C++ one is mostly a preference I have. > > > > Got it, would be done! > > Also remove the file name(s) from file(s). If we would ever rename the file, > its name will be additional churn inside file (or often being forgotten). OK, would use this pattern for all my future work now. > > > > Is this really a limitation of the *regulator* or is it a > > > limitation of the consumer? The combination of the way this is > > > written and the register layout makes it look like it's a > > > consumer limitation in which case leave it up to the consumer to > > > figure out what constraints it has. > > > > This is a tricky point. Since there is no datasheet available from > > Key word "publicly". > > I may look for it some like in November and perhaps be able to answer to > (some) questions. That would be really great! The main point that needs to be clarified at this moment is about voltages provided by vsdio cell, and if it safe to include all of them all in the vsel table. > > > Intel on this IP - I went with a safe option of taking this part from > > original Intel patch, which they did for Aero board as the range was > > described there in exactly this way. > > > > This *definitely* appears to be board specific configuration and > > > should be defined for the board. > > > > Above those two points above: I totally agree this is not the > > regulator configuration but rather a specific board one. The only > > thing I was not able to locate is a correct board file to put this > > into. > > See below. > > > Maybe you or Hans can guide me here on where to have this code as best? > > > > [Mark]: I *think* drivers/platform/x86 might be what you're looking > > for but I'm not super familiar with x86. There's also > > arch/x86/platform but I think they're also trying to push things out > > of arch/. > > It's more likely drivers/acpi/acpi_lpss.c. Thanks for the hint, I'd definitely would have a look at it! > > -- > With Best Regards, > Andy Shevchenko > > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC 2019-10-24 14:29 [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin @ 2019-10-24 14:29 ` Andrey Zhizhikin 2019-10-25 8:06 ` Andy Shevchenko 2019-10-25 7:53 ` [PATCH 0/2] add regulator driver and mfd cell for Intel " Andy Shevchenko 2 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-24 14:29 UTC (permalink / raw) To: lgirdwood, broonie, andriy.shevchenko, lee.jones, linux-kernel Cc: Andrey Zhizhikin Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to supply various voltage rails. In addition, make the initialization of this mfd driver early enough in order to provide regulator cell to mmc sub-system when it is initialized. Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@leica-geosystems.com> --- drivers/mfd/intel_soc_pmic_chtwc.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c index be84bb2aa837..47c0d9994784 100644 --- a/drivers/mfd/intel_soc_pmic_chtwc.c +++ b/drivers/mfd/intel_soc_pmic_chtwc.c @@ -17,6 +17,7 @@ #include <linux/mfd/core.h> #include <linux/mfd/intel_soc_pmic.h> #include <linux/regmap.h> +#include <linux/regulator/intel-cht-wc-regulator.h> /* PMIC device registers */ #define REG_OFFSET_MASK GENMASK(7, 0) @@ -58,6 +59,11 @@ static struct mfd_cell cht_wc_dev[] = { .name = "cht_wcove_ext_chgr", .num_resources = ARRAY_SIZE(cht_wc_ext_charger_resources), .resources = cht_wc_ext_charger_resources, + }, { + .name = "cht_wcove_regulator", + .id = CHT_WC_REGULATOR_VSDIO + 1, + .num_resources = 0, + .resources = NULL, }, { .name = "cht_wcove_region", }, { .name = "cht_wcove_leds", }, @@ -215,7 +221,7 @@ static const struct acpi_device_id cht_wc_acpi_ids[] = { { } }; -static struct i2c_driver cht_wc_driver = { +static struct i2c_driver cht_wc_i2c_driver = { .driver = { .name = "CHT Whiskey Cove PMIC", .pm = &cht_wc_pm_ops, @@ -225,4 +231,9 @@ static struct i2c_driver cht_wc_driver = { .shutdown = cht_wc_shutdown, .id_table = cht_wc_i2c_id, }; -builtin_i2c_driver(cht_wc_driver); + +static int __init cht_wc_i2c_init(void) +{ + return i2c_add_driver(&cht_wc_i2c_driver); +} +subsys_initcall(cht_wc_i2c_init); -- 2.17.1 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC 2019-10-24 14:29 ` [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin @ 2019-10-25 8:06 ` Andy Shevchenko 2019-10-25 9:16 ` Andrey Zhizhikin 0 siblings, 1 reply; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 8:06 UTC (permalink / raw) To: Andrey Zhizhikin Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote: > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to > supply various voltage rails. > > In addition, make the initialization of this mfd driver early enough in > order to provide regulator cell to mmc sub-system when it is initialized. Doesn't deferred probe mechanism work for you? MMC core returns that error till we have the driver initialized. > + }, { > + .name = "cht_wcove_regulator", > + .id = CHT_WC_REGULATOR_VSDIO + 1, > + .num_resources = 0, > + .resources = NULL, No need to put these. > -static struct i2c_driver cht_wc_driver = { > +static struct i2c_driver cht_wc_i2c_driver = { Renaming is not explained in the commit message. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC 2019-10-25 8:06 ` Andy Shevchenko @ 2019-10-25 9:16 ` Andrey Zhizhikin 2019-10-25 10:49 ` Andy Shevchenko 0 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 9:16 UTC (permalink / raw) To: Andy Shevchenko Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote: > > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to > > supply various voltage rails. > > > > In addition, make the initialization of this mfd driver early enough in > > order to provide regulator cell to mmc sub-system when it is initialized. > > Doesn't deferred probe mechanism work for you? > MMC core returns that error till we have the driver initialized. This would work for mmc sub-system, but my idea was that later when more cells are added to this mfd - it might turn out that we would require an early initialization anyway. So I decided to take an opportunity to adjust it with this patch as well, and this is what I roughly explained in the commit message. When I'm reading it now, exactly this point was not mentioned in commit message at all, and I rather coupled the early init with mmc sub-system, which creates a source of confusion here. I guess if there would be no other objections about early init - I'd go with v2 of this patch, where I would clean-up the point below and adjust the commit description. Thanks a lot for pointing this out! > > > + }, { > > + .name = "cht_wcove_regulator", > > + .id = CHT_WC_REGULATOR_VSDIO + 1, > > > + .num_resources = 0, > > + .resources = NULL, > > No need to put these. Agreed, this was forgotten. Sorry for that. > > > -static struct i2c_driver cht_wc_driver = { > > +static struct i2c_driver cht_wc_i2c_driver = { > > Renaming is not explained in the commit message. True, this point I forgot to mention. Actually, this is tightly coupled with the fact that mfd driver has been moved to an earlier init stage and since it does belong to I2C sub-system (and represented by i2c_device structure) - I decided to make a name sound more logical. > > -- > With Best Regards, > Andy Shevchenko > > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC 2019-10-25 9:16 ` Andrey Zhizhikin @ 2019-10-25 10:49 ` Andy Shevchenko 2019-10-25 12:00 ` Andrey Zhizhikin 0 siblings, 1 reply; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 10:49 UTC (permalink / raw) To: Andrey Zhizhikin Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 11:16:22AM +0200, Andrey Zhizhikin wrote: > On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote: > > > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to > > > supply various voltage rails. > > > > > > In addition, make the initialization of this mfd driver early enough in > > > order to provide regulator cell to mmc sub-system when it is initialized. > > > > Doesn't deferred probe mechanism work for you? > > MMC core returns that error till we have the driver initialized. > > This would work for mmc sub-system, but my idea was that later when > more cells are added to this mfd - it might turn out that we would > require an early initialization anyway. So I decided to take an > opportunity to adjust it with this patch as well, and this is what I > roughly explained in the commit message. When I'm reading it now, > exactly this point was not mentioned in commit message at all, and I > rather coupled the early init with mmc sub-system, which creates a > source of confusion here. I guess if there would be no other > objections about early init - I'd go with v2 of this patch, where I > would clean-up the point below and adjust the commit description. > > Thanks a lot for pointing this out! > > > -static struct i2c_driver cht_wc_driver = { > > > +static struct i2c_driver cht_wc_i2c_driver = { > > > > Renaming is not explained in the commit message. > > True, this point I forgot to mention. Actually, this is tightly > coupled with the fact that mfd driver has been moved to an earlier > init stage and since it does belong to I2C sub-system (and represented > by i2c_device structure) - I decided to make a name sound more > logical. So, seems you have three changes in one patch. Please, split accordingly. In v2 also Cc all stakeholders I mentioned. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC 2019-10-25 10:49 ` Andy Shevchenko @ 2019-10-25 12:00 ` Andrey Zhizhikin 0 siblings, 0 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 12:00 UTC (permalink / raw) To: Andy Shevchenko Cc: lgirdwood, broonie, Lee Jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 12:49 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 25, 2019 at 11:16:22AM +0200, Andrey Zhizhikin wrote: > > On Fri, Oct 25, 2019 at 10:07 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Thu, Oct 24, 2019 at 02:29:39PM +0000, Andrey Zhizhikin wrote: > > > > Add a regulator mfd cell to Whiskey Cove PMIC driver, which is used to > > > > supply various voltage rails. > > > > > > > > In addition, make the initialization of this mfd driver early enough in > > > > order to provide regulator cell to mmc sub-system when it is initialized. > > > > > > Doesn't deferred probe mechanism work for you? > > > MMC core returns that error till we have the driver initialized. > > > > This would work for mmc sub-system, but my idea was that later when > > more cells are added to this mfd - it might turn out that we would > > require an early initialization anyway. So I decided to take an > > opportunity to adjust it with this patch as well, and this is what I > > roughly explained in the commit message. When I'm reading it now, > > exactly this point was not mentioned in commit message at all, and I > > rather coupled the early init with mmc sub-system, which creates a > > source of confusion here. I guess if there would be no other > > objections about early init - I'd go with v2 of this patch, where I > > would clean-up the point below and adjust the commit description. > > > > Thanks a lot for pointing this out! > > > > > -static struct i2c_driver cht_wc_driver = { > > > > +static struct i2c_driver cht_wc_i2c_driver = { > > > > > > Renaming is not explained in the commit message. > > > > True, this point I forgot to mention. Actually, this is tightly > > coupled with the fact that mfd driver has been moved to an earlier > > init stage and since it does belong to I2C sub-system (and represented > > by i2c_device structure) - I decided to make a name sound more > > logical. > > So, seems you have three changes in one patch. Please, split accordingly. Got it, thanks! Now I see it would be more logical to have 3 patches in the following order: - i2c driver renaming - additional regulator cell - early init If you don't mind - I'd wait for other people to comment on this patch, collect all points, and then come up with v2 of this patch (and whole series in fact). > > In v2 also Cc all stakeholders I mentioned. Would certainly do! > > -- > With Best Regards, > Andy Shevchenko > > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-24 14:29 [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin @ 2019-10-25 7:53 ` Andy Shevchenko 2019-10-25 7:55 ` Andy Shevchenko 2019-10-25 9:38 ` Hans de Goede 2 siblings, 2 replies; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 7:53 UTC (permalink / raw) To: Andrey Zhizhikin, Hans de Goede Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > This patchset introduces additional regulator driver for Intel Cherry > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > PMIC, which is used to instantiate this regulator. > > Regulator support for this PMIC was present in kernel release from Intel > targeted Aero platform, but was not entirely ported upstream and has > been omitted in mainline kernel releases. Consecutively, absence of > regulator caused the SD Card interface not to be provided with Vqcc > voltage source needed to operate with UHS-I cards. > > Following patches are addessing this issue and making sd card interface > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > of the SD Card interface in consumers and exposes optional "vqmmc" > voltage source, which mmc driver uses to switch signalling voltages > between 1.8V and 3.3V. > > This set contains of 2 patches: one is implementing the regulator driver > (based on a non upstreamed version from Intel Aero), and another patch > registers this driver as mfd cell in exising Whiskey Cove PMIC driver. Thank you. Hans, Cc'ed, has quite interested in these kind of patches. Am I right, Hans? > > > Andrey Zhizhikin (2): > regulator: add support for Intel Cherry Whiskey Cove regulator > mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC > > drivers/mfd/intel_soc_pmic_chtwc.c | 15 +- > drivers/regulator/Kconfig | 10 + > drivers/regulator/Makefile | 1 + > drivers/regulator/intel-cht-wc-regulator.c | 433 ++++++++++++++++++ > .../linux/regulator/intel-cht-wc-regulator.h | 64 +++ > 5 files changed, 521 insertions(+), 2 deletions(-) > create mode 100644 drivers/regulator/intel-cht-wc-regulator.c > create mode 100644 include/linux/regulator/intel-cht-wc-regulator.h > > -- > 2.17.1 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 7:53 ` [PATCH 0/2] add regulator driver and mfd cell for Intel " Andy Shevchenko @ 2019-10-25 7:55 ` Andy Shevchenko 2019-10-25 8:51 ` Andrey Zhizhikin 2019-10-28 12:41 ` Adrian Hunter 2019-10-25 9:38 ` Hans de Goede 1 sibling, 2 replies; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 7:55 UTC (permalink / raw) To: Andrey Zhizhikin, Hans de Goede, Adrian Hunter Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote: > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > > This patchset introduces additional regulator driver for Intel Cherry > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > > PMIC, which is used to instantiate this regulator. > > > > Regulator support for this PMIC was present in kernel release from Intel > > targeted Aero platform, but was not entirely ported upstream and has > > been omitted in mainline kernel releases. Consecutively, absence of > > regulator caused the SD Card interface not to be provided with Vqcc > > voltage source needed to operate with UHS-I cards. > > > > Following patches are addessing this issue and making sd card interface > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > > of the SD Card interface in consumers and exposes optional "vqmmc" > > voltage source, which mmc driver uses to switch signalling voltages > > between 1.8V and 3.3V. > > > > This set contains of 2 patches: one is implementing the regulator driver > > (based on a non upstreamed version from Intel Aero), and another patch > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > > Thank you. > Hans, Cc'ed, has quite interested in these kind of patches. > Am I right, Hans? Since it's about UHS/SD, Cc to Adrian as well. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 7:55 ` Andy Shevchenko @ 2019-10-25 8:51 ` Andrey Zhizhikin 2019-10-28 12:41 ` Adrian Hunter 1 sibling, 0 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 8:51 UTC (permalink / raw) To: Andy Shevchenko Cc: Hans de Goede, Adrian Hunter, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 9:55 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote: > > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > > > This patchset introduces additional regulator driver for Intel Cherry > > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > > > PMIC, which is used to instantiate this regulator. > > > > > > Regulator support for this PMIC was present in kernel release from Intel > > > targeted Aero platform, but was not entirely ported upstream and has > > > been omitted in mainline kernel releases. Consecutively, absence of > > > regulator caused the SD Card interface not to be provided with Vqcc > > > voltage source needed to operate with UHS-I cards. > > > > > > Following patches are addessing this issue and making sd card interface > > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > > > of the SD Card interface in consumers and exposes optional "vqmmc" > > > voltage source, which mmc driver uses to switch signalling voltages > > > between 1.8V and 3.3V. > > > > > > This set contains of 2 patches: one is implementing the regulator driver > > > (based on a non upstreamed version from Intel Aero), and another patch > > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > > > > Thank you. > > Hans, Cc'ed, has quite interested in these kind of patches. > > Am I right, Hans? > > Since it's about UHS/SD, Cc to Adrian as well. Thanks for looking into this, and including also interested people here. I've included only maintainers, but all interested parties are also very welcomed. > > -- > With Best Regards, > Andy Shevchenko > > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 7:55 ` Andy Shevchenko 2019-10-25 8:51 ` Andrey Zhizhikin @ 2019-10-28 12:41 ` Adrian Hunter 2019-10-28 12:45 ` Mark Brown 2019-10-28 14:40 ` Andrey Zhizhikin 1 sibling, 2 replies; 35+ messages in thread From: Adrian Hunter @ 2019-10-28 12:41 UTC (permalink / raw) To: Andy Shevchenko, Andrey Zhizhikin, Hans de Goede Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On 25/10/19 10:55 AM, Andy Shevchenko wrote: > On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote: >> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: >>> This patchset introduces additional regulator driver for Intel Cherry >>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this >>> PMIC, which is used to instantiate this regulator. >>> >>> Regulator support for this PMIC was present in kernel release from Intel >>> targeted Aero platform, but was not entirely ported upstream and has >>> been omitted in mainline kernel releases. Consecutively, absence of >>> regulator caused the SD Card interface not to be provided with Vqcc >>> voltage source needed to operate with UHS-I cards. >>> >>> Following patches are addessing this issue and making sd card interface >>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id >>> of the SD Card interface in consumers and exposes optional "vqmmc" >>> voltage source, which mmc driver uses to switch signalling voltages >>> between 1.8V and 3.3V. >>> >>> This set contains of 2 patches: one is implementing the regulator driver >>> (based on a non upstreamed version from Intel Aero), and another patch >>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. >> >> Thank you. >> Hans, Cc'ed, has quite interested in these kind of patches. >> Am I right, Hans? > > Since it's about UHS/SD, Cc to Adrian as well. > My only concern is that the driver might conflict with ACPI methods trying to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT with code like this: If ((Arg2 == 0x03)) { ADBG ("DSM 1p8") If ((^^I2C7.AVBL == One)) { If ((PMID == One)) { DATA = 0x59 ^^I2C7.DL03 = BUFF /* \_SB_.PCI0.SHC1.BUFF */ } ElseIf ((PMID == 0x02)) { BUFF = ^^I2C7.XD31 /* \_SB_.PCI0.I2C7.XD31 */ If ((STAT == Zero)) { DATA |= 0x10 ^^I2C7.XD31 = BUFF /* \_SB_.PCI0.SHC1.BUFF */ } BUFF = ^^I2C7.XD32 /* \_SB_.PCI0.I2C7.XD32 */ If ((STAT == Zero)) { DATA |= 0x0B DATA &= 0xEB ^^I2C7.XD32 = BUFF /* \_SB_.PCI0.SHC1.BUFF */ } Sleep (0x0A) BUFF = ^^I2C7.XD31 /* \_SB_.PCI0.I2C7.XD31 */ If ((STAT == Zero)) { DATA |= 0x20 ^^I2C7.XD31 = BUFF /* \_SB_.PCI0.SHC1.BUFF */ } } ElseIf ((PMID == 0x03)) { Local0 = ^^I2C7.PMI5.GET (One, 0x6E, 0x67) Sleep (0x0A) Local0 &= 0xF8 ^^I2C7.PMI5.SET (One, 0x6E, 0x67, Local0) Sleep (0x64) Local0 = ^^I2C7.PMI5.GET (One, 0x6E, 0x67) Sleep (0x0A) Local0 |= One Local0 &= 0xF9 ^^I2C7.PMI5.SET (One, 0x6E, 0x67, Local0) Sleep (0x0A) ^^I2C7.PMI5.SET (One, 0x6E, 0xC6, 0x1F) Sleep (0x0A) } } SDVL = One Return (0x03) } ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 12:41 ` Adrian Hunter @ 2019-10-28 12:45 ` Mark Brown 2019-10-28 13:26 ` Hans de Goede 2019-10-28 14:40 ` Andrey Zhizhikin 1 sibling, 1 reply; 35+ messages in thread From: Mark Brown @ 2019-10-28 12:45 UTC (permalink / raw) To: Adrian Hunter Cc: Andy Shevchenko, Andrey Zhizhikin, Hans de Goede, lgirdwood, lee.jones, linux-kernel, Andrey Zhizhikin [-- Attachment #1: Type: text/plain, Size: 536 bytes --] On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: > On 25/10/19 10:55 AM, Andy Shevchenko wrote: > > Since it's about UHS/SD, Cc to Adrian as well. > My only concern is that the driver might conflict with ACPI methods trying > to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT > with code like this: That's certainly what's idiomatic for ACPI (though machine specific quirks are too!). The safe thing to do would be to only register the supply on systems where we know there's no ACPI method. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 12:45 ` Mark Brown @ 2019-10-28 13:26 ` Hans de Goede 2019-10-28 15:01 ` Andrey Zhizhikin 2019-10-28 16:28 ` Andy Shevchenko 0 siblings, 2 replies; 35+ messages in thread From: Hans de Goede @ 2019-10-28 13:26 UTC (permalink / raw) To: Mark Brown, Adrian Hunter Cc: Andy Shevchenko, Andrey Zhizhikin, lgirdwood, lee.jones, linux-kernel, Andrey Zhizhikin Hi, On 28-10-2019 13:45, Mark Brown wrote: > On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: >> On 25/10/19 10:55 AM, Andy Shevchenko wrote: > >>> Since it's about UHS/SD, Cc to Adrian as well. > >> My only concern is that the driver might conflict with ACPI methods trying >> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT >> with code like this: Oh, right that is a very good point. > That's certainly what's idiomatic for ACPI (though machine specific > quirks are too!). The safe thing to do would be to only register the > supply on systems where we know there's no ACPI method. Right, so as I mentioned before Andrey told me about the evaluation board he is using I was aware of only 3 Cherry Trail devices using the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo Yoga book. I've checked the DSDT of all 3 and all 3 of them offer voltage control through the Intel _DSM method for voltage control. I've also actually tested this on the GPD win and 1.8V signalling works fine there without needing Andrey's patch. So it seems that Andrey's patch should only be active on his dev-board, as actual production hardware ships with the _DSM method. I believe that the best solution is for the Whiskey Cove MFD driver: drivers/mfd/intel_soc_pmic_chtwc.c To only register the new cell on Andrey's evaluation board model (based in a DMI match I guess). Another option would be to do the DMI check in the regulator driver, but that would mean udev will needlessly modprobe the regulator driver on production hardware, so doing it in the MFD driver and not registering the cell seems best, Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 13:26 ` Hans de Goede @ 2019-10-28 15:01 ` Andrey Zhizhikin 2019-10-29 12:03 ` Hans de Goede 2019-10-28 16:28 ` Andy Shevchenko 1 sibling, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-28 15:01 UTC (permalink / raw) To: Hans de Goede Cc: Mark Brown, Adrian Hunter, Andy Shevchenko, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin Hi Hans, On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 28-10-2019 13:45, Mark Brown wrote: > > On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: > >> On 25/10/19 10:55 AM, Andy Shevchenko wrote: > > > >>> Since it's about UHS/SD, Cc to Adrian as well. > > > >> My only concern is that the driver might conflict with ACPI methods trying > >> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT > >> with code like this: > > Oh, right that is a very good point. > > > That's certainly what's idiomatic for ACPI (though machine specific > > quirks are too!). The safe thing to do would be to only register the > > supply on systems where we know there's no ACPI method. > > Right, so as I mentioned before Andrey told me about the evaluation > board he is using I was aware of only 3 Cherry Trail devices using > the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo > Yoga book. I've checked the DSDT of all 3 and all 3 of them offer > voltage control through the Intel _DSM method for voltage control. > > I've also actually tested this on the GPD win and 1.8V signalling > works fine there without needing Andrey's patch. Thanks a lot for checking this one out! At least this proves now that the only platform affected is in fact Intel Aero board, and the patch as it is might not be necessary to accommodate for all CHT-based products with Whiskey Cove. > > So it seems that Andrey's patch should only be active on his > dev-board, as actual production hardware ships with the _DSM method. > > I believe that the best solution is for the Whiskey Cove MFD driver: > drivers/mfd/intel_soc_pmic_chtwc.c > > To only register the new cell on Andrey's evaluation board model > (based in a DMI match I guess). Another option would be to do > the DMI check in the regulator driver, but that would mean > udev will needlessly modprobe the regulator driver on production > hardware, so doing it in the MFD driver and not registering the cell > seems best, I tend to lean to a solution to perform a DMI check in MFD rather than in the regulator driver, since this would keep the regulator more-or-less agnostic to the where it is running on. Or maybe it would even make more sense to create a board-specific hook (like it was suggested for vqmmc voltage and sdmmc ACPI d of consumer), and then only register a cell for Aero match? This would actually keep the regulator consumer and mfd cell together and would not allow the device-specific code to leak into generic driver implementation. In this case I'd go with mfd_add_cell() if I get a DMI match and register a vqmmc consumer in there. In that case, can you please tell me what you think about it and if the drivers/acpi/acpi_lpss.c would still be an appropriate location to put this code to? Thanks a lot! > > Regards, > > Hans > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 15:01 ` Andrey Zhizhikin @ 2019-10-29 12:03 ` Hans de Goede 2019-10-29 16:57 ` Andrey Zhizhikin 0 siblings, 1 reply; 35+ messages in thread From: Hans de Goede @ 2019-10-29 12:03 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Mark Brown, Adrian Hunter, Andy Shevchenko, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin Hi, On 28-10-2019 16:01, Andrey Zhizhikin wrote: > Hi Hans, > > On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 28-10-2019 13:45, Mark Brown wrote: >>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: >>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote: >>> >>>>> Since it's about UHS/SD, Cc to Adrian as well. >>> >>>> My only concern is that the driver might conflict with ACPI methods trying >>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT >>>> with code like this: >> >> Oh, right that is a very good point. >> >>> That's certainly what's idiomatic for ACPI (though machine specific >>> quirks are too!). The safe thing to do would be to only register the >>> supply on systems where we know there's no ACPI method. >> >> Right, so as I mentioned before Andrey told me about the evaluation >> board he is using I was aware of only 3 Cherry Trail devices using >> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo >> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer >> voltage control through the Intel _DSM method for voltage control. >> >> I've also actually tested this on the GPD win and 1.8V signalling >> works fine there without needing Andrey's patch. > > Thanks a lot for checking this one out! At least this proves now that > the only platform affected is in fact Intel Aero board, and the patch > as it is might not be necessary to accommodate for all CHT-based > products with Whiskey Cove. > >> >> So it seems that Andrey's patch should only be active on his >> dev-board, as actual production hardware ships with the _DSM method. >> >> I believe that the best solution is for the Whiskey Cove MFD driver: >> drivers/mfd/intel_soc_pmic_chtwc.c >> >> To only register the new cell on Andrey's evaluation board model >> (based in a DMI match I guess). Another option would be to do >> the DMI check in the regulator driver, but that would mean >> udev will needlessly modprobe the regulator driver on production >> hardware, so doing it in the MFD driver and not registering the cell >> seems best, > > I tend to lean to a solution to perform a DMI check in MFD rather than > in the regulator driver, since this would keep the regulator > more-or-less agnostic to the where it is running on. > > Or maybe it would even make more sense to create a board-specific hook > (like it was suggested for vqmmc voltage and sdmmc ACPI d of > consumer), and then only register a cell for Aero match? This would > actually keep the regulator consumer and mfd cell together and would > not allow the device-specific code to leak into generic driver > implementation. In this case I'd go with mfd_add_cell() if I get a DMI > match and register a vqmmc consumer in there. > > In that case, can you please tell me what you think about it and if > the drivers/acpi/acpi_lpss.c would still be an appropriate location to > put this code to? I do not think that drivers/acpi/acpi_lpss.c is a good place. Thinking a bit more about this, my preferred solutions would be: 1. A BIOS update fixing the DSDT, as Andy suggested. Note we can lso use an overlay DSDT in the initrd, but that will only help users which take manual steps to add this to their initrd... 2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing, unlike that one you do not need to register a platform_device from the module_init() function, you can just add the mfd-cell and the regulator constraints from the module_init() function. Assuming 1. is not an option (I do not know if Intel still supports the Aero), then 2 will nicely isolate all the Aero specific code into a driver which will only auto-load on the Aero. Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-29 12:03 ` Hans de Goede @ 2019-10-29 16:57 ` Andrey Zhizhikin 2019-10-29 17:14 ` Hans de Goede 0 siblings, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-29 16:57 UTC (permalink / raw) To: Hans de Goede Cc: Mark Brown, Adrian Hunter, Andy Shevchenko, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin Hi Hans! On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 28-10-2019 16:01, Andrey Zhizhikin wrote: > > Hi Hans, > > > > On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 28-10-2019 13:45, Mark Brown wrote: > >>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: > >>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote: > >>> > >>>>> Since it's about UHS/SD, Cc to Adrian as well. > >>> > >>>> My only concern is that the driver might conflict with ACPI methods trying > >>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT > >>>> with code like this: > >> > >> Oh, right that is a very good point. > >> > >>> That's certainly what's idiomatic for ACPI (though machine specific > >>> quirks are too!). The safe thing to do would be to only register the > >>> supply on systems where we know there's no ACPI method. > >> > >> Right, so as I mentioned before Andrey told me about the evaluation > >> board he is using I was aware of only 3 Cherry Trail devices using > >> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo > >> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer > >> voltage control through the Intel _DSM method for voltage control. > >> > >> I've also actually tested this on the GPD win and 1.8V signalling > >> works fine there without needing Andrey's patch. > > > > Thanks a lot for checking this one out! At least this proves now that > > the only platform affected is in fact Intel Aero board, and the patch > > as it is might not be necessary to accommodate for all CHT-based > > products with Whiskey Cove. > > > >> > >> So it seems that Andrey's patch should only be active on his > >> dev-board, as actual production hardware ships with the _DSM method. > >> > >> I believe that the best solution is for the Whiskey Cove MFD driver: > >> drivers/mfd/intel_soc_pmic_chtwc.c > >> > >> To only register the new cell on Andrey's evaluation board model > >> (based in a DMI match I guess). Another option would be to do > >> the DMI check in the regulator driver, but that would mean > >> udev will needlessly modprobe the regulator driver on production > >> hardware, so doing it in the MFD driver and not registering the cell > >> seems best, > > > > I tend to lean to a solution to perform a DMI check in MFD rather than > > in the regulator driver, since this would keep the regulator > > more-or-less agnostic to the where it is running on. > > > > Or maybe it would even make more sense to create a board-specific hook > > (like it was suggested for vqmmc voltage and sdmmc ACPI d of > > consumer), and then only register a cell for Aero match? This would > > actually keep the regulator consumer and mfd cell together and would > > not allow the device-specific code to leak into generic driver > > implementation. In this case I'd go with mfd_add_cell() if I get a DMI > > match and register a vqmmc consumer in there. > > > > In that case, can you please tell me what you think about it and if > > the drivers/acpi/acpi_lpss.c would still be an appropriate location to > > put this code to? > > I do not think that drivers/acpi/acpi_lpss.c is a good place. Thanks a lot for clarifying this point, I was also not sure whether I would need combine the platform-specific functionality with LPSS implementation. > > Thinking a bit more about this, my preferred solutions would be: > > 1. A BIOS update fixing the DSDT, as Andy suggested. Note we can > lso use an overlay DSDT in the initrd, but that will only help users > which take manual steps to add this to their initrd... This I believe would not be an option since the Aero platform has been phased-out from Intel, and Insyde most probably would not do an update on the BIOS. I can go with DSDT overlay, but I was not sure whether this is a good way to solve this. > > 2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo > board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing, > unlike that one you do not need to register a platform_device from > the module_init() function, you can just add the mfd-cell and the > regulator constraints from the module_init() function. This would be my preferred solution, since in this case I can contain all the Aero-specific modifications to it's own board file. If there would be further modifications needed for it - they would be nicely contained within that board file. > > Assuming 1. is not an option (I do not know if Intel still > supports the Aero), then 2 will nicely isolate all the Aero > specific code into a driver which will only auto-load on > the Aero. Just as I indicated above, chances that BIOS would receive an update are between slim and nil. If no one have any objection, I'd prefer to go with the approach [2] from above. > > Regards, > > Hans > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-29 16:57 ` Andrey Zhizhikin @ 2019-10-29 17:14 ` Hans de Goede 0 siblings, 0 replies; 35+ messages in thread From: Hans de Goede @ 2019-10-29 17:14 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Mark Brown, Adrian Hunter, Andy Shevchenko, lgirdwood, Lee Jones, linux-kernel, Andrey Zhizhikin Hi, On 29-10-2019 17:57, Andrey Zhizhikin wrote: > Hi Hans! > > On Tue, Oct 29, 2019 at 1:04 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 28-10-2019 16:01, Andrey Zhizhikin wrote: >>> Hi Hans, >>> >>> On Mon, Oct 28, 2019 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> Hi, >>>> >>>> On 28-10-2019 13:45, Mark Brown wrote: >>>>> On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: >>>>>> On 25/10/19 10:55 AM, Andy Shevchenko wrote: >>>>> >>>>>>> Since it's about UHS/SD, Cc to Adrian as well. >>>>> >>>>>> My only concern is that the driver might conflict with ACPI methods trying >>>>>> to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT >>>>>> with code like this: >>>> >>>> Oh, right that is a very good point. >>>> >>>>> That's certainly what's idiomatic for ACPI (though machine specific >>>>> quirks are too!). The safe thing to do would be to only register the >>>>> supply on systems where we know there's no ACPI method. >>>> >>>> Right, so as I mentioned before Andrey told me about the evaluation >>>> board he is using I was aware of only 3 Cherry Trail devices using >>>> the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo >>>> Yoga book. I've checked the DSDT of all 3 and all 3 of them offer >>>> voltage control through the Intel _DSM method for voltage control. >>>> >>>> I've also actually tested this on the GPD win and 1.8V signalling >>>> works fine there without needing Andrey's patch. >>> >>> Thanks a lot for checking this one out! At least this proves now that >>> the only platform affected is in fact Intel Aero board, and the patch >>> as it is might not be necessary to accommodate for all CHT-based >>> products with Whiskey Cove. >>> >>>> >>>> So it seems that Andrey's patch should only be active on his >>>> dev-board, as actual production hardware ships with the _DSM method. >>>> >>>> I believe that the best solution is for the Whiskey Cove MFD driver: >>>> drivers/mfd/intel_soc_pmic_chtwc.c >>>> >>>> To only register the new cell on Andrey's evaluation board model >>>> (based in a DMI match I guess). Another option would be to do >>>> the DMI check in the regulator driver, but that would mean >>>> udev will needlessly modprobe the regulator driver on production >>>> hardware, so doing it in the MFD driver and not registering the cell >>>> seems best, >>> >>> I tend to lean to a solution to perform a DMI check in MFD rather than >>> in the regulator driver, since this would keep the regulator >>> more-or-less agnostic to the where it is running on. >>> >>> Or maybe it would even make more sense to create a board-specific hook >>> (like it was suggested for vqmmc voltage and sdmmc ACPI d of >>> consumer), and then only register a cell for Aero match? This would >>> actually keep the regulator consumer and mfd cell together and would >>> not allow the device-specific code to leak into generic driver >>> implementation. In this case I'd go with mfd_add_cell() if I get a DMI >>> match and register a vqmmc consumer in there. >>> >>> In that case, can you please tell me what you think about it and if >>> the drivers/acpi/acpi_lpss.c would still be an appropriate location to >>> put this code to? >> >> I do not think that drivers/acpi/acpi_lpss.c is a good place. > > Thanks a lot for clarifying this point, I was also not sure whether I > would need combine the platform-specific functionality with LPSS > implementation. > >> >> Thinking a bit more about this, my preferred solutions would be: >> >> 1. A BIOS update fixing the DSDT, as Andy suggested. Note we can >> lso use an overlay DSDT in the initrd, but that will only help users >> which take manual steps to add this to their initrd... > > This I believe would not be an option since the Aero platform has been > phased-out from Intel, and Insyde most probably would not do an update > on the BIOS. I can go with DSDT overlay, but I was not sure whether > this is a good way to solve this. > >> >> 2. A new drivers/platform/x86 driver binding to the dmi-ids of the Areo >> board, like e.g. drivers/platform/x86/intel_oaktrail.c is doing, >> unlike that one you do not need to register a platform_device from >> the module_init() function, you can just add the mfd-cell and the >> regulator constraints from the module_init() function. > > This would be my preferred solution, since in this case I can contain > all the Aero-specific modifications to it's own board file. If there > would be further modifications needed for it - they would be nicely > contained within that board file. > >> >> Assuming 1. is not an option (I do not know if Intel still >> supports the Aero), then 2 will nicely isolate all the Aero >> specific code into a driver which will only auto-load on >> the Aero. > > Just as I indicated above, chances that BIOS would receive an update > are between slim and nil. If no one have any objection, I'd prefer to > go with the approach [2] from above. I agree that 2. probably is the best, so I say go for it :) Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 13:26 ` Hans de Goede 2019-10-28 15:01 ` Andrey Zhizhikin @ 2019-10-28 16:28 ` Andy Shevchenko 1 sibling, 0 replies; 35+ messages in thread From: Andy Shevchenko @ 2019-10-28 16:28 UTC (permalink / raw) To: Hans de Goede Cc: Mark Brown, Adrian Hunter, Andrey Zhizhikin, lgirdwood, lee.jones, linux-kernel, Andrey Zhizhikin On Mon, Oct 28, 2019 at 02:26:21PM +0100, Hans de Goede wrote: > On 28-10-2019 13:45, Mark Brown wrote: > > On Mon, Oct 28, 2019 at 02:41:46PM +0200, Adrian Hunter wrote: > > > On 25/10/19 10:55 AM, Andy Shevchenko wrote: > > > > > > Since it's about UHS/SD, Cc to Adrian as well. > > > > > My only concern is that the driver might conflict with ACPI methods trying > > > to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT > > > with code like this: > > Oh, right that is a very good point. > > > That's certainly what's idiomatic for ACPI (though machine specific > > quirks are too!). The safe thing to do would be to only register the > > supply on systems where we know there's no ACPI method. > > Right, so as I mentioned before Andrey told me about the evaluation > board he is using I was aware of only 3 Cherry Trail devices using > the Whiskey Cove PMIC. The GPD win, the GPD pocket and the Lenovo > Yoga book. I've checked the DSDT of all 3 and all 3 of them offer > voltage control through the Intel _DSM method for voltage control. > > I've also actually tested this on the GPD win and 1.8V signalling > works fine there without needing Andrey's patch. > > So it seems that Andrey's patch should only be active on his > dev-board, as actual production hardware ships with the _DSM method. > > I believe that the best solution is for the Whiskey Cove MFD driver: > drivers/mfd/intel_soc_pmic_chtwc.c > > To only register the new cell on Andrey's evaluation board model > (based in a DMI match I guess). Another option would be to do > the DMI check in the regulator driver, but that would mean > udev will needlessly modprobe the regulator driver on production > hardware, so doing it in the MFD driver and not registering the cell > seems best, I'm wondering if we can upgrade DSDT to provide _DSM for Aero platform. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-28 12:41 ` Adrian Hunter 2019-10-28 12:45 ` Mark Brown @ 2019-10-28 14:40 ` Andrey Zhizhikin 1 sibling, 0 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-28 14:40 UTC (permalink / raw) To: Adrian Hunter Cc: Andy Shevchenko, Hans de Goede, lgirdwood, Mark Brown, Lee Jones, linux-kernel, Andrey Zhizhikin On Mon, Oct 28, 2019 at 1:42 PM Adrian Hunter <adrian.hunter@intel.com> wrote: > > On 25/10/19 10:55 AM, Andy Shevchenko wrote: > > On Fri, Oct 25, 2019 at 10:53:35AM +0300, Andy Shevchenko wrote: > >> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > >>> This patchset introduces additional regulator driver for Intel Cherry > >>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > >>> PMIC, which is used to instantiate this regulator. > >>> > >>> Regulator support for this PMIC was present in kernel release from Intel > >>> targeted Aero platform, but was not entirely ported upstream and has > >>> been omitted in mainline kernel releases. Consecutively, absence of > >>> regulator caused the SD Card interface not to be provided with Vqcc > >>> voltage source needed to operate with UHS-I cards. > >>> > >>> Following patches are addessing this issue and making sd card interface > >>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > >>> of the SD Card interface in consumers and exposes optional "vqmmc" > >>> voltage source, which mmc driver uses to switch signalling voltages > >>> between 1.8V and 3.3V. > >>> > >>> This set contains of 2 patches: one is implementing the regulator driver > >>> (based on a non upstreamed version from Intel Aero), and another patch > >>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > >> > >> Thank you. > >> Hans, Cc'ed, has quite interested in these kind of patches. > >> Am I right, Hans? > > > > Since it's about UHS/SD, Cc to Adrian as well. > > > > My only concern is that the driver might conflict with ACPI methods trying > to do the same thing, e.g. there is one ACPI SDHC instance from GPDWin DSDT > with code like this: > [... cut ...] That's a good point, and this is what I tried at first when I started to work on this problem. In my case INTEL_DSM_V18_SWITCH (fn=0x3) got executed in the mmc sub-system, but without the regulator or DSDT support the card did not get the proper voltage. The DSDT on Intel Aero reference platform looks different here. Currently I don't have it on hands, but would be able to look it up next week. -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 7:53 ` [PATCH 0/2] add regulator driver and mfd cell for Intel " Andy Shevchenko 2019-10-25 7:55 ` Andy Shevchenko @ 2019-10-25 9:38 ` Hans de Goede 2019-10-25 10:11 ` Andrey Zhizhikin 2019-10-25 10:44 ` Andy Shevchenko 1 sibling, 2 replies; 35+ messages in thread From: Hans de Goede @ 2019-10-25 9:38 UTC (permalink / raw) To: Andy Shevchenko, Andrey Zhizhikin Cc: lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin Hi, On 25-10-2019 09:53, Andy Shevchenko wrote: > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: >> This patchset introduces additional regulator driver for Intel Cherry >> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this >> PMIC, which is used to instantiate this regulator. >> >> Regulator support for this PMIC was present in kernel release from Intel >> targeted Aero platform, but was not entirely ported upstream and has >> been omitted in mainline kernel releases. Consecutively, absence of >> regulator caused the SD Card interface not to be provided with Vqcc >> voltage source needed to operate with UHS-I cards. >> >> Following patches are addessing this issue and making sd card interface >> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id >> of the SD Card interface in consumers and exposes optional "vqmmc" >> voltage source, which mmc driver uses to switch signalling voltages >> between 1.8V and 3.3V. >> >> This set contains of 2 patches: one is implementing the regulator driver >> (based on a non upstreamed version from Intel Aero), and another patch >> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > > Thank you. > Hans, Cc'ed, has quite interested in these kind of patches. > Am I right, Hans? Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm always interested in CHT specific patches. Overall this series looks good (from a high level view I did not do a detailed review) but I wonder if we really want to export all the regulators when we just need the vsdio one? Most regulators are controlled by either the P-Unit inside the CHT SoC or through ACPI OpRegion accesses. Luckily the regulator subsys does not expose a sysfs interface for users to directly poke regulators, but this will still make it somewhat easier for users to poke regulators which they should leave alone. Note I'm not saying this is wrong, having support for all regulators in place in case we need it in the future is also kinda nice. OTOH if we just need the one now, maybe we should just support the one for now ? Andy do you have an opinion on this? Andrey, may I ask which device you are testing this on? Anyways overall good work, thank you for doing this! Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 9:38 ` Hans de Goede @ 2019-10-25 10:11 ` Andrey Zhizhikin 2019-10-25 10:45 ` Hans de Goede 2019-10-25 10:44 ` Andy Shevchenko 1 sibling, 1 reply; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 10:11 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 25-10-2019 09:53, Andy Shevchenko wrote: > > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > >> This patchset introduces additional regulator driver for Intel Cherry > >> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > >> PMIC, which is used to instantiate this regulator. > >> > >> Regulator support for this PMIC was present in kernel release from Intel > >> targeted Aero platform, but was not entirely ported upstream and has > >> been omitted in mainline kernel releases. Consecutively, absence of > >> regulator caused the SD Card interface not to be provided with Vqcc > >> voltage source needed to operate with UHS-I cards. > >> > >> Following patches are addessing this issue and making sd card interface > >> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > >> of the SD Card interface in consumers and exposes optional "vqmmc" > >> voltage source, which mmc driver uses to switch signalling voltages > >> between 1.8V and 3.3V. > >> > >> This set contains of 2 patches: one is implementing the regulator driver > >> (based on a non upstreamed version from Intel Aero), and another patch > >> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > > > > Thank you. > > Hans, Cc'ed, has quite interested in these kind of patches. > > Am I right, Hans? > > Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm > always interested in CHT specific patches. > > Overall this series looks good (from a high level view I did not > do a detailed review) but I wonder if we really want to export all the > regulators when we just need the vsdio one? I thought about this point, and actually came to a personal conclusion that if I do this as a new driver - then it is better to list all possible regulators, creating some sort of "skeleton" which people could then work on if need be. I do agree that at the present moment the one regulator which is exposed is the one for vsdio, but listing all possibilities should not hurt. This was my motivation to put them all into the driver on the first place. If you believe additional regulator elements should be removed from this version of the driver - I can clean them up and come up with v2 here. > > Most regulators are controlled by either the P-Unit inside the CHT SoC > or through ACPI OpRegion accesses. Luckily the regulator subsys does not > expose a sysfs interface for users to directly poke regulators, but this will > still make it somewhat easier for users to poke regulators which they should > leave alone. Agree, this is a valid point. But honestly I would really be surprised if a user would directly touch something which can burn his silicon to pieces. Regulators are usually not approached by users; unless they are really HW engineers and know what they are doing. > > Note I'm not saying this is wrong, having support for all regulators in place > in case we need it in the future is also kinda nice. OTOH if we just need the > one now, maybe we should just support the one for now ? This I've already covered above I guess. > Andrey, may I ask which device you are testing this on? Sure, I use the original Intel Aero board. It used to have an official image from Aero team with a heavily patched 4.4.y kernel, but when I decided to have this updated to the latest stable branch - I've faced the issue of missing core functionality, which led me to this patch. > > Anyways overall good work, thank you for doing this! You're welcome, and thanks for looking into this! > > Regards, > > Hans > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 10:11 ` Andrey Zhizhikin @ 2019-10-25 10:45 ` Hans de Goede 2019-10-25 11:54 ` Andrey Zhizhikin 2019-10-25 12:05 ` Mark Brown 0 siblings, 2 replies; 35+ messages in thread From: Hans de Goede @ 2019-10-25 10:45 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Andy Shevchenko, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin Hi, On 25-10-2019 12:11, Andrey Zhizhikin wrote: > On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 25-10-2019 09:53, Andy Shevchenko wrote: >>> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: >>>> This patchset introduces additional regulator driver for Intel Cherry >>>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this >>>> PMIC, which is used to instantiate this regulator. >>>> >>>> Regulator support for this PMIC was present in kernel release from Intel >>>> targeted Aero platform, but was not entirely ported upstream and has >>>> been omitted in mainline kernel releases. Consecutively, absence of >>>> regulator caused the SD Card interface not to be provided with Vqcc >>>> voltage source needed to operate with UHS-I cards. >>>> >>>> Following patches are addessing this issue and making sd card interface >>>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id >>>> of the SD Card interface in consumers and exposes optional "vqmmc" >>>> voltage source, which mmc driver uses to switch signalling voltages >>>> between 1.8V and 3.3V. >>>> >>>> This set contains of 2 patches: one is implementing the regulator driver >>>> (based on a non upstreamed version from Intel Aero), and another patch >>>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. >>> >>> Thank you. >>> Hans, Cc'ed, has quite interested in these kind of patches. >>> Am I right, Hans? >> >> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm >> always interested in CHT specific patches. >> >> Overall this series looks good (from a high level view I did not >> do a detailed review) but I wonder if we really want to export all the >> regulators when we just need the vsdio one? > > I thought about this point, and actually came to a personal conclusion > that if I do this as a new driver - then it is better to list all > possible regulators, creating some sort of "skeleton" which people > could then work on if need be. I do agree that at the present moment > the one regulator which is exposed is the one for vsdio, but listing > all possibilities should not hurt. This was my motivation to put them > all into the driver on the first place. > > If you believe additional regulator elements should be removed from > this version of the driver - I can clean them up and come up with v2 > here. > >> >> Most regulators are controlled by either the P-Unit inside the CHT SoC >> or through ACPI OpRegion accesses. Luckily the regulator subsys does not >> expose a sysfs interface for users to directly poke regulators, but this will >> still make it somewhat easier for users to poke regulators which they should >> leave alone. > > Agree, this is a valid point. But honestly I would really be surprised > if a user would directly touch something which can burn his silicon to > pieces. Regulators are usually not approached by users; unless they > are really HW engineers and know what they are doing. True. Hmm, I do just realize that the regulator subsystem turns off regulators which are not in use from its pov, which would be kinda bad here. It does this when the kernel is done initializing / switches to the initrd or directly to the rootfs. Its been a while since I've last touched this. Have you tried building the regulator driver (and mfd stuff and i2c controller, etc.) into the kernel, so that the regulators work without modules? I'm afraid that if you do that they will all get turned off, with expecting results... Although I guess the regulator subsystem may skip this step for regulators without any constrains, to be sure I would have to re-read the code. Anyways this is a serious concern which we need to resolve. >> Andrey, may I ask which device you are testing this on? > > Sure, I use the original Intel Aero board. It used to have an official > image from Aero team with a heavily patched 4.4.y kernel, but when I > decided to have this updated to the latest stable branch - I've faced > the issue of missing core functionality, which led me to this patch. Ah I see, the reason I was asking is because I only know of three CHT devices using a Whiskey Cove PMIC (instead of the AXP288, Crystal Cove or TI one): the GPD Win, GPD pocket and one of the Lenovo Book versions (the one with the CHT SoC). Lets say I want to test this patch-set on one of the GPD devices, I guess I need an UHS-I capable card and then what should I see in sysfs for the mmc settings ? Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 10:45 ` Hans de Goede @ 2019-10-25 11:54 ` Andrey Zhizhikin 2019-10-25 12:05 ` Mark Brown 1 sibling, 0 replies; 35+ messages in thread From: Andrey Zhizhikin @ 2019-10-25 11:54 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin Hi, On Fri, Oct 25, 2019 at 12:45 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 25-10-2019 12:11, Andrey Zhizhikin wrote: > > On Fri, Oct 25, 2019 at 11:38 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> Hi, > >> > >> On 25-10-2019 09:53, Andy Shevchenko wrote: > >>> On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > >>>> This patchset introduces additional regulator driver for Intel Cherry > >>>> Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > >>>> PMIC, which is used to instantiate this regulator. > >>>> > >>>> Regulator support for this PMIC was present in kernel release from Intel > >>>> targeted Aero platform, but was not entirely ported upstream and has > >>>> been omitted in mainline kernel releases. Consecutively, absence of > >>>> regulator caused the SD Card interface not to be provided with Vqcc > >>>> voltage source needed to operate with UHS-I cards. > >>>> > >>>> Following patches are addessing this issue and making sd card interface > >>>> to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > >>>> of the SD Card interface in consumers and exposes optional "vqmmc" > >>>> voltage source, which mmc driver uses to switch signalling voltages > >>>> between 1.8V and 3.3V. > >>>> > >>>> This set contains of 2 patches: one is implementing the regulator driver > >>>> (based on a non upstreamed version from Intel Aero), and another patch > >>>> registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > >>> > >>> Thank you. > >>> Hans, Cc'ed, has quite interested in these kind of patches. > >>> Am I right, Hans? > >> > >> Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm > >> always interested in CHT specific patches. > >> > >> Overall this series looks good (from a high level view I did not > >> do a detailed review) but I wonder if we really want to export all the > >> regulators when we just need the vsdio one? > > > > I thought about this point, and actually came to a personal conclusion > > that if I do this as a new driver - then it is better to list all > > possible regulators, creating some sort of "skeleton" which people > > could then work on if need be. I do agree that at the present moment > > the one regulator which is exposed is the one for vsdio, but listing > > all possibilities should not hurt. This was my motivation to put them > > all into the driver on the first place. > > > > If you believe additional regulator elements should be removed from > > this version of the driver - I can clean them up and come up with v2 > > here. > > > >> > >> Most regulators are controlled by either the P-Unit inside the CHT SoC > >> or through ACPI OpRegion accesses. Luckily the regulator subsys does not > >> expose a sysfs interface for users to directly poke regulators, but this will > >> still make it somewhat easier for users to poke regulators which they should > >> leave alone. > > > > Agree, this is a valid point. But honestly I would really be surprised > > if a user would directly touch something which can burn his silicon to > > pieces. Regulators are usually not approached by users; unless they > > are really HW engineers and know what they are doing. > > True. > > Hmm, I do just realize that the regulator subsystem turns off regulators > which are not in use from its pov, which would be kinda bad here. Interesting point, this I did not know. > > It does this when the kernel is done initializing / switches to the > initrd or directly to the rootfs. Its been a while since I've last touched > this. Have you tried building the regulator driver (and mfd stuff and i2c > controller, etc.) into the kernel, so that the regulators work without modules? This is the first thing that I've tested - I've selected the new regulator as built-in, and it is fully operable. > > I'm afraid that if you do that they will all get turned off, with expecting > results... Although I guess the regulator subsystem may skip this step for > regulators without any constrains, to be sure I would have to re-read the code. I guess (although to 100% sure), that this is exactly the reason why this regulator remains operable - it does have neither source nor constraints, therefore it remains enabled. > > Anyways this is a serious concern which we need to resolve. > > >> Andrey, may I ask which device you are testing this on? > > > > Sure, I use the original Intel Aero board. It used to have an official > > image from Aero team with a heavily patched 4.4.y kernel, but when I > > decided to have this updated to the latest stable branch - I've faced > > the issue of missing core functionality, which led me to this patch. > > Ah I see, the reason I was asking is because I only know of three CHT > devices using a Whiskey Cove PMIC (instead of the AXP288, Crystal Cove or > TI one): the GPD Win, GPD pocket and one of the Lenovo Book versions > (the one with the CHT SoC). I've stumbled upon those platforms, specifically when I was searching for information about the sdmmc interface. It looks like that they all are affected by this missing vqmmc piece, since a lot of people have raised complaints some SD Cards are not recognized. > > Lets say I want to test this patch-set on one of the GPD devices, > I guess I need an UHS-I capable card and then what should I see > in sysfs for the mmc settings ? This is rather trivial: with the absence of 1.8v on the interface, UHS-I cards are failing during the speed switch sped in initialization. If you would plug in the UHS-I SD Card in the slot, then driver outputs the error message in dmesg: mmc2: error -110 whilst initialising SD card Error codes returned are either ETIMEDOUT or EILSEQ, depending on how the SD Card controller handles improper voltages on interface. With the patch applied, when UHS-I card is inserted, following message in recorded in dmesg: mmc2: new ultra high speed SDR104 SDHC card at address aaaa SDR104 mode is only for UHS-capable cards (Grade I and above); that message is for example recorded when SanDisk Extreme 32GB card is inserted. I've also tested different other cards from Verbatim, Swissbit and SanDisk, and they all were recognized OK. BTW, as a side note: there is a way to disable 1.8v switching in mmc sub-system via quirks2, this makes all UHS-I cards to operate as High Speed (HS) cards at 3.3v IO level. This is rather a hack, but people using the CHT-based products tend to do exactly that (or at least this is a common solution proposed in Internet). As for sysfs attributes: I actually didn't look into those to really tell you which values for which parameters would definitely distinguish UHS cards and settings from HS ones. > > Regards, > > Hans > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 10:45 ` Hans de Goede 2019-10-25 11:54 ` Andrey Zhizhikin @ 2019-10-25 12:05 ` Mark Brown 2019-10-25 12:18 ` Hans de Goede 1 sibling, 1 reply; 35+ messages in thread From: Mark Brown @ 2019-10-25 12:05 UTC (permalink / raw) To: Hans de Goede Cc: Andrey Zhizhikin, Andy Shevchenko, lgirdwood, lee.jones, linux-kernel, Andrey Zhizhikin [-- Attachment #1: Type: text/plain, Size: 588 bytes --] On Fri, Oct 25, 2019 at 12:45:30PM +0200, Hans de Goede wrote: > Hmm, I do just realize that the regulator subsystem turns off regulators > which are not in use from its pov, which would be kinda bad here. It will only do this for regulators which have constraints which enable the kernel to change the power state of the regulator, the regulator core will never make any change to hardware that was not explicitly permitted by constraints so should be perfectly safe unless constraints are provided in which case it's up to whoever provides the constraints to make sure they are safe. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 12:05 ` Mark Brown @ 2019-10-25 12:18 ` Hans de Goede 0 siblings, 0 replies; 35+ messages in thread From: Hans de Goede @ 2019-10-25 12:18 UTC (permalink / raw) To: Mark Brown Cc: Andrey Zhizhikin, Andy Shevchenko, lgirdwood, lee.jones, linux-kernel, Andrey Zhizhikin Hi, On 25-10-2019 14:05, Mark Brown wrote: > On Fri, Oct 25, 2019 at 12:45:30PM +0200, Hans de Goede wrote: > >> Hmm, I do just realize that the regulator subsystem turns off regulators >> which are not in use from its pov, which would be kinda bad here. > > It will only do this for regulators which have constraints which > enable the kernel to change the power state of the regulator, the > regulator core will never make any change to hardware that was > not explicitly permitted by constraints so should be perfectly > safe unless constraints are provided in which case it's up to > whoever provides the constraints to make sure they are safe. Ok, thank you for the explanation. Then I believe it is fine to keep the driver as is, with support for all regulators in the PMIC. Regards, Hans ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC 2019-10-25 9:38 ` Hans de Goede 2019-10-25 10:11 ` Andrey Zhizhikin @ 2019-10-25 10:44 ` Andy Shevchenko 1 sibling, 0 replies; 35+ messages in thread From: Andy Shevchenko @ 2019-10-25 10:44 UTC (permalink / raw) To: Hans de Goede Cc: Andrey Zhizhikin, lgirdwood, broonie, lee.jones, linux-kernel, Andrey Zhizhikin On Fri, Oct 25, 2019 at 11:38:52AM +0200, Hans de Goede wrote: > On 25-10-2019 09:53, Andy Shevchenko wrote: > > On Thu, Oct 24, 2019 at 02:29:37PM +0000, Andrey Zhizhikin wrote: > > > This patchset introduces additional regulator driver for Intel Cherry > > > Trail Whiskey Cove PMIC. It also adds a cell in mfd driver for this > > > PMIC, which is used to instantiate this regulator. > > > > > > Regulator support for this PMIC was present in kernel release from Intel > > > targeted Aero platform, but was not entirely ported upstream and has > > > been omitted in mainline kernel releases. Consecutively, absence of > > > regulator caused the SD Card interface not to be provided with Vqcc > > > voltage source needed to operate with UHS-I cards. > > > > > > Following patches are addessing this issue and making sd card interface > > > to be fully operable with UHS-I cards. Regulator driver lists an ACPI id > > > of the SD Card interface in consumers and exposes optional "vqmmc" > > > voltage source, which mmc driver uses to switch signalling voltages > > > between 1.8V and 3.3V. > > > > > > This set contains of 2 patches: one is implementing the regulator driver > > > (based on a non upstreamed version from Intel Aero), and another patch > > > registers this driver as mfd cell in exising Whiskey Cove PMIC driver. > > > > Thank you. > > Hans, Cc'ed, has quite interested in these kind of patches. > > Am I right, Hans? > > Yes since I do a lot of work on Bay and Cherry Trail hw enablement I'm > always interested in CHT specific patches. > > Overall this series looks good (from a high level view I did not > do a detailed review) but I wonder if we really want to export all the > regulators when we just need the vsdio one? > > Most regulators are controlled by either the P-Unit inside the CHT SoC > or through ACPI OpRegion accesses. Luckily the regulator subsys does not > expose a sysfs interface for users to directly poke regulators, but this will > still make it somewhat easier for users to poke regulators which they should > leave alone. > > Note I'm not saying this is wrong, having support for all regulators in place > in case we need it in the future is also kinda nice. OTOH if we just need the > one now, maybe we should just support the one for now ? > > Andy do you have an opinion on this? Usually on such patches I have two (a bit contradicted) wishes: - be as little as needed - for sake of documentation purposes keep as many definitions as we can But you know the dangling code / definitions not accepted by all maintainers. So, that said, I have no strong opinion here, basically rely on maintainer's point of view. Perhaps also good idea is to put a reference as URL to the published code from which this has been derived. > > Andrey, may I ask which device you are testing this on? > > Anyways overall good work, thank you for doing this! +1 to the cheering up! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-10-29 17:14 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-24 14:29 [PATCH 0/2] add regulator driver and mfd cell for Intel Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 1/2] regulator: add support for Intel Cherry Whiskey Cove regulator Andrey Zhizhikin 2019-10-25 8:01 ` Andy Shevchenko 2019-10-25 8:58 ` Andrey Zhizhikin 2019-10-25 9:14 ` Andy Shevchenko 2019-10-25 9:31 ` Andrey Zhizhikin 2019-10-25 10:47 ` Andy Shevchenko 2019-10-25 12:17 ` Mark Brown 2019-10-25 15:26 ` Andrey Zhizhikin 2019-10-25 16:02 ` Andy Shevchenko 2019-10-25 16:21 ` Andrey Zhizhikin 2019-10-24 14:29 ` [PATCH 2/2] mfd: add regulator cell to Cherry Trail Whiskey Cove PMIC Andrey Zhizhikin 2019-10-25 8:06 ` Andy Shevchenko 2019-10-25 9:16 ` Andrey Zhizhikin 2019-10-25 10:49 ` Andy Shevchenko 2019-10-25 12:00 ` Andrey Zhizhikin 2019-10-25 7:53 ` [PATCH 0/2] add regulator driver and mfd cell for Intel " Andy Shevchenko 2019-10-25 7:55 ` Andy Shevchenko 2019-10-25 8:51 ` Andrey Zhizhikin 2019-10-28 12:41 ` Adrian Hunter 2019-10-28 12:45 ` Mark Brown 2019-10-28 13:26 ` Hans de Goede 2019-10-28 15:01 ` Andrey Zhizhikin 2019-10-29 12:03 ` Hans de Goede 2019-10-29 16:57 ` Andrey Zhizhikin 2019-10-29 17:14 ` Hans de Goede 2019-10-28 16:28 ` Andy Shevchenko 2019-10-28 14:40 ` Andrey Zhizhikin 2019-10-25 9:38 ` Hans de Goede 2019-10-25 10:11 ` Andrey Zhizhikin 2019-10-25 10:45 ` Hans de Goede 2019-10-25 11:54 ` Andrey Zhizhikin 2019-10-25 12:05 ` Mark Brown 2019-10-25 12:18 ` Hans de Goede 2019-10-25 10:44 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).