Re: [PATCH 2/5] Staging: bcm: nvm.c: Outsourced chunk of code into function
On Sun, Jul 20, 2014 at 03:14:10PM +0200, Matthias Beyer wrote: This patch outsources a chunk of code into an own function. It also refactors the variable names which are used within this function. The function name may be not appropriate. Signed-off-by: Matthias Beyer m...@beyermatthias.de --- drivers/staging/bcm/nvm.c | 70 --- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c index 76c86eb..4aa195c 100644 --- a/drivers/staging/bcm/nvm.c +++ b/drivers/staging/bcm/nvm.c @@ -1033,6 +1033,44 @@ static ULONG BcmFlashUnProtectBlock(struct bcm_mini_adapter *Adapter, unsigned i return ulStatus; } +static int bulk_read_complete_sector(struct bcm_mini_adapter *ad, + UCHAR read_bk[], + PCHAR tmpbuff, + unsigned int offset, + unsigned int partoff, + unsigned int i) i should just be a local variable here. Could you send a follow on patch to clean that up? Also this code has that disease that every variable is unsigned int. It can't go higher than MAX_SECTOR_SIZE. It should just be int and the same for j because that is a number between 0-15. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/5] Staging: bcm: nvm.c: Outsourced chunk of code into function
On 23-07-2014 13:48:11, Dan Carpenter wrote: On Sun, Jul 20, 2014 at 03:14:10PM +0200, Matthias Beyer wrote: This patch outsources a chunk of code into an own function. It also refactors the variable names which are used within this function. The function name may be not appropriate. Signed-off-by: Matthias Beyer m...@beyermatthias.de --- drivers/staging/bcm/nvm.c | 70 --- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c index 76c86eb..4aa195c 100644 --- a/drivers/staging/bcm/nvm.c +++ b/drivers/staging/bcm/nvm.c @@ -1033,6 +1033,44 @@ static ULONG BcmFlashUnProtectBlock(struct bcm_mini_adapter *Adapter, unsigned i return ulStatus; } +static int bulk_read_complete_sector(struct bcm_mini_adapter *ad, +UCHAR read_bk[], +PCHAR tmpbuff, +unsigned int offset, +unsigned int partoff, +unsigned int i) i should just be a local variable here. Could you send a follow on patch to clean that up? Also this code has that disease that every variable is unsigned int. It can't go higher than MAX_SECTOR_SIZE. It should just be int and the same for j because that is a number between 0-15. regards, dan carpenter sure I can, but the variable which is named 'i' in the argument list is passed from the calling function where it is an unsigned int! Should I fix this up, too? I agree with you that the name is inappropriate, though! -- Mit freundlichen Grüßen, Kind regards, Matthias Beyer Proudly sent with mutt. Happily signed with gnupg. pgpn_0AgGatOs.pgp Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/5] Staging: bcm: nvm.c: Outsourced chunk of code into function
On Wed, Jul 23, 2014 at 12:52:37PM +0200, Matthias Beyer wrote: On 23-07-2014 13:48:11, Dan Carpenter wrote: On Sun, Jul 20, 2014 at 03:14:10PM +0200, Matthias Beyer wrote: This patch outsources a chunk of code into an own function. It also refactors the variable names which are used within this function. The function name may be not appropriate. Signed-off-by: Matthias Beyer m...@beyermatthias.de --- drivers/staging/bcm/nvm.c | 70 --- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c index 76c86eb..4aa195c 100644 --- a/drivers/staging/bcm/nvm.c +++ b/drivers/staging/bcm/nvm.c @@ -1033,6 +1033,44 @@ static ULONG BcmFlashUnProtectBlock(struct bcm_mini_adapter *Adapter, unsigned i return ulStatus; } +static int bulk_read_complete_sector(struct bcm_mini_adapter *ad, + UCHAR read_bk[], + PCHAR tmpbuff, + unsigned int offset, + unsigned int partoff, + unsigned int i) i should just be a local variable here. Could you send a follow on patch to clean that up? Also this code has that disease that every variable is unsigned int. It can't go higher than MAX_SECTOR_SIZE. It should just be int and the same for j because that is a number between 0-15. regards, dan carpenter sure I can, but the variable which is named 'i' in the argument list is passed from the calling function where it is an unsigned int! We shouldn't be passing it. Should I fix this up, too? There is so much to fix up in this driver. :P I wouldn't worry about little things too much at this point. regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/5] Staging: bcm: nvm.c: Outsourced chunk of code into function
This patch outsources a chunk of code into an own function. It also
refactors the variable names which are used within this function.
The function name may be not appropriate.
Signed-off-by: Matthias Beyer m...@beyermatthias.de
---
drivers/staging/bcm/nvm.c | 70 ---
1 file changed, 48 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
index 76c86eb..4aa195c 100644
--- a/drivers/staging/bcm/nvm.c
+++ b/drivers/staging/bcm/nvm.c
@@ -1033,6 +1033,44 @@ static ULONG BcmFlashUnProtectBlock(struct
bcm_mini_adapter *Adapter, unsigned i
return ulStatus;
}
+static int bulk_read_complete_sector(struct bcm_mini_adapter *ad,
+UCHAR read_bk[],
+PCHAR tmpbuff,
+unsigned int offset,
+unsigned int partoff,
+unsigned int i)
+{
+ unsigned int j;
+ int bulk_read_stat;
+
+ for (i = 0; i ad-uiSectorSize; i += MAX_RW_SIZE) {
+ bulk_read_stat = BeceemFlashBulkRead(ad,
+(PUINT)read_bk,
+offset + i,
+MAX_RW_SIZE);
+
+ if (bulk_read_stat == STATUS_SUCCESS) {
+ if (ad-ulFlashWriteSize == 1) {
+ for (j = 0; j 16; j++) {
+ if (read_bk[j] != tmpbuff[i+j]) {
+ if (STATUS_SUCCESS !=
(*ad-fpFlashWriteWithStatusCheck)(ad, partoff + i + j, tmpbuff[i+j])) {
+ return STATUS_FAILURE;
+ }
+ }
+ }
+ } else {
+ if (memcmp(read_bk, tmpbuff[i], MAX_RW_SIZE)) {
+ if (STATUS_SUCCESS !=
(*ad-fpFlashWriteWithStatusCheck)(ad, partoff + i, tmpbuff[i])) {
+ return STATUS_FAILURE;
+ }
+ }
+ }
+ }
+ }
+
+ return STATUS_SUCCESS;
+}
+
/*
* Procedure: BeceemFlashBulkWrite
*
@@ -1169,29 +1207,17 @@ static int BeceemFlashBulkWrite(struct bcm_mini_adapter
*Adapter,
/* do_gettimeofday(tw);
* BCM_DEBUG_PRINT(Adapter,DBG_TYPE_PRINTK, 0, 0, Total time
taken in Write to Flash :%ld ms\n, (tw.tv_sec *1000 + tw.tv_usec/1000) -
(te.tv_sec *1000 + te.tv_usec/1000));
*/
- for (uiIndex = 0; uiIndex Adapter-uiSectorSize; uiIndex +=
MAX_RW_SIZE) {
- if (STATUS_SUCCESS == BeceemFlashBulkRead(Adapter,
(PUINT)ucReadBk, uiOffsetFromSectStart + uiIndex, MAX_RW_SIZE)) {
- if (Adapter-ulFlashWriteSize == 1) {
- unsigned int uiReadIndex = 0;
-
- for (uiReadIndex = 0; uiReadIndex 16;
uiReadIndex++) {
- if (ucReadBk[uiReadIndex] !=
pTempBuff[uiIndex + uiReadIndex]) {
- if (STATUS_SUCCESS !=
(*Adapter-fpFlashWriteWithStatusCheck)(Adapter, uiPartOffset + uiIndex +
uiReadIndex, pTempBuff[uiIndex+uiReadIndex])) {
- Status =
STATUS_FAILURE;
- goto
BeceemFlashBulkWrite_EXIT;
- }
- }
- }
- } else {
- if (memcmp(ucReadBk,
pTempBuff[uiIndex], MAX_RW_SIZE)) {
- if (STATUS_SUCCESS !=
(*Adapter-fpFlashWriteWithStatusCheck)(Adapter, uiPartOffset + uiIndex,
pTempBuff[uiIndex])) {
- Status = STATUS_FAILURE;
- goto
BeceemFlashBulkWrite_EXIT;
- }
- }
- }
- }
+
+ if (STATUS_FAILURE == bulk_read_complete_sector(Adapter,
+ ucReadBk,
+ pTempBuff,
+
uiOffsetFromSectStart,
+ uiPartOffset,
+
