linux-kernel - Re: [Patch V2 6/9] I2C: Add smbus word/block process call helper function

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]

Message-ID: <20140517102110.GB2602@katana>
Date:	Sat, 17 May 2014 12:21:10 +0200
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Lan Tianyu <tianyu.lan@...el.com>
Cc:	rjw@...ysocki.net, mika.westerberg@...ux.intel.com,
	awilliam@...hat.com, lenb@...nel.org, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org
Subject: Re: [Patch V2 6/9] I2C: Add smbus word/block process call helper
 function

On Mon, Apr 28, 2014 at 10:27:45PM +0800, Lan Tianyu wrote:
> Add i2c_smbus_word/block_proc_call() helper function. These will be used
> in the implementation of i2c ACPI address space handler.
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> Signed-off-by: Lan Tianyu <tianyu.lan@...el.com>
> ---
>  drivers/i2c/i2c-core.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h    |  4 ++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 3bf0048..638befd 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -2306,6 +2306,30 @@ s32 i2c_smbus_write_word_data(const struct i2c_client *client, u8 command,
>  EXPORT_SYMBOL(i2c_smbus_write_word_data);
>  
>  /**
> + * i2c_smbus_word_proc_call - SMBus "word proc call" protocol
> + * @client: Handle to slave device
> + * @command: Byte interpreted by slave
> + * @value: 16-bit "word" being written
> + *
> + * This executes the SMBus "word proc all" protocol, returning negative errno
> + * else a 16-bit unsigned "word" received from the device.
> + */
> +s32 i2c_smbus_word_proc_call(const struct i2c_client *client, u8 command,
> +			      u16 value)
> +{
> +	union i2c_smbus_data data;
> +	int status;
> +
> +	data.word = value;
> +	status = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +			      I2C_SMBUS_READ, command,
> +			      I2C_SMBUS_PROC_CALL, &data);
> +
> +	return (status < 0) ? status : data.word;
> +}
> +EXPORT_SYMBOL(i2c_smbus_word_proc_call);
> +
> +/**

Same as smbus_quick function before, documentation updates are missing.
Also similar, we had such a function before which was removed due to no
users? It looks very much like the function here, so we should stick to
the old naming IMO. Is it the old one? Such information would be nice
since it affects reviewing.

>   * i2c_smbus_read_block_data - SMBus "block read" protocol
>   * @client: Handle to slave device
>   * @command: Byte interpreted by slave
> @@ -2362,6 +2386,38 @@ s32 i2c_smbus_write_block_data(const struct i2c_client *client, u8 command,
>  }
>  EXPORT_SYMBOL(i2c_smbus_write_block_data);
>  
> +/**
> + * i2c_smbus_block_proc_call - SMBus "block write" protocol
> + * @client: Handle to slave device
> + * @command: Byte interpreted by slave
> + * @length: Size of data block; SMBus allows at most 32 bytes
> + * @values: Byte array which will be written.
> + *
> + * This executes the SMBus "block proc call" protocol, returning negative errno
> + * else the number of read bytes.
> + */
> +s32 i2c_smbus_block_proc_call(const struct i2c_client *client, u8 command,
> +			       u8 length, u8 *values)

Please rename to "i2c_smbus_block_process_call" as the old function was
named. Needs documentation updates.

> +	if (length > I2C_SMBUS_BLOCK_MAX)
> +		length = I2C_SMBUS_BLOCK_MAX;

I think we should return an error here.

BTW what were your test scenarios for these functions and smbus quick?


Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)