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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 2 Jun 2010 05:02:58 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	sonic zhang <sonic.adi@...il.com>
Cc:	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	uclinux-dist-devel <uclinux-dist-devel@...ckfin.uclinux.org>,
	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: Re: [Uclinux-dist-devel] [PATCH v2] regulator: new drivers for AD5398 
	and AD5821

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@...log.com>

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

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

> +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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/