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

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.

Quoted 7 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.

Are you sure it is useless?

You have changed the logic here, you are now thinking that this value can never change, while before you were ensured of getting the "correct" value as it is under the lock when the function is called.

I can't take this type of change as a "cleanup" patch for outreachy unless you have the hardware as it is NOT a basic "checkpatch" style cleanup at all.

If you want to get this change accepted, please work with the maintainer of the code and get them to agree that the change is correct. And if it is, odds are more things also would need to be cleaned up at the same time, right?

thanks,

greg k-h

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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 +0 -0

index 4c1f27898a29..a69a61a69283 100644

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

 	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

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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 static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

julia

Quoted 49 lines

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

 	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

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

If you are asking something different, could you please elaborate?

./drv

Quoted 4 lines

julia

Quoted 1 line

Quoted 44 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

Quoted 1 line

If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

Quoted 45 lines · Deepak R Varma

Quoted 44 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

Quoted 4 lines

Quoted 1 line

If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

In my kernel there is an occurrence of the type name in service_done_flag. But I have the mainline, not Greg's staging tree, so there could be some differences.

When I do git grep dim_ch_state_t, I get two occurrences in drivers/staging/most/dim2/dim2.c

julia

Quoted 54 lines · Julia Lawall

Quoted 45 lines · Deepak R Varma

Quoted 44 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

Quoted 4 lines

Quoted 1 line

If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

Quoted 6 lines

In my kernel there is an occurrence of the type name in service_done_flag. But I have the mainline, not Greg's staging tree, so there could be some differences.

When I do git grep dim_ch_state_t, I get two occurrences in drivers/staging/most/dim2/dim2.c

Okay. Still unclear. Following snip is what I see in my local staging-testing branch.

<snip>

	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
	drv@debian:~/git/kernels/staging$
</snip>

Does that help?

Quoted 76 lines · Deepak R Varma

Quoted 54 lines · Julia Lawall

Quoted 45 lines · Deepak R Varma

Quoted 44 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

Quoted 4 lines

Quoted 1 line

If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

Quoted 6 lines

In my kernel there is an occurrence of the type name in service_done_flag. But I have the mainline, not Greg's staging tree, so there could be some differences.

When I do git grep dim_ch_state_t, I get two occurrences in drivers/staging/most/dim2/dim2.c

Okay. Still unclear. Following snip is what I see in my local staging-testing branch.

<snip>

	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
	drv@debian:~/git/kernels/staging$
</snip>

Quoted 1 line

Does that help?

Not at all, as you did not test with your change applied:

CC [M] drivers/gpu/drm/vmwgfx/vmwgfx_drv.o
drivers/staging/most/dim2/dim2.c: In function ‘service_done_flag’:
drivers/staging/most/dim2/dim2.c:262:31: error: storage size of ‘st’ isn’t known
262 | struct dim_ch_state_t st;
| ^~
drivers/staging/most/dim2/dim2.c:262:31: error: unused variable ‘st’ [-Werror=unused-variable]

:(

Quoted 54 lines · Julia Lawall

Quoted 45 lines · Deepak R Varma

Quoted 44 lines · Deepak R Varma

Quoted 23 lines · Julia Lawall

Quoted 19 lines · Deepak R Varma

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

Suggested-by Julia Lawall <[email protected]>

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

---

Changes in v4:
1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from [email protected]

Changes in v3:
1. Patch introduced in the patch set

 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(-)

Quoted 12 lines

drivers/staging/most/dim2/dim2.c static int try_start_dim_transfer(struct hdm_channel *hdm_ch) · line 164

Quoted 10 lines

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;

Is there another use in service_done_flag?

Hi,
I did not understand your question fully. This is from a different function try_start_dim_transfer where the variable st is used down the line in the execution. This time the channel state is retrieved by calling dim_get_channel_state function. The state is simply computed and set. Should I improve this as well?

Quoted 4 lines

Quoted 1 line

If you are asking something different, could you please elaborate?

Hi Julia,
Can you please review and comment on my response?

Quoted 6 lines

In my kernel there is an occurrence of the type name in service_done_flag. But I have the mainline, not Greg's staging tree, so there could be some differences.

When I do git grep dim_ch_state_t, I get two occurrences in drivers/staging/most/dim2/dim2.c

Okay. Still unclear. Following snip is what I see in my local staging-testing branch.

<snip>

	drv@debian:~/git/kernels/staging$ git grep dim_ch_state_t
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/dim2.c:       struct dim_ch_state_t st;
	drivers/staging/most/dim2/hal.c:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.c:                                             struct dim_ch_state_t *state_ptr)
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t {
	drivers/staging/most/dim2/hal.h:struct dim_ch_state_t *dim_get_channel_state(struct dim_channel *ch,
	drivers/staging/most/dim2/hal.h:                                             struct dim_ch_state_t *state_ptr);
	drv@debian:~/git/kernels/staging$
</snip>

Does that help?