Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-13 Thread SF Markus Elfring 
>> Add a jump target so that the setting of a specific error code is stored
>> only once at the end of this function.
>>
>> Signed-off-by: Markus Elfring 
>> ---
>> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btmrvl_sdio.c 
>> b/drivers/bluetooth/btmrvl_sdio.c
>> index 05c78fcc13ff..24ed62fe2aeb 100644
>> --- a/drivers/bluetooth/btmrvl_sdio.c
>> +++ b/drivers/bluetooth/btmrvl_sdio.c
>> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base0 = 0x%04X(%d)."
>>  " Terminating download",
>>  base0, base0);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  base1 = sdio_readb(card->func,
>>  card->reg->sq_read_base_addr_a1, );
>> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base1 = 0x%04X(%d)."
>>  " Terminating download",
>>  base1, base1);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>
>>  len = (((u16) base1) << 8) | base0;
>> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  if (count > MAX_WRITE_IOMEM_RETRY) {
>>  BT_ERR("FW download failure @%d, "
>>  "over max retry count", offset);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  BT_ERR("FW CRC error indicated by the helper: "
>>  "len = 0x%04X, txlen = %d", len, txlen);
>> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  kfree(tmpfwbuf);
>>  release_firmware(fw_firmware);
>>  return ret;
>> +
>> +e_io:
>> +ret = -EIO;
>> +goto done;
>> }
> 
> I am not applying this one. I see zero benefit in this change.

Would you care for a bit of object code reduction in this function 
implementation.


> It is not even saving a single line since it actually is more code.

Should I fiddle with any other source code layout?

Do you find an extra blank line inappropriate before the added jump target
in this use case?

Regards,
Markus

Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-13 Thread SF Markus Elfring 
>> Add a jump target so that the setting of a specific error code is stored
>> only once at the end of this function.
>>
>> Signed-off-by: Markus Elfring 
>> ---
>> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btmrvl_sdio.c 
>> b/drivers/bluetooth/btmrvl_sdio.c
>> index 05c78fcc13ff..24ed62fe2aeb 100644
>> --- a/drivers/bluetooth/btmrvl_sdio.c
>> +++ b/drivers/bluetooth/btmrvl_sdio.c
>> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base0 = 0x%04X(%d)."
>>  " Terminating download",
>>  base0, base0);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  base1 = sdio_readb(card->func,
>>  card->reg->sq_read_base_addr_a1, );
>> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  " base1 = 0x%04X(%d)."
>>  " Terminating download",
>>  base1, base1);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>
>>  len = (((u16) base1) << 8) | base0;
>> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  if (count > MAX_WRITE_IOMEM_RETRY) {
>>  BT_ERR("FW download failure @%d, "
>>  "over max retry count", offset);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>>  }
>>  BT_ERR("FW CRC error indicated by the helper: "
>>  "len = 0x%04X, txlen = %d", len, txlen);
>> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
>> btmrvl_sdio_card *card)
>>  kfree(tmpfwbuf);
>>  release_firmware(fw_firmware);
>>  return ret;
>> +
>> +e_io:
>> +ret = -EIO;
>> +goto done;
>> }
> 
> I am not applying this one. I see zero benefit in this change.

Would you care for a bit of object code reduction in this function 
implementation.


> It is not even saving a single line since it actually is more code.

Should I fiddle with any other source code layout?

Do you find an extra blank line inappropriate before the added jump target
in this use case?

Regards,
Markus

Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-12 Thread Marcel Holtmann 
Hi Markus,

> From: Markus Elfring 
> Date: Mon, 12 Mar 2018 11:30:28 +0100
> 
> Add a jump target so that the setting of a specific error code is stored
> only once at the end of this function.
> 
> Signed-off-by: Markus Elfring 
> ---
> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 05c78fcc13ff..24ed62fe2aeb 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   " base0 = 0x%04X(%d)."
>   " Terminating download",
>   base0, base0);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
>   base1 = sdio_readb(card->func,
>   card->reg->sq_read_base_addr_a1, );
> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   " base1 = 0x%04X(%d)."
>   " Terminating download",
>   base1, base1);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
> 
>   len = (((u16) base1) << 8) | base0;
> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   if (count > MAX_WRITE_IOMEM_RETRY) {
>   BT_ERR("FW download failure @%d, "
>   "over max retry count", offset);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
>   BT_ERR("FW CRC error indicated by the helper: "
>   "len = 0x%04X, txlen = %d", len, txlen);
> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   kfree(tmpfwbuf);
>   release_firmware(fw_firmware);
>   return ret;
> +
> +e_io:
> + ret = -EIO;
> + goto done;
> }

