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

[Sonic Zhang]

On Wed, Jun 2, 2010 at 5:02 PM, Mike Frysinger <vapier.adi@xxxxxxxxx> 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 <sonic.zhang@xxxxxxxxxx>
>
> 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 <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 ?
>

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
>
--
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/