linux-kernel - Re: [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]

Message-ID: <20100602103329.GA2457@opensource.wolfsonmicro.com>
Date:	Wed, 2 Jun 2010 11:33:29 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	sonic zhang <sonic.adi@...il.com>
Cc:	Liam Girdwood <lrg@...mlogic.co.uk>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	uclinux-dist-devel <uclinux-dist-devel@...ckfin.uclinux.org>
Subject: Re: [PATCH v2] regulator: new drivers for AD5398 and AD5821

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