The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current
sink capability. They feature an internal reference and operates from
a single 2.7 V to 5.5 V supply.
This driver supports both the AD5398 and the AD5821. It adapts into the
voltage and current framework.
Signed-off-by: Sonic Zhang <[email protected]>
---
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile | 1 +
drivers/regulator/ad5398.c | 285 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 292 insertions(+), 0 deletions(-)
create mode 100644 drivers/regulator/ad5398.c
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..4d6cb26
--- /dev/null
+++ b/drivers/regulator/ad5398.c
@@ -0,0 +1,285 @@
+/*
+ * 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/machine.h>
+
+#define AD5398_CURRENT_EN_MASK 0x8000
+
+struct ad5398_chip_info {
+ struct i2c_client *client;
+ int min_uA;
+ 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_read_reg(struct i2c_client *client, unsigned short *data)
+{
+ unsigned short val;
+ int ret;
+
+ ret = i2c_master_recv(client, (char *)&val, 2);
+ if (ret < 0) {
+ dev_err(&client->dev, "I2C read error\n");
+ return ret;
+ }
+ *data = swab16(val);
+
+ return ret;
+}
+
+static int ad5398_write_reg(struct i2c_client *client, const unsigned short data)
+{
+ unsigned short val;
+ int ret;
+
+ val = swab16(data);
+ ret = i2c_master_send(client, (char *)&val, 2);
+ if (ret < 0)
+ dev_err(&client->dev, "I2C write error\n");
+
+ return ret;
+}
+
+static int ad5398_get_current_limit(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 = ad5398_read_reg(client, &data);
+ if (ret < 0)
+ return ret;
+
+ ret = (data & chip->current_mask) >> chip->current_offset;
+
+ return ad5398_calc_current(chip, ret);
+}
+
+static int ad5398_set_current_limit(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 = ad5398_read_reg(client, &data);
+ if (ret < 0)
+ return ret;
+
+ /* prepare register data */
+ selector = (selector << chip->current_offset) & chip->current_mask;
+ selector |= (data & AD5398_CURRENT_EN_MASK);
+
+ /* write the new current value back as well as enable bit */
+ ret = ad5398_write_reg(client, (unsigned short)selector);
+
+ 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 = ad5398_read_reg(client, &data);
+ if (ret < 0)
+ return ret;
+
+ if (data & AD5398_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 = ad5398_read_reg(client, &data);
+ if (ret < 0)
+ return ret;
+
+ if (data & AD5398_CURRENT_EN_MASK)
+ return 0;
+
+ data |= AD5398_CURRENT_EN_MASK;
+
+ ret = ad5398_write_reg(client, data);
+
+ 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 = ad5398_read_reg(client, &data);
+ if (ret < 0)
+ return ret;
+
+ if (!(data & AD5398_CURRENT_EN_MASK))
+ return 0;
+
+ data &= ~AD5398_CURRENT_EN_MASK;
+
+ ret = ad5398_write_reg(client, data);
+
+ return ret;
+}
+
+static struct regulator_ops ad5398_ops = {
+ .get_current_limit = ad5398_get_current_limit,
+ .set_current_limit = ad5398_set_current_limit,
+ .enable = ad5398_enable,
+ .disable = ad5398_disable,
+ .is_enabled = ad5398_is_enabled,
+};
+
+static struct regulator_desc ad5398_reg = {
+ .name = "isink",
+ .id = 0,
+ .ops = &ad5398_ops,
+ .type = REGULATOR_CURRENT,
+ .owner = THIS_MODULE,
+};
+
+struct ad5398_current_data_format {
+ int current_bits;
+ int current_offset;
+};
+
+static const struct ad5398_current_data_format ad5398_df = {10, 4};
+static const struct ad5398_current_data_format ad5821_df = {10, 4};
+
+static const struct i2c_device_id ad5398_id[] = {
+ { "ad5398", (kernel_ulong_t)&ad5398_df },
+ { "ad5821", (kernel_ulong_t)&ad5821_df },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, ad5398_id);
+
+static int ad5398_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct regulator_dev *rdev;
+ struct regulator_init_data *init_data = client->dev.platform_data;
+ struct ad5398_chip_info *chip;
+ struct ad5398_current_data_format *df =
+ (struct ad5398_current_data_format *)id->driver_data;
+ int ret;
+
+ if (!init_data)
+ return -EINVAL;
+
+ chip = kzalloc(sizeof(struct ad5398_chip_info), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->client = client;
+
+ chip->min_uA = init_data->constraints.min_uA;
+ chip->max_uA = init_data->constraints.max_uA;
+ chip->current_level = 1 << df->current_bits;
+ chip->current_offset = df->current_offset;
+ chip->current_mask = (chip->current_level - 1) << chip->current_offset;
+
+ rdev = regulator_register(&ad5398_reg, &client->dev, init_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_dbg(&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);
+
+ regulator_unregister(&chip->rdev);
+ kfree(chip);
+ i2c_set_clientdata(client, NULL);
+
+ return 0;
+}
+
+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");
--
1.6.0
On Wed, Jun 2, 2010 at 04:51, sonic zhang wrote:
> The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current
> sink capability. They feature an internal reference and operates from
> a single 2.7 V to 5.5 V supply.
>
> This driver supports both the AD5398 and the AD5821. It adapts into the
> voltage and current framework.
>
>
> Signed-off-by: Sonic Zhang <[email protected]>
the "From:" doesnt match your s-o-b tag ...
also, this dropped my s-o-b:
Signed-off-by: Mike Frysinger <[email protected]>
> +config REGULATOR_AD5398
> + tristate "Ananlog Devices AD5398/AD5821 regulators"
Analog
> + depends on I2C
> + help
> + This driver supports AD5398 and AD5821 current regulator chips.
should mention the module name if built as a module
> --- /dev/null
> +++ b/drivers/regulator/ad5398.c
> @@ -0,0 +1,285 @@
> +/*
> + * ad5398.c -- Voltage and current regulation for AD5398 and AD5821
dont think the filename needs to be here
> + */
> +#include <linux/module.h>
should prob be a newline between these
> + /* write the new current value back as well as enable bit */
> + ret = ad5398_write_reg(client, (unsigned short)selector);
the prototype of write_reg takes an unsigned short, so this cast is useless
> +static struct regulator_ops ad5398_ops = {
> + .get_current_limit = ad5398_get_current_limit,
> + .set_current_limit = ad5398_set_current_limit,
> + .enable = ad5398_enable,
> + .disable = ad5398_disable,
> + .is_enabled = ad5398_is_enabled,
> +};
> +
> +static struct regulator_desc ad5398_reg = {
not specific to this driver, but it looks like regulator_ops and
regulator_desc really should be constified
> +static const struct ad5398_current_data_format ad5398_df = {10, 4};
> +static const struct ad5398_current_data_format ad5821_df = {10, 4};
> +
> +static const struct i2c_device_id ad5398_id[] = {
> + { "ad5398", (kernel_ulong_t)&ad5398_df },
> + { "ad5821", (kernel_ulong_t)&ad5821_df },
> + { }
> +};
do you really need sep storage for these _df vars ?
> +static int ad5398_probe(struct i2c_client *client,
> +static int ad5398_remove(struct i2c_client *client)
> + .remove = ad5398_remove,
missing hotplug section markings
> + struct ad5398_current_data_format *df =
> + (struct ad5398_current_data_format *)id->driver_data;
this too should be const
> + chip = kzalloc(sizeof(struct ad5398_chip_info), GFP_KERNEL);
sizeof(*chip)
> + dev_dbg(&client->dev, "%s regulator driver loaded\n", id->name);
does the regulator core take care of displaying a notice ? if not,
i'd make this dev_info(). also, this should be "registered", not
"loaded".
> +MODULE_DESCRIPTION("AD5398 and AD5821 current regulator driver");
> +MODULE_AUTHOR("Sonic Zhang");
> +MODULE_LICENSE("GPL");
should there be a MODULE_ALIAS() ?
-mike
On Wed, Jun 2, 2010 at 5:02 PM, Mike Frysinger <[email protected]> wrote:
> On Wed, Jun 2, 2010 at 04:51, sonic zhang wrote:
>> The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current
>> sink capability. They feature an internal reference and operates from
>> a single 2.7 V to 5.5 V supply.
>>
>> This driver supports both the AD5398 and the AD5821. ?It adapts into the
>> voltage and current framework.
>>
>>
>> Signed-off-by: Sonic Zhang <[email protected]>
>
> the "From:" doesnt match your s-o-b tag ...
>
Does this need to be matched? I prefer to discuss via gmail account
while keep company email in the patch owner information.
> also, this dropped my s-o-b:
> Signed-off-by: Mike Frysinger <[email protected]>
>
>> +config REGULATOR_AD5398
>> + ? ? ? tristate "Ananlog Devices AD5398/AD5821 regulators"
>
> Analog
>
>> + ? ? ? depends on I2C
>> + ? ? ? help
>> + ? ? ? ? This driver supports AD5398 and AD5821 current regulator chips.
>
> should mention the module name if built as a module
>
>> --- /dev/null
>> +++ b/drivers/regulator/ad5398.c
>> @@ -0,0 +1,285 @@
>> +/*
>> + * ad5398.c ?-- ?Voltage and current regulation for AD5398 and AD5821
>
> dont think the filename needs to be here
>
>> + */
>> +#include <linux/module.h>
>
> should prob be a newline between these
>
>> + ? ? ? /* write the new current value back as well as enable bit */
>> + ? ? ? ret = ad5398_write_reg(client, (unsigned short)selector);
>
> the prototype of write_reg takes an unsigned short, so this cast is useless
>
>> +static struct regulator_ops ad5398_ops = {
>> + ? ? ? .get_current_limit = ad5398_get_current_limit,
>> + ? ? ? .set_current_limit = ad5398_set_current_limit,
>> + ? ? ? .enable = ad5398_enable,
>> + ? ? ? .disable = ad5398_disable,
>> + ? ? ? .is_enabled = ad5398_is_enabled,
>> +};
>> +
>> +static struct regulator_desc ad5398_reg = {
>
> not specific to this driver, but it looks like regulator_ops and
> regulator_desc really should be constified
>
>> +static const struct ad5398_current_data_format ad5398_df = {10, 4};
>> +static const struct ad5398_current_data_format ad5821_df = {10, 4};
>> +
>> +static const struct i2c_device_id ad5398_id[] = {
>> + ? ? ? { "ad5398", (kernel_ulong_t)&ad5398_df },
>> + ? ? ? { "ad5821", (kernel_ulong_t)&ad5821_df },
>> + ? ? ? { }
>> +};
>
> do you really need sep storage for these _df vars ?
>
Yes, this makes probe code simpler.
>> +static int ad5398_probe(struct i2c_client *client,
>> +static int ad5398_remove(struct i2c_client *client)
>> + ? ? ? .remove = ad5398_remove,
>
> missing hotplug section markings
>
>> + ? ? ? struct ad5398_current_data_format *df =
>> + ? ? ? ? ? ? ? ? ? ? ? (struct ad5398_current_data_format *)id->driver_data;
>
> this too should be const
>
>> + ? ? ? chip = kzalloc(sizeof(struct ad5398_chip_info), GFP_KERNEL);
>
> sizeof(*chip)
>
>> + ? ? ? dev_dbg(&client->dev, "%s regulator driver loaded\n", id->name);
>
> does the regulator core take care of displaying a notice ? ?if not,
> i'd make this dev_info(). ?also, this should be "registered", not
> "loaded".
>
Mark prefers to have it printed only for debugging mode.
>> +MODULE_DESCRIPTION("AD5398 and AD5821 current regulator driver");
>> +MODULE_AUTHOR("Sonic Zhang");
>> +MODULE_LICENSE("GPL");
>
> should there be a MODULE_ALIAS() ?
> -mike
>
On Wed, Jun 2, 2010 at 05:29, Sonic Zhang wrote:
> On Wed, Jun 2, 2010 at 5:02 PM, Mike Frysinger wrote:
>> On Wed, Jun 2, 2010 at 04:51, sonic zhang wrote:
>>> The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current
>>> sink capability. They feature an internal reference and operates from
>>> a single 2.7 V to 5.5 V supply.
>>>
>>> This driver supports both the AD5398 and the AD5821. It adapts into the
>>> voltage and current framework.
>>>
>>>
>>> Signed-off-by: Sonic Zhang <[email protected]>
>>
>> the "From:" doesnt match your s-o-b tag ...
>
> Does this need to be matched? I prefer to discuss via gmail account
> while keep company email in the patch owner information.
so set the From: in the body as the first line like git-send-email does it:
From: <author info>
<commit message>
<other tags>
---
<diffstat>
..........
>>> +static const struct ad5398_current_data_format ad5398_df = {10, 4};
>>> +static const struct ad5398_current_data_format ad5821_df = {10, 4};
>>> +
>>> +static const struct i2c_device_id ad5398_id[] = {
>>> + { "ad5398", (kernel_ulong_t)&ad5398_df },
>>> + { "ad5821", (kernel_ulong_t)&ad5821_df },
>>> + { }
>>> +};
>>
>> do you really need sep storage for these _df vars ?
>
> Yes, this makes probe code simpler.
how does it make any difference to the probe code what each id is
pointing to ? it isnt comparing the private data pointers to any
other storage pointers.
from what i can see, this should give the same exact behavior:
static const struct ad5398_current_data_format df_10_4 = {10, 4};
static const struct i2c_device_id ad5398_id[] = {
{ "ad5398", (kernel_ulong_t)&df_10_4 },
{ "ad5821", (kernel_ulong_t)&df_10_4 },
-mike
On Wed, Jun 02, 2010 at 05:02:58AM -0400, Mike Frysinger wrote:
> > + dev_dbg(&client->dev, "%s regulator driver loaded\n", id->name);
> does the regulator core take care of displaying a notice ? if not,
> i'd make this dev_info(). also, this should be "registered", not
> "loaded".
A message will be displayed if the device actually has any constraints
(and is therefore in use) so if it makes any difference to the system
there will be some chat.
> > +MODULE_DESCRIPTION("AD5398 and AD5821 current regulator driver");
> > +MODULE_AUTHOR("Sonic Zhang");
> > +MODULE_LICENSE("GPL");
> should there be a MODULE_ALIAS() ?
Possibly, though it's unlikely that actual systems will ever build a
regulator they're using as a module.
On Wed, Jun 2, 2010 at 5:36 PM, Mike Frysinger <[email protected]> wrote:
> On Wed, Jun 2, 2010 at 05:29, Sonic Zhang wrote:
>> On Wed, Jun 2, 2010 at 5:02 PM, Mike Frysinger wrote:
>>> On Wed, Jun 2, 2010 at 04:51, sonic zhang wrote:
>>>> The AD5398 and AD5821 are single 10-bit DAC with 120 mA output current
>>>> sink capability. They feature an internal reference and operates from
>>>> a single 2.7 V to 5.5 V supply.
>>>>
>>>> This driver supports both the AD5398 and the AD5821. ?It adapts into the
>>>> voltage and current framework.
>>>>
>>>>
>>>> Signed-off-by: Sonic Zhang <[email protected]>
>>>
>>> the "From:" doesnt match your s-o-b tag ...
>>
>> Does this need to be matched? I prefer to discuss via gmail account
>> while keep company email in the patch owner information.
>
> so set the From: in the body as the first line like git-send-email does it:
> From: <author info>
>
> <commit message>
>
> <other tags>
> ---
> ?<diffstat>
> ..........
>
>>>> +static const struct ad5398_current_data_format ad5398_df = {10, 4};
>>>> +static const struct ad5398_current_data_format ad5821_df = {10, 4};
>>>> +
>>>> +static const struct i2c_device_id ad5398_id[] = {
>>>> + ? ? ? { "ad5398", (kernel_ulong_t)&ad5398_df },
>>>> + ? ? ? { "ad5821", (kernel_ulong_t)&ad5821_df },
>>>> + ? ? ? { }
>>>> +};
>>>
>>> do you really need sep storage for these _df vars ?
>>
>> Yes, this makes probe code simpler.
>
> how does it make any difference to the probe code what each id is
> pointing to ? ?it isnt comparing the private data pointers to any
> other storage pointers.
>
> from what i can see, this should give the same exact behavior:
> static const struct ad5398_current_data_format df_10_4 = {10, 4};
> static const struct i2c_device_id ad5398_id[] = {
> ? ? ? { "ad5398", (kernel_ulong_t)&df_10_4 },
> ? ? ? { "ad5821", (kernel_ulong_t)&df_10_4 },
> -mike
>
Yes, the behavior is the same for ad5398 and ad5821. But, if more
chips are enabled in this driver, they may differ.
This line is used as an example for future chips.
Sonic
On Wed, Jun 2, 2010 at 06:10, Sonic Zhang wrote:
> On Wed, Jun 2, 2010 at 5:36 PM, Mike Frysinger wrote:
>> On Wed, Jun 2, 2010 at 05:29, Sonic Zhang wrote:
>>> On Wed, Jun 2, 2010 at 5:02 PM, Mike Frysinger wrote:
>>>> On Wed, Jun 2, 2010 at 04:51, sonic zhang wrote:
>>>>> +static const struct ad5398_current_data_format ad5398_df = {10, 4};
>>>>> +static const struct ad5398_current_data_format ad5821_df = {10, 4};
>>>>> +
>>>>> +static const struct i2c_device_id ad5398_id[] = {
>>>>> + { "ad5398", (kernel_ulong_t)&ad5398_df },
>>>>> + { "ad5821", (kernel_ulong_t)&ad5821_df },
>>>>> + { }
>>>>> +};
>>>>
>>>> do you really need sep storage for these _df vars ?
>>>
>>> Yes, this makes probe code simpler.
>>
>> how does it make any difference to the probe code what each id is
>> pointing to ? it isnt comparing the private data pointers to any
>> other storage pointers.
>>
>> from what i can see, this should give the same exact behavior:
>> static const struct ad5398_current_data_format df_10_4 = {10, 4};
>> static const struct i2c_device_id ad5398_id[] = {
>> { "ad5398", (kernel_ulong_t)&df_10_4 },
>> { "ad5821", (kernel_ulong_t)&df_10_4 },
>
> Yes, the behavior is the same for ad5398 and ad5821. But, if more
> chips are enabled in this driver, they may differ.
> This line is used as an example for future chips.
seems like a weak reason for otherwise useless overhead. especially
considering my simpler example should also be pretty obvious to extend
for future drivers should the need arise. you're a smart guy and dont
need things spelled out explicitly.
-mike
On Wed, Jun 02, 2010 at 04:51:34PM +0800, sonic zhang wrote:
> +static int ad5398_read_reg(struct i2c_client *client, unsigned short *data)
> +{
> + unsigned short val;
> + int ret;
> +
> + ret = i2c_master_recv(client, (char *)&val, 2);
> + if (ret < 0) {
> + dev_err(&client->dev, "I2C read error\n");
> + return ret;
> + }
> + *data = swab16(val);
Should this not be a be16_to_cpu() or similar rather than an
unconditional byte swap? Presumably the byte swap is not going to be
needed if the CPU has the same endianness as the CPU that the system is
using.
> + /* read chip enable bit */
> + ret = ad5398_read_reg(client, &data);
> + if (ret < 0)
> + return ret;
> + /* prepare register data */
> + selector = (selector << chip->current_offset) & chip->current_mask;
> + selector |= (data & AD5398_CURRENT_EN_MASK);
The reason I was querying this code on the original submission is that
it is more normal to write this as something like
data = selector | (data & ~chip->current_mask);
ie, to write the code as "set these bits" rather than "preserve these
bits". This is more clearly robust to the reader since it's clear that
there aren't other register bits which should be set.
> + chip->min_uA = init_data->constraints.min_uA;
> + chip->max_uA = init_data->constraints.max_uA;
This looks very wrong, especially given that you use the min_uA and
max_uA settings to scale the register values being written in to the
chip. I would expect that either the limits would be fixed in the
silicon or (if they depend on things like the associated passives which
can be configured per-board) that there would be some other setting in
the platform data which specifies what's actually being changed.
The constraints being specified by the platform should not influence the
physical properties of the chip, they control which values are allowed
in a particular design (for example, saying that values over a given
limit are not allowed due to the limits of the hardware connected to the
regulator) and are separate to what the chip is capable of.
On Wed, Jun 2, 2010 at 6:33 PM, Mark Brown
<[email protected]> wrote:
> On Wed, Jun 02, 2010 at 04:51:34PM +0800, sonic zhang wrote:
>
>> +static int ad5398_read_reg(struct i2c_client *client, unsigned short *data)
>> +{
>> + ? ? unsigned short val;
>> + ? ? int ret;
>> +
>> + ? ? ret = i2c_master_recv(client, (char *)&val, 2);
>> + ? ? if (ret < 0) {
>> + ? ? ? ? ? ? dev_err(&client->dev, "I2C read error\n");
>> + ? ? ? ? ? ? return ret;
>> + ? ? }
>> + ? ? *data = swab16(val);
>
> Should this not be a be16_to_cpu() or similar rather than an
> unconditional byte swap? ?Presumably the byte swap is not going to be
> needed if the CPU has the same endianness as the CPU that the system is
> using.
I made a mistake to mix simple i2c transfer and smbus i2c transfer
here. For smbus i2c transfer, byte swap is unconditional.
>
>> + ? ? /* read chip enable bit */
>> + ? ? ret = ad5398_read_reg(client, &data);
>> + ? ? if (ret < 0)
>> + ? ? ? ? ? ? return ret;
>
>> + ? ? /* prepare register data */
>> + ? ? selector = (selector << chip->current_offset) & chip->current_mask;
>> + ? ? selector |= (data & AD5398_CURRENT_EN_MASK);
>
> The reason I was querying this code on the original submission is that
> it is more normal to write this as something like
>
> ? ?data = selector | (data & ~chip->current_mask);
>
> ie, to write the code as "set these bits" rather than "preserve these
> bits". ?This is more clearly robust to the reader since it's clear that
> there aren't other register bits which should be set.
OK.
>
>> + ? ? ? chip->min_uA = init_data->constraints.min_uA;
>> + ? ? ? chip->max_uA = init_data->constraints.max_uA;
>
> This looks very wrong, especially given that you use the min_uA and
> max_uA settings to scale the register values being written in to the
> chip. ?I would expect that either the limits would be fixed in the
> silicon or (if they depend on things like the associated passives which
> can be configured per-board) that there would be some other setting in
> the platform data which specifies what's actually being changed.
>
> The constraints being specified by the platform should not influence the
> physical properties of the chip, they control which values are allowed
> in a particular design (for example, saying that values over a given
> limit are not allowed due to the limits of the hardware connected to the
> regulator) and are separate to what the chip is capable of.
>
I will predefine the chip physical min max values in the driver and add user
defined limitation based on both initial constraints and chip spec.
Sonic