Re: [PATCH 07/14] regulator: SY8106A regulator driver

From: Chen-Yu Tsai
Date: Thu Jun 23 2016 - 23:42:05 EST

Next message: Joe Perches: "[PATCH V3] printk: Create pr_<level> functions"
Previous message: Xiaolong Wang: "[PATCH] powercap/intel_rapl: Add support for Ivy Bridge server"
In reply to: megous: "[PATCH 07/14] regulator: SY8106A regulator driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jun 24, 2016 at 3:20 AM, <megous@xxxxxxxxxx> wrote:
> From: Ondrej Jirman <megous@xxxxxxxxxx>
>
> SY8106A is I2C attached single output voltage regulator
> made by Silergy.
>
> Signed-off-by: Ondrej Jirman <megous@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Regulator device driver for Silergy SY8106A");
> +MODULE_LICENSE("GPL v2");
> --
> 2.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Next message: Joe Perches: "[PATCH V3] printk: Create pr_<level> functions"
Previous message: Xiaolong Wang: "[PATCH] powercap/intel_rapl: Add support for Ivy Bridge server"
In reply to: megous: "[PATCH 07/14] regulator: SY8106A regulator driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]