Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1430106 > unrolled thread
| Started by | megous@megous.com |
|---|---|
| First post | 2016-06-23 21:40 +0200 |
| Last post | 2016-06-25 03:10 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 07/14] regulator: SY8106A regulator driver megous@megous.com - 2016-06-23 21:40 +0200
Re: [PATCH 07/14] regulator: SY8106A regulator driver Chen-Yu Tsai <wens@csie.org> - 2016-06-24 05:50 +0200
Re: [PATCH 07/14] regulator: SY8106A regulator driver Ondřej Jirman <megous@megous.com> - 2016-06-25 02:20 +0200
Re: [PATCH 07/14] regulator: SY8106A regulator driver Chen-Yu Tsai <wens@csie.org> - 2016-06-25 03:10 +0200
| From | megous@megous.com |
|---|---|
| Date | 2016-06-23 21:40 +0200 |
| Subject | [PATCH 07/14] regulator: SY8106A regulator driver |
| Message-ID | <rNiDN-6cT-49@gated-at.bofh.it> |
From: Ondrej Jirman <megous@megous.com>
SY8106A is I2C attached single output voltage regulator
made by Silergy.
Signed-off-by: Ondrej Jirman <megous@megous.com>
---
drivers/regulator/Kconfig | 8 +-
drivers/regulator/Makefile | 2 +-
drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
3 files changed, 161 insertions(+), 2 deletions(-)
create mode 100644 drivers/regulator/sy8106a-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 144cbf5..fc3fae2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -860,5 +860,11 @@ config REGULATOR_WM8994
This driver provides support for the voltage regulators on the
WM8994 CODEC.
-endif
+config REGULATOR_SY8106A
+ tristate "Silergy SY8106A"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ This driver provides support for the voltage regulator SY8106A.
+endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 85a1d44..f382095 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
-
+obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-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 0000000..34bd69c
--- /dev/null
+++ b/drivers/regulator/sy8106a-regulator.c
@@ -0,0 +1,153 @@
+/*
+ * sy8106a-regulator.c - Regulator device driver for SY8106A
+ *
+ * Copyright (C) 2016 Ondřej Jirman <megous@megous.com>
+ *
+ * This library 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 library 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.
+ *
+ * You should have received a copy of the GNU Library General Public
+ * License along with this library; if not, write to the
+ * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regmap.h>
+
+#define SY8106A_REG_VOUT1_SEL 0x01
+#define SY8106A_REG_VOUT_COM 0x02
+#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
+#define SY8106A_DISABLE_REG 0x01
+
+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 sel)
+{
+ return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
+ 0xff, sel | 0x80);
+}
+
+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,
+};
+
+/* 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,
+ .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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
+ config.driver_data = chip;
+ config.regmap = chip->regmap;
+ config.of_node = dev->of_node;
+
+ /* Probe regulator */
+ error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
+ dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
+ 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)) {
+ dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
+ return PTR_ERR(rdev);
+ }
+
+ chip->rdev = rdev;
+
+ i2c_set_clientdata(i2c, chip);
+
+ return 0;
+}
+
+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",
+ },
+ .probe = sy8106a_i2c_probe,
+ .id_table = sy8106a_i2c_id,
+};
+
+module_i2c_driver(sy8106a_regulator_driver);
+
+MODULE_AUTHOR("Ondřej Jirman <megous@megous.com>");
+MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
+MODULE_LICENSE("GPL v2");
--
2.9.0
[toc] | [next] | [standalone]
| From | Chen-Yu Tsai <wens@csie.org> |
|---|---|
| Date | 2016-06-24 05:50 +0200 |
| Message-ID | <rNqhX-2Uf-3@gated-at.bofh.it> |
| In reply to | #1430106 |
On Fri, Jun 24, 2016 at 3:20 AM, <megous@megous.com> wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> SY8106A is I2C attached single output voltage regulator
> made by Silergy.
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
> drivers/regulator/Kconfig | 8 +-
> drivers/regulator/Makefile | 2 +-
> drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
> 3 files changed, 161 insertions(+), 2 deletions(-)
> create mode 100644 drivers/regulator/sy8106a-regulator.c
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 144cbf5..fc3fae2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
> This driver provides support for the voltage regulators on the
> WM8994 CODEC.
>
> -endif
> +config REGULATOR_SY8106A
> + tristate "Silergy SY8106A"
> + depends on I2C
Maybe you should also depend on OF since the driver is going to crippled
without any constraints set, or (OF || COMPILE_TEST) if you want some
compile test coverage.
> + select REGMAP_I2C
> + help
> + This driver provides support for the voltage regulator SY8106A.
>
> +endif
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 85a1d44..f382095 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
> -
> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
Follow the existing ordering in the Makefile.
>
> ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
> diff --git a/drivers/regulator/sy8106a-regulator.c b/drivers/regulator/sy8106a-regulator.c
> new file mode 100644
> index 0000000..34bd69c
> --- /dev/null
> +++ b/drivers/regulator/sy8106a-regulator.c
> @@ -0,0 +1,153 @@
> +/*
> + * sy8106a-regulator.c - Regulator device driver for SY8106A
> + *
> + * Copyright (C) 2016 Ondřej Jirman <megous@megous.com>
> + *
> + * This library 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 library 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.
> + *
> + * You should have received a copy of the GNU Library General Public
> + * License along with this library; if not, write to the
> + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
Do you need this one?
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
And this one?
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/regmap.h>
Sort alphabetically please.
> +
> +#define SY8106A_REG_VOUT1_SEL 0x01
> +#define SY8106A_REG_VOUT_COM 0x02
> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
> +#define SY8106A_DISABLE_REG 0x01
BIT(0) would be clearer.
> +
> +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 sel)
> +{
> + return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
> + 0xff, sel | 0x80);
Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?
> +}
> +
> +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,
> +};
> +
> +/* 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,
> + .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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
Check for possible failures?
> + config.driver_data = chip;
> + config.regmap = chip->regmap;
> + config.of_node = dev->of_node;
> +
> + /* Probe regulator */
> + error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
> + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
> + 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)) {
> + dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
> + return PTR_ERR(rdev);
> + }
> +
> + chip->rdev = rdev;
> +
> + i2c_set_clientdata(i2c, chip);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id sy8106a_i2c_id[] = {
> + {"sy8106a", 0},
> + {},
> +};
> +
Extra line.
> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
IMHO probing explicitly through DT (of_device_id) would be better.
Regards
ChenYu
> +
> +static struct i2c_driver sy8106a_regulator_driver = {
> + .driver = {
> + .name = "sy8106a",
> + },
> + .probe = sy8106a_i2c_probe,
> + .id_table = sy8106a_i2c_id,
> +};
> +
> +module_i2c_driver(sy8106a_regulator_driver);
> +
> +MODULE_AUTHOR("Ondřej Jirman <megous@megous.com>");
> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[toc] | [prev] | [next] | [standalone]
| From | Ondřej Jirman <megous@megous.com> |
|---|---|
| Date | 2016-06-25 02:20 +0200 |
| Message-ID | <rNJui-6KT-27@gated-at.bofh.it> |
| In reply to | #1430334 |
[Multipart message — attachments visible in raw view] — view raw
Hi,
thank you for the review. I've resolved most of the issues. Some more
comments below.
On 24.6.2016 05:41, Chen-Yu Tsai wrote:
> On Fri, Jun 24, 2016 at 3:20 AM, <megous@megous.com> wrote:
>> From: Ondrej Jirman <megous@megous.com>
>>
>> SY8106A is I2C attached single output voltage regulator
>> made by Silergy.
>>
>> Signed-off-by: Ondrej Jirman <megous@megous.com>
>> ---
>> drivers/regulator/Kconfig | 8 +-
>> drivers/regulator/Makefile | 2 +-
>> drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 161 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/regulator/sy8106a-regulator.c
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 144cbf5..fc3fae2 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
>> This driver provides support for the voltage regulators on the
>> WM8994 CODEC.
>>
>> -endif
>> +config REGULATOR_SY8106A
>> + tristate "Silergy SY8106A"
>> + depends on I2C
>
> Maybe you should also depend on OF since the driver is going to crippled
> without any constraints set, or (OF || COMPILE_TEST) if you want some
> compile test coverage.
>
>> + select REGMAP_I2C
>> + help
>> + This driver provides support for the voltage regulator SY8106A.
>>
>> +endif
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index 85a1d44..f382095 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>> -
>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>
> Follow the existing ordering in the Makefile.
>
>>
>> ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
>> diff --git a/drivers/regulator/sy8106a-regulator.c b/drivers/regulator/sy8106a-regulator.c
>> new file mode 100644
>> index 0000000..34bd69c
>> --- /dev/null
>> +++ b/drivers/regulator/sy8106a-regulator.c
>> @@ -0,0 +1,153 @@
>> +/*
>> + * sy8106a-regulator.c - Regulator device driver for SY8106A
>> + *
>> + * Copyright (C) 2016 Ondřej Jirman <megous@megous.com>
>> + *
>> + * This library 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 library 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.
>> + *
>> + * You should have received a copy of the GNU Library General Public
>> + * License along with this library; if not, write to the
>> + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
>> + * Boston, MA 02110-1301, USA.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>
> Do you need this one?
>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>
> And this one?
>
>> +#include <linux/regulator/of_regulator.h>
>> +#include <linux/regmap.h>
>
> Sort alphabetically please.
>
>> +
>> +#define SY8106A_REG_VOUT1_SEL 0x01
>> +#define SY8106A_REG_VOUT_COM 0x02
>> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
>> +#define SY8106A_DISABLE_REG 0x01
>
> BIT(0) would be clearer.
>
>> +
>> +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 sel)
>> +{
>> + return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>> + 0xff, sel | 0x80);
>
> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?
I understand the code savings, but I'd rather avoid that, because it
would involve 2 needless readouts and rewrites of the voltage setting
register. I'd rather change this to regmap_write, so that the regulator
only receives writes over I2C, to minimize possibility of bit flip error
by minimizing the communication over the I2C bus.
>> +}
>> +
>> +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,
>> +};
>> +
>> +/* 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,
>> + .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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
>
> Check for possible failures?
>
>> + config.driver_data = chip;
>> + config.regmap = chip->regmap;
>> + config.of_node = dev->of_node;
>> +
>> + /* Probe regulator */
>> + error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
>> + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
>> + 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)) {
>> + dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
>> + return PTR_ERR(rdev);
>> + }
>> +
>> + chip->rdev = rdev;
>> +
>> + i2c_set_clientdata(i2c, chip);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id sy8106a_i2c_id[] = {
>> + {"sy8106a", 0},
>> + {},
>> +};
>> +
>
> Extra line.
>
>> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
>
> IMHO probing explicitly through DT (of_device_id) would be better.
I checked out a few recently added i2c regulator drivers, and this is
the way they're written. I'd rather not differ, especially since I'm a
beginner with this thing, atm. Also, I'm not sure what chanhge in
particular you're suggesting.
thanks,
Ondrej
>
> Regards
> ChenYu
>
>> +
>> +static struct i2c_driver sy8106a_regulator_driver = {
>> + .driver = {
>> + .name = "sy8106a",
>> + },
>> + .probe = sy8106a_i2c_probe,
>> + .id_table = sy8106a_i2c_id,
>> +};
>> +
>> +module_i2c_driver(sy8106a_regulator_driver);
>> +
>> +MODULE_AUTHOR("Ondřej Jirman <megous@megous.com>");
>> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 2.9.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[toc] | [prev] | [next] | [standalone]
| From | Chen-Yu Tsai <wens@csie.org> |
|---|---|
| Date | 2016-06-25 03:10 +0200 |
| Message-ID | <rNKgF-7lq-3@gated-at.bofh.it> |
| In reply to | #1431024 |
On Sat, Jun 25, 2016 at 8:11 AM, Ondřej Jirman <megous@megous.com> wrote:
> Hi,
>
> thank you for the review. I've resolved most of the issues. Some more
> comments below.
>
> On 24.6.2016 05:41, Chen-Yu Tsai wrote:
>> On Fri, Jun 24, 2016 at 3:20 AM, <megous@megous.com> wrote:
>>> From: Ondrej Jirman <megous@megous.com>
>>>
>>> SY8106A is I2C attached single output voltage regulator
>>> made by Silergy.
>>>
>>> Signed-off-by: Ondrej Jirman <megous@megous.com>
>>> ---
>>> drivers/regulator/Kconfig | 8 +-
>>> drivers/regulator/Makefile | 2 +-
>>> drivers/regulator/sy8106a-regulator.c | 153 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 161 insertions(+), 2 deletions(-)
>>> create mode 100644 drivers/regulator/sy8106a-regulator.c
>>>
>>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>>> index 144cbf5..fc3fae2 100644
>>> --- a/drivers/regulator/Kconfig
>>> +++ b/drivers/regulator/Kconfig
>>> @@ -860,5 +860,11 @@ config REGULATOR_WM8994
>>> This driver provides support for the voltage regulators on the
>>> WM8994 CODEC.
>>>
>>> -endif
>>> +config REGULATOR_SY8106A
>>> + tristate "Silergy SY8106A"
>>> + depends on I2C
>>
>> Maybe you should also depend on OF since the driver is going to crippled
>> without any constraints set, or (OF || COMPILE_TEST) if you want some
>> compile test coverage.
>>
>>> + select REGMAP_I2C
>>> + help
>>> + This driver provides support for the voltage regulator SY8106A.
>>>
>>> +endif
>>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>>> index 85a1d44..f382095 100644
>>> --- a/drivers/regulator/Makefile
>>> +++ b/drivers/regulator/Makefile
>>> @@ -110,6 +110,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o
>>> obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o
>>> obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o
>>> obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o
>>> -
>>> +obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
>>
>> Follow the existing ordering in the Makefile.
>>
>>>
>>> ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
>>> diff --git a/drivers/regulator/sy8106a-regulator.c b/drivers/regulator/sy8106a-regulator.c
>>> new file mode 100644
>>> index 0000000..34bd69c
>>> --- /dev/null
>>> +++ b/drivers/regulator/sy8106a-regulator.c
>>> @@ -0,0 +1,153 @@
>>> +/*
>>> + * sy8106a-regulator.c - Regulator device driver for SY8106A
>>> + *
>>> + * Copyright (C) 2016 Ondřej Jirman <megous@megous.com>
>>> + *
>>> + * This library 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 library 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.
>>> + *
>>> + * You should have received a copy of the GNU Library General Public
>>> + * License along with this library; if not, write to the
>>> + * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor,
>>> + * Boston, MA 02110-1301, USA.
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>
>> Do you need this one?
>>
>>> +#include <linux/regulator/driver.h>
>>> +#include <linux/regulator/machine.h>
>>
>> And this one?
>>
>>> +#include <linux/regulator/of_regulator.h>
>>> +#include <linux/regmap.h>
>>
>> Sort alphabetically please.
>>
>>> +
>>> +#define SY8106A_REG_VOUT1_SEL 0x01
>>> +#define SY8106A_REG_VOUT_COM 0x02
>>> +#define SY8106A_REG_VOUT1_SEL_MASK 0x7f
>>> +#define SY8106A_DISABLE_REG 0x01
>>
>> BIT(0) would be clearer.
>>
>>> +
>>> +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 sel)
>>> +{
>>> + return regmap_update_bits(rdev->regmap, rdev->desc->vsel_reg,
>>> + 0xff, sel | 0x80);
>>
>> Can you use .apply_bit / .apply_reg with regulator_set_voltage_sel_regmap?
>
> I understand the code savings, but I'd rather avoid that, because it
> would involve 2 needless readouts and rewrites of the voltage setting
> register. I'd rather change this to regmap_write, so that the regulator
> only receives writes over I2C, to minimize possibility of bit flip error
> by minimizing the communication over the I2C bus.
OK. It's best to add a comment then, in case someone comes in and refactors it.
>
>>> +}
>>> +
>>> +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,
>>> +};
>>> +
>>> +/* 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,
>>> + .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.init_data = of_get_regulator_init_data(dev, dev->of_node, &sy8106a_reg);
>>
>> Check for possible failures?
>>
>>> + config.driver_data = chip;
>>> + config.regmap = chip->regmap;
>>> + config.of_node = dev->of_node;
>>> +
>>> + /* Probe regulator */
>>> + error = regmap_read(chip->regmap, SY8106A_REG_VOUT1_SEL, &selector);
>>> + dev_info(&i2c->dev, "SY8106A voltage at boot: %u mV\n", SY8106A_MIN_MV + SY8106A_STEP_MV * (selector & SY8106A_REG_VOUT1_SEL_MASK));
>>> + 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)) {
>>> + dev_err(&i2c->dev, "Failed to register SY8106A regulator\n");
>>> + return PTR_ERR(rdev);
>>> + }
>>> +
>>> + chip->rdev = rdev;
>>> +
>>> + i2c_set_clientdata(i2c, chip);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id sy8106a_i2c_id[] = {
>>> + {"sy8106a", 0},
>>> + {},
>>> +};
>>> +
>>
>> Extra line.
>>
>>> +MODULE_DEVICE_TABLE(i2c, sy8106a_i2c_id);
>>
>> IMHO probing explicitly through DT (of_device_id) would be better.
>
> I checked out a few recently added i2c regulator drivers, and this is
> the way they're written. I'd rather not differ, especially since I'm a
> beginner with this thing, atm. Also, I'm not sure what chanhge in
> particular you're suggesting.
See drivers/mfd/axp20x-i2c.c , the last 50 lines or so.
ChenYu
>
> thanks,
> Ondrej
>
>>
>> Regards
>> ChenYu
>>
>>> +
>>> +static struct i2c_driver sy8106a_regulator_driver = {
>>> + .driver = {
>>> + .name = "sy8106a",
>>> + },
>>> + .probe = sy8106a_i2c_probe,
>>> + .id_table = sy8106a_i2c_id,
>>> +};
>>> +
>>> +module_i2c_driver(sy8106a_regulator_driver);
>>> +
>>> +MODULE_AUTHOR("Ondřej Jirman <megous@megous.com>");
>>> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
>>> +MODULE_LICENSE("GPL v2");
>>> --
>>> 2.9.0
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web