lkml.org 
[lkml]   [2010]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2] regulator: new drivers for AD5398 and AD5821
From
On Wed, Jun 2, 2010 at 6:33 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> 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.

I made a mistake to mix simple i2c transfer and smbus i2c transfer
here. For smbus i2c transfer, byte swap is unconditional.

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

OK.

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

I will predefine the chip physical min max values in the driver and add user
defined limitation based on both initial constraints and chip spec.


Sonic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-06-03 04:59    [W:0.104 / U:0.920 seconds]
©2003-2014 Jasper Spaans. Advertise on this site