linux-kernel - Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus

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]

Date:	Tue, 15 Apr 2014 09:36:13 +0200
From:	Boris BREZILLON <boris.brezillon@...e-electrons.com>
To:	Mark Brown <broonie@...nel.org>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Shuge <shuge@...winnertech.com>, kevin@...winnertech.com,
	Chen-Yu Tsai <wens@...e.org>,
	Hans de Goede <hdegoede@...hat.com>,
	Carlo Caione <carlo@...one.org>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	dev@...ux-sunxi.org
Subject: Re: [RFC PATCH v2] regmap: smbus: add support for regmap over SMBus

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


On 14/04/2014 23:04, Mark Brown wrote:
> On Mon, Apr 14, 2014 at 03:08:05PM +0200, Boris BREZILLON wrote:
>> SMBus is a subset of the I2C protocol, oftenly used to access
registers on
>> external devices.
> 
> This is basically fine.  However...
> 
>> +    switch (ctx->transfer_type) { +    case
>> REGMAP_SMBUS_BYTE_TRANSFER: +        while (count > 0 && !ret) { 
>> +            ret = i2c_smbus_write_byte_data(ctx->i2c, reg++, +
>> *(u8 *)data++); + +            count--; +        } +
>> break;
> 
> The transfer type gets set once per device at init time so why not
> just parameterise based on val_bytes?

Actually, you may want to transfer 1 byte registers using the block
method (if your device only support block transfers). This depends on
the device being accessed and what it supports, but I'm not sure we can
assume 1 byte registers will always be transferred using SMBUS byte
transfers.

> 
> 
>> +    case REGMAP_SMBUS_I2C_BLOCK_TRANSFER: +        while (count
>> > 0 && !ret) { +            ret =
>> i2c_smbus_write_i2c_block_data(ctx->i2c, +
>> reg, +                                 ctx->val_bytes, +
>> (const u8 *)data);
> 
> Fix the const correctness of the API rather than casting.

The API is correct because the i2c_smbus_write_i2c_block does not modify
the data pointer.
Just removing the const keyword in the cast should be enough, because
you can safely cast a non const pointer to a const one.

> 
> 
>> +            reg += ctx->val_bytes; +            count -=
>> ctx->val_bytes; +            data += ctx->val_bytes; +        }
> 
> I'm assuming this will only be used if val_bytes isn't 1 or 2?

As explained earlier, this depends on the device being accessed, but it
might be used for smaller transfers (1 or 2 bytes).

Thanks for the review.

Best Regards,

Boris

- -- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTTOFkAAoJEHLimErDqMKyhlcH/jeKnphnxL9Sc+OKsd1sbvZ4
dgjwm0HVy/oQO4dwGHbGXDrvoqdcuPKjKJv0An1RAW0u8CBQHCHbn2xQZtHkrNhK
C4g+8ws3R050ppDAAvFGgnx2bIoS8+Elbcd0GNCQW+xYU/xOySH9qgPitFF7PgGw
X9wgM+o4x6W2Lo/NOxmkXeZVWl3UrfUL24UTXfqysJK6ciYtW7GDP1uInfK2eH1G
RL7eD7LanEH95bk8eQtVfZZzcoAZCo1ri2z6iPf2WS4Nep84fyAr6Q+B1Ltmhsus
QXZgXWnVY45ZYFRNjMX8axCUUoJf7DSn9ayi3Gil1i7fLmCPKhpxc8HNxWvIMkk=
=FFGW
-----END PGP SIGNATURE-----
--
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/