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] [day] [month] [year] [list]

Date:	Thu, 3 Jun 2010 10:57:02 +0800
From:	Sonic Zhang <sonic.adi@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.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 2, 2010 at 6:33 PM, Mark Brown
<broonie@...nsource.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/