* [RFC V1] COMMIT 1: DA9210 driver files
@ 2013-06-20 13:42 Steve Twiss
2013-06-21 9:58 ` Guennadi Liakhovetski
2013-06-21 15:15 ` Mark Brown
0 siblings, 2 replies; 9+ messages in thread
From: Steve Twiss @ 2013-06-20 13:42 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown; +Cc: David Dajun Chen, Guennadi Liakhovetski, LKML
From: Steve Twiss <stwiss.opensource@diasemi.com>
This is the regulator driver for the Dialog DA9210 Multi-phase Buck.
The patch is relative to linux-next next-20130620
The regulator implements the functions for .enable, .set_voltage,
.get_voltage, .set_current_limit, and .get_current_limit. It uses the
kernel standard functions for .disable, .is_enabled and .list_voltage.
This file contains a regulator driver and I2C driver combined into the
same file and has been tested on a Samsung SMDK6410 connected to a Dialog
DA9210 Evaluation Board through a I2C connection.
It would be appreciated if you could add any comments you may have about
the driver.
Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com>
Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
---
Checks performed with linux-next/next-20130620/scripts/checkpatch.pl
Kconfig total: 0 errors, 0 warnings, 535 lines checked
da9210-regulator.c total: 0 errors, 0 warnings, 334 lines checked
Makefile total: 0 errors, 0 warnings, 77 lines checked
da9210-regulator.h total: 0 errors, 0 warnings, 286 lines checked
drivers/regulator/Kconfig | 7 +
drivers/regulator/Makefile | 1 +
drivers/regulator/da9210-regulator.c | 334 ++++++++++++++++++++++++++++
include/linux/regulator/da9210-regulator.h | 286 ++++++++++++++++++++++++
4 files changed, 628 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/da9210-regulator.c
create mode 100644 include/linux/regulator/da9210-regulator.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9296425..aea2fc8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -293,6 +293,13 @@ config REGULATOR_LP8788
help
This driver supports LP8788 voltage regulator chip.
+config REGULATOR_DA9210
+ bool "Dialog Semiconductor DA9210 Regulator"
+ depends on I2C=y
+ select REGMAP_I2C
+ help
+ Support for the Dialog Semiconductor DA9210 chip.
+
config REGULATOR_PCF50633
tristate "NXP PCF50633 regulator driver"
depends on MFD_PCF50633
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 26e6c4a..0155e2a 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
obj-$(CONFIG_REGULATOR_DA9055) += da9055-regulator.o
+obj-$(CONFIG_REGULATOR_DA9210) += da9210-regulator.o
obj-$(CONFIG_REGULATOR_DBX500_PRCMU) += dbx500-prcmu.o
obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o
diff --git a/drivers/regulator/da9210-regulator.c b/drivers/regulator/da9210-regulator.c
new file mode 100644
index 0000000..5e6691e
--- /dev/null
+++ b/drivers/regulator/da9210-regulator.c
@@ -0,0 +1,334 @@
+
+/* da9210-regulator.c - Regulator device driver for DA9210
+ * Copyright (C) 2013 Dialog Semiconductor Ltd.
+ *
+ * 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/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/da9210-regulator.h>
+#include <linux/regmap.h>
+
+#define DRIVER_NAME "da9210"
+
+struct da9210_regulator_info {
+ int min_uV;
+ int max_uV;
+ unsigned step_uV;
+ unsigned n_steps;
+
+ unsigned n_current_limits;
+ const int *current_limits;
+};
+
+struct da9210 {
+ struct i2c_client *i2c;
+ struct device *dev;
+ struct mutex io_mutex;
+ const struct da9210_regulator_info *info;
+ struct regulator_desc desc;
+ struct regulator_dev *rdev;
+ struct regmap *regmap;
+};
+
+static struct regmap_config da9210_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static int da9210_enable(struct regulator_dev *rdev);
+static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
+ int max_uV, unsigned *selector);
+static int da9210_get_voltage(struct regulator_dev *rdev);
+static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
+ int max_uA);
+static int da9210_get_current_limit(struct regulator_dev *rdev);
+
+static struct regulator_ops da9210_buck_ops = {
+ .enable = da9210_enable,
+ .disable = regulator_disable_regmap,
+ .is_enabled = regulator_is_enabled_regmap,
+ .set_voltage = da9210_set_voltage,
+ .get_voltage = da9210_get_voltage,
+ .list_voltage = regulator_list_voltage_linear,
+ .set_current_limit = da9210_set_current_limit,
+ .get_current_limit = da9210_get_current_limit,
+};
+
+/* Default limits measured in millivolts and milliamps */
+#define DA9210_MIN_MV 300
+#define DA9210_MAX_MV 1570
+#define DA9210_MIN_MA 1600
+#define DA9210_MAX_MA 4600
+#define DA9210_STEP_MV 10
+
+/* Current limits for buck (uA) indices corresponds with register values */
+static const int da9210_buck_limits[] = {
+ 1600000, 1800000, 2000000, 2200000, 2400000, 2600000, 2800000, 3000000,
+ 3200000, 3400000, 3600000, 3800000, 4000000, 4200000, 4400000, 4600000
+};
+
+static const struct da9210_regulator_info da9210_info = {
+ .min_uV = (DA9210_MIN_MV * 1000),
+ .max_uV = (DA9210_MAX_MV * 1000),
+ .step_uV = (DA9210_STEP_MV * 1000),
+ .n_steps = (((DA9210_MAX_MV) - (DA9210_MIN_MV)) / (DA9210_STEP_MV)) + 1,
+ .current_limits = da9210_buck_limits,
+ .n_current_limits = ARRAY_SIZE(da9210_buck_limits),
+};
+
+static struct regulator_consumer_supply __initdata def_da9210_consumers[] = {
+ REGULATOR_SUPPLY("DA9210", NULL),
+};
+
+static struct regulator_init_data __initdata default_da9210_constraints = {
+ .constraints = {
+ .name = "DA9210-DEFAULT",
+ .min_uV = (DA9210_MIN_MV * 1000),
+ .max_uV = (DA9210_MAX_MV * 1000),
+ .min_uA = (DA9210_MIN_MA * 1000),
+ .max_uA = (DA9210_MAX_MA * 1000),
+ .uV_offset = 0,
+ .always_on = 0,
+ .boot_on = 0,
+ .apply_uV = 1,
+ .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |
+ REGULATOR_CHANGE_CURRENT |
+ REGULATOR_CHANGE_STATUS,
+ },
+ .num_consumer_supplies = ARRAY_SIZE(def_da9210_consumers),
+ .consumer_supplies = def_da9210_consumers,
+};
+
+static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
+ int max_uV, unsigned *selector)
+{
+ struct da9210 *chip = rdev_get_drvdata(rdev);
+ int val;
+ int ret;
+
+ val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
+ if (val < 0)
+ return -EINVAL;
+
+ ret = regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
+ DA9210_VBUCK_MASK, val);
+ return ret;
+}
+
+static int da9210_get_voltage_sel(struct regulator_dev *rdev)
+{
+ struct da9210 *chip = rdev_get_drvdata(rdev);
+ unsigned int data;
+ int sel;
+ int ret;
+
+ ret = regmap_read(chip->regmap, DA9210_REG_VBUCK_A, &data);
+ if (ret < 0)
+ return ret;
+
+ sel = (data & DA9210_VBUCK_MASK) >> DA9210_VBUCK_SHIFT;
+ sel -= DA9210_VBUCK_BIAS;
+ if (sel < 0)
+ sel = 0;
+ if (sel >= chip->info->n_steps)
+ sel = chip->info->n_steps - 1;
+
+ return sel;
+}
+
+static int da9210_get_voltage(struct regulator_dev *rdev)
+{
+ struct da9210 *chip = rdev_get_drvdata(rdev);
+ int sel = da9210_get_voltage_sel(rdev);
+
+ if (sel < 0)
+ return sel;
+
+ return (chip->info->step_uV * sel) + chip->info->min_uV;
+}
+
+static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
+ int max_uA)
+{
+ struct da9210 *chip = rdev_get_drvdata(rdev);
+ unsigned int sel;
+ int i;
+
+ if (!chip->info->current_limits)
+ return -EINVAL;
+
+ /* search for closest to maximum */
+ for (i = chip->info->n_current_limits - 1; i >= 0; i--) {
+ if (min_uA <= chip->info->current_limits[i] &&
+ max_uA >= chip->info->current_limits[i]) {
+ sel = i;
+ sel = sel << DA9210_BUCK_ILIM_SHIFT;
+ return regmap_update_bits(chip->regmap,
+ DA9210_REG_BUCK_ILIM,
+ DA9210_BUCK_ILIM_MASK, sel);
+ }
+ }
+
+ return -EINVAL;
+}
+
+static int da9210_get_current_limit(struct regulator_dev *rdev)
+{
+ struct da9210 *chip = rdev_get_drvdata(rdev);
+ unsigned int data;
+ int sel;
+ int ret;
+
+ ret = regmap_read(chip->regmap, DA9210_REG_BUCK_ILIM, &data);
+ if (ret < 0)
+ return ret;
+
+ sel = (data & DA9210_BUCK_ILIM_MASK) >> DA9210_BUCK_ILIM_SHIFT;
+ if (sel < 0)
+ sel = 0;
+ if (sel >= chip->info->n_current_limits)
+ sel = chip->info->n_current_limits - 1;
+
+ return chip->info->current_limits[sel];
+}
+
+static int da9210_enable(struct regulator_dev *rdev)
+{
+ return regulator_enable_regmap(rdev);
+}
+
+/* I2C driver interface functions.
+ */
+
+static int da9210_i2c_probe(struct i2c_client *i2c,
+ const struct i2c_device_id *id)
+{
+ struct da9210 *chip;
+ const struct regulator_init_data *init_data = NULL;
+ struct regulator_dev *rdev = NULL;
+ struct regulator_config config = { };
+ int error;
+
+ chip = devm_kzalloc(&i2c->dev, sizeof(struct da9210), GFP_KERNEL);
+ if (NULL == chip) {
+ dev_err(&i2c->dev,
+ "Cannot kzalloc memory for regulator structure\n");
+ return -ENOMEM;
+ }
+
+ chip->regmap = devm_regmap_init_i2c(i2c, &da9210_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;
+ }
+
+ chip->dev = &i2c->dev;
+ chip->i2c = i2c;
+ chip->info = &da9210_info;
+
+ mutex_init(&chip->io_mutex);
+
+ chip->desc.name = "DA9210";
+ chip->desc.ops = &da9210_buck_ops;
+ chip->desc.n_voltages = chip->info->n_steps;
+ chip->desc.type = REGULATOR_VOLTAGE;
+ chip->desc.owner = THIS_MODULE;
+ chip->desc.id = 0;
+ chip->desc.min_uV = chip->info->min_uV;
+ chip->desc.uV_step = chip->info->step_uV;
+ chip->desc.enable_reg = DA9210_REG_BUCK_CONT;
+ chip->desc.enable_mask = DA9210_BUCK_EN;
+
+ if (!i2c->dev.platform_data)
+ init_data = &default_da9210_constraints;
+ else
+ init_data = i2c->dev.platform_data;
+
+ config.dev = &i2c->dev;
+ config.init_data = init_data;
+ config.driver_data = chip;
+ config.regmap = chip->regmap;
+
+ rdev = regulator_register(&chip->desc, &config);
+ if (IS_ERR(rdev)) {
+ dev_err(chip->dev, "Failed to register DA9210 regulator\n");
+ chip->rdev = NULL;
+ return PTR_ERR(rdev);
+ }
+
+ chip->rdev = rdev;
+
+ i2c_set_clientdata(i2c, chip);
+
+ dev_info(chip->dev, "Device DA9210 detected.\n");
+ return 0;
+}
+
+static int da9210_i2c_remove(struct i2c_client *i2c)
+{
+ struct da9210 *chip = i2c_get_clientdata(i2c);
+ regulator_unregister(chip->rdev);
+ return 0;
+}
+
+static const struct i2c_device_id da9210_i2c_id[] = {
+ {DRIVER_NAME, 0},
+ {},
+};
+
+MODULE_DEVICE_TABLE(i2c, da9210_i2c_id);
+
+static struct i2c_driver da9210_regulator_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ },
+ .probe = da9210_i2c_probe,
+ .remove = da9210_i2c_remove,
+ .id_table = da9210_i2c_id,
+};
+
+static int __init da9210_regulator_init(void)
+{
+ int ret;
+
+ ret = i2c_add_driver(&da9210_regulator_driver);
+ if (0 != ret)
+ pr_err("Failed to register da9210 I2C driver\n");
+
+ return ret;
+}
+
+subsys_initcall(da9210_regulator_init);
+
+static void __exit da9210_regulator_cleanup(void)
+{
+ i2c_del_driver(&da9210_regulator_driver);
+}
+
+module_exit(da9210_regulator_cleanup);
+
+MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
+MODULE_DESCRIPTION("Regulator device driver for Dialog DA9210");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRIVER_NAME);
diff --git a/include/linux/regulator/da9210-regulator.h b/include/linux/regulator/da9210-regulator.h
new file mode 100644
index 0000000..e6748d6
--- /dev/null
+++ b/include/linux/regulator/da9210-regulator.h
@@ -0,0 +1,286 @@
+
+/* da9210-regulator.h - Regulator definitions for DA9210
+ * Copyright (C) 2013 Dialog Semiconductor Ltd.
+ *
+ * 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.
+ */
+
+#ifndef __DA9210_REGISTERS_H__
+#define __DA9210_REGISTERS_H__
+
+/* Page selection I2C or SPI always in the begining of any page. */
+/* Page 0 : I2C access 0x000 - 0x0FF SPI access 0x000 - 0x07F */
+/* Page 1 : SPI access 0x080 - 0x0FF */
+/* Page 2 : I2C access 0x100 - 0x17F SPI access 0x100 - 0x17F */
+#define DA9210_REG_PAGE_CON 0x00
+
+/* System Control and Event Registers */
+#define DA9210_REG_STATUS_A 0x50
+#define DA9210_REG_STATUS_B 0x51
+#define DA9210_REG_EVENT_A 0x52
+#define DA9210_REG_EVENT_B 0x53
+#define DA9210_REG_MASK_A 0x54
+#define DA9210_REG_MASK_B 0x55
+#define DA9210_REG_CONTROL_A 0x56
+
+/* GPIO Control Registers */
+#define DA9210_REG_GPIO_0_1 0x58
+#define DA9210_REG_GPIO_2_3 0x59
+#define DA9210_REG_GPIO_4_5 0x5A
+#define DA9210_REG_GPIO_6 0x5B
+
+/* Regulator Registers */
+#define DA9210_REG_BUCK_CONT 0x5D
+#define DA9210_REG_BUCK_ILIM 0xD0
+#define DA9210_REG_BUCK_CONF1 0xD1
+#define DA9210_REG_BUCK_CONF2 0xD2
+#define DA9210_REG_VBACK_AUTO 0xD4
+#define DA9210_REG_VBACK_BASE 0xD5
+#define DA9210_REG_VBACK_MAX_DVC_IF 0xD6
+#define DA9210_REG_VBACK_DVC 0xD7
+#define DA9210_REG_VBUCK_A 0xD8
+#define DA9210_REG_VBUCK_B 0xD9
+
+/* I2C Interface Settings */
+#define DA9210_REG_INTERFACE 0x105
+
+/* OTP */
+#define DA9210_REG_OPT_COUNT 0x140
+#define DA9210_REG_OPT_ADDR 0x141
+#define DA9210_REG_OPT_DATA 0x142
+
+/* Customer Trim and Configuration */
+#define DA9210_REG_CONFIG_A 0x143
+#define DA9210_REG_CONFIG_B 0x144
+#define DA9210_REG_CONFIG_C 0x145
+#define DA9210_REG_CONFIG_D 0x146
+#define DA9210_REG_CONFIG_E 0x147
+
+
+/*
+ * Registers bits
+ */
+/* DA9210_REG_PAGE_CON (addr=0x00) */
+#define DA9210_PEG_PAGE_SHIFT 0
+#define DA9210_REG_PAGE_MASK 0x0F
+/* On I2C registers 0x00 - 0xFF */
+#define DA9210_REG_PAGE0 0
+/* On I2C registers 0x100 - 0x1FF */
+#define DA9210_REG_PAGE2 2
+#define DA9210_PAGE_WRITE_MODE 0x00
+#define DA9210_REPEAT_WRITE_MODE 0x40
+#define DA9210_PAGE_REVERT 0x80
+
+/* DA9210_REG_STATUS_A (addr=0x50) */
+#define DA9210_GPI0 0x01
+#define DA9210_GPI1 0x02
+#define DA9210_GPI2 0x04
+#define DA9210_GPI3 0x08
+#define DA9210_GPI4 0x10
+#define DA9210_GPI5 0x20
+#define DA9210_GPI6 0x40
+
+/* DA9210_REG_EVENT_A (addr=0x52) */
+#define DA9210_E_GPI0 0x01
+#define DA9210_E_GPI1 0x02
+#define DA9210_E_GPI2 0x04
+#define DA9210_E_GPI3 0x08
+#define DA9210_E_GPI4 0x10
+#define DA9210_E_GPI5 0x20
+#define DA9210_E_GPI6 0x40
+
+/* DA9210_REG_EVENT_B (addr=0x53) */
+#define DA9210_E_OVCURR 0x01
+#define DA9210_E_NPWRGOOD 0x02
+#define DA9210_E_TEMP_WARN 0x04
+#define DA9210_E_TEMP_CRIT 0x08
+#define DA9210_E_VMAX 0x10
+
+/* DA9210_REG_MASK_A (addr=0x54) */
+#define DA9210_M_GPI0 0x01
+#define DA9210_M_GPI1 0x02
+#define DA9210_M_GPI2 0x04
+#define DA9210_M_GPI3 0x08
+#define DA9210_M_GPI4 0x10
+#define DA9210_M_GPI5 0x20
+#define DA9210_M_GPI6 0x40
+
+/* DA9210_REG_MASK_B (addr=0x55) */
+#define DA9210_M_OVCURR 0x01
+#define DA9210_M_NPWRGOOD 0x02
+#define DA9210_M_TEMP_WARN 0x04
+#define DA9210_M_TEMP_CRIT 0x08
+#define DA9210_M_VMAX 0x10
+
+/* DA9210_REG_CONTROL_A (addr=0x56) */
+#define DA9210_DEBOUNCING_SHIFT 0
+#define DA9210_DEBOUNCING_MASK 0x07
+#define DA9210_SLEW_RATE_SHIFT 3
+#define DA9210_SLEW_RATE_MASK 0x18
+#define DA9210_V_LOCK 0x20
+
+/* DA9210_REG_GPIO_0_1 (addr=0x58) */
+#define DA9210_GPIO0_PIN_SHIFT 0
+#define DA9210_GPIO0_PIN_MASK 0x03
+#define DA9210_GPIO0_PIN_GPI 0x00
+#define DA9210_GPIO0_PIN_GPO_OD 0x02
+#define DA9210_GPIO0_PIN_GPO 0x03
+#define DA9210_GPIO0_TYPE 0x04
+#define DA9210_GPIO0_TYPE_GPI 0x00
+#define DA9210_GPIO0_TYPE_GPO 0x04
+#define DA9210_GPIO0_MODE 0x08
+#define DA9210_GPIO1_PIN_SHIFT 4
+#define DA9210_GPIO1_PIN_MASK 0x30
+#define DA9210_GPIO1_PIN_GPI 0x00
+#define DA9210_GPIO1_PIN_VERROR 0x10
+#define DA9210_GPIO1_PIN_GPO_OD 0x20
+#define DA9210_GPIO1_PIN_GPO 0x30
+#define DA9210_GPIO1_TYPE_SHIFT 0x40
+#define DA9210_GPIO1_TYPE_GPI 0x00
+#define DA9210_GPIO1_TYPE_GPO 0x40
+#define DA9210_GPIO1_MODE 0x80
+
+/* DA9210_REG_GPIO_2_3 (addr=0x59) */
+#define DA9210_GPIO2_PIN_SHIFT 0
+#define DA9210_GPIO2_PIN_MASK 0x03
+#define DA9210_GPIO2_PIN_GPI 0x00
+#define DA9210_GPIO5_PIN_BUCK_CLK 0x10
+#define DA9210_GPIO2_PIN_GPO_OD 0x02
+#define DA9210_GPIO2_PIN_GPO 0x03
+#define DA9210_GPIO2_TYPE 0x04
+#define DA9210_GPIO2_TYPE_GPI 0x00
+#define DA9210_GPIO2_TYPE_GPO 0x04
+#define DA9210_GPIO2_MODE 0x08
+#define DA9210_GPIO3_PIN_SHIFT 4
+#define DA9210_GPIO3_PIN_MASK 0x30
+#define DA9210_GPIO3_PIN_GPI 0x00
+#define DA9210_GPIO3_PIN_IERROR 0x10
+#define DA9210_GPIO3_PIN_GPO_OD 0x20
+#define DA9210_GPIO3_PIN_GPO 0x30
+#define DA9210_GPIO3_TYPE_SHIFT 0x40
+#define DA9210_GPIO3_TYPE_GPI 0x00
+#define DA9210_GPIO3_TYPE_GPO 0x40
+#define DA9210_GPIO3_MODE 0x80
+
+/* DA9210_REG_GPIO_4_5 (addr=0x5A) */
+#define DA9210_GPIO4_PIN_SHIFT 0
+#define DA9210_GPIO4_PIN_MASK 0x03
+#define DA9210_GPIO4_PIN_GPI 0x00
+#define DA9210_GPIO4_PIN_GPO_OD 0x02
+#define DA9210_GPIO4_PIN_GPO 0x03
+#define DA9210_GPIO4_TYPE 0x04
+#define DA9210_GPIO4_TYPE_GPI 0x00
+#define DA9210_GPIO4_TYPE_GPO 0x04
+#define DA9210_GPIO4_MODE 0x08
+#define DA9210_GPIO5_PIN_SHIFT 4
+#define DA9210_GPIO5_PIN_MASK 0x30
+#define DA9210_GPIO5_PIN_GPI 0x00
+#define DA9210_GPIO5_PIN_INTERFACE 0x01
+#define DA9210_GPIO5_PIN_GPO_OD 0x20
+#define DA9210_GPIO5_PIN_GPO 0x30
+#define DA9210_GPIO5_TYPE_SHIFT 0x40
+#define DA9210_GPIO5_TYPE_GPI 0x00
+#define DA9210_GPIO5_TYPE_GPO 0x40
+#define DA9210_GPIO5_MODE 0x80
+
+/* DA9210_REG_GPIO_6 (addr=0x5B) */
+#define DA9210_GPIO6_PIN_SHIFT 0
+#define DA9210_GPIO6_PIN_MASK 0x03
+#define DA9210_GPIO6_PIN_GPI 0x00
+#define DA9210_GPIO6_PIN_INTERFACE 0x01
+#define DA9210_GPIO6_PIN_GPO_OD 0x02
+#define DA9210_GPIO6_PIN_GPO 0x03
+#define DA9210_GPIO6_TYPE 0x04
+#define DA9210_GPIO6_TYPE_GPI 0x00
+#define DA9210_GPIO6_TYPE_GPO 0x04
+#define DA9210_GPIO6_MODE 0x08
+
+/* DA9210_REG_BUCK_CONT (addr=0x5D) */
+#define DA9210_BUCK_EN 0x01
+#define DA9210_BUCK_GPI_SHIFT 1
+#define DA9210_BUCK_GPI_MASK 0x06
+#define DA9210_BUCK_GPI_OFF 0x00
+#define DA9210_BUCK_GPI_GPIO0 0x02
+#define DA9210_BUCK_GPI_GPIO3 0x04
+#define DA9210_BUCK_GPI_GPIO4 0x06
+#define DA9210_BUCK_PD_DIS 0x08
+#define DA9210_VBUCK_SEL 0x10
+#define DA9210_VBUCK_SEL_A 0x00
+#define DA9210_VBUCK_SEL_B 0x10
+#define DA9210_VBUCK_GPI_SHIFT 5
+#define DA9210_VBUCK_GPI_MASK 0x60
+#define DA9210_VBUCK_GPI_OFF 0x00
+#define DA9210_VBUCK_GPI_GPIO0 0x20
+#define DA9210_VBUCK_GPI_GPIO3 0x40
+#define DA9210_VBUCK_GPI_GPIO4 0x60
+#define DA9210_DVC_CTRL_EN 0x80
+
+/* DA9210_REG_BUCK_ILIM (addr=0xD0) */
+#define DA9210_BUCK_ILIM_SHIFT 0
+#define DA9210_BUCK_ILIM_MASK 0x0F
+#define DA9210_BUCK_IALARM 0x10
+
+/* DA9210_REG_BUCK_CONF1 (addr=0xD1) */
+#define DA9210_BUCK_MODE_SHIFT 0
+#define DA9210_BUCK_MODE_MASK 0x03
+#define DA9210_BUCK_MODE_MANUAL 0x00
+#define DA9210_BUCK_MODE_SLEEP 0x01
+#define DA9210_BUCK_MODE_SYNC 0x02
+#define DA9210_BUCK_MODE_AUTO 0x03
+#define DA9210_STARTUP_CTRL_SHIFT 2
+#define DA9210_STARTUP_CTRL_MASK 0x1C
+#define DA9210_PWR_DOWN_CTRL_SHIFT 5
+#define DA9210_PWR_DOWN_CTRL_MASK 0xE0
+
+/* DA9210_REG_BUCK_CONF2 (addr=0xD2) */
+#define DA9210_PHASE_SEL_SHIFT 0
+#define DA9210_PHASE_SEL_MASK 0x03
+#define DA9210_FREQ_SEL 0x40
+
+/* DA9210_REG_BUCK_AUTO (addr=0xD4) */
+#define DA9210_VBUCK_AUTO_SHIFT 0
+#define DA9210_VBUCK_AUTO_MASK 0x7F
+
+/* DA9210_REG_BUCK_BASE (addr=0xD5) */
+#define DA9210_VBUCK_BASE_SHIFT 0
+#define DA9210_VBUCK_BASE_MASK 0x7F
+
+/* DA9210_REG_VBUCK_MAX_DVC_IF (addr=0xD6) */
+#define DA9210_VBUCK_MAX_SHIFT 0
+#define DA9210_VBUCK_MAX_MASK 0x7F
+#define DA9210_DVC_STEP_SIZE 0x80
+#define DA9210_DVC_STEP_SIZE_10MV 0x00
+#define DA9210_DVC_STEP_SIZE_20MV 0x80
+
+/* DA9210_REG_VBUCK_DVC (addr=0xD7) */
+#define DA9210_VBUCK_DVC_SHIFT 0
+#define DA9210_VBUCK_DVC_MASK 0x7F
+
+/* DA9210_REG_VBUCK_A/B (addr=0xD8/0xD9) */
+#define DA9210_VBUCK_SHIFT 0
+#define DA9210_VBUCK_MASK 0x7F
+#define DA9210_VBUCK_BIAS 0
+#define DA9210_BUCK_SL 0x80
+
+/* DA9210_REG_INTERFACE (addr=0x105) */
+#define DA9210_IF_BASE_ADDR_SHIFT 4
+#define DA9210_IF_BASE_ADDR_MASK 0xF0
+
+/* DA9210_REG_CONFIG_E (addr=0x147) */
+#define DA9210_STAND_ALONE 0x01
+
+#endif /* __DA9210_REGISTERS_H__ */
+
--
end-of-patch for RFC V1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-20 13:42 [RFC V1] COMMIT 1: DA9210 driver files Steve Twiss
@ 2013-06-21 9:58 ` Guennadi Liakhovetski
2013-06-24 9:17 ` Opensource [Steve Twiss]
2013-06-21 15:15 ` Mark Brown
1 sibling, 1 reply; 9+ messages in thread
From: Guennadi Liakhovetski @ 2013-06-21 9:58 UTC (permalink / raw)
To: Steve Twiss; +Cc: Liam Girdwood, Mark Brown, David Dajun Chen, LKML
Hi Steve
Thanks a lot for the patch! I'm not an expert in this area, so, just some
comments. I'm sure other persons on your CC list will have more of them.
On Thu, 20 Jun 2013, Steve Twiss wrote:
> From: Steve Twiss <stwiss.opensource@diasemi.com>
>
> This is the regulator driver for the Dialog DA9210 Multi-phase Buck.
> The patch is relative to linux-next next-20130620
>
> The regulator implements the functions for .enable, .set_voltage,
> .get_voltage, .set_current_limit, and .get_current_limit. It uses the
> kernel standard functions for .disable, .is_enabled and .list_voltage.
>
> This file contains a regulator driver and I2C driver combined into the
> same file and has been tested on a Samsung SMDK6410 connected to a Dialog
> DA9210 Evaluation Board through a I2C connection.
>
> It would be appreciated if you could add any comments you may have about
> the driver.
>
> Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com>
> Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
> ---
> Checks performed with linux-next/next-20130620/scripts/checkpatch.pl
> Kconfig total: 0 errors, 0 warnings, 535 lines checked
> da9210-regulator.c total: 0 errors, 0 warnings, 334 lines checked
> Makefile total: 0 errors, 0 warnings, 77 lines checked
> da9210-regulator.h total: 0 errors, 0 warnings, 286 lines checked
>
> drivers/regulator/Kconfig | 7 +
> drivers/regulator/Makefile | 1 +
> drivers/regulator/da9210-regulator.c | 334 ++++++++++++++++++++++++++++
> include/linux/regulator/da9210-regulator.h | 286 ++++++++++++++++++++++++
> 4 files changed, 628 insertions(+), 0 deletions(-)
> create mode 100644 drivers/regulator/da9210-regulator.c
> create mode 100644 include/linux/regulator/da9210-regulator.h
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 9296425..aea2fc8 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -293,6 +293,13 @@ config REGULATOR_LP8788
> help
> This driver supports LP8788 voltage regulator chip.
>
> +config REGULATOR_DA9210
> + bool "Dialog Semiconductor DA9210 Regulator"
You don't support modular build? I think you should.
> + depends on I2C=y
Then you won't need =y either
> + select REGMAP_I2C
> + help
> + Support for the Dialog Semiconductor DA9210 chip.
> +
> config REGULATOR_PCF50633
> tristate "NXP PCF50633 regulator driver"
> depends on MFD_PCF50633
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 26e6c4a..0155e2a 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
> obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
> obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
> obj-$(CONFIG_REGULATOR_DA9055) += da9055-regulator.o
> +obj-$(CONFIG_REGULATOR_DA9210) += da9210-regulator.o
> obj-$(CONFIG_REGULATOR_DBX500_PRCMU) += dbx500-prcmu.o
> obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
> obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o
> diff --git a/drivers/regulator/da9210-regulator.c b/drivers/regulator/da9210-regulator.c
> new file mode 100644
> index 0000000..5e6691e
> --- /dev/null
> +++ b/drivers/regulator/da9210-regulator.c
> @@ -0,0 +1,334 @@
> +
> +/* da9210-regulator.c - Regulator device driver for DA9210
> + * Copyright (C) 2013 Dialog Semiconductor Ltd.
> + *
> + * 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/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/da9210-regulator.h>
> +#include <linux/regmap.h>
This driver doesn't support DT. This is ok for the first submission, I
think, but we'll definitely need it soon.
Some headers are also missing here, e.g. slab.h, init.h
> +
> +#define DRIVER_NAME "da9210"
> +
> +struct da9210_regulator_info {
> + int min_uV;
> + int max_uV;
> + unsigned step_uV;
> + unsigned n_steps;
> +
> + unsigned n_current_limits;
> + const int *current_limits;
> +};
> +
> +struct da9210 {
> + struct i2c_client *i2c;
> + struct device *dev;
> + struct mutex io_mutex;
> + const struct da9210_regulator_info *info;
> + struct regulator_desc desc;
> + struct regulator_dev *rdev;
> + struct regmap *regmap;
> +};
> +
> +static struct regmap_config da9210_regmap_config = {
you can make this const
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static int da9210_enable(struct regulator_dev *rdev);
> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
> + int max_uV, unsigned *selector);
> +static int da9210_get_voltage(struct regulator_dev *rdev);
> +static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
> + int max_uA);
> +static int da9210_get_current_limit(struct regulator_dev *rdev);
> +
> +static struct regulator_ops da9210_buck_ops = {
> + .enable = da9210_enable,
Why don't you just assign regulator_enable_regmap here?
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_voltage = da9210_set_voltage,
> + .get_voltage = da9210_get_voltage,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_current_limit = da9210_set_current_limit,
> + .get_current_limit = da9210_get_current_limit,
> +};
> +
> +/* Default limits measured in millivolts and milliamps */
> +#define DA9210_MIN_MV 300
> +#define DA9210_MAX_MV 1570
> +#define DA9210_MIN_MA 1600
> +#define DA9210_MAX_MA 4600
> +#define DA9210_STEP_MV 10
> +
> +/* Current limits for buck (uA) indices corresponds with register values */
> +static const int da9210_buck_limits[] = {
> + 1600000, 1800000, 2000000, 2200000, 2400000, 2600000, 2800000, 3000000,
> + 3200000, 3400000, 3600000, 3800000, 4000000, 4200000, 4400000, 4600000
> +};
> +
> +static const struct da9210_regulator_info da9210_info = {
> + .min_uV = (DA9210_MIN_MV * 1000),
> + .max_uV = (DA9210_MAX_MV * 1000),
> + .step_uV = (DA9210_STEP_MV * 1000),
> + .n_steps = (((DA9210_MAX_MV) - (DA9210_MIN_MV)) / (DA9210_STEP_MV)) + 1,
Way too many parenthesis above. Strictly speaking you don't need this
struct, you could just use macros / constant variables and remove a bunch
of pointer dereferences, but it's up to you, I think.
> + .current_limits = da9210_buck_limits,
> + .n_current_limits = ARRAY_SIZE(da9210_buck_limits),
> +};
> +
> +static struct regulator_consumer_supply __initdata def_da9210_consumers[] = {
> + REGULATOR_SUPPLY("DA9210", NULL),
> +};
> +
> +static struct regulator_init_data __initdata default_da9210_constraints = {
> + .constraints = {
> + .name = "DA9210-DEFAULT",
> + .min_uV = (DA9210_MIN_MV * 1000),
> + .max_uV = (DA9210_MAX_MV * 1000),
> + .min_uA = (DA9210_MIN_MA * 1000),
> + .max_uA = (DA9210_MAX_MA * 1000),
> + .uV_offset = 0,
> + .always_on = 0,
> + .boot_on = 0,
> + .apply_uV = 1,
> + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |
> + REGULATOR_CHANGE_CURRENT |
> + REGULATOR_CHANGE_STATUS,
> + },
> + .num_consumer_supplies = ARRAY_SIZE(def_da9210_consumers),
> + .consumer_supplies = def_da9210_consumers,
> +};
Oops, no, none of the above two structs belong here. Please, remove.
> +
> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
> + int max_uV, unsigned *selector)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + int val;
> + int ret;
> +
> + val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
> + if (val < 0)
> + return -EINVAL;
"return val" would be better
> +
> + ret = regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
> + DA9210_VBUCK_MASK, val);
> + return ret;
just
+ return regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
+ DA9210_VBUCK_MASK, val);
> +}
> +
> +static int da9210_get_voltage_sel(struct regulator_dev *rdev)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + unsigned int data;
> + int sel;
> + int ret;
> +
> + ret = regmap_read(chip->regmap, DA9210_REG_VBUCK_A, &data);
> + if (ret < 0)
> + return ret;
> +
> + sel = (data & DA9210_VBUCK_MASK) >> DA9210_VBUCK_SHIFT;
> + sel -= DA9210_VBUCK_BIAS;
> + if (sel < 0)
> + sel = 0;
> + if (sel >= chip->info->n_steps)
> + sel = chip->info->n_steps - 1;
> +
> + return sel;
> +}
> +
> +static int da9210_get_voltage(struct regulator_dev *rdev)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + int sel = da9210_get_voltage_sel(rdev);
> +
> + if (sel < 0)
> + return sel;
> +
> + return (chip->info->step_uV * sel) + chip->info->min_uV;
superfluous parenthesis
> +}
> +
> +static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
> + int max_uA)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + unsigned int sel;
> + int i;
> +
> + if (!chip->info->current_limits)
> + return -EINVAL;
It's not really possible :) But, well, if you want this check...
> +
> + /* search for closest to maximum */
> + for (i = chip->info->n_current_limits - 1; i >= 0; i--) {
> + if (min_uA <= chip->info->current_limits[i] &&
> + max_uA >= chip->info->current_limits[i]) {
> + sel = i;
> + sel = sel << DA9210_BUCK_ILIM_SHIFT;
> + return regmap_update_bits(chip->regmap,
> + DA9210_REG_BUCK_ILIM,
> + DA9210_BUCK_ILIM_MASK, sel);
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int da9210_get_current_limit(struct regulator_dev *rdev)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + unsigned int data;
> + int sel;
> + int ret;
> +
> + ret = regmap_read(chip->regmap, DA9210_REG_BUCK_ILIM, &data);
> + if (ret < 0)
> + return ret;
> +
> + sel = (data & DA9210_BUCK_ILIM_MASK) >> DA9210_BUCK_ILIM_SHIFT;
> + if (sel < 0)
Don't think sel can be < 0 here.
> + sel = 0;
> + if (sel >= chip->info->n_current_limits)
Your sel is (x & 0xf) >> 0, so, it's 0 <= sel < 16. n_current_limits above
== 16, so, this is impossible too. If you had simple constants instead of
structs, this would have been simpler to follow.
> + sel = chip->info->n_current_limits - 1;
> +
> + return chip->info->current_limits[sel];
> +}
> +
> +static int da9210_enable(struct regulator_dev *rdev)
> +{
> + return regulator_enable_regmap(rdev);
> +}
> +
> +/* I2C driver interface functions.
> + */
> +
> +static int da9210_i2c_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct da9210 *chip;
> + const struct regulator_init_data *init_data = NULL;
> + struct regulator_dev *rdev = NULL;
> + struct regulator_config config = { };
> + int error;
> +
> + chip = devm_kzalloc(&i2c->dev, sizeof(struct da9210), GFP_KERNEL);
> + if (NULL == chip) {
> + dev_err(&i2c->dev,
> + "Cannot kzalloc memory for regulator structure\n");
> + return -ENOMEM;
> + }
> +
> + chip->regmap = devm_regmap_init_i2c(i2c, &da9210_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;
> + }
> +
> + chip->dev = &i2c->dev;
> + chip->i2c = i2c;
you don't use either of the above two pointers. To be precise, you do use
chip->dev a couple of times below, but you could just use &i2c->dev there
too.
> + chip->info = &da9210_info;
> +
> + mutex_init(&chip->io_mutex);
unused?
> +
> + chip->desc.name = "DA9210";
> + chip->desc.ops = &da9210_buck_ops;
> + chip->desc.n_voltages = chip->info->n_steps;
> + chip->desc.type = REGULATOR_VOLTAGE;
> + chip->desc.owner = THIS_MODULE;
> + chip->desc.id = 0;
> + chip->desc.min_uV = chip->info->min_uV;
> + chip->desc.uV_step = chip->info->step_uV;
> + chip->desc.enable_reg = DA9210_REG_BUCK_CONT;
> + chip->desc.enable_mask = DA9210_BUCK_EN;
> +
> + if (!i2c->dev.platform_data)
> + init_data = &default_da9210_constraints;
> + else
> + init_data = i2c->dev.platform_data;
> +
> + config.dev = &i2c->dev;
> + config.init_data = init_data;
> + config.driver_data = chip;
> + config.regmap = chip->regmap;
> +
> + rdev = regulator_register(&chip->desc, &config);
> + if (IS_ERR(rdev)) {
> + dev_err(chip->dev, "Failed to register DA9210 regulator\n");
> + chip->rdev = NULL;
You don't need this assignment
> + return PTR_ERR(rdev);
> + }
> +
> + chip->rdev = rdev;
> +
> + i2c_set_clientdata(i2c, chip);
> +
> + dev_info(chip->dev, "Device DA9210 detected.\n");
> + return 0;
> +}
> +
> +static int da9210_i2c_remove(struct i2c_client *i2c)
> +{
> + struct da9210 *chip = i2c_get_clientdata(i2c);
> + regulator_unregister(chip->rdev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id da9210_i2c_id[] = {
> + {DRIVER_NAME, 0},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, da9210_i2c_id);
> +
> +static struct i2c_driver da9210_regulator_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = da9210_i2c_probe,
> + .remove = da9210_i2c_remove,
> + .id_table = da9210_i2c_id,
> +};
> +
> +static int __init da9210_regulator_init(void)
> +{
> + int ret;
> +
> + ret = i2c_add_driver(&da9210_regulator_driver);
> + if (0 != ret)
> + pr_err("Failed to register da9210 I2C driver\n");
> +
> + return ret;
> +}
> +
> +subsys_initcall(da9210_regulator_init);
> +
> +static void __exit da9210_regulator_cleanup(void)
> +{
> + i2c_del_driver(&da9210_regulator_driver);
> +}
> +
> +module_exit(da9210_regulator_cleanup);
> +
> +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
> +MODULE_DESCRIPTION("Regulator device driver for Dialog DA9210");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> diff --git a/include/linux/regulator/da9210-regulator.h b/include/linux/regulator/da9210-regulator.h
> new file mode 100644
> index 0000000..e6748d6
> --- /dev/null
> +++ b/include/linux/regulator/da9210-regulator.h
You don't need this header under include/... Please, either move it to
drivers/regulator/ or just move all these defines into the .c file.
Thanks
Guennadi
> @@ -0,0 +1,286 @@
> +
> +/* da9210-regulator.h - Regulator definitions for DA9210
> + * Copyright (C) 2013 Dialog Semiconductor Ltd.
> + *
> + * 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.
> + */
> +
> +#ifndef __DA9210_REGISTERS_H__
> +#define __DA9210_REGISTERS_H__
> +
> +/* Page selection I2C or SPI always in the begining of any page. */
> +/* Page 0 : I2C access 0x000 - 0x0FF SPI access 0x000 - 0x07F */
> +/* Page 1 : SPI access 0x080 - 0x0FF */
> +/* Page 2 : I2C access 0x100 - 0x17F SPI access 0x100 - 0x17F */
> +#define DA9210_REG_PAGE_CON 0x00
> +
> +/* System Control and Event Registers */
> +#define DA9210_REG_STATUS_A 0x50
> +#define DA9210_REG_STATUS_B 0x51
> +#define DA9210_REG_EVENT_A 0x52
> +#define DA9210_REG_EVENT_B 0x53
> +#define DA9210_REG_MASK_A 0x54
> +#define DA9210_REG_MASK_B 0x55
> +#define DA9210_REG_CONTROL_A 0x56
> +
> +/* GPIO Control Registers */
> +#define DA9210_REG_GPIO_0_1 0x58
> +#define DA9210_REG_GPIO_2_3 0x59
> +#define DA9210_REG_GPIO_4_5 0x5A
> +#define DA9210_REG_GPIO_6 0x5B
> +
> +/* Regulator Registers */
> +#define DA9210_REG_BUCK_CONT 0x5D
> +#define DA9210_REG_BUCK_ILIM 0xD0
> +#define DA9210_REG_BUCK_CONF1 0xD1
> +#define DA9210_REG_BUCK_CONF2 0xD2
> +#define DA9210_REG_VBACK_AUTO 0xD4
> +#define DA9210_REG_VBACK_BASE 0xD5
> +#define DA9210_REG_VBACK_MAX_DVC_IF 0xD6
> +#define DA9210_REG_VBACK_DVC 0xD7
> +#define DA9210_REG_VBUCK_A 0xD8
> +#define DA9210_REG_VBUCK_B 0xD9
> +
> +/* I2C Interface Settings */
> +#define DA9210_REG_INTERFACE 0x105
> +
> +/* OTP */
> +#define DA9210_REG_OPT_COUNT 0x140
> +#define DA9210_REG_OPT_ADDR 0x141
> +#define DA9210_REG_OPT_DATA 0x142
> +
> +/* Customer Trim and Configuration */
> +#define DA9210_REG_CONFIG_A 0x143
> +#define DA9210_REG_CONFIG_B 0x144
> +#define DA9210_REG_CONFIG_C 0x145
> +#define DA9210_REG_CONFIG_D 0x146
> +#define DA9210_REG_CONFIG_E 0x147
> +
> +
> +/*
> + * Registers bits
> + */
> +/* DA9210_REG_PAGE_CON (addr=0x00) */
> +#define DA9210_PEG_PAGE_SHIFT 0
> +#define DA9210_REG_PAGE_MASK 0x0F
> +/* On I2C registers 0x00 - 0xFF */
> +#define DA9210_REG_PAGE0 0
> +/* On I2C registers 0x100 - 0x1FF */
> +#define DA9210_REG_PAGE2 2
> +#define DA9210_PAGE_WRITE_MODE 0x00
> +#define DA9210_REPEAT_WRITE_MODE 0x40
> +#define DA9210_PAGE_REVERT 0x80
> +
> +/* DA9210_REG_STATUS_A (addr=0x50) */
> +#define DA9210_GPI0 0x01
> +#define DA9210_GPI1 0x02
> +#define DA9210_GPI2 0x04
> +#define DA9210_GPI3 0x08
> +#define DA9210_GPI4 0x10
> +#define DA9210_GPI5 0x20
> +#define DA9210_GPI6 0x40
> +
> +/* DA9210_REG_EVENT_A (addr=0x52) */
> +#define DA9210_E_GPI0 0x01
> +#define DA9210_E_GPI1 0x02
> +#define DA9210_E_GPI2 0x04
> +#define DA9210_E_GPI3 0x08
> +#define DA9210_E_GPI4 0x10
> +#define DA9210_E_GPI5 0x20
> +#define DA9210_E_GPI6 0x40
> +
> +/* DA9210_REG_EVENT_B (addr=0x53) */
> +#define DA9210_E_OVCURR 0x01
> +#define DA9210_E_NPWRGOOD 0x02
> +#define DA9210_E_TEMP_WARN 0x04
> +#define DA9210_E_TEMP_CRIT 0x08
> +#define DA9210_E_VMAX 0x10
> +
> +/* DA9210_REG_MASK_A (addr=0x54) */
> +#define DA9210_M_GPI0 0x01
> +#define DA9210_M_GPI1 0x02
> +#define DA9210_M_GPI2 0x04
> +#define DA9210_M_GPI3 0x08
> +#define DA9210_M_GPI4 0x10
> +#define DA9210_M_GPI5 0x20
> +#define DA9210_M_GPI6 0x40
> +
> +/* DA9210_REG_MASK_B (addr=0x55) */
> +#define DA9210_M_OVCURR 0x01
> +#define DA9210_M_NPWRGOOD 0x02
> +#define DA9210_M_TEMP_WARN 0x04
> +#define DA9210_M_TEMP_CRIT 0x08
> +#define DA9210_M_VMAX 0x10
> +
> +/* DA9210_REG_CONTROL_A (addr=0x56) */
> +#define DA9210_DEBOUNCING_SHIFT 0
> +#define DA9210_DEBOUNCING_MASK 0x07
> +#define DA9210_SLEW_RATE_SHIFT 3
> +#define DA9210_SLEW_RATE_MASK 0x18
> +#define DA9210_V_LOCK 0x20
> +
> +/* DA9210_REG_GPIO_0_1 (addr=0x58) */
> +#define DA9210_GPIO0_PIN_SHIFT 0
> +#define DA9210_GPIO0_PIN_MASK 0x03
> +#define DA9210_GPIO0_PIN_GPI 0x00
> +#define DA9210_GPIO0_PIN_GPO_OD 0x02
> +#define DA9210_GPIO0_PIN_GPO 0x03
> +#define DA9210_GPIO0_TYPE 0x04
> +#define DA9210_GPIO0_TYPE_GPI 0x00
> +#define DA9210_GPIO0_TYPE_GPO 0x04
> +#define DA9210_GPIO0_MODE 0x08
> +#define DA9210_GPIO1_PIN_SHIFT 4
> +#define DA9210_GPIO1_PIN_MASK 0x30
> +#define DA9210_GPIO1_PIN_GPI 0x00
> +#define DA9210_GPIO1_PIN_VERROR 0x10
> +#define DA9210_GPIO1_PIN_GPO_OD 0x20
> +#define DA9210_GPIO1_PIN_GPO 0x30
> +#define DA9210_GPIO1_TYPE_SHIFT 0x40
> +#define DA9210_GPIO1_TYPE_GPI 0x00
> +#define DA9210_GPIO1_TYPE_GPO 0x40
> +#define DA9210_GPIO1_MODE 0x80
> +
> +/* DA9210_REG_GPIO_2_3 (addr=0x59) */
> +#define DA9210_GPIO2_PIN_SHIFT 0
> +#define DA9210_GPIO2_PIN_MASK 0x03
> +#define DA9210_GPIO2_PIN_GPI 0x00
> +#define DA9210_GPIO5_PIN_BUCK_CLK 0x10
> +#define DA9210_GPIO2_PIN_GPO_OD 0x02
> +#define DA9210_GPIO2_PIN_GPO 0x03
> +#define DA9210_GPIO2_TYPE 0x04
> +#define DA9210_GPIO2_TYPE_GPI 0x00
> +#define DA9210_GPIO2_TYPE_GPO 0x04
> +#define DA9210_GPIO2_MODE 0x08
> +#define DA9210_GPIO3_PIN_SHIFT 4
> +#define DA9210_GPIO3_PIN_MASK 0x30
> +#define DA9210_GPIO3_PIN_GPI 0x00
> +#define DA9210_GPIO3_PIN_IERROR 0x10
> +#define DA9210_GPIO3_PIN_GPO_OD 0x20
> +#define DA9210_GPIO3_PIN_GPO 0x30
> +#define DA9210_GPIO3_TYPE_SHIFT 0x40
> +#define DA9210_GPIO3_TYPE_GPI 0x00
> +#define DA9210_GPIO3_TYPE_GPO 0x40
> +#define DA9210_GPIO3_MODE 0x80
> +
> +/* DA9210_REG_GPIO_4_5 (addr=0x5A) */
> +#define DA9210_GPIO4_PIN_SHIFT 0
> +#define DA9210_GPIO4_PIN_MASK 0x03
> +#define DA9210_GPIO4_PIN_GPI 0x00
> +#define DA9210_GPIO4_PIN_GPO_OD 0x02
> +#define DA9210_GPIO4_PIN_GPO 0x03
> +#define DA9210_GPIO4_TYPE 0x04
> +#define DA9210_GPIO4_TYPE_GPI 0x00
> +#define DA9210_GPIO4_TYPE_GPO 0x04
> +#define DA9210_GPIO4_MODE 0x08
> +#define DA9210_GPIO5_PIN_SHIFT 4
> +#define DA9210_GPIO5_PIN_MASK 0x30
> +#define DA9210_GPIO5_PIN_GPI 0x00
> +#define DA9210_GPIO5_PIN_INTERFACE 0x01
> +#define DA9210_GPIO5_PIN_GPO_OD 0x20
> +#define DA9210_GPIO5_PIN_GPO 0x30
> +#define DA9210_GPIO5_TYPE_SHIFT 0x40
> +#define DA9210_GPIO5_TYPE_GPI 0x00
> +#define DA9210_GPIO5_TYPE_GPO 0x40
> +#define DA9210_GPIO5_MODE 0x80
> +
> +/* DA9210_REG_GPIO_6 (addr=0x5B) */
> +#define DA9210_GPIO6_PIN_SHIFT 0
> +#define DA9210_GPIO6_PIN_MASK 0x03
> +#define DA9210_GPIO6_PIN_GPI 0x00
> +#define DA9210_GPIO6_PIN_INTERFACE 0x01
> +#define DA9210_GPIO6_PIN_GPO_OD 0x02
> +#define DA9210_GPIO6_PIN_GPO 0x03
> +#define DA9210_GPIO6_TYPE 0x04
> +#define DA9210_GPIO6_TYPE_GPI 0x00
> +#define DA9210_GPIO6_TYPE_GPO 0x04
> +#define DA9210_GPIO6_MODE 0x08
> +
> +/* DA9210_REG_BUCK_CONT (addr=0x5D) */
> +#define DA9210_BUCK_EN 0x01
> +#define DA9210_BUCK_GPI_SHIFT 1
> +#define DA9210_BUCK_GPI_MASK 0x06
> +#define DA9210_BUCK_GPI_OFF 0x00
> +#define DA9210_BUCK_GPI_GPIO0 0x02
> +#define DA9210_BUCK_GPI_GPIO3 0x04
> +#define DA9210_BUCK_GPI_GPIO4 0x06
> +#define DA9210_BUCK_PD_DIS 0x08
> +#define DA9210_VBUCK_SEL 0x10
> +#define DA9210_VBUCK_SEL_A 0x00
> +#define DA9210_VBUCK_SEL_B 0x10
> +#define DA9210_VBUCK_GPI_SHIFT 5
> +#define DA9210_VBUCK_GPI_MASK 0x60
> +#define DA9210_VBUCK_GPI_OFF 0x00
> +#define DA9210_VBUCK_GPI_GPIO0 0x20
> +#define DA9210_VBUCK_GPI_GPIO3 0x40
> +#define DA9210_VBUCK_GPI_GPIO4 0x60
> +#define DA9210_DVC_CTRL_EN 0x80
> +
> +/* DA9210_REG_BUCK_ILIM (addr=0xD0) */
> +#define DA9210_BUCK_ILIM_SHIFT 0
> +#define DA9210_BUCK_ILIM_MASK 0x0F
> +#define DA9210_BUCK_IALARM 0x10
> +
> +/* DA9210_REG_BUCK_CONF1 (addr=0xD1) */
> +#define DA9210_BUCK_MODE_SHIFT 0
> +#define DA9210_BUCK_MODE_MASK 0x03
> +#define DA9210_BUCK_MODE_MANUAL 0x00
> +#define DA9210_BUCK_MODE_SLEEP 0x01
> +#define DA9210_BUCK_MODE_SYNC 0x02
> +#define DA9210_BUCK_MODE_AUTO 0x03
> +#define DA9210_STARTUP_CTRL_SHIFT 2
> +#define DA9210_STARTUP_CTRL_MASK 0x1C
> +#define DA9210_PWR_DOWN_CTRL_SHIFT 5
> +#define DA9210_PWR_DOWN_CTRL_MASK 0xE0
> +
> +/* DA9210_REG_BUCK_CONF2 (addr=0xD2) */
> +#define DA9210_PHASE_SEL_SHIFT 0
> +#define DA9210_PHASE_SEL_MASK 0x03
> +#define DA9210_FREQ_SEL 0x40
> +
> +/* DA9210_REG_BUCK_AUTO (addr=0xD4) */
> +#define DA9210_VBUCK_AUTO_SHIFT 0
> +#define DA9210_VBUCK_AUTO_MASK 0x7F
> +
> +/* DA9210_REG_BUCK_BASE (addr=0xD5) */
> +#define DA9210_VBUCK_BASE_SHIFT 0
> +#define DA9210_VBUCK_BASE_MASK 0x7F
> +
> +/* DA9210_REG_VBUCK_MAX_DVC_IF (addr=0xD6) */
> +#define DA9210_VBUCK_MAX_SHIFT 0
> +#define DA9210_VBUCK_MAX_MASK 0x7F
> +#define DA9210_DVC_STEP_SIZE 0x80
> +#define DA9210_DVC_STEP_SIZE_10MV 0x00
> +#define DA9210_DVC_STEP_SIZE_20MV 0x80
> +
> +/* DA9210_REG_VBUCK_DVC (addr=0xD7) */
> +#define DA9210_VBUCK_DVC_SHIFT 0
> +#define DA9210_VBUCK_DVC_MASK 0x7F
> +
> +/* DA9210_REG_VBUCK_A/B (addr=0xD8/0xD9) */
> +#define DA9210_VBUCK_SHIFT 0
> +#define DA9210_VBUCK_MASK 0x7F
> +#define DA9210_VBUCK_BIAS 0
> +#define DA9210_BUCK_SL 0x80
> +
> +/* DA9210_REG_INTERFACE (addr=0x105) */
> +#define DA9210_IF_BASE_ADDR_SHIFT 4
> +#define DA9210_IF_BASE_ADDR_MASK 0xF0
> +
> +/* DA9210_REG_CONFIG_E (addr=0x147) */
> +#define DA9210_STAND_ALONE 0x01
> +
> +#endif /* __DA9210_REGISTERS_H__ */
> +
> --
> end-of-patch for RFC V1
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-20 13:42 [RFC V1] COMMIT 1: DA9210 driver files Steve Twiss
2013-06-21 9:58 ` Guennadi Liakhovetski
@ 2013-06-21 15:15 ` Mark Brown
2013-06-24 9:18 ` Opensource [Steve Twiss]
1 sibling, 1 reply; 9+ messages in thread
From: Mark Brown @ 2013-06-21 15:15 UTC (permalink / raw)
To: Steve Twiss; +Cc: Liam Girdwood, David Dajun Chen, Guennadi Liakhovetski, LKML
[-- Attachment #1: Type: text/plain, Size: 4818 bytes --]
On Thu, Jun 20, 2013 at 02:42:03PM +0100, Steve Twiss wrote:
> @@ -293,6 +293,13 @@ config REGULATOR_LP8788
> help
> This driver supports LP8788 voltage regulator chip.
>
> +config REGULATOR_DA9210
> + bool "Dialog Semiconductor DA9210 Regulator"
> + depends on I2C=y
> + select REGMAP_I2C
> + help
> + Support for the Dialog Semiconductor DA9210 chip.
> +
> config REGULATOR_PCF50633
This file ought to be kept sorted though it seems that's not been
happening, and I'm not seeing any reason why this can't be a tristate.
> +#define DRIVER_NAME "da9210"
Why?
> +struct da9210 {
> + struct i2c_client *i2c;
> + struct device *dev;
> + struct mutex io_mutex;
Why do you need an I/O lock?
> +static int da9210_enable(struct regulator_dev *rdev);
> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
> + int max_uV, unsigned *selector);
> +static int da9210_get_voltage(struct regulator_dev *rdev);
> +static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
> + int max_uA);
> +static int da9210_get_current_limit(struct regulator_dev *rdev);
> +
> +static struct regulator_ops da9210_buck_ops = {
> + .enable = da9210_enable,
> + .disable = regulator_disable_regmap,
> + .is_enabled = regulator_is_enabled_regmap,
> + .set_voltage = da9210_set_voltage,
> + .get_voltage = da9210_get_voltage,
> + .list_voltage = regulator_list_voltage_linear,
> + .set_current_limit = da9210_set_current_limit,
> + .get_current_limit = da9210_get_current_limit,
> +};
Drivers are normally written to avoid forward declarations like this.
> +static struct regulator_consumer_supply __initdata def_da9210_consumers[] = {
> + REGULATOR_SUPPLY("DA9210", NULL),
> +};
> +
> +static struct regulator_init_data __initdata default_da9210_constraints = {
> + .constraints = {
This is not at all sensible, there's a good solid reason why regulator
drivers don't generally do this... please review the machine interface
and its purpose.
> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
> + int max_uV, unsigned *selector)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + int val;
> + int ret;
> +
> + val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
> + if (val < 0)
> + return -EINVAL;
> +
> + ret = regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
> + DA9210_VBUCK_MASK, val);
> + return ret;
> +}
Why not just use set_voltage_sel()?
> +static int da9210_get_voltage_sel(struct regulator_dev *rdev)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + unsigned int data;
> + int sel;
> + int ret;
> +
> + ret = regmap_read(chip->regmap, DA9210_REG_VBUCK_A, &data);
> + if (ret < 0)
> + return ret;
> +
> + sel = (data & DA9210_VBUCK_MASK) >> DA9210_VBUCK_SHIFT;
> + sel -= DA9210_VBUCK_BIAS;
> + if (sel < 0)
> + sel = 0;
> + if (sel >= chip->info->n_steps)
> + sel = chip->info->n_steps - 1;
This looks like poor error handling, if the value is out of range isn't
there an error state?
> +static int da9210_get_voltage(struct regulator_dev *rdev)
> +{
> + struct da9210 *chip = rdev_get_drvdata(rdev);
> + int sel = da9210_get_voltage_sel(rdev);
> +
> + if (sel < 0)
> + return sel;
> +
> + return (chip->info->step_uV * sel) + chip->info->min_uV;
> +}
Why are you open coding core functionalit?
> +static int da9210_enable(struct regulator_dev *rdev)
> +{
> + return regulator_enable_regmap(rdev);
> +}
This is pointless, just use the generic function directly.
> +
> + dev_info(chip->dev, "Device DA9210 detected.\n");
This is just noise.
> +static const struct i2c_device_id da9210_i2c_id[] = {
> + {DRIVER_NAME, 0},
> + {},
Just use the string.
> +static struct i2c_driver da9210_regulator_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
Similarly here.
> + .owner = THIS_MODULE,
> + },
Indentation.
> +static int __init da9210_regulator_init(void)
> +{
> + int ret;
> +
> + ret = i2c_add_driver(&da9210_regulator_driver);
> + if (0 != ret)
> + pr_err("Failed to register da9210 I2C driver\n");
> +
> + return ret;
> +}
> +
> +subsys_initcall(da9210_regulator_init);
Just use module_platform_driver() now we have probe deferral.
> +/*
> + * Registers bits
> + */
> +/* DA9210_REG_PAGE_CON (addr=0x00) */
> +#define DA9210_PEG_PAGE_SHIFT 0
> +#define DA9210_REG_PAGE_MASK 0x0F
> +/* On I2C registers 0x00 - 0xFF */
> +#define DA9210_REG_PAGE0 0
> +/* On I2C registers 0x100 - 0x1FF */
> +#define DA9210_REG_PAGE2 2
> +#define DA9210_PAGE_WRITE_MODE 0x00
> +#define DA9210_REPEAT_WRITE_MODE 0x40
> +#define DA9210_PAGE_REVERT 0x80
This looks liike you should be using a regmap range.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-21 9:58 ` Guennadi Liakhovetski
@ 2013-06-24 9:17 ` Opensource [Steve Twiss]
2013-06-26 7:55 ` Guennadi Liakhovetski
0 siblings, 1 reply; 9+ messages in thread
From: Opensource [Steve Twiss] @ 2013-06-24 9:17 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Liam Girdwood, Mark Brown, David Dajun Chen, LKML
On Fri, 21 Jun 2013, Guennadi Liakhovetski wrote:
>
>On Thu, 20 Jun 2013, Steve Twiss wrote:
>
>> From: Steve Twiss <stwiss.opensource@diasemi.com>
>>
>> This is the regulator driver for the Dialog DA9210 Multi-phase Buck.
>> The patch is relative to linux-next next-20130620
>>
>> The regulator implements the functions for .enable, .set_voltage,
>> .get_voltage, .set_current_limit, and .get_current_limit. It uses the
>> kernel standard functions for .disable, .is_enabled and .list_voltage.
>>
>> This file contains a regulator driver and I2C driver combined into the
>> same file and has been tested on a Samsung SMDK6410 connected to a
>> Dialog
>> DA9210 Evaluation Board through a I2C connection.
>>
>> It would be appreciated if you could add any comments you may have
>> about the driver.
>>
>> Signed-off-by: Steve Twiss <stwiss.opensource@diasemi.com>
>> Signed-off-by: David Dajun Chen <david.chen@diasemi.com>
>> ---
>> Checks performed with linux-next/next-20130620/scripts/checkpatch.pl
>> Kconfig total: 0 errors, 0 warnings, 535 lines checked
>> da9210-regulator.c total: 0 errors, 0 warnings, 334 lines checked
>> Makefile total: 0 errors, 0 warnings, 77 lines checked
>> da9210-regulator.h total: 0 errors, 0 warnings, 286 lines checked
>>
>> drivers/regulator/Kconfig | 7 +
>> drivers/regulator/Makefile | 1 +
>> drivers/regulator/da9210-regulator.c | 334 ++++++++++++++++++++++++++++
>> include/linux/regulator/da9210-regulator.h | 286
>> ++++++++++++++++++++++++
>> 4 files changed, 628 insertions(+), 0 deletions(-) create mode
>> 100644 drivers/regulator/da9210-regulator.c
>> create mode 100644 include/linux/regulator/da9210-regulator.h
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 9296425..aea2fc8 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -293,6 +293,13 @@ config REGULATOR_LP8788
>> help
>> This driver supports LP8788 voltage regulator chip.
>>
>> +config REGULATOR_DA9210
>> + bool "Dialog Semiconductor DA9210 Regulator"
>
>You don't support modular build? I think you should.
>
>> + depends on I2C=y
>
>Then you won't need =y either
>
Yes I should have added this option.
I will add this and re-submit.
>> + select REGMAP_I2C
>> + help
>> + Support for the Dialog Semiconductor DA9210 chip.
>> +
>> config REGULATOR_PCF50633
>> tristate "NXP PCF50633 regulator driver"
>> depends on MFD_PCF50633
>> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
>> index 26e6c4a..0155e2a 100644
>> --- a/drivers/regulator/Makefile
>> +++ b/drivers/regulator/Makefile
>> @@ -20,6 +20,7 @@ obj-$(CONFIG_REGULATOR_AS3711) += as3711-regulator.o
>> obj-$(CONFIG_REGULATOR_DA903X) += da903x.o
>> obj-$(CONFIG_REGULATOR_DA9052) += da9052-regulator.o
>> obj-$(CONFIG_REGULATOR_DA9055) += da9055-regulator.o
>> +obj-$(CONFIG_REGULATOR_DA9210) += da9210-regulator.o
>> obj-$(CONFIG_REGULATOR_DBX500_PRCMU) += dbx500-prcmu.o
>> obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
>> obj-$(CONFIG_REGULATOR_FAN53555) += fan53555.o diff --git
>> a/drivers/regulator/da9210-regulator.c
>> b/drivers/regulator/da9210-regulator.c
>> new file mode 100644
>> index 0000000..5e6691e
>> --- /dev/null
>> +++ b/drivers/regulator/da9210-regulator.c
>> @@ -0,0 +1,334 @@
>> +
>> +/* da9210-regulator.c - Regulator device driver for DA9210
>> + * Copyright (C) 2013 Dialog Semiconductor Ltd.
>> + *
>> + * 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/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +#include <linux/regulator/da9210-regulator.h>
>> +#include <linux/regmap.h>
>
>This driver doesn't support DT. This is ok for the first submission, I think, but we'll
>definitely need it soon.
>
>Some headers are also missing here, e.g. slab.h, init.h
>
Added <linux/init.h> and <linux/slab.h>
>> +
>> +#define DRIVER_NAME "da9210"
>> +
>> +struct da9210_regulator_info {
>> + int min_uV;
>> + int max_uV;
>> + unsigned step_uV;
>> + unsigned n_steps;
>> +
>> + unsigned n_current_limits;
>> + const int *current_limits;
>> +};
>> +
>> +struct da9210 {
>> + struct i2c_client *i2c;
>> + struct device *dev;
>> + struct mutex io_mutex;
>> + const struct da9210_regulator_info *info;
>> + struct regulator_desc desc;
>> + struct regulator_dev *rdev;
>> + struct regmap *regmap;
>> +};
>> +
>> +static struct regmap_config da9210_regmap_config = {
>
>you can make this const
>
Yes. Have made it static const struct regmap_config da9210_regmap_config
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> +};
>> +
>> +static int da9210_enable(struct regulator_dev *rdev); static int
>> +da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
>> + int max_uV, unsigned *selector); static int
>> +da9210_get_voltage(struct regulator_dev *rdev); static int
>> +da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
>> + int max_uA);
>> +static int da9210_get_current_limit(struct regulator_dev *rdev);
>> +
>> +static struct regulator_ops da9210_buck_ops = {
>> + .enable = da9210_enable,
>
>Why don't you just assign regulator_enable_regmap here?
>
Oh!
Done: now using regulator_enable_regmap instead of function just containing regulator_enable_regmap.
>> + .disable = regulator_disable_regmap,
>> + .is_enabled = regulator_is_enabled_regmap,
>> + .set_voltage = da9210_set_voltage,
>> + .get_voltage = da9210_get_voltage,
>> + .list_voltage = regulator_list_voltage_linear,
>> + .set_current_limit = da9210_set_current_limit,
>> + .get_current_limit = da9210_get_current_limit, };
>> +
>> +/* Default limits measured in millivolts and milliamps */
>> +#define DA9210_MIN_MV 300
>> +#define DA9210_MAX_MV 1570
>> +#define DA9210_MIN_MA 1600
>> +#define DA9210_MAX_MA 4600
>> +#define DA9210_STEP_MV 10
>> +
>> +/* Current limits for buck (uA) indices corresponds with register
>> +values */ static const int da9210_buck_limits[] = {
>> + 1600000, 1800000, 2000000, 2200000, 2400000, 2600000, 2800000,
>3000000,
>> + 3200000, 3400000, 3600000, 3800000, 4000000, 4200000, 4400000,
>> +4600000 };
>> +
>> +static const struct da9210_regulator_info da9210_info = {
>> + .min_uV = (DA9210_MIN_MV * 1000),
>> + .max_uV = (DA9210_MAX_MV * 1000),
>> + .step_uV = (DA9210_STEP_MV * 1000),
>> + .n_steps = (((DA9210_MAX_MV) - (DA9210_MIN_MV)) / (DA9210_STEP_MV))
>> ++ 1,
>
>Way too many parenthesis above. Strictly speaking you don't need this struct, you
>could just use macros / constant variables and remove a bunch of pointer dereferences,
>but it's up to you, I think.
>
Yep.
Cleaned that up.
>> + .current_limits = da9210_buck_limits,
>> + .n_current_limits = ARRAY_SIZE(da9210_buck_limits), };
>> +
>> +static struct regulator_consumer_supply __initdata def_da9210_consumers[] = {
>> + REGULATOR_SUPPLY("DA9210", NULL),
>> +};
>> +
>> +static struct regulator_init_data __initdata default_da9210_constraints = {
>> + .constraints = {
>> + .name = "DA9210-DEFAULT",
>> + .min_uV = (DA9210_MIN_MV * 1000),
>> + .max_uV = (DA9210_MAX_MV * 1000),
>> + .min_uA = (DA9210_MIN_MA * 1000),
>> + .max_uA = (DA9210_MAX_MA * 1000),
>> + .uV_offset = 0,
>> + .always_on = 0,
>> + .boot_on = 0,
>> + .apply_uV = 1,
>> + .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE |
>> + REGULATOR_CHANGE_CURRENT |
>> + REGULATOR_CHANGE_STATUS,
>> + },
>> + .num_consumer_supplies = ARRAY_SIZE(def_da9210_consumers),
>> + .consumer_supplies = def_da9210_consumers, };
>
>Oops, no, none of the above two structs belong here. Please, remove.
>
I have received a similar comment from Mark Brown.
This part will need to be re-written to follow his suggestions.
>> +
>> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
>> + int max_uV, unsigned *selector) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + int val;
>> + int ret;
>> +
>> + val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
>> + if (val < 0)
>> + return -EINVAL;
>
>"return val" would be better
>
Yes. Done it.
>> +
>> + ret = regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
>> + DA9210_VBUCK_MASK, val);
>> + return ret;
>
>just
>
>+ return regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
>+ DA9210_VBUCK_MASK, val);
>
Yes. That done that too.
>> +}
>> +
>> +static int da9210_get_voltage_sel(struct regulator_dev *rdev) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + unsigned int data;
>> + int sel;
>> + int ret;
>> +
>> + ret = regmap_read(chip->regmap, DA9210_REG_VBUCK_A, &data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sel = (data & DA9210_VBUCK_MASK) >> DA9210_VBUCK_SHIFT;
>> + sel -= DA9210_VBUCK_BIAS;
>> + if (sel < 0)
>> + sel = 0;
>> + if (sel >= chip->info->n_steps)
>> + sel = chip->info->n_steps - 1;
>> +
>> + return sel;
>> +}
>> +
>> +static int da9210_get_voltage(struct regulator_dev *rdev) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + int sel = da9210_get_voltage_sel(rdev);
>> +
>> + if (sel < 0)
>> + return sel;
>> +
>> + return (chip->info->step_uV * sel) + chip->info->min_uV;
>
>superfluous parenthesis
>
Mark Brown also commented that this was core functionality, so I need to review this again.
>> +}
>> +
>> +static int da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
>> + int max_uA)
>> +{
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + unsigned int sel;
>> + int i;
>> +
>> + if (!chip->info->current_limits)
>> + return -EINVAL;
>
>It's not really possible :) But, well, if you want this check...
>
Ah, this was my attempt at being rigorous
I have removed the check.
>> +
>> + /* search for closest to maximum */
>> + for (i = chip->info->n_current_limits - 1; i >= 0; i--) {
>> + if (min_uA <= chip->info->current_limits[i] &&
>> + max_uA >= chip->info->current_limits[i]) {
>> + sel = i;
>> + sel = sel << DA9210_BUCK_ILIM_SHIFT;
>> + return regmap_update_bits(chip->regmap,
>> + DA9210_REG_BUCK_ILIM,
>> + DA9210_BUCK_ILIM_MASK, sel);
>> + }
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int da9210_get_current_limit(struct regulator_dev *rdev) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + unsigned int data;
>> + int sel;
>> + int ret;
>> +
>> + ret = regmap_read(chip->regmap, DA9210_REG_BUCK_ILIM, &data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sel = (data & DA9210_BUCK_ILIM_MASK) >> DA9210_BUCK_ILIM_SHIFT;
>> + if (sel < 0)
>
>Don't think sel can be < 0 here.
>
>> + sel = 0;
>> + if (sel >= chip->info->n_current_limits)
>
>Your sel is (x & 0xf) >> 0, so, it's 0 <= sel < 16. n_current_limits above == 16, so, this is
>impossible too. If you had simple constants instead of structs, this would have been
>simpler to follow.
>
Yes. This would be simpler if there were constants instead of pointer dereferences
all over the place. I will clean this method up and re-submit it.
>> + sel = chip->info->n_current_limits - 1;
>> +
>> + return chip->info->current_limits[sel]; }
>> +
>> +static int da9210_enable(struct regulator_dev *rdev) {
>> + return regulator_enable_regmap(rdev); }
>> +
>> +/* I2C driver interface functions.
>> + */
>> +
>> +static int da9210_i2c_probe(struct i2c_client *i2c,
>> + const struct i2c_device_id *id) {
>> + struct da9210 *chip;
>> + const struct regulator_init_data *init_data = NULL;
>> + struct regulator_dev *rdev = NULL;
>> + struct regulator_config config = { };
>> + int error;
>> +
>> + chip = devm_kzalloc(&i2c->dev, sizeof(struct da9210), GFP_KERNEL);
>> + if (NULL == chip) {
>> + dev_err(&i2c->dev,
>> + "Cannot kzalloc memory for regulator structure\n");
>> + return -ENOMEM;
>> + }
>> +
>> + chip->regmap = devm_regmap_init_i2c(i2c, &da9210_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;
>> + }
>> +
>> + chip->dev = &i2c->dev;
>> + chip->i2c = i2c;
>
>you don't use either of the above two pointers. To be precise, you do use
>chip->dev a couple of times below, but you could just use &i2c->dev
>chip->there
>too.
>
Removed unused dev and i2c.
>> + chip->info = &da9210_info;
>> +
>> + mutex_init(&chip->io_mutex);
>
>unused?
>
Removed that too.
>> +
>> + chip->desc.name = "DA9210";
>> + chip->desc.ops = &da9210_buck_ops;
>> + chip->desc.n_voltages = chip->info->n_steps;
>> + chip->desc.type = REGULATOR_VOLTAGE;
>> + chip->desc.owner = THIS_MODULE;
>> + chip->desc.id = 0;
>> + chip->desc.min_uV = chip->info->min_uV;
>> + chip->desc.uV_step = chip->info->step_uV;
>> + chip->desc.enable_reg = DA9210_REG_BUCK_CONT;
>> + chip->desc.enable_mask = DA9210_BUCK_EN;
>> +
>> + if (!i2c->dev.platform_data)
>> + init_data = &default_da9210_constraints;
>> + else
>> + init_data = i2c->dev.platform_data;
>> +
>> + config.dev = &i2c->dev;
>> + config.init_data = init_data;
>> + config.driver_data = chip;
>> + config.regmap = chip->regmap;
>> +
>> + rdev = regulator_register(&chip->desc, &config);
>> + if (IS_ERR(rdev)) {
>> + dev_err(chip->dev, "Failed to register DA9210 regulator\n");
>> + chip->rdev = NULL;
>
>You don't need this assignment
>
Removed setting chip->rdev=NULL.
>> + return PTR_ERR(rdev);
>> + }
>> +
>> + chip->rdev = rdev;
>> +
>> + i2c_set_clientdata(i2c, chip);
>> +
>> + dev_info(chip->dev, "Device DA9210 detected.\n");
>> + return 0;
>> +}
>> +
>> +static int da9210_i2c_remove(struct i2c_client *i2c) {
>> + struct da9210 *chip = i2c_get_clientdata(i2c);
>> + regulator_unregister(chip->rdev);
>> + return 0;
>> +}
>> +
>> +static const struct i2c_device_id da9210_i2c_id[] = {
>> + {DRIVER_NAME, 0},
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, da9210_i2c_id);
>> +
>> +static struct i2c_driver da9210_regulator_driver = {
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = da9210_i2c_probe,
>> + .remove = da9210_i2c_remove,
>> + .id_table = da9210_i2c_id,
>> +};
>> +
>> +static int __init da9210_regulator_init(void) {
>> + int ret;
>> +
>> + ret = i2c_add_driver(&da9210_regulator_driver);
>> + if (0 != ret)
>> + pr_err("Failed to register da9210 I2C driver\n");
>> +
>> + return ret;
>> +}
>> +
>> +subsys_initcall(da9210_regulator_init);
>> +
>> +static void __exit da9210_regulator_cleanup(void) {
>> + i2c_del_driver(&da9210_regulator_driver);
>> +}
>> +
>> +module_exit(da9210_regulator_cleanup);
>> +
>> +MODULE_AUTHOR("S Twiss <stwiss.opensource@diasemi.com>");
>> +MODULE_DESCRIPTION("Regulator device driver for Dialog DA9210");
>> +MODULE_LICENSE("GPL v2"); MODULE_ALIAS("platform:" DRIVER_NAME);
>> diff --git a/include/linux/regulator/da9210-regulator.h
>> b/include/linux/regulator/da9210-regulator.h
>> new file mode 100644
>> index 0000000..e6748d6
>> --- /dev/null
>> +++ b/include/linux/regulator/da9210-regulator.h
>
>You don't need this header under include/... Please, either move it to drivers/regulator/
>or just move all these defines into the .c file.
>
>Thanks
>Guennadi
>
I was stumped where this should I go: since this sort of thing normally goes
into ./include/linux/mfd
I thought about cutting down the majority of this content and putting it
directly into the C file, but there have been requests for the full list of registers
(in case somebody wants to implement additional functionality).
I will resubmit this with the header file in ./drivers/regulator
>> @@ -0,0 +1,286 @@
>> +
>> +/* da9210-regulator.h - Regulator definitions for DA9210
>> + * Copyright (C) 2013 Dialog Semiconductor Ltd.
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __DA9210_REGISTERS_H__
>> +#define __DA9210_REGISTERS_H__
>> +
>> +/* Page selection I2C or SPI always in the begining of any page. */
>> +/* Page 0 : I2C access 0x000 - 0x0FF SPI access 0x000 - 0x07F */
>> +/* Page 1 : SPI access 0x080 - 0x0FF */
>> +/* Page 2 : I2C access 0x100 - 0x17F SPI access 0x100 - 0x17F */
>> +#define DA9210_REG_PAGE_CON 0x00
>> +
>> +/* System Control and Event Registers */
>> +#define DA9210_REG_STATUS_A 0x50
>> +#define DA9210_REG_STATUS_B 0x51
>> +#define DA9210_REG_EVENT_A 0x52
>> +#define DA9210_REG_EVENT_B 0x53
>> +#define DA9210_REG_MASK_A 0x54
>> +#define DA9210_REG_MASK_B 0x55
>> +#define DA9210_REG_CONTROL_A 0x56
>> +
>> +/* GPIO Control Registers */
>> +#define DA9210_REG_GPIO_0_1 0x58
>> +#define DA9210_REG_GPIO_2_3 0x59
>> +#define DA9210_REG_GPIO_4_5 0x5A
>> +#define DA9210_REG_GPIO_6 0x5B
>> +
>> +/* Regulator Registers */
>> +#define DA9210_REG_BUCK_CONT 0x5D
>> +#define DA9210_REG_BUCK_ILIM 0xD0
>> +#define DA9210_REG_BUCK_CONF1 0xD1
>> +#define DA9210_REG_BUCK_CONF2 0xD2
>> +#define DA9210_REG_VBACK_AUTO 0xD4
>> +#define DA9210_REG_VBACK_BASE 0xD5
>> +#define DA9210_REG_VBACK_MAX_DVC_IF 0xD6
>> +#define DA9210_REG_VBACK_DVC 0xD7
>> +#define DA9210_REG_VBUCK_A 0xD8
>> +#define DA9210_REG_VBUCK_B 0xD9
>> +
>> +/* I2C Interface Settings */
>> +#define DA9210_REG_INTERFACE 0x105
>> +
>> +/* OTP */
>> +#define DA9210_REG_OPT_COUNT 0x140
>> +#define DA9210_REG_OPT_ADDR 0x141
>> +#define DA9210_REG_OPT_DATA 0x142
>> +
>> +/* Customer Trim and Configuration */
>> +#define DA9210_REG_CONFIG_A 0x143
>> +#define DA9210_REG_CONFIG_B 0x144
>> +#define DA9210_REG_CONFIG_C 0x145
>> +#define DA9210_REG_CONFIG_D 0x146
>> +#define DA9210_REG_CONFIG_E 0x147
>> +
>> +
>> +/*
>> + * Registers bits
>> + */
>> +/* DA9210_REG_PAGE_CON (addr=0x00) */
>> +#define DA9210_PEG_PAGE_SHIFT 0
>> +#define DA9210_REG_PAGE_MASK 0x0F
>> +/* On I2C registers 0x00 - 0xFF */
>> +#define DA9210_REG_PAGE0 0
>> +/* On I2C registers 0x100 - 0x1FF */
>> +#define DA9210_REG_PAGE2 2
>> +#define DA9210_PAGE_WRITE_MODE 0x00
>> +#define DA9210_REPEAT_WRITE_MODE 0x40
>> +#define DA9210_PAGE_REVERT 0x80
>> +
>> +/* DA9210_REG_STATUS_A (addr=0x50) */
>> +#define DA9210_GPI0 0x01
>> +#define DA9210_GPI1 0x02
>> +#define DA9210_GPI2 0x04
>> +#define DA9210_GPI3 0x08
>> +#define DA9210_GPI4 0x10
>> +#define DA9210_GPI5 0x20
>> +#define DA9210_GPI6 0x40
>> +
>> +/* DA9210_REG_EVENT_A (addr=0x52) */
>> +#define DA9210_E_GPI0 0x01
>> +#define DA9210_E_GPI1 0x02
>> +#define DA9210_E_GPI2 0x04
>> +#define DA9210_E_GPI3 0x08
>> +#define DA9210_E_GPI4 0x10
>> +#define DA9210_E_GPI5 0x20
>> +#define DA9210_E_GPI6 0x40
>> +
>> +/* DA9210_REG_EVENT_B (addr=0x53) */
>> +#define DA9210_E_OVCURR 0x01
>> +#define DA9210_E_NPWRGOOD 0x02
>> +#define DA9210_E_TEMP_WARN 0x04
>> +#define DA9210_E_TEMP_CRIT 0x08
>> +#define DA9210_E_VMAX 0x10
>> +
>> +/* DA9210_REG_MASK_A (addr=0x54) */
>> +#define DA9210_M_GPI0 0x01
>> +#define DA9210_M_GPI1 0x02
>> +#define DA9210_M_GPI2 0x04
>> +#define DA9210_M_GPI3 0x08
>> +#define DA9210_M_GPI4 0x10
>> +#define DA9210_M_GPI5 0x20
>> +#define DA9210_M_GPI6 0x40
>> +
>> +/* DA9210_REG_MASK_B (addr=0x55) */
>> +#define DA9210_M_OVCURR 0x01
>> +#define DA9210_M_NPWRGOOD 0x02
>> +#define DA9210_M_TEMP_WARN 0x04
>> +#define DA9210_M_TEMP_CRIT 0x08
>> +#define DA9210_M_VMAX 0x10
>> +
>> +/* DA9210_REG_CONTROL_A (addr=0x56) */
>> +#define DA9210_DEBOUNCING_SHIFT 0
>> +#define DA9210_DEBOUNCING_MASK 0x07
>> +#define DA9210_SLEW_RATE_SHIFT 3
>> +#define DA9210_SLEW_RATE_MASK 0x18
>> +#define DA9210_V_LOCK 0x20
>> +
>> +/* DA9210_REG_GPIO_0_1 (addr=0x58) */
>> +#define DA9210_GPIO0_PIN_SHIFT 0
>> +#define DA9210_GPIO0_PIN_MASK 0x03
>> +#define DA9210_GPIO0_PIN_GPI 0x00
>> +#define DA9210_GPIO0_PIN_GPO_OD 0x02
>> +#define DA9210_GPIO0_PIN_GPO 0x03
>> +#define DA9210_GPIO0_TYPE 0x04
>> +#define DA9210_GPIO0_TYPE_GPI 0x00
>> +#define DA9210_GPIO0_TYPE_GPO 0x04
>> +#define DA9210_GPIO0_MODE 0x08
>> +#define DA9210_GPIO1_PIN_SHIFT 4
>> +#define DA9210_GPIO1_PIN_MASK 0x30
>> +#define DA9210_GPIO1_PIN_GPI 0x00
>> +#define DA9210_GPIO1_PIN_VERROR 0x10
>> +#define DA9210_GPIO1_PIN_GPO_OD 0x20
>> +#define DA9210_GPIO1_PIN_GPO 0x30
>> +#define DA9210_GPIO1_TYPE_SHIFT 0x40
>> +#define DA9210_GPIO1_TYPE_GPI 0x00
>> +#define DA9210_GPIO1_TYPE_GPO 0x40
>> +#define DA9210_GPIO1_MODE 0x80
>> +
>> +/* DA9210_REG_GPIO_2_3 (addr=0x59) */
>> +#define DA9210_GPIO2_PIN_SHIFT 0
>> +#define DA9210_GPIO2_PIN_MASK 0x03
>> +#define DA9210_GPIO2_PIN_GPI 0x00
>> +#define DA9210_GPIO5_PIN_BUCK_CLK 0x10
>> +#define DA9210_GPIO2_PIN_GPO_OD 0x02
>> +#define DA9210_GPIO2_PIN_GPO 0x03
>> +#define DA9210_GPIO2_TYPE 0x04
>> +#define DA9210_GPIO2_TYPE_GPI 0x00
>> +#define DA9210_GPIO2_TYPE_GPO 0x04
>> +#define DA9210_GPIO2_MODE 0x08
>> +#define DA9210_GPIO3_PIN_SHIFT 4
>> +#define DA9210_GPIO3_PIN_MASK 0x30
>> +#define DA9210_GPIO3_PIN_GPI 0x00
>> +#define DA9210_GPIO3_PIN_IERROR 0x10
>> +#define DA9210_GPIO3_PIN_GPO_OD 0x20
>> +#define DA9210_GPIO3_PIN_GPO 0x30
>> +#define DA9210_GPIO3_TYPE_SHIFT 0x40
>> +#define DA9210_GPIO3_TYPE_GPI 0x00
>> +#define DA9210_GPIO3_TYPE_GPO 0x40
>> +#define DA9210_GPIO3_MODE 0x80
>> +
>> +/* DA9210_REG_GPIO_4_5 (addr=0x5A) */
>> +#define DA9210_GPIO4_PIN_SHIFT 0
>> +#define DA9210_GPIO4_PIN_MASK 0x03
>> +#define DA9210_GPIO4_PIN_GPI 0x00
>> +#define DA9210_GPIO4_PIN_GPO_OD 0x02
>> +#define DA9210_GPIO4_PIN_GPO 0x03
>> +#define DA9210_GPIO4_TYPE 0x04
>> +#define DA9210_GPIO4_TYPE_GPI 0x00
>> +#define DA9210_GPIO4_TYPE_GPO 0x04
>> +#define DA9210_GPIO4_MODE 0x08
>> +#define DA9210_GPIO5_PIN_SHIFT 4
>> +#define DA9210_GPIO5_PIN_MASK 0x30
>> +#define DA9210_GPIO5_PIN_GPI 0x00
>> +#define DA9210_GPIO5_PIN_INTERFACE 0x01
>> +#define DA9210_GPIO5_PIN_GPO_OD 0x20
>> +#define DA9210_GPIO5_PIN_GPO 0x30
>> +#define DA9210_GPIO5_TYPE_SHIFT 0x40
>> +#define DA9210_GPIO5_TYPE_GPI 0x00
>> +#define DA9210_GPIO5_TYPE_GPO 0x40
>> +#define DA9210_GPIO5_MODE 0x80
>> +
>> +/* DA9210_REG_GPIO_6 (addr=0x5B) */
>> +#define DA9210_GPIO6_PIN_SHIFT 0
>> +#define DA9210_GPIO6_PIN_MASK 0x03
>> +#define DA9210_GPIO6_PIN_GPI 0x00
>> +#define DA9210_GPIO6_PIN_INTERFACE 0x01
>> +#define DA9210_GPIO6_PIN_GPO_OD 0x02
>> +#define DA9210_GPIO6_PIN_GPO 0x03
>> +#define DA9210_GPIO6_TYPE 0x04
>> +#define DA9210_GPIO6_TYPE_GPI 0x00
>> +#define DA9210_GPIO6_TYPE_GPO 0x04
>> +#define DA9210_GPIO6_MODE 0x08
>> +
>> +/* DA9210_REG_BUCK_CONT (addr=0x5D) */
>> +#define DA9210_BUCK_EN 0x01
>> +#define DA9210_BUCK_GPI_SHIFT 1
>> +#define DA9210_BUCK_GPI_MASK 0x06
>> +#define DA9210_BUCK_GPI_OFF 0x00
>> +#define DA9210_BUCK_GPI_GPIO0 0x02
>> +#define DA9210_BUCK_GPI_GPIO3 0x04
>> +#define DA9210_BUCK_GPI_GPIO4 0x06
>> +#define DA9210_BUCK_PD_DIS 0x08
>> +#define DA9210_VBUCK_SEL 0x10
>> +#define DA9210_VBUCK_SEL_A 0x00
>> +#define DA9210_VBUCK_SEL_B 0x10
>> +#define DA9210_VBUCK_GPI_SHIFT 5
>> +#define DA9210_VBUCK_GPI_MASK 0x60
>> +#define DA9210_VBUCK_GPI_OFF 0x00
>> +#define DA9210_VBUCK_GPI_GPIO0 0x20
>> +#define DA9210_VBUCK_GPI_GPIO3 0x40
>> +#define DA9210_VBUCK_GPI_GPIO4 0x60
>> +#define DA9210_DVC_CTRL_EN 0x80
>> +
>> +/* DA9210_REG_BUCK_ILIM (addr=0xD0) */
>> +#define DA9210_BUCK_ILIM_SHIFT 0
>> +#define DA9210_BUCK_ILIM_MASK 0x0F
>> +#define DA9210_BUCK_IALARM 0x10
>> +
>> +/* DA9210_REG_BUCK_CONF1 (addr=0xD1) */
>> +#define DA9210_BUCK_MODE_SHIFT 0
>> +#define DA9210_BUCK_MODE_MASK 0x03
>> +#define DA9210_BUCK_MODE_MANUAL 0x00
>> +#define DA9210_BUCK_MODE_SLEEP 0x01
>> +#define DA9210_BUCK_MODE_SYNC 0x02
>> +#define DA9210_BUCK_MODE_AUTO 0x03
>> +#define DA9210_STARTUP_CTRL_SHIFT 2
>> +#define DA9210_STARTUP_CTRL_MASK 0x1C
>> +#define DA9210_PWR_DOWN_CTRL_SHIFT 5
>> +#define DA9210_PWR_DOWN_CTRL_MASK 0xE0
>> +
>> +/* DA9210_REG_BUCK_CONF2 (addr=0xD2) */
>> +#define DA9210_PHASE_SEL_SHIFT 0
>> +#define DA9210_PHASE_SEL_MASK 0x03
>> +#define DA9210_FREQ_SEL 0x40
>> +
>> +/* DA9210_REG_BUCK_AUTO (addr=0xD4) */
>> +#define DA9210_VBUCK_AUTO_SHIFT 0
>> +#define DA9210_VBUCK_AUTO_MASK 0x7F
>> +
>> +/* DA9210_REG_BUCK_BASE (addr=0xD5) */
>> +#define DA9210_VBUCK_BASE_SHIFT 0
>> +#define DA9210_VBUCK_BASE_MASK 0x7F
>> +
>> +/* DA9210_REG_VBUCK_MAX_DVC_IF (addr=0xD6) */
>> +#define DA9210_VBUCK_MAX_SHIFT 0
>> +#define DA9210_VBUCK_MAX_MASK 0x7F
>> +#define DA9210_DVC_STEP_SIZE 0x80
>> +#define DA9210_DVC_STEP_SIZE_10MV 0x00
>> +#define DA9210_DVC_STEP_SIZE_20MV 0x80
>> +
>> +/* DA9210_REG_VBUCK_DVC (addr=0xD7) */
>> +#define DA9210_VBUCK_DVC_SHIFT 0
>> +#define DA9210_VBUCK_DVC_MASK 0x7F
>> +
>> +/* DA9210_REG_VBUCK_A/B (addr=0xD8/0xD9) */
>> +#define DA9210_VBUCK_SHIFT 0
>> +#define DA9210_VBUCK_MASK 0x7F
>> +#define DA9210_VBUCK_BIAS 0
>> +#define DA9210_BUCK_SL 0x80
>> +
>> +/* DA9210_REG_INTERFACE (addr=0x105) */
>> +#define DA9210_IF_BASE_ADDR_SHIFT 4
>> +#define DA9210_IF_BASE_ADDR_MASK 0xF0
>> +
>> +/* DA9210_REG_CONFIG_E (addr=0x147) */
>> +#define DA9210_STAND_ALONE 0x01
>> +
>> +#endif /* __DA9210_REGISTERS_H__ */
>> +
>> --
>> end-of-patch for RFC V1
>>
>
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer
>http://www.open-technology.de/
Thank you for the comments.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-21 15:15 ` Mark Brown
@ 2013-06-24 9:18 ` Opensource [Steve Twiss]
0 siblings, 0 replies; 9+ messages in thread
From: Opensource [Steve Twiss] @ 2013-06-24 9:18 UTC (permalink / raw)
To: Mark Brown; +Cc: Liam Girdwood, David Dajun Chen, Guennadi Liakhovetski, LKML
On Fri, 21 June 2013, Mark Brown wrote:
>
>On Thu, Jun 20, 2013 at 02:42:03PM +0100, Steve Twiss wrote:
>
>> @@ -293,6 +293,13 @@ config REGULATOR_LP8788
>> help
>> This driver supports LP8788 voltage regulator chip.
>>
>> +config REGULATOR_DA9210
>> + bool "Dialog Semiconductor DA9210 Regulator"
>> + depends on I2C=y
>> + select REGMAP_I2C
>> + help
>> + Support for the Dialog Semiconductor DA9210 chip.
>> +
>> config REGULATOR_PCF50633
>
>This file ought to be kept sorted though it seems that's not been happening, and I'm not
>seeing any reason why this can't be a tristate.
>
I have made this alphabetical now.
I will also change this so it is capable of being built as a module then re-submit.
>> +#define DRIVER_NAME "da9210"
>
>Why?
>
This has been removed and the other places have been replaced with a string instead.
>> +struct da9210 {
>> + struct i2c_client *i2c;
>> + struct device *dev;
>> + struct mutex io_mutex;
>
>Why do you need an I/O lock?
>
This has been removed.
>> +static int da9210_enable(struct regulator_dev *rdev); static int
>> +da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
>> + int max_uV, unsigned *selector); static int
>> +da9210_get_voltage(struct regulator_dev *rdev); static int
>> +da9210_set_current_limit(struct regulator_dev *rdev, int min_uA,
>> + int max_uA);
>> +static int da9210_get_current_limit(struct regulator_dev *rdev);
>> +
>> +static struct regulator_ops da9210_buck_ops = {
>> + .enable = da9210_enable,
>> + .disable = regulator_disable_regmap,
>> + .is_enabled = regulator_is_enabled_regmap,
>> + .set_voltage = da9210_set_voltage,
>> + .get_voltage = da9210_get_voltage,
>> + .list_voltage = regulator_list_voltage_linear,
>> + .set_current_limit = da9210_set_current_limit,
>> + .get_current_limit = da9210_get_current_limit, };
>
>Drivers are normally written to avoid forward declarations like this.
>
I will review this.
>> +static struct regulator_consumer_supply __initdata def_da9210_consumers[] = {
>> + REGULATOR_SUPPLY("DA9210", NULL),
>> +};
>> +
>> +static struct regulator_init_data __initdata default_da9210_constraints = {
>> + .constraints = {
>
>This is not at all sensible, there's a good solid reason why regulator drivers don't
>generally do this... please review the machine interface and its purpose.
>
I will do this then re-submit.
>> +static int da9210_set_voltage(struct regulator_dev *rdev, int min_uV,
>> + int max_uV, unsigned *selector) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + int val;
>> + int ret;
>> +
>> + val = regulator_map_voltage_linear(rdev, min_uV, max_uV);
>> + if (val < 0)
>> + return -EINVAL;
>> +
>> + ret = regmap_update_bits(chip->regmap, DA9210_REG_VBUCK_A,
>> + DA9210_VBUCK_MASK, val);
>> + return ret;
>> +}
>
>Why not just use set_voltage_sel()?
>
Will re-write this to use core functionality
>> +static int da9210_get_voltage_sel(struct regulator_dev *rdev) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + unsigned int data;
>> + int sel;
>> + int ret;
>> +
>> + ret = regmap_read(chip->regmap, DA9210_REG_VBUCK_A, &data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + sel = (data & DA9210_VBUCK_MASK) >> DA9210_VBUCK_SHIFT;
>> + sel -= DA9210_VBUCK_BIAS;
>> + if (sel < 0)
>> + sel = 0;
>> + if (sel >= chip->info->n_steps)
>> + sel = chip->info->n_steps - 1;
>
>This looks like poor error handling, if the value is out of range isn't there an error state?
>
I will review this
>> +static int da9210_get_voltage(struct regulator_dev *rdev) {
>> + struct da9210 *chip = rdev_get_drvdata(rdev);
>> + int sel = da9210_get_voltage_sel(rdev);
>> +
>> + if (sel < 0)
>> + return sel;
>> +
>> + return (chip->info->step_uV * sel) + chip->info->min_uV; }
>
>Why are you open coding core functionalit?
>
>> +static int da9210_enable(struct regulator_dev *rdev) {
>> + return regulator_enable_regmap(rdev); }
>
>This is pointless, just use the generic function directly.
>
Done: now using regulator_enable_regmap instead of function just containing regulator_enable_regmap.
>> +
>> + dev_info(chip->dev, "Device DA9210 detected.\n");
>
>This is just noise.
>
Removed this.
>> +static const struct i2c_device_id da9210_i2c_id[] = {
>> + {DRIVER_NAME, 0},
>> + {},
>
>Just use the string.
>
>> +static struct i2c_driver da9210_regulator_driver = {
>> + .driver = {
>> + .name = DRIVER_NAME,
>
>Similarly here.
>
Yep, remove this now.
>> + .owner = THIS_MODULE,
>> + },
>
>Indentation.
>
Done.
>> +static int __init da9210_regulator_init(void) {
>> + int ret;
>> +
>> + ret = i2c_add_driver(&da9210_regulator_driver);
>> + if (0 != ret)
>> + pr_err("Failed to register da9210 I2C driver\n");
>> +
>> + return ret;
>> +}
>> +
>> +subsys_initcall(da9210_regulator_init);
>
>Just use module_platform_driver() now we have probe deferral.
>
Yes. I will look at altering this then re-submit.
>> +/*
>> + * Registers bits
>> + */
>> +/* DA9210_REG_PAGE_CON (addr=0x00) */
>> +#define DA9210_PEG_PAGE_SHIFT 0
>> +#define DA9210_REG_PAGE_MASK 0x0F
>> +/* On I2C registers 0x00 - 0xFF */
>> +#define DA9210_REG_PAGE0 0
>> +/* On I2C registers 0x100 - 0x1FF */
>> +#define DA9210_REG_PAGE2 2
>> +#define DA9210_PAGE_WRITE_MODE 0x00
>> +#define DA9210_REPEAT_WRITE_MODE 0x40
>> +#define DA9210_PAGE_REVERT 0x80
>
>This looks liike you should be using a regmap range.
I will look at this again and then resubmit.
Thanks for your comments.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-24 9:17 ` Opensource [Steve Twiss]
@ 2013-06-26 7:55 ` Guennadi Liakhovetski
2013-06-28 10:32 ` Opensource [Steve Twiss]
0 siblings, 1 reply; 9+ messages in thread
From: Guennadi Liakhovetski @ 2013-06-26 7:55 UTC (permalink / raw)
To: Opensource [Steve Twiss]
Cc: Liam Girdwood, Mark Brown, David Dajun Chen, LKML
Hi Steve
On Mon, 24 Jun 2013, Opensource [Steve Twiss] wrote:
[snip]
> Thank you for the comments.
Thanks for your work on this! I've got one more question to you though: it
looks like you're doing Linux kernel driver open-sourcing for DiaSemi ATM.
In the past
http://lwn.net/Articles/513859/
a patch series has been submitted by your employer to support the da9063
PMIC. Are there plans to continue that work and update the driver? Any
specific dates when a new submission can be expected?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-26 7:55 ` Guennadi Liakhovetski
@ 2013-06-28 10:32 ` Opensource [Steve Twiss]
2013-06-28 11:12 ` Mark Brown
0 siblings, 1 reply; 9+ messages in thread
From: Opensource [Steve Twiss] @ 2013-06-28 10:32 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Liam Girdwood, Mark Brown, David Dajun Chen, LKML
On Wed, 26 June 2013, Guennadi Liakhovetski wrote:
>
>Hi Steve
>
>On Mon, 24 Jun 2013, Opensource [Steve Twiss] wrote:
>
>[snip]
>
>> Thank you for the comments.
>
>Thanks for your work on this! I've got one more question to you though: it looks like
>you're doing Linux kernel driver open-sourcing for DiaSemi ATM.
>In the past
>
>http://lwn.net/Articles/513859/
>
>a patch series has been submitted by your employer to support the da9063 PMIC. Are
>there plans to continue that work and update the driver? Any specific dates when a new
>submission can be expected?
Hi Guennadi,
Dialog are planning to submit a DA9063 driver that 'stands alone' instead of being fully integrated with a combined DA9210 driver (i.e. the one that was uploaded to the mailing list about a year ago). Having said that, there will be a strong similarity between the combined DA9063/DA9210 mailing-list driver and the upcoming stand-alone DA9063 driver.
This DA9063 work is continuing and it will be submitted to LKML after the DA9210 work has [hopefully] been accepted. In this way I am aiming to try and side-step as many DA9063 submission pitfalls as possible by ensuring any relevant comments from this DA9210 submission work have been added into the new driver.
I hope that helps...
Regards,
Steve
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer http://www.open-technology.de/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-28 10:32 ` Opensource [Steve Twiss]
@ 2013-06-28 11:12 ` Mark Brown
2013-06-28 13:22 ` Opensource [Steve Twiss]
0 siblings, 1 reply; 9+ messages in thread
From: Mark Brown @ 2013-06-28 11:12 UTC (permalink / raw)
To: Opensource [Steve Twiss]
Cc: Guennadi Liakhovetski, Liam Girdwood, David Dajun Chen, LKML
[-- Attachment #1: Type: text/plain, Size: 664 bytes --]
On Fri, Jun 28, 2013 at 10:32:08AM +0000, Opensource [Steve Twiss] wrote:
Please fix your mail client to word wrap between paragraphs for
legibility.
> Dialog are planning to submit a DA9063 driver that 'stands alone'
> instead of being fully integrated with a combined DA9210 driver (i.e.
> the one that was uploaded to the mailing list about a year ago).
> Having said that, there will be a strong similarity between the
> combined DA9063/DA9210 mailing-list driver and the upcoming
> stand-alone DA9063 driver.
If there's a lot of similarity then one of the obvious review comments
is going to be that the two drivers should be merged to promote code
reuse.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [RFC V1] COMMIT 1: DA9210 driver files
2013-06-28 11:12 ` Mark Brown
@ 2013-06-28 13:22 ` Opensource [Steve Twiss]
0 siblings, 0 replies; 9+ messages in thread
From: Opensource [Steve Twiss] @ 2013-06-28 13:22 UTC (permalink / raw)
To: Mark Brown; +Cc: Guennadi Liakhovetski, Liam Girdwood, David Dajun Chen, LKML
On Fri, Jun 28, 2013, Mark Brown wrote:
>
>On Fri, Jun 28, 2013 at 10:32:08AM +0000, Opensource [Steve Twiss] wrote:
>
>Please fix your mail client to word wrap between paragraphs for legibility.
>
Hi Mark,
Apologies for the word-wrap.
>> Dialog are planning to submit a DA9063 driver that 'stands alone'
>> instead of being fully integrated with a combined DA9210 driver (i.e.
>> the one that was uploaded to the mailing list about a year ago).
>> Having said that, there will be a strong similarity between the
>> combined DA9063/DA9210 mailing-list driver and the upcoming
>> stand-alone DA9063 driver.
>
>If there's a lot of similarity then one of the obvious review comments is going to be that
>the two drivers should be merged to promote code reuse.
I should have been clearer.
The DA9063 and DA9210 are two totally different devices. Both in
function and in size. It was a mistake to implement one single driver
for two separate devices.
Also, I meant that before I submit the new stand-alone DA9063 driver,
I will ensure that I make use of any 'request for comments' made
during previous postings.
Sorry for the confusion.
Thanks,
Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-06-28 13:22 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-20 13:42 [RFC V1] COMMIT 1: DA9210 driver files Steve Twiss
2013-06-21 9:58 ` Guennadi Liakhovetski
2013-06-24 9:17 ` Opensource [Steve Twiss]
2013-06-26 7:55 ` Guennadi Liakhovetski
2013-06-28 10:32 ` Opensource [Steve Twiss]
2013-06-28 11:12 ` Mark Brown
2013-06-28 13:22 ` Opensource [Steve Twiss]
2013-06-21 15:15 ` Mark Brown
2013-06-24 9:18 ` Opensource [Steve Twiss]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®