Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755417AbdGXGSH convert rfc822-to-8bit (ORCPT ); Mon, 24 Jul 2017 02:18:07 -0400 Received: from smtp.csie.ntu.edu.tw ([140.112.30.61]:54322 "EHLO smtp.csie.ntu.edu.tw" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751605AbdGXGSA (ORCPT ); Mon, 24 Jul 2017 02:18:00 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170723102749.17323-1-icenowy@aosc.io> <20170723102749.17323-3-icenowy@aosc.io> From: Chen-Yu Tsai Date: Mon, 24 Jul 2017 14:17:35 +0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [linux-sunxi] [PATCH 02/10] regulator: add support for SY8106A regulator To: Icenowy Zheng Cc: Chen-Yu Tsai , Ondrej Jirman , devicetree , "open list:THERMAL" , linux-sunxi , linux-kernel , Liam Girdwood , Mark Brown , Carlo Caione , Maxime Ripard , linux-clk , linux-arm-kernel Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13951 Lines: 390 On Mon, Jul 24, 2017 at 11:38 AM, wrote: > 在 2017-07-24 11:33，Chen-Yu Tsai 写道： >> >> On Mon, Jul 24, 2017 at 11:18 AM, wrote: >>> >>> 在 2017-07-24 11:03，Chen-Yu Tsai 写道： >>>> >>>> >>>> On Sun, Jul 23, 2017 at 6:27 PM, Icenowy Zheng wrote: >>>>> >>>>> >>>>> From: Ondrej Jirman >>>>> >>>>> SY8106A is an I2C attached single output regulator made by Silergy >>>>> Corp, >>>>> which is used on several Allwinner H3/H5 SBCs to control the power >>>>> supply of the ARM cores. >>>>> >>>>> Add a driver for it. >>>>> >>>>> Signed-off-by: Ondrej Jirman >>>>> [Icenowy: Change commit message] >>>>> Signed-off-by: Icenowy Zheng >>>>> --- >>>>> drivers/regulator/Kconfig | 8 +- >>>>> drivers/regulator/Makefile | 2 +- >>>>> drivers/regulator/sy8106a-regulator.c | 163 >>>>> ++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 171 insertions(+), 2 deletions(-) >>>>> create mode 100644 drivers/regulator/sy8106a-regulator.c >>>>> >>>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >>>>> index 99b9362331b5..1efa73e18d07 100644 >>>>> --- a/drivers/regulator/Kconfig >>>>> +++ b/drivers/regulator/Kconfig >>>>> @@ -764,6 +764,13 @@ config REGULATOR_STW481X_VMMC >>>>> This driver supports the internal VMMC regulator in the >>>>> STw481x >>>>> PMIC chips. >>>>> >>>>> +config REGULATOR_SY8106A >>>>> + tristate "Silergy SY8106A regulator" >>>>> + depends on I2C && (OF || COMPILE_TEST) >>>>> + select REGMAP_I2C >>>>> + help >>>>> + This driver supports SY8106A single output regulator. >>>>> + >>>>> config REGULATOR_TPS51632 >>>>> tristate "TI TPS51632 Power Regulator" >>>>> depends on I2C >>>>> @@ -938,4 +945,3 @@ config REGULATOR_WM8994 >>>>> WM8994 CODEC. >>>>> >>>>> endif >>>>> - >>>>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >>>>> index 95b1e86ae692..f5120252f86a 100644 >>>>> --- a/drivers/regulator/Makefile >>>>> +++ b/drivers/regulator/Makefile >>>>> @@ -95,6 +95,7 @@ obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o >>>>> obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o >>>>> obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o >>>>> obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o >>>>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o >>>>> obj-$(CONFIG_REGULATOR_TI_ABB) += ti-abb-regulator.o >>>>> obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o >>>>> obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o >>>>> @@ -120,5 +121,4 @@ obj-$(CONFIG_REGULATOR_WM8350) += >>>>> wm8350-regulator.o >>>>> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o >>>>> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o >>>>> >>>>> - >>>>> ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG >>>>> diff --git a/drivers/regulator/sy8106a-regulator.c >>>>> b/drivers/regulator/sy8106a-regulator.c >>>>> new file mode 100644 >>>>> index 000000000000..1df889f68b3d >>>>> --- /dev/null >>>>> +++ b/drivers/regulator/sy8106a-regulator.c >>>>> @@ -0,0 +1,163 @@ >>>>> +/* >>>>> + * sy8106a-regulator.c - Regulator device driver for SY8106A >>>>> + * >>>>> + * Copyright (C) 2016 Ondřej Jirman >>>>> + * >>>>> + * This program is free software; you can redistribute it and/or >>>>> + * modify it under the terms of the GNU Library General Public >>>>> + * License as published by the Free Software Foundation; either >>>>> + * version 2 of the License, or (at your option) any later version. >>>>> + * >>>>> + * This program is distributed in the hope that it will be useful, >>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >>>>> + * Library General Public License for more details. >>>>> + */ >>>>> + >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> + >>>>> +#define SY8106A_REG_VOUT1_SEL 0x01 >>>>> +#define SY8106A_REG_VOUT_COM 0x02 >>>>> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f >>>>> +#define SY8106A_DISABLE_REG BIT(0) >>>>> +#define SY8106A_GO_BIT BIT(7) >>>>> + >>>>> +struct sy8106a { >>>>> + struct regulator_dev *rdev; >>>>> + struct regmap *regmap; >>>>> +}; >>>>> + >>>>> +static const struct regmap_config sy8106a_regmap_config = { >>>>> + .reg_bits = 8, >>>>> + .val_bits = 8, >>>>> +}; >>>>> + >>>>> +static int sy8106a_set_voltage_sel(struct regulator_dev *rdev, >>>>> unsigned >>>>> int sel) >>>>> +{ >>>>> + /* We use our set_voltage_sel in order to avoid unnecessary I2C >>>>> + * chatter, because the regulator_get_voltage_sel_regmap using >>>>> + * apply_bit would perform 4 unnecessary transfers instead of >>>>> one, >>>>> + * increasing the chance of error. >>>>> + */ >>>>> + return regmap_write(rdev->regmap, rdev->desc->vsel_reg, >>>>> + sel | SY8106A_GO_BIT); >>>> >>>> >>>> >>>> There should also be an explanation of what the "go bit" does. >>>> In this case it enables control of the voltage over I2C. >>>> >>>>> +} >>>>> + >>>>> +static const struct regulator_ops sy8106a_ops = { >>>>> + .is_enabled = regulator_is_enabled_regmap, >>>>> + .set_voltage_sel = sy8106a_set_voltage_sel, >>>>> + .set_voltage_time_sel = regulator_set_voltage_time_sel, >>>>> + .get_voltage_sel = regulator_get_voltage_sel_regmap, >>>>> + .list_voltage = regulator_list_voltage_linear, >>>> >>>> >>>> >>>> No enable/disable ops? >>>> >>>>> +}; >>>>> + >>>>> +/* Default limits measured in millivolts and milliamps */ >>>>> +#define SY8106A_MIN_MV 680 >>>>> +#define SY8106A_MAX_MV 1950 >>>>> +#define SY8106A_STEP_MV 10 >>>>> + >>>>> +static const struct regulator_desc sy8106a_reg = { >>>>> + .name = "SY8106A", >>>>> + .id = 0, >>>>> + .ops = &sy8106a_ops, >>>>> + .type = REGULATOR_VOLTAGE, >>>>> + .n_voltages = ((SY8106A_MAX_MV - SY8106A_MIN_MV) / >>>>> SY8106A_STEP_MV) + 1, >>>>> + .min_uV = (SY8106A_MIN_MV * 1000), >>>>> + .uV_step = (SY8106A_STEP_MV * 1000), >>>>> + .vsel_reg = SY8106A_REG_VOUT1_SEL, >>>>> + .vsel_mask = SY8106A_REG_VOUT1_SEL_MASK, >>>>> + .enable_reg = SY8106A_REG_VOUT_COM, >>>>> + .enable_mask = SY8106A_DISABLE_REG, >>>>> + .disable_val = SY8106A_DISABLE_REG, >>>>> + .enable_is_inverted = 1, >>>> >>>> >>>> >>>> As the regulator core helpers currently are, there is no way to disable >>>> this regulator. regulator_disable_regmap looks like: >>> >>> >>> >>> By reading the current code, I think removing the disable_val variable >>> will >>> just make it work -- when enabling, the enable_reg will be set to >>> disable_val >>> (as enable_is_inverted is 1), and disable_val is defaultly 0; when >>> disabling, >>> the enable_reg will be set to enable_mask (fallbacked from enable_val, >>> which >>> is also the default value 0), and the regulator successfully get >>> disabled. >> >> >> (Gmail is wrapping the lines :( ) >> >> enable_mask is set to SY8106A_DISABLE_REG, which equals disable_val here, >> which is not 0. This is correct, as it is the bit mask passed to >> regmap_update_bits. So no, it does not work. > > > From my personal understanding of the code, the behavior of > enable_is_inverted > is that switch the roles of enable_val and disable_val, so if > enable_is_inverted is set, the regulator_disable_regmap function will write > enable_val (if it's 0, fallback to enable_mask) to the register, and > regulator_enable_regmap will write disable_val. > > Maybe this design is used to ensure enable_val (then fallback to > enable_mask) > is not 0. I think the changes needed to the core are probably too invasive, i.e. affect too many other drivers, to be worth doing. Especially since we won't really be using enable/disable much, as the regulator supplies the CPU cores. For now, lets just remove the enable/disable related fields, and leave a TODO note citing the possibly required changes to the core instead. You haven't assigned the callbacks anyway. ChenYu > > >> >> It seems quite a lot of drivers depend on this fallback behavior though. >> >> ChenYu >> >>> >>>> >>>> if (rdev->desc->enable_is_inverted) { >>>> val = rdev->desc->enable_val; >>>> if (!val) >>>> val = rdev->desc->enable_mask; >>>> } else { >>>> val = rdev->desc->disable_val; >>>> } >>>> >>>> return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg, >>>> rdev->desc->enable_mask, val); >>>> >>>> Note the fallback to enable_mask if enable_val is not set. The enable >>>> helper has a similar structure. This regulator uses an enable value >>>> of 0, and disable value of 1, it is impossible to get it right. As >>>> it is now, it doesn't really make sense to me. >>>> >>>> The code was introduced in ca5d1b3524b4 ("regulator: helpers: Modify >>>> helpers enabling multi-bit control"). I wonder if it was an unintended >>>> consequence of the change? (original author CC-ed) >>>> >>>>> + .owner = THIS_MODULE, >>>>> +}; >>>>> + >>>>> +/* >>>>> + * I2C driver interface functions >>>>> + */ >>>>> +static int sy8106a_i2c_probe(struct i2c_client *i2c, >>>>> + const struct i2c_device_id *id) >>>>> +{ >>>>> + struct sy8106a *chip; >>>>> + struct device *dev = &i2c->dev; >>>>> + struct regulator_dev *rdev = NULL; >>>>> + struct regulator_config config = { }; >>>>> + unsigned int selector; >>>>> + int error; >>>>> + >>>>> + chip = devm_kzalloc(&i2c->dev, sizeof(struct sy8106a), >>>>> GFP_KERNEL); >>>>> + if (!chip) >>>>> + return -ENOMEM; >>>>> + >>>>> + chip->regmap = devm_regmap_init_i2c(i2c, >>>>> &sy8106a_regmap_config); >>>>> + if (IS_ERR(chip->regmap)) { >>>>> + error = PTR_ERR(chip->regmap); >>>>> + dev_err(&i2c->dev, "Failed to allocate register map: >>>>> %d\n", >>>>> + error); >>>>> + return error; >>>>> + } >>>>> + >>>>> + config.dev = &i2c->dev; >>>>> + config.regmap = chip->regmap; >>>>> + config.driver_data = chip; >>>>> + >>>>> + config.of_node = dev->of_node; >>>>> + config.init_data = of_get_regulator_init_data(dev, >>>>> dev->of_node, >>>>> + &sy8106a_reg); >>>>> + >>>>> + if (!config.init_data) >>>>> + return -ENOMEM; >>>>> + >>>>> + /* Probe regulator */ >>>>> + error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, >>>>> &selector); >>>>> + if (error) { >>>>> + dev_err(&i2c->dev, "Failed to read voltage at probe >>>>> time: >>>>> %d\n", error); >>>>> + return error; >>>>> + } >>>>> + >>>>> + rdev = devm_regulator_register(&i2c->dev, &sy8106a_reg, >>>>> &config); >>>>> + if (IS_ERR(rdev)) { >>>>> + error = PTR_ERR(rdev); >>>>> + dev_err(&i2c->dev, "Failed to register SY8106A >>>>> regulator: >>>>> %d\n", error); >>>>> + return error; >>>>> + } >>>>> + >>>>> + chip->rdev = rdev; >>>>> + >>>>> + i2c_set_clientdata(i2c, chip); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static const struct of_device_id sy8106a_i2c_of_match[] = { >>>>> + { .compatible = "silergy,sy8106a" }, >>>>> + { }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(of, sy8106a_i2c_of_match); >>>>> + >>>>> +static const struct i2c_device_id sy8106a_i2c_id[] = { >>>>> + { "sy8106a", 0 }, >>>>> + { }, >>>>> +}; >>>>> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id); >>>>> + >>>>> +static struct i2c_driver sy8106a_regulator_driver = { >>>>> + .driver = { >>>>> + .name = "sy8106a", >>>>> + .of_match_table = of_match_ptr(sy8106a_i2c_of_match), >>>>> + }, >>>>> + .probe = sy8106a_i2c_probe, >>>>> + .id_table = sy8106a_i2c_id, >>>>> +}; >>>>> + >>>>> +module_i2c_driver(sy8106a_regulator_driver); >>>>> + >>>>> +MODULE_AUTHOR("Ondřej Jirman "); >>>>> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A"); >>>>> +MODULE_LICENSE("GPL v2"); >>>> >>>> >>>> >>>> This should be "GPL", to match what the license at the very top says. >>>> >>>> Regards >>>> ChenYu >>>> >>>>> -- >>>>> 2.13.0 >>>>> >>>>> -- >>>>> You received this message because you are subscribed to the Google >>>>> Groups >>>>> "linux-sunxi" group. >>>>> To unsubscribe from this group and stop receiving emails from it, send >>>>> an >>>>> email to linux-sunxi+unsubscribe@googlegroups.com. >>>>> For more options, visit https://groups.google.com/d/optout. >>>> >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> >>> >>> -- >>> You received this message because you are subscribed to the Google Groups >>> "linux-sunxi" group. >>> To unsubscribe from this group and stop receiving emails from it, send an >>> email to linux-sunxi+unsubscribe@googlegroups.com. >>> For more options, visit https://groups.google.com/d/optout. > > > -- > You received this message because you are subscribed to the Google Groups > "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.