Re: [PATCH v2] regulator: new drivers for AD5398 and AD5821

[Mark Brown]

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