staging: most: dim2: remove unnecessary function call and variable usage

PATCH In review Deepak R Varma 3 participants 8 messages Oct 18, 2022

Subsystems (2)

Staging Most(r) technology driver

lore

Share & tools

lore

Deepak R Varma

author

2022-10-18 16:49 UTC #1 lore raw

PATCH 1/2 staging: most: dim2: read done_buffers count locally from HDM channel

The function dim_get_channel_state only serves to initialize the ready and done_buffers fields of the structure passed as its second argument. In service_done_flag, this structure is never used again and the only purpose of the call is to get the value that is put in the done_buffers field. But that value is just the done_sw_buffers_number field of the call's first argument. So the whole call is useless, and we can just replace it with an access to this field.

This change implies that the variable st is no longer used, so drop it as well.

Signed-off-by Deepak R Varma <[email protected]>

---

PLEASE NOTE:
1. I have only built the module on my machine, but have not tested it. I am not sure how to test this change. I am willing to test it with appropriate guidance provided I have the necessary hardware.
2. This was a standalone patch earlier. It is now combined into a patch set with another patch for the same driver. Hence I am carry forwarding the change log for this patch here:

Changes in v3:
1. The patch log message is further improved. This revised verbiage is as thankfully provided by [email protected]

Changes in v2:
1. Update patch log message to be more descriptive about the reason for change. Feedback provided by [email protected]

 drivers/staging/most/dim2/dim2.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/most/dim2/dim2.c +1 -2

