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.031 / U:2.672 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site