From: Sonic Zhang <[email protected]>
This driver supports both the AD5398 and the AD5821. It adapts into the
voltage and current framework. The generic userspace-consumer and virtual
consumer should be selected to access devices via this driver.
Signed-off-by: Sonic Zhang <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
drivers/regulator/ad5398.c | 270 ++++++++++++++++++++++++++++++++++++++
include/linux/regulator/ad5398.h | 30 ++++
4 files changed, 307 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/ad5398.c
create mode 100644 include/linux/regulator/ad5398.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 04f2e08..2ade09c 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -201,5 +201,11 @@ config REGULATOR_88PM8607
help
This driver supports 88PM8607 voltage regulator chips.
+config REGULATOR_AD5398
+ tristate "Ananlog Devices AD5398/AD5821 regulators"
+ depends on I2C
+ help
+ This driver supports AD5398 and AD5821 current regulator chips.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 4e7feec..c256668 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o
obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
+obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
obj-$(CONFIG_REGULATOR_DUMMY) += dummy.o
obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
new file mode 100644
index 0000000..9dbfc05
--- /dev/null
+++ b/drivers/regulator/ad5398.c
@@ -0,0 +1,270 @@
+/*
+ * ad5398.c -- Voltage and current regulation for AD5398 and AD5821
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Enter bugs at http://blackfin.uclinux.org/
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/ad5398.h>
+
+struct ad5398_chip_info {
+ struct i2c_client *client;
+ unsigned int min_uA;
+ unsigned int max_uA;
+ unsigned int current_level;
+ unsigned int current_mask;
+ unsigned int current_offset;
+ struct regulator_dev rdev;
+};
+
+static int ad5398_calc_current(struct ad5398_chip_info *chip,
+ unsigned selector)
+{
+ unsigned range_uA = chip->max_uA - chip->min_uA;
+
+ return chip->min_uA + (selector * range_uA / chip->current_level);
+}
+
+static int ad5398_get(struct regulator_dev *rdev)
+{
+ struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
+ struct i2c_client *client = chip->client;
+ unsigned short data;
+ int ret;
+
+ ret = i2c_master_recv(client, (char *)&data, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+
+ ret = (be16_to_cpu(data) & chip->current_mask) >> chip->current_offset;
+
+ return ad5398_calc_current(chip, ret);
+}
+
+static int ad5398_set(struct regulator_dev *rdev, int min_uA, int max_uA)
+{
+ struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
+ struct i2c_client *client = chip->client;
+ unsigned range_uA = chip->max_uA - chip->min_uA;
+ unsigned selector;
+ unsigned short data;
+ int ret;
+
+ if (min_uA > chip->max_uA || max_uA < chip->min_uA)
+ return -EINVAL;
+ if (min_uA < chip->min_uA)
+ min_uA = chip->min_uA;
+
+ selector = ((min_uA - chip->min_uA) * chip->current_level +
+ range_uA - 1) / range_uA;
+ if (ad5398_calc_current(chip, selector) > max_uA)
+ return -EINVAL;
+
+ dev_dbg(&client->dev, "changing current %dmA\n",
+ ad5398_calc_current(chip, selector) / 1000);
+
+ /* read chip enable bit */
+ ret = i2c_master_recv(client, (char *)&data, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+ data = be16_to_cpu(data);
+
+ /* prepare register data */
+ selector = (selector << chip->current_offset) & chip->current_mask;
+ selector |= (data & CURRENT_EN_MASK);
+
+ /* write the new current value back as well as enable bit */
+ data = cpu_to_be16((unsigned short)selector);
+ ret = i2c_master_send(client, (char *)&data, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "I2C write error\n");
+
+ return ret;
+}
+
+static int ad5398_is_enabled(struct regulator_dev *rdev)
+{
+ struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
+ struct i2c_client *client = chip->client;
+ unsigned short data;
+ int ret;
+
+ ret = i2c_master_recv(client, (char *)&data, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+
+ if (be16_to_cpu(data) & CURRENT_EN_MASK)
+ return 1;
+ else
+ return 0;
+}
+
+static int ad5398_enable(struct regulator_dev *rdev)
+{
+ struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
+ struct i2c_client *client = chip->client;
+ unsigned short data;
+ int ret;
+
+ ret = i2c_master_recv(client, (char *)&data, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+ data = be16_to_cpu(data);
+ if (data & CURRENT_EN_MASK)
+ return 0;
+
+ data = cpu_to_be16(data | CURRENT_EN_MASK);
+
+ ret = i2c_master_send(client, (char *)&data, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "I2C write error\n");
+
+ return ret;
+}
+
+static int ad5398_disable(struct regulator_dev *rdev)
+{
+ struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
+ struct i2c_client *client = chip->client;
+ unsigned short data;
+ int ret;
+
+ ret = i2c_master_recv(client, (char *)&data, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+ if (!(data & CURRENT_EN_MASK))
+ return 0;
+
+ data = cpu_to_be16(data & ~CURRENT_EN_MASK);
+
+ ret = i2c_master_send(client, (char *)&data, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "I2C write error\n");
+
+ return ret;
+}
+
+static struct regulator_ops ad5398_ops = {
+ .get_current_limit = ad5398_get,
+ .set_current_limit = ad5398_set,
+ .enable = ad5398_enable,
+ .disable = ad5398_disable,
+ .is_enabled = ad5398_is_enabled,
+ .set_suspend_enable = ad5398_enable,
+ .set_suspend_disable = ad5398_disable,
+};
+
+static struct regulator_desc ad5398_reg = {
+ .name = "isink",
+ .id = 0,
+ .ops = &ad5398_ops,
+ .type = REGULATOR_CURRENT,
+ .owner = THIS_MODULE,
+};
+
+static int ad5398_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct regulator_dev *rdev;
+ struct ad5398_platform_data *pdata = client->dev.platform_data;
+ struct ad5398_chip_info *chip;
+ int ret;
+
+ if (!pdata || !(pdata->regulator_data))
+ return -EINVAL;
+
+ if (pdata->current_bits >= CURRENT_BITS_MAX)
+ return -EINVAL;
+
+ chip = kzalloc(sizeof(struct ad5398_chip_info), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->client = client;
+
+ chip->min_uA = pdata->regulator_data->constraints.min_uA;
+ chip->max_uA = pdata->regulator_data->constraints.max_uA;
+ chip->current_level = 1 << pdata->current_bits;
+ chip->current_offset = pdata->current_offset;
+ chip->current_mask = (chip->current_level - 1) << chip->current_offset;
+
+ rdev = regulator_register(&ad5398_reg, &client->dev,
+ pdata->regulator_data, chip);
+ if (IS_ERR(rdev)) {
+ ret = PTR_ERR(rdev);
+ dev_err(&client->dev, "failed to register %s %s\n",
+ id->name, ad5398_reg.name);
+ goto err;
+ }
+
+ i2c_set_clientdata(client, chip);
+ dev_info(&client->dev, "%s regulator driver loaded\n", id->name);
+ return 0;
+
+err:
+ kfree(chip);
+ return ret;
+}
+
+static int ad5398_remove(struct i2c_client *client)
+{
+ struct ad5398_chip_info *chip = i2c_get_clientdata(client);
+
+ if (chip) {
+ regulator_unregister(&chip->rdev);
+ kfree(chip);
+ }
+ i2c_set_clientdata(client, NULL);
+
+ return 0;
+}
+
+static const struct i2c_device_id ad5398_id[] = {
+ { "ad5398", 0 },
+ { "ad5821", 1 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ad5398_id);
+
+static struct i2c_driver ad5398_driver = {
+ .probe = ad5398_probe,
+ .remove = ad5398_remove,
+ .driver = {
+ .name = "ad5398",
+ },
+ .id_table = ad5398_id,
+};
+
+static int __init ad5398_init(void)
+{
+ return i2c_add_driver(&ad5398_driver);
+}
+module_init(ad5398_init);
+
+static void __exit ad5398_exit(void)
+{
+ i2c_del_driver(&ad5398_driver);
+}
+module_exit(ad5398_exit);
+
+MODULE_DESCRIPTION("AD5398 and AD5821 current regulator driver");
+MODULE_AUTHOR("Sonic Zhang");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regulator/ad5398.h b/include/linux/regulator/ad5398.h
new file mode 100644
index 0000000..92c9fcb
--- /dev/null
+++ b/include/linux/regulator/ad5398.h
@@ -0,0 +1,30 @@
+/*
+ * ad5398.h -- Voltage and current regulation for AD5398 and AD5821
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Enter bugs at http://blackfin.uclinux.org/
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef REGULATOR_AD5398
+#define REGULATOR_AD5398
+
+#include <linux/regulator/machine.h>
+
+#define CURRENT_EN_MASK 0x8000
+#define CURRENT_BITS_MAX 16
+
+/**
+ * ad5398_platform_data - platform data for ad5398
+ * @current_bits: effective bits in register
+ * @current_offset: offset of effective bits in register
+ * @ad5398_init_data: regulator init data
+ */
+struct ad5398_platform_data {
+ unsigned short current_bits;
+ unsigned short current_offset;
+ struct regulator_init_data *regulator_data;
+};
+
+#endif
--
1.7.1
From: Sonic Zhang <[email protected]>
With userspace consumer enabled, power devices with only one switch
PIN connect to GPIO port can be enabled and disable via sysfs interface.
Following chips are supported.
AD122, AD123, AD124, AD125, AD150, AD5022.
Signed-off-by: Sonic Zhang <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
drivers/regulator/adp_switch.c | 172 ++++++++++++++++++++++++++++++++++
include/linux/regulator/adp_switch.h | 26 +++++
4 files changed, 205 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/adp_switch.c
create mode 100644 include/linux/regulator/adp_switch.h
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 2ade09c..7760f16 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -207,5 +207,11 @@ config REGULATOR_AD5398
help
This driver supports AD5398 and AD5821 current regulator chips.
+config REGULATOR_ADP_SWITCH
+ tristate "Ananlog Devices Switch only power regulators"
+ help
+ This driver supports ADP series siwtch only power regulator chips.
+ ADP122, ADP123, ADP124, ADP125, ADP150, ADP5022, etc.
+
endif
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index c256668..10e7a5a 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o
obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o
obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o
+obj-$(CONFIG_REGULATOR_ADP_SWITCH) += adp_switch.o
obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o
obj-$(CONFIG_REGULATOR_DUMMY) += dummy.o
obj-$(CONFIG_REGULATOR_LP3971) += lp3971.o
diff --git a/drivers/regulator/adp_switch.c b/drivers/regulator/adp_switch.c
new file mode 100644
index 0000000..37951d2
--- /dev/null
+++ b/drivers/regulator/adp_switch.c
@@ -0,0 +1,172 @@
+/*
+ * adp_switch.c -- Voltage regulation for switch only power devices
+ * AD122, AD123, AD124, AD125, AD150, AD5022, etc
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Enter bugs at http://blackfin.uclinux.org/
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/adp_switch.h>
+
+struct adp_switch_chip_info {
+ unsigned short gpio_port;
+ unsigned short enabled;
+ struct regulator_dev *rdev;
+ struct regulator_desc rdesc;
+};
+
+static int adp_switch_is_enabled(struct regulator_dev *rdev)
+{
+ struct adp_switch_chip_info *chip = rdev_get_drvdata(rdev);
+
+ return chip->enabled;
+}
+
+static int adp_switch_enable(struct regulator_dev *rdev)
+{
+ struct adp_switch_chip_info *chip = rdev_get_drvdata(rdev);
+
+ gpio_set_value(chip->gpio_port, 1);
+
+ return 0;
+}
+
+static int adp_switch_disable(struct regulator_dev *rdev)
+{
+ struct adp_switch_chip_info *chip = rdev_get_drvdata(rdev);
+
+ gpio_set_value(chip->gpio_port, 0);
+
+ return 0;
+}
+
+static struct regulator_ops adp_switch_ops = {
+ .enable = adp_switch_enable,
+ .disable = adp_switch_disable,
+ .is_enabled = adp_switch_is_enabled,
+ .set_suspend_enable = adp_switch_enable,
+ .set_suspend_disable = adp_switch_disable,
+};
+
+static int __devinit adp_switch_probe(struct platform_device *pdev)
+{
+ struct regulator_dev *rdev;
+ struct adp_switch_platform_data *pdata = pdev->dev.platform_data;
+ struct adp_switch_chip_info *chip;
+ int i, ret = 0;
+
+ dev_dbg(&pdev->dev, "%s enter\n", __func__);
+
+ if (!pdata || !pdata->regulator_num)
+ return -EINVAL;
+
+ chip = kzalloc(sizeof(struct adp_switch_chip_info) *
+ pdata->regulator_num, GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ for (i = 0; i < pdata->regulator_num; i++) {
+ /*
+ * The GPIO port of the specific regulator is in driver_data field
+ * of struct regulator_init_data defined in its platform board file.
+ */
+ chip[i].gpio_port =
+ (unsigned short)(unsigned int)pdata->regulator_data[i].driver_data;
+ chip[i].rdesc.name = pdata->regulator_data[i].consumer_supplies->supply;
+ chip[i].rdesc.id = i,
+ chip[i].rdesc.ops = &adp_switch_ops,
+ chip[i].rdesc.type = REGULATOR_CURRENT,
+ chip[i].rdesc.owner = THIS_MODULE,
+
+ ret = gpio_request(chip[i].gpio_port, "adp_switch");
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Fail to request adp switch peripherals\n");
+ goto err;
+ }
+
+ rdev = regulator_register(&chip[i].rdesc, &pdev->dev,
+ &pdata->regulator_data[i], &chip[i]);
+ if (IS_ERR(rdev)) {
+ ret = PTR_ERR(rdev);
+ dev_err(&pdev->dev, "failed to register %s\n",
+ chip[i].rdesc.name);
+ gpio_free(chip[i].gpio_port);
+ goto err;
+ }
+
+ gpio_direction_output(chip[i].gpio_port, 0);
+ chip[i].rdev = rdev;
+ }
+
+ dev_set_drvdata(&pdev->dev, chip);
+ dev_info(&pdev->dev, "regulator driver loaded\n");
+ return 0;
+
+err:
+ while (--i >= 0) {
+ regulator_unregister(chip[i].rdev);
+ gpio_free(chip[i].gpio_port);
+ }
+ kfree(chip);
+ return ret;
+}
+
+static int __devexit adp_switch_remove(struct platform_device *pdev)
+{
+ struct adp_switch_chip_info *chip = platform_get_drvdata(pdev);
+ struct adp_switch_platform_data *pdata = pdev->dev.platform_data;
+ int i;
+
+ dev_dbg(&pdev->dev, "%s enter\n", __func__);
+ dev_set_drvdata(&pdev->dev, NULL);
+
+
+ if (chip) {
+ for (i = 0; i < pdata->regulator_num; i++) {
+ regulator_unregister(chip[i].rdev);
+ gpio_free(chip[i].gpio_port);
+ }
+ kfree(chip);
+ }
+
+ return 0;
+}
+
+static struct platform_driver adp_switch_driver = {
+ .probe = adp_switch_probe,
+ .remove = __devexit_p(adp_switch_remove),
+ .driver = {
+ .name = "adp_switch",
+ },
+};
+
+static int __init adp_switch_init(void)
+{
+ int ret;
+
+ ret = platform_driver_register(&adp_switch_driver);
+ if (ret)
+ pr_err("failed to register adp switch driver:%d\n", ret);
+
+ return ret;
+}
+module_init(adp_switch_init);
+
+static void __exit adp_switch_exit(void)
+{
+ platform_driver_unregister(&adp_switch_driver);
+}
+module_exit(adp_switch_exit);
+
+MODULE_DESCRIPTION("Switch only power regulator driver");
+MODULE_AUTHOR("Sonic Zhang");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regulator/adp_switch.h b/include/linux/regulator/adp_switch.h
new file mode 100644
index 0000000..bbfac61
--- /dev/null
+++ b/include/linux/regulator/adp_switch.h
@@ -0,0 +1,26 @@
+/*
+ * adp_switch.h -- Voltage regulation for switch only power devices.
+ * AD122, AD123, AD124, AD125, AD150, AD5022, etc.
+ *
+ * Copyright 2010 Analog Devices Inc.
+ *
+ * Enter bugs at http://blackfin.uclinux.org/
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef REGULATOR_ADP_SWITCH
+#define REGULATOR_ADP_SWITCH
+
+#include <linux/regulator/machine.h>
+
+/**
+ * adp_switch_platform_data - platform data for power swtich
+ * @regulator_num: number of regulators
+ * @regulator_data: regulator init data
+ */
+struct adp_switch_platform_data {
+ unsigned short regulator_num;
+ struct regulator_init_data *regulator_data;
+};
+
+#endif
--
1.7.1
On Tue, Jun 01, 2010 at 09:33:59AM -0400, Mike Frysinger wrote:
> With userspace consumer enabled, power devices with only one switch
> PIN connect to GPIO port can be enabled and disable via sysfs interface.
This looks exactly like a fixed voltage regualtor except there is no
facility for reporting the voltage. Why is a new driver being supplied?
On Tue, Jun 01, 2010 at 09:33:58AM -0400, Mike Frysinger wrote:
> From: Sonic Zhang <[email protected]>
> This driver supports both the AD5398 and the AD5821. It adapts into the
> voltage and current framework. The generic userspace-consumer and virtual
Might be useful to put something here saying what these chips do...
> consumer should be selected to access devices via this driver.
Please remove the comment about the userspace and virtual consumers from
the driver, the driver should support all consumer drivers and the use
of userspace consumers will normally be very unusual.
> +static int ad5398_get(struct regulator_dev *rdev)
> +{
> + struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
> + struct i2c_client *client = chip->client;
> + unsigned short data;
> + int ret;
> +
> + ret = i2c_master_recv(client, (char *)&data, 2);
> + if (ret < 0) {
> + dev_err(&client->dev, "I2C read error\n");
> + return ret;
> + }
I strongly suggest implementing register I/O functions rather than open
coding with the I2C API each time.
> +
> + ret = (be16_to_cpu(data) & chip->current_mask) >> chip->current_offset;
> +
> + return ad5398_calc_current(chip, ret);
This function should really have a much clearer name than just plain
_get() - it looks like it's intended to read the current limit.
> +static int ad5398_set(struct regulator_dev *rdev, int min_uA, int max_uA)
> +{
Same here - this should have a much clearer name saying what's being
set.
> + /* prepare register data */
> + selector = (selector << chip->current_offset) & chip->current_mask;
> + selector |= (data & CURRENT_EN_MASK);
This CURRENT_EN_MASK configuration looks fishy, what does that setting
do and why is it being unconditionally set?
> +static struct regulator_ops ad5398_ops = {
> + .get_current_limit = ad5398_get,
> + .set_current_limit = ad5398_set,
> + .enable = ad5398_enable,
> + .disable = ad5398_disable,
> + .is_enabled = ad5398_is_enabled,
> + .set_suspend_enable = ad5398_enable,
> + .set_suspend_disable = ad5398_disable,
If the chip does not have suspend mode configuration it should not
supply any suspend mode configuration functions.
> + i2c_set_clientdata(client, chip);
> + dev_info(&client->dev, "%s regulator driver loaded\n", id->name);
Remove this or tone it down to a debug level print.
> +static int ad5398_remove(struct i2c_client *client)
> +{
> + struct ad5398_chip_info *chip = i2c_get_clientdata(client);
> +
> + if (chip) {
> + regulator_unregister(&chip->rdev);
> + kfree(chip);
> + }
If you failed to register the regulator you should've failed the probe
and therefore this check for chip should not be required.
> +#define CURRENT_EN_MASK 0x8000
> +#define CURRENT_BITS_MAX 16
These defines should be namespaced.
> +/**
> + * ad5398_platform_data - platform data for ad5398
> + * @current_bits: effective bits in register
> + * @current_offset: offset of effective bits in register
> + * @ad5398_init_data: regulator init data
> + */
> +struct ad5398_platform_data {
> + unsigned short current_bits;
> + unsigned short current_offset;
> + struct regulator_init_data *regulator_data;
> +};
Why are the current bits and offset being suppied as platform data? I
would *very* strongly expect that the location of the control in the
register will be fixed by the device type and should therefore be
figured out by the driver. Having the machine specifying this seems
redundant and error prone.
On Tue, Jun 1, 2010 at 10:25 PM, Mark Brown
<[email protected]> wrote:
> On Tue, Jun 01, 2010 at 09:33:59AM -0400, Mike Frysinger wrote:
>
>> With userspace consumer enabled, power devices with only one switch
>> PIN connect to GPIO port can be enabled and disable via sysfs interface.
>
> This looks exactly like a fixed voltage regualtor except there is no
> facility for reporting the voltage. ?Why is a new driver being supplied?
Yes, you are right. AD122, AD123, AD124, AD125, AD150 and AD5022 can
reuse the fixed voltage regulator.
Please ignore this patch.
Sonic
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Tue, Jun 1, 2010 at 10:37 PM, Mark Brown
<[email protected]> wrote:
> On Tue, Jun 01, 2010 at 09:33:58AM -0400, Mike Frysinger wrote:
>> From: Sonic Zhang <[email protected]>
>
>> This driver supports both the AD5398 and the AD5821. ?It adapts into the
>> voltage and current framework. ?The generic userspace-consumer and virtual
>
> Might be useful to put something here saying what these chips do...
>
OK.
>> consumer should be selected to access devices via this driver.
>
> Please remove the comment about the userspace and virtual consumers from
> the driver, the driver should support all consumer drivers and the use
> of userspace consumers will normally be very unusual.
>
OK.
>> +static int ad5398_get(struct regulator_dev *rdev)
>> +{
>> + ? ? struct ad5398_chip_info *chip = rdev_get_drvdata(rdev);
>> + ? ? struct i2c_client *client = chip->client;
>> + ? ? unsigned short data;
>> + ? ? int ret;
>> +
>> + ? ? ret = i2c_master_recv(client, (char *)&data, 2);
>> + ? ? if (ret < 0) {
>> + ? ? ? ? ? ? dev_err(&client->dev, "I2C read error\n");
>> + ? ? ? ? ? ? return ret;
>> + ? ? }
>
> I strongly suggest implementing register I/O functions rather than open
> coding with the I2C API each time.
>
No problem.
>> +
>> + ? ? ret = (be16_to_cpu(data) & chip->current_mask) >> chip->current_offset;
>> +
>> + ? ? return ad5398_calc_current(chip, ret);
>
> This function should really have a much clearer name than just plain
> _get() - it looks like it's intended to read the current limit.
>
>> +static int ad5398_set(struct regulator_dev *rdev, int min_uA, int max_uA)
>> +{
>
> Same here - this should have a much clearer name saying what's being
> set.
>
OK.
>> + ? ? /* prepare register data */
>> + ? ? selector = (selector << chip->current_offset) & chip->current_mask;
>> + ? ? selector |= (data & CURRENT_EN_MASK);
>
> This CURRENT_EN_MASK configuration looks fishy, what does that setting
> do and why is it being unconditionally set?
>
CURRENT_EN_MASK is the power on control bit of these current regulator chip.
It is part of the current value register. It is not touched when
setting the current value.
>> +static struct regulator_ops ad5398_ops = {
>> + ? ? .get_current_limit = ad5398_get,
>> + ? ? .set_current_limit = ad5398_set,
>> + ? ? .enable = ad5398_enable,
>> + ? ? .disable = ad5398_disable,
>> + ? ? .is_enabled = ad5398_is_enabled,
>> + ? ? .set_suspend_enable = ad5398_enable,
>> + ? ? .set_suspend_disable = ad5398_disable,
>
> If the chip does not have suspend mode configuration it should not
> supply any suspend mode configuration functions.
>
OK. I will remove them.
>> + ? ? i2c_set_clientdata(client, chip);
>> + ? ? dev_info(&client->dev, "%s regulator driver loaded\n", id->name);
>
> Remove this or tone it down to a debug level print.
>
>> +static int ad5398_remove(struct i2c_client *client)
>> +{
>> + ? ? struct ad5398_chip_info *chip = i2c_get_clientdata(client);
>> +
>> + ? ? if (chip) {
>> + ? ? ? ? ? ? regulator_unregister(&chip->rdev);
>> + ? ? ? ? ? ? kfree(chip);
>> + ? ? }
>
> If you failed to register the regulator you should've failed the probe
> and therefore this check for chip should not be required.
>
OK.
>> +#define CURRENT_EN_MASK ? ? ? ? ? ? ?0x8000
>> +#define CURRENT_BITS_MAX ? ? 16
>
> These defines should be namespaced.
>
No problem.
>> +/**
>> + * ad5398_platform_data - platform data for ad5398
>> + * @current_bits: effective bits in register
>> + * @current_offset: offset of effective bits in register
>> + * @ad5398_init_data: regulator init data
>> + */
>> +struct ad5398_platform_data {
>> + ? ? unsigned short current_bits;
>> + ? ? unsigned short current_offset;
>> + ? ? struct regulator_init_data *regulator_data;
>> +};
>
> Why are the current bits and offset being suppied as platform data? ?I
> would *very* strongly expect that the location of the control in the
> register will be fixed by the device type and should therefore be
> figured out by the driver. ?Having the machine specifying this seems
> redundant and error prone.
Yes, define these data format information in driver for each supported
chip is a better way.
I will send out the new patch soon.
Sonic
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at ?http://www.tux.org/lkml/
>
On Tue 1 Jun 2010 10:37, Mark Brown pondered:
> On Tue, Jun 01, 2010 at 09:33:58AM -0400, Mike Frysinger wrote:
> > From: Sonic Zhang <[email protected]>
>
> > This driver supports both the AD5398 and the AD5821. It adapts into the
> > voltage and current framework. The generic userspace-consumer and virtual
>
> Might be useful to put something here saying what these chips do...
[snip]
> > +/**
> > + * ad5398_platform_data - platform data for ad5398
> > + * @current_bits: effective bits in register
> > + * @current_offset: offset of effective bits in register
> > + * @ad5398_init_data: regulator init data
> > + */
> > +struct ad5398_platform_data {
> > + unsigned short current_bits;
> > + unsigned short current_offset;
> > + struct regulator_init_data *regulator_data;
> > +};
>
> Why are the current bits and offset being suppied as platform data? I
> would *very* strongly expect that the location of the control in the
> register will be fixed by the device type and should therefore be
> figured out by the driver. Having the machine specifying this seems
> redundant and error prone.
Since the parts are general purpose (their not PMU, but people use them to
build their own PCB defined power management system) - so it depends on the
PCB implementation...
http://www.analog.com/ad5398
http://www.analog.com/ad5821
are both I2C D/A Converter, with 120mA sink capability
-Robin
On Thu, Jun 03, 2010 at 02:59:53PM -0400, Robin Getz wrote:
> On Tue 1 Jun 2010 10:37, Mark Brown pondered:
> > Why are the current bits and offset being suppied as platform data? I
> > would *very* strongly expect that the location of the control in the
> > register will be fixed by the device type and should therefore be
> > figured out by the driver. Having the machine specifying this seems
> > redundant and error prone.
> Since the parts are general purpose (their not PMU, but people use them to
> build their own PCB defined power management system) - so it depends on the
> PCB implementation...
Please look more closely at what's actually being done by the code here.
This is controlling the location of the bitfields in the register map.
It would be exceptionally unusal for the PCB design and layout to affect
the register map of the device at all - this will be fixed in silicon no
matter how the system is wired up. Even without a regulator framework
there would be no need to specify where the bitfields in the register
map are located via platform data.
As far as system specific integration goes the regulator driver should
only be worrying about the properties of the chip, other parts of the
regulator API handle the board-specific setup.
> http://www.analog.com/ad5398
> http://www.analog.com/ad5821
> are both I2C D/A Converter, with 120mA sink capability
This is not at all unusual for drivers/regulator except in that it's a
current regulator and voltage regulators are very much more common.