| Message ID | 20180423144657.63264-3-icenowy@aosc.io |
|---|---|
| State | New |
| Headers | show |
| Series | SY8106 regulator support and enable it on Orange Pi PC | expand |
On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote: > --- /dev/null > +++ b/drivers/regulator/sy8106a-regulator.c > @@ -0,0 +1,176 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * sy8106a-regulator.c - Regulator device driver for SY8106A Just make the entire thing a C++ comment so it looks consistent and joined up. > +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); Why would it do these extra transfers? Is this just the fact that you didn't set up a register cache (though the r/m/w cycle should only add the read)? We could put some logic in the core regmap code to detect that an _update_bits() call is going to write to the whole register, though it'd be even easier to just let this register be cached. Generally if we can usefully optimize things we should do it at the framework level. > + if (reg & SY8106A_GO_BIT) > + return reg & rdev->desc->vsel_mask; > + else > + return (chip->fixed_voltage - rdev->desc->min_uV) / > + rdev->desc->uV_step; You could use the standard get_voltage_sel() if you provide a mapping operation that set everything with _GO_BIT set to return the fixed voltage. Though looking at this it seems that the fixed voltage will always be one that could be set via the register anyway so I'm wondering if the easiest thing here isn't to just have the driver turn off _GO_BIT during probe() and not worry about the special case at runtime.
于 2018年4月25日 GMT+08:00 上午1:07:33, Mark Brown <broonie@kernel.org> 写到: >On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote: > >> --- /dev/null >> +++ b/drivers/regulator/sy8106a-regulator.c >> @@ -0,0 +1,176 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * sy8106a-regulator.c - Regulator device driver for SY8106A > >Just make the entire thing a C++ comment so it looks consistent and >joined up. SPDX identifier is special -- it should be in a seperated comment block. > >> +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); > >Why would it do these extra transfers? Is this just the fact that you >didn't set up a register cache (though the r/m/w cycle should only add >the read)? We could put some logic in the core regmap code to detect >that an _update_bits() call is going to write to the whole register, >though it'd be even easier to just let this register be cached. > >Generally if we can usefully optimize things we should do it at the >framework level. Oh maybe the comment needs to be changed, but it's needed to enable it to change voltage, as it might be not enabled at boot. > >> + if (reg & SY8106A_GO_BIT) >> + return reg & rdev->desc->vsel_mask; >> + else >> + return (chip->fixed_voltage - rdev->desc->min_uV) / >> + rdev->desc->uV_step; > >You could use the standard get_voltage_sel() if you provide a mapping >operation that set everything with _GO_BIT set to return the fixed >voltage. Though looking at this it seems that the fixed voltage will >always be one that could be set via the register anyway so I'm >wondering >if the easiest thing here isn't to just have the driver turn off >_GO_BIT Do you mean "turn on" here? >during probe() and not worry about the special case at runtime.
On Wed, Apr 25, 2018 at 07:41:35AM +0800, Icenowy Zheng wrote: > 于 2018年4月25日 GMT+08:00 上午1:07:33, Mark Brown <broonie@kernel.org> 写到: > >On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote: > >> --- /dev/null > >> +++ b/drivers/regulator/sy8106a-regulator.c > >> @@ -0,0 +1,176 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * sy8106a-regulator.c - Regulator device driver for SY8106A > >Just make the entire thing a C++ comment so it looks consistent and > >joined up. > SPDX identifier is special -- it should be in a seperated > comment block. No, it just needs to be a C++ comment on the first line to ease machine parsing. The rest of the file doesn't matter for that. > >> + if (reg & SY8106A_GO_BIT) > >> + return reg & rdev->desc->vsel_mask; > >> + else > >> + return (chip->fixed_voltage - rdev->desc->min_uV) / > >> + rdev->desc->uV_step; > >You could use the standard get_voltage_sel() if you provide a mapping > >operation that set everything with _GO_BIT set to return the fixed > >voltage. Though looking at this it seems that the fixed voltage will > >always be one that could be set via the register anyway so I'm > >wondering > >if the easiest thing here isn't to just have the driver turn off > >_GO_BIT > Do you mean "turn on" here? Yes.
于 2018年4月25日 GMT+08:00 下午6:53:09, Mark Brown <broonie@kernel.org> 写到: >On Wed, Apr 25, 2018 at 07:41:35AM +0800, Icenowy Zheng wrote: >> 于 2018年4月25日 GMT+08:00 上午1:07:33, Mark Brown <broonie@kernel.org> 写到: >> >On Mon, Apr 23, 2018 at 10:46:56PM +0800, Icenowy Zheng wrote: > >> >> --- /dev/null >> >> +++ b/drivers/regulator/sy8106a-regulator.c >> >> @@ -0,0 +1,176 @@ >> >> +// SPDX-License-Identifier: GPL-2.0+ >> >> +/* >> >> + * sy8106a-regulator.c - Regulator device driver for SY8106A > >> >Just make the entire thing a C++ comment so it looks consistent and >> >joined up. > >> SPDX identifier is special -- it should be in a seperated >> comment block. > >No, it just needs to be a C++ comment on the first line to ease machine >parsing. The rest of the file doesn't matter for that. Okay. Thus I will use a seperate /* */ block for it in the next revision, to follow the rule. > >> >> + if (reg & SY8106A_GO_BIT) >> >> + return reg & rdev->desc->vsel_mask; >> >> + else >> >> + return (chip->fixed_voltage - rdev->desc->min_uV) / >> >> + rdev->desc->uV_step; > >> >You could use the standard get_voltage_sel() if you provide a >mapping >> >operation that set everything with _GO_BIT set to return the fixed >> >voltage. Though looking at this it seems that the fixed voltage >will >> >always be one that could be set via the register anyway so I'm >> >wondering >> >if the easiest thing here isn't to just have the driver turn off >> >_GO_BIT > >> Do you mean "turn on" here? > >Yes. Okay. I will do it in this way and drop custom get/set_voltage_sel.
On Wed, Apr 25, 2018 at 06:55:10PM +0800, Icenowy Zheng wrote: > 于 2018年4月25日 GMT+08:00 下午6:53:09, Mark Brown <broonie@kernel.org> 写到: > >> SPDX identifier is special -- it should be in a seperated > >> comment block. > >No, it just needs to be a C++ comment on the first line to ease machine > >parsing. The rest of the file doesn't matter for that. > Okay. Thus I will use a seperate /* */ block for it in the > next revision, to follow the rule. Just make the whole copyright block a C++ comment. It's much easier and even with a blank line between the SPDX line and the rest of it it'll still look messy and odd.
diff --git a/MAINTAINERS b/MAINTAINERS index a33643739820..e0cb78568458 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13464,6 +13464,12 @@ S: Supported F: net/switchdev/ F: include/net/switchdev.h +SY8106A REGULATOR DRIVER +M: Icenowy Zheng <icenowy@aosc.io> +S: Maintained +F: drivers/regulator/sy8106a-regulator.c +F: Documentation/devicetree/bindings/regulator/sy8106a-regulator.txt + SYNC FILE FRAMEWORK M: Sumit Semwal <sumit.semwal@linaro.org> R: Gustavo Padovan <gustavo@padovan.org> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index 097f61784a7d..4efae3b7e746 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -801,6 +801,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 diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 590674fbecd7..d81fb02bd6e9 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -100,6 +100,7 @@ obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.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 @@ -125,5 +126,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..6dbe41044ab9 --- /dev/null +++ b/drivers/regulator/sy8106a-regulator.c @@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * sy8106a-regulator.c - Regulator device driver for SY8106A + * + * Copyright (C) 2016 Ondřej Jirman <megous@megous.com> + * Copyright (c) 2017-2018 Icenowy Zheng <icenowy@aosc.io> + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/of_regulator.h> + +#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) +/* + * The I2C controlled voltage will only work when this bit is set; otherwise + * it will behave like a fixed regulator. + */ +#define SY8106A_GO_BIT BIT(7) + +struct sy8106a { + struct regulator_dev *rdev; + struct regmap *regmap; + u32 fixed_voltage; +}; + +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); +} + +static int sy8106a_get_voltage_sel(struct regulator_dev *rdev) +{ + struct sy8106a *chip = rdev_get_drvdata(rdev); + unsigned int reg; + int error; + + error = regmap_read(rdev->regmap, rdev->desc->vsel_reg, ®); + if (error) + return error; + + if (reg & SY8106A_GO_BIT) + return reg & rdev->desc->vsel_mask; + else + return (chip->fixed_voltage - rdev->desc->min_uV) / + rdev->desc->uV_step; +} + +static const struct regulator_ops sy8106a_ops = { + .set_voltage_sel = sy8106a_set_voltage_sel, + .set_voltage_time_sel = regulator_set_voltage_time_sel, + .get_voltage_sel = sy8106a_get_voltage_sel, + .list_voltage = regulator_list_voltage_linear, + /* Enabling/disabling the regulator is not yet implemented */ +}; + +/* Default limits measured in millivolts */ +#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, + /* + * This ramp_delay is a conservative default value which works on + * H3/H5 boards VDD-CPUX situations. + */ + .ramp_delay = 200, + .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 = { }; + int error; + + chip = devm_kzalloc(&i2c->dev, sizeof(struct sy8106a), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + error = of_property_read_u32(dev->of_node, "silergy,fixed-microvolt", + &chip->fixed_voltage); + if (error) + return error; + + chip->regmap = devm_regmap_init_i2c(i2c, &sy8106a_regmap_config); + if (IS_ERR(chip->regmap)) { + error = PTR_ERR(chip->regmap); + dev_err(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 */ + 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 <megous@megous.com>"); +MODULE_AUTHOR("Icenowy Zheng <icenowy@aosc.io>"); +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A"); +MODULE_LICENSE("GPL");