diff mbox series

[7/8] i2c: i801: Add SMBUS_LEN_SENTINEL

Message ID 07acdfa5-0ab6-4885-a588-d169a31793c4@gmail.com
State New
Headers show
Series i2c: i801: collection of further improvements / refactorings | expand

Commit Message

Heiner Kallweit Sept. 22, 2023, 7:40 p.m. UTC
Add a sentinel length value that is used to check whether we should
read and use the length value provided by the slave device.
This simplifies the currently used checks.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Andi Shyti Jan. 30, 2024, 12:23 a.m. UTC | #1
On Fri, Sep 22, 2023 at 09:40:25PM +0200, Heiner Kallweit wrote:
> Add a sentinel length value that is used to check whether we should
> read and use the length value provided by the slave device.
> This simplifies the currently used checks.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a9d3dfd9e..30a2f9268 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -204,6 +204,8 @@
>  #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
>  				 STATUS_ERROR_FLAGS)
>  
> +#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
> +
>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
> @@ -541,8 +543,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>  {
>  	if (priv->is_read) {
>  		/* For SMBus block reads, length is received with first byte */
> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
> -		    (priv->count == 0)) {
> +		if (priv->len == SMBUS_LEN_SENTINEL) {
>  			priv->len = inb_p(SMBHSTDAT0(priv));
>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
>  				dev_err(&priv->pci_dev->dev,
> @@ -697,8 +698,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  		if (status)
>  			return status;
>  
> -		if (i == 1 && read_write == I2C_SMBUS_READ
> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
> +		if (len == SMBUS_LEN_SENTINEL) {
>  			len = inb_p(SMBHSTDAT0(priv));
>  			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
>  				dev_err(&priv->pci_dev->dev,
> @@ -805,7 +805,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
>  					u8 addr, u8 hstcmd, char read_write, int command)
>  {
>  	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
> -		data->block[0] = I2C_SMBUS_BLOCK_MAX;
> +		data->block[0] = SMBUS_LEN_SENTINEL;

This patch is good, but a few comments for each change to tell
where the sentinel will be used and where the sentinel was set
would help to better understand the use of the sentinel.

Thanks,
Andi

>  	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>  		return -EPROTO;
>  
> -- 
> 2.42.0
> 
>
Heiner Kallweit Jan. 30, 2024, 7:11 a.m. UTC | #2
On 30.01.2024 01:23, Andi Shyti wrote:
> On Fri, Sep 22, 2023 at 09:40:25PM +0200, Heiner Kallweit wrote:
>> Add a sentinel length value that is used to check whether we should
>> read and use the length value provided by the slave device.
>> This simplifies the currently used checks.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a9d3dfd9e..30a2f9268 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -204,6 +204,8 @@
>>  #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
>>  				 STATUS_ERROR_FLAGS)
>>  
>> +#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
>> +
>>  /* Older devices have their ID defined in <linux/pci_ids.h> */
>>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
>>  #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
>> @@ -541,8 +543,7 @@ static void i801_isr_byte_done(struct i801_priv *priv)
>>  {
>>  	if (priv->is_read) {
>>  		/* For SMBus block reads, length is received with first byte */
>> -		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
>> -		    (priv->count == 0)) {
>> +		if (priv->len == SMBUS_LEN_SENTINEL) {
>>  			priv->len = inb_p(SMBHSTDAT0(priv));
>>  			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
>>  				dev_err(&priv->pci_dev->dev,
>> @@ -697,8 +698,7 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>>  		if (status)
>>  			return status;
>>  
>> -		if (i == 1 && read_write == I2C_SMBUS_READ
>> -		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>> +		if (len == SMBUS_LEN_SENTINEL) {
>>  			len = inb_p(SMBHSTDAT0(priv));
>>  			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
>>  				dev_err(&priv->pci_dev->dev,
>> @@ -805,7 +805,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
>>  					u8 addr, u8 hstcmd, char read_write, int command)
>>  {
>>  	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
>> -		data->block[0] = I2C_SMBUS_BLOCK_MAX;
>> +		data->block[0] = SMBUS_LEN_SENTINEL;
> 
> This patch is good, but a few comments for each change to tell
> where the sentinel will be used and where the sentinel was set
> would help to better understand the use of the sentinel.
> 

OK

> Thanks,
> Andi
> 
>>  	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
>>  		return -EPROTO;
>>  
>> -- 
>> 2.42.0
>>
>>
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9d3dfd9e..30a2f9268 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -204,6 +204,8 @@ 
 #define STATUS_FLAGS		(SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
 				 STATUS_ERROR_FLAGS)
 
+#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1)
+
 /* Older devices have their ID defined in <linux/pci_ids.h> */
 #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS		0x02a3
 #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS		0x06a3
@@ -541,8 +543,7 @@  static void i801_isr_byte_done(struct i801_priv *priv)
 {
 	if (priv->is_read) {
 		/* For SMBus block reads, length is received with first byte */
-		if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) &&
-		    (priv->count == 0)) {
+		if (priv->len == SMBUS_LEN_SENTINEL) {
 			priv->len = inb_p(SMBHSTDAT0(priv));
 			if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
@@ -697,8 +698,7 @@  static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 		if (status)
 			return status;
 
-		if (i == 1 && read_write == I2C_SMBUS_READ
-		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
+		if (len == SMBUS_LEN_SENTINEL) {
 			len = inb_p(SMBHSTDAT0(priv));
 			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) {
 				dev_err(&priv->pci_dev->dev,
@@ -805,7 +805,7 @@  static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_
 					u8 addr, u8 hstcmd, char read_write, int command)
 {
 	if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA)
-		data->block[0] = I2C_SMBUS_BLOCK_MAX;
+		data->block[0] = SMBUS_LEN_SENTINEL;
 	else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX)
 		return -EPROTO;