index ab72e11ac5ab..4c1f27898a29 100644
@@ -259,7 +259,6 @@ static void retrieve_netinfo(struct dim2_hdm *dev, struct mbo *mbo)
 static void service_done_flag(struct dim2_hdm *dev, int ch_idx)
 {
 	struct hdm_channel *hdm_ch = dev->hch + ch_idx;
-	struct dim_ch_state_t st;
 	struct list_head *head;
 	struct mbo *mbo;
 	int done_buffers;

@@ -271,7 +270,7 @@ static void service_done_flag(struct dim2_hdm *dev, int ch_idx)
 
 	spin_lock_irqsave(&dim_lock, flags);
 
-	done_buffers = dim_get_channel_state(&hdm_ch->ch, &st)->done_buffers;
+	done_buffers = hdm_ch->ch.done_sw_buffers_number;
 	if (!done_buffers) {
 		spin_unlock_irqrestore(&dim_lock, flags);
 		return;

--
2.30.2

Greg KH

maintainer

2022-10-18 17:12 UTC #2 lore raw

Re: [PATCH 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

Quoted 22 lines · Deepak R Varma

The function dim_get_channel_state only serves to initialize the ready and done_buffers fields of the structure passed as its second argument. In service_done_flag, this structure is never used again and the only purpose of the call is to get the value that is put in the done_buffers field. But that value is just the done_sw_buffers_number field of the call's first argument. So the whole call is useless, and we can just replace it with an access to this field.
This change implies that the variable st is no longer used, so drop it as well.
Signed-off-by Deepak R Varma <[email protected]>
---
PLEASE NOTE:
1. I have only built the module on my machine, but have not tested it. I am not sure how to test this change. I am willing to test it with appropriate guidance provided I have the necessary hardware.
2. This was a standalone patch earlier. It is now combined into a patch set with another patch for the same driver. Hence I am carry forwarding the change log for this patch here:

Quoted 8 lines

Changes in v3:
1. The patch log message is further improved. This revised verbiage is as thankfully provided by [email protected]
Changes in v2:
1. Update patch log message to be more descriptive about the reason for change. Feedback provided by [email protected]

You need to say this is a v3 in the subject line as documented :(

Deepak R Varma

author

2022-10-18 17:40 UTC #3 lore raw

Re: [PATCH 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

Quoted 27 lines · Greg KH

Quoted 26 lines · Deepak R Varma
The function dim_get_channel_state only serves to initialize the ready and done_buffers fields of the structure passed as its second argument. In service_done_flag, this structure is never used again and the only purpose of the call is to get the value that is put in the done_buffers field. But that value is just the done_sw_buffers_number field of the call's first argument. So the whole call is useless, and we can just replace it with an access to this field.
This change implies that the variable st is no longer used, so drop it as well.
Signed-off-by Deepak R Varma <[email protected]>
---
PLEASE NOTE:
1. I have only built the module on my machine, but have not tested it. I am not sure how to test this change. I am willing to test it with appropriate guidance provided I have the necessary hardware.
2. This was a standalone patch earlier. It is now combined into a patch set with another patch for the same driver. Hence I am carry forwarding the change log for this patch here:
Changes in v3:
1. The patch log message is further improved. This revised verbiage is as thankfully provided by [email protected]

Quoted 7 lines

Quoted 4 lines
Changes in v2:
1. Update patch log message to be more descriptive about the reason for change. Feedback provided by [email protected]
You need to say this is a v3 in the subject line as documented :(

Hi Greg,
This was a standalone patch earlier and first time included in the patch set. I was not aware I am allowed to version a patch individually, different from the cover letter. I have posted a query to Outreachy mailing list for additional clarification. I will send in a v4 once I have clarity.

Thank you,
./drv

Greg KH

maintainer

2022-10-18 18:07 UTC #4 lore raw

Re: [PATCH 1/2] staging: most: dim2: read done_buffers count locally from HDM channel

Quoted 33 lines · Deepak R Varma

Quoted 32 lines · Greg KH
Quoted 30 lines · Deepak R Varma
The function dim_get_channel_state only serves to initialize the ready and done_buffers fields of the structure passed as its second argument. In service_done_flag, this structure is never used again and the only purpose of the call is to get the value that is put in the done_buffers field. But that value is just the done_sw_buffers_number field of the call's first argument. So the whole call is useless, and we can just replace it with an access to this field.
This change implies that the variable st is no longer used, so drop it as well.
Signed-off-by Deepak R Varma <[email protected]>
---
PLEASE NOTE:
1. I have only built the module on my machine, but have not tested it. I am not sure how to test this change. I am willing to test it with appropriate guidance provided I have the necessary hardware.
2. This was a standalone patch earlier. It is now combined into a patch set with another patch for the same driver. Hence I am carry forwarding the change log for this patch here:
Changes in v3:
1. The patch log message is further improved. This revised verbiage is as thankfully provided by [email protected]
Changes in v2:
1. Update patch log message to be more descriptive about the reason for change. Feedback provided by [email protected]

Quoted 7 lines

Quoted 2 lines
You need to say this is a v3 in the subject line as documented :(
Hi Greg,
This was a standalone patch earlier and first time included in the patch set. I was not aware I am allowed to version a patch individually, different from the cover letter. I have posted a query to Outreachy mailing list for additional clarification. I will send in a v4 once I have clarity.

The cover letter will have v4 and so will each individual patch. Turning 1 patch into a series or adding new patches to a series does not mean you start over with the numbering. Otherwise that would be impossible for us to track, right?

thanks,

greg k-h

Deepak R Varma

author

2022-10-18 16:50 UTC #5 lore raw

PATCH 2/2 staging: most: dim2: correct misleading variable name

In reply to Deepak R Varma

Correct misleading struct variable name dim_ch_state_t to dim_ch_state since this not a typedef but a normal variable declaration.

Signed-off-by Deepak R Varma <[email protected]>

---
 drivers/staging/most/dim2/dim2.c | 2 +-
 drivers/staging/most/dim2/hal.c  | 4 ++--
 drivers/staging/most/dim2/hal.h  | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/most/dim2/dim2.c +1 -1

index 4c1f27898a29..a69a61a69283 100644
@@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
 	struct list_head *head = &hdm_ch->pending_list;
 	struct mbo *mbo;
 	unsigned long flags;
-	struct dim_ch_state_t st;
+	struct dim_ch_state st;
 
 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
 	return channel_service(ch);
 }
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr)
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr)
 {
 	if (!ch || !state_ptr)
 		return NULL;

drivers/staging/most/dim2/hal.h +3 -3

index 20531449acab..ef10a8741c10 100644
@@ -27,7 +27,7 @@ enum mlb_clk_speed {
 	CLK_8192FS = 7,
 };
 
-struct dim_ch_state_t {
+struct dim_ch_state {
 	bool ready; /* Shows readiness to enqueue next buffer */
 	u16 done_buffers; /* Number of completed buffers */
 };

@@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);
 
 u8 dim_service_channel(struct dim_channel *ch);
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr);
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr);
 
 u16 dim_dbr_space(struct dim_channel *ch);

--
2.30.2

Julia Lawall

2022-10-18 17:00 UTC #6 lore raw

Re: [PATCH 2/2] staging: most: dim2: correct misleading variable name

Quoted 2 lines · Deepak R Varma

Correct misleading struct variable name dim_ch_state_t to dim_ch_state since this not a typedef but a normal variable declaration.

Actually, it's not the name of a variable, but the name of a type.

julia

Quoted 68 lines

Signed-off-by Deepak R Varma <[email protected]>

---
 drivers/staging/most/dim2/dim2.c | 2 +-
 drivers/staging/most/dim2/hal.c  | 4 ++--
 drivers/staging/most/dim2/hal.h  | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/most/dim2/dim2.c +1 -1

index 4c1f27898a29..a69a61a69283 100644
@@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
 	struct list_head *head = &hdm_ch->pending_list;
 	struct mbo *mbo;
 	unsigned long flags;
-	struct dim_ch_state_t st;
+	struct dim_ch_state st;
 
 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
 	return channel_service(ch);
 }
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr)
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr)
 {
 	if (!ch || !state_ptr)
 		return NULL;

drivers/staging/most/dim2/hal.h +3 -4

index 20531449acab..ef10a8741c10 100644
@@ -27,7 +27,7 @@ enum mlb_clk_speed {
 	CLK_8192FS = 7,
 };
 
-struct dim_ch_state_t {
+struct dim_ch_state {
 	bool ready; /* Shows readiness to enqueue next buffer */
 	u16 done_buffers; /* Number of completed buffers */
 };

@@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);
 
 u8 dim_service_channel(struct dim_channel *ch);
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr);
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr);
 
 u16 dim_dbr_space(struct dim_channel *ch);
 
