Re: [Uclinux-dist-devel] [PATCH v2] regulator: new drivers for AD5398 and AD5821

[Mike Frysinger]

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 <sonic.zhang@xxxxxxxxxx>

the "From:" doesnt match your s-o-b tag ...

also, this dropped my s-o-b:
Signed-off-by: Mike Frysinger <vapier@xxxxxxxxxx>

> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/