I am not applying this one. I see zero benefit in this change. It is not even 
saving a single line since it actually is more code.

Regards

Marcel

Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-12 Thread Marcel Holtmann 
Hi Markus,

> From: Markus Elfring 
> Date: Mon, 12 Mar 2018 11:30:28 +0100
> 
> Add a jump target so that the setting of a specific error code is stored
> only once at the end of this function.
> 
> Signed-off-by: Markus Elfring 
> ---
> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
> 1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 05c78fcc13ff..24ed62fe2aeb 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   " base0 = 0x%04X(%d)."
>   " Terminating download",
>   base0, base0);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
>   base1 = sdio_readb(card->func,
>   card->reg->sq_read_base_addr_a1, );
> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   " base1 = 0x%04X(%d)."
>   " Terminating download",
>   base1, base1);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
> 
>   len = (((u16) base1) << 8) | base0;
> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   if (count > MAX_WRITE_IOMEM_RETRY) {
>   BT_ERR("FW download failure @%d, "
>   "over max retry count", offset);
> - ret = -EIO;
> - goto done;
> + goto e_io;
>   }
>   BT_ERR("FW CRC error indicated by the helper: "
>   "len = 0x%04X, txlen = %d", len, txlen);
> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
> btmrvl_sdio_card *card)
>   kfree(tmpfwbuf);
>   release_firmware(fw_firmware);
>   return ret;
> +
> +e_io:
> + ret = -EIO;
> + goto done;
> }

I am not applying this one. I see zero benefit in this change. It is not even 
saving a single line since it actually is more code.

Regards

Marcel

[PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-12 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:30:28 +0100

Add a jump target so that the setting of a specific error code is stored
only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 05c78fcc13ff..24ed62fe2aeb 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base0 = 0x%04X(%d)."
" Terminating download",
base0, base0);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
base1 = sdio_readb(card->func,
card->reg->sq_read_base_addr_a1, );
@@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base1 = 0x%04X(%d)."
" Terminating download",
base1, base1);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
 
len = (((u16) base1) << 8) | base0;
@@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
if (count > MAX_WRITE_IOMEM_RETRY) {
BT_ERR("FW download failure @%d, "
"over max retry count", offset);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
BT_ERR("FW CRC error indicated by the helper: "
"len = 0x%04X, txlen = %d", len, txlen);
@@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
kfree(tmpfwbuf);
release_firmware(fw_firmware);
return ret;
+
+e_io:
+   ret = -EIO;
+   goto done;
 }
 
 static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv)
-- 
2.16.2

[PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()

2018-03-12 Thread SF Markus Elfring 
From: Markus Elfring 
Date: Mon, 12 Mar 2018 11:30:28 +0100

Add a jump target so that the setting of a specific error code is stored
only once at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/bluetooth/btmrvl_sdio.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 05c78fcc13ff..24ed62fe2aeb 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base0 = 0x%04X(%d)."
" Terminating download",
base0, base0);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
base1 = sdio_readb(card->func,
card->reg->sq_read_base_addr_a1, );
@@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
" base1 = 0x%04X(%d)."
" Terminating download",
base1, base1);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
 
len = (((u16) base1) << 8) | base0;
@@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
if (count > MAX_WRITE_IOMEM_RETRY) {
BT_ERR("FW download failure @%d, "
"over max retry count", offset);
-   ret = -EIO;
-   goto done;
+   goto e_io;
}
BT_ERR("FW CRC error indicated by the helper: "
"len = 0x%04X, txlen = %d", len, txlen);
@@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct 
btmrvl_sdio_card *card)
kfree(tmpfwbuf);
release_firmware(fw_firmware);
return ret;
+
+e_io:
+   ret = -EIO;
+   goto done;
 }
 
 static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv)
-- 
2.16.2