--

2.30.2

Deepak R Varma

author

2022-10-18 17:41 UTC #7 lore raw

Re: [PATCH 2/2] staging: most: dim2: correct misleading variable name

Quoted 8 lines · Julia Lawall

Quoted 2 lines · Deepak R Varma
Correct misleading struct variable name dim_ch_state_t to dim_ch_state since this not a typedef but a normal variable declaration.
Actually, it's not the name of a variable, but the name of a type.

oops.. my bad. I will wait for any other feedback on this patch set and accordingly send in the revision.

Thank you,
./drv

Quoted 72 lines

julia

Quoted 68 lines

Signed-off-by Deepak R Varma <[email protected]>

---
 drivers/staging/most/dim2/dim2.c | 2 +-
 drivers/staging/most/dim2/hal.c  | 4 ++--
 drivers/staging/most/dim2/hal.h  | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

drivers/staging/most/dim2/dim2.c +1 -1

index 4c1f27898a29..a69a61a69283 100644
@@ -161,7 +161,7 @@ static int try_start_dim_transfer(struct hdm_channel *hdm_ch)
 	struct list_head *head = &hdm_ch->pending_list;
 	struct mbo *mbo;
 	unsigned long flags;
-	struct dim_ch_state_t st;
+	struct dim_ch_state st;
 
 	BUG_ON(!hdm_ch);
 	BUG_ON(!hdm_ch->is_initialized);

drivers/staging/most/dim2/hal.c +2 -2

index 65282c276862..a5d40b5b138a 100644
@@ -943,8 +943,8 @@ u8 dim_service_channel(struct dim_channel *ch)
 	return channel_service(ch);
 }
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr)
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr)
 {
 	if (!ch || !state_ptr)
 		return NULL;

drivers/staging/most/dim2/hal.h +3 -4

index 20531449acab..ef10a8741c10 100644
@@ -27,7 +27,7 @@ enum mlb_clk_speed {
 	CLK_8192FS = 7,
 };
 
-struct dim_ch_state_t {
+struct dim_ch_state {
 	bool ready; /* Shows readiness to enqueue next buffer */
 	u16 done_buffers; /* Number of completed buffers */
 };

@@ -87,8 +87,8 @@ void dim_service_ahb_int_irq(struct dim_channel *const *channels);
 
 u8 dim_service_channel(struct dim_channel *ch);
 
-struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
-					     struct dim_ch_state_t *state_ptr);
+struct dim_ch_state *dim_get_channel_state(struct dim_channel *ch,
+					   struct dim_ch_state *state_ptr);
 
 u16 dim_dbr_space(struct dim_channel *ch);
 
--

2.30.2