* [PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage @ 2022-10-19 19:53 Deepak R Varma 2022-10-19 19:54 ` [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel Deepak R Varma 2022-10-19 19:56 ` [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name Deepak R Varma 0 siblings, 2 replies; 13+ messages in thread From: Deepak R Varma @ 2022-10-19 19:53 UTC (permalink / raw) To: outreachy, gregkh, linux-staging, linux-kernel Cc: kumarpraveen, saurabh.truth Patch set simplifies service_done_flag function by eliminating call to dim_get_channel_state. Also corrects the misleading dim_ch_state_t variable type name. Changes in v4: 1. Patch set versioning missed earlier. Now added. [feedback from gregkh@linuxfoundation.org] 2. Patch 1/2 : None. 3. Patch 2/2 : Correct patch subject and log. [feedback from julia.lawall@inria.fr] Change in v3: 1. Patch set introduced since another patch from same area added. Deepak R Varma (2): staging: most: dim2: read done_buffers count locally from HDM channel staging: most: dim2: correct misleading struct type name drivers/staging/most/dim2/dim2.c | 5 ++--- drivers/staging/most/dim2/hal.c | 4 ++-- drivers/staging/most/dim2/hal.h | 6 +++--- 3 files changed, 7 insertions(+), 8 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel 2022-10-19 19:53 [PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage Deepak R Varma @ 2022-10-19 19:54 ` Deepak R Varma 2022-10-20 15:03 ` Greg KH 2022-10-19 19:56 ` [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name Deepak R Varma 1 sibling, 1 reply; 13+ messages in thread From: Deepak R Varma @ 2022-10-19 19:54 UTC (permalink / raw) To: outreachy, gregkh, linux-staging, linux-kernel Cc: kumarpraveen, saurabh.truth 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 <drv@mailo.com> --- 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 v4: -- None. Changes in v3: 1. The patch log message is further improved. This revised verbiage is as thankfully provided by julia.lawall@inria.fr Changes in v2: 1. Update patch log message to be more descriptive about the reason for change. Feedback provided by julia.lawall@inria.fr drivers/staging/most/dim2/dim2.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c index ab72e11ac5ab..4c1f27898a29 100644 --- a/drivers/staging/most/dim2/dim2.c +++ b/drivers/staging/most/dim2/dim2.c @@ -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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel 2022-10-19 19:54 ` [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel Deepak R Varma @ 2022-10-20 15:03 ` Greg KH 2022-10-20 16:41 ` Deepak R Varma 0 siblings, 1 reply; 13+ messages in thread From: Greg KH @ 2022-10-20 15:03 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 01:24:53AM +0530, Deepak R Varma wrote: > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel 2022-10-20 15:03 ` Greg KH @ 2022-10-20 16:41 ` Deepak R Varma 0 siblings, 0 replies; 13+ messages in thread From: Deepak R Varma @ 2022-10-20 16:41 UTC (permalink / raw) To: Greg KH; +Cc: outreachy, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 05:03:14PM +0200, Greg KH wrote: > On Thu, Oct 20, 2022 at 01:24:53AM +0530, Deepak R Varma wrote: > > 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? Hello, pardon my limited understanding, but I think this function call is not necessary. > > 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 may be wrong, but I do not think there is a change in the long, but I may entirely wrong. The function was called from inside the lock scope, now we are extracting the value directly, still inside the lock scope. This should be safe. > > 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. Sure. That is fine. > > 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? I am eagerly waiting for a feedback from the maintainer. If they agree with my viewpoint, I will continue to work on this change outside of the clean up patch tasks. I will be happy to :) Thank you Greg! ./drv > > thanks, > > greg k-h > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-19 19:53 [PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage Deepak R Varma 2022-10-19 19:54 ` [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel Deepak R Varma @ 2022-10-19 19:56 ` Deepak R Varma 2022-10-19 20:08 ` Julia Lawall 1 sibling, 1 reply; 13+ messages in thread From: Deepak R Varma @ 2022-10-19 19:56 UTC (permalink / raw) To: outreachy, gregkh, linux-staging, linux-kernel Cc: kumarpraveen, saurabh.truth 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 <julia.lawall@inria.fr> Signed-off-by: Deepak R Varma <drv@mailo.com> --- Changes in v4: 1. Correct patch subject and log message. Use struct type name instead of variable name for the change description. Feedback from julia.lawall@inria.fr 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(-) diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c index 4c1f27898a29..a69a61a69283 100644 --- a/drivers/staging/most/dim2/dim2.c +++ b/drivers/staging/most/dim2/dim2.c @@ -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); diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c index 65282c276862..a5d40b5b138a 100644 --- a/drivers/staging/most/dim2/hal.c +++ b/drivers/staging/most/dim2/hal.c @@ -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; diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h index 20531449acab..ef10a8741c10 100644 --- a/drivers/staging/most/dim2/hal.h +++ b/drivers/staging/most/dim2/hal.h @@ -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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-19 19:56 ` [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name Deepak R Varma @ 2022-10-19 20:08 ` Julia Lawall 2022-10-19 20:30 ` Deepak R Varma 0 siblings, 1 reply; 13+ messages in thread From: Julia Lawall @ 2022-10-19 20:08 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, 20 Oct 2022, Deepak R Varma wrote: > 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 <julia.lawall@inria.fr> > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > > Changes in v4: > 1. Correct patch subject and log message. Use struct type name instead of > variable name for the change description. Feedback from julia.lawall@inria.fr > > 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(-) > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > index 4c1f27898a29..a69a61a69283 100644 > --- a/drivers/staging/most/dim2/dim2.c > +++ b/drivers/staging/most/dim2/dim2.c > @@ -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 > > BUG_ON(!hdm_ch); > BUG_ON(!hdm_ch->is_initialized); > diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c > index 65282c276862..a5d40b5b138a 100644 > --- a/drivers/staging/most/dim2/hal.c > +++ b/drivers/staging/most/dim2/hal.c > @@ -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; > diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h > index 20531449acab..ef10a8741c10 100644 > --- a/drivers/staging/most/dim2/hal.h > +++ b/drivers/staging/most/dim2/hal.h > @@ -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 > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-19 20:08 ` Julia Lawall @ 2022-10-19 20:30 ` Deepak R Varma 2022-10-20 11:48 ` Deepak R Varma 0 siblings, 1 reply; 13+ messages in thread From: Deepak R Varma @ 2022-10-19 20:30 UTC (permalink / raw) To: Julia Lawall Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > 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 <julia.lawall@inria.fr> > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > > > Changes in v4: > > 1. Correct patch subject and log message. Use struct type name instead of > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > 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(-) > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > index 4c1f27898a29..a69a61a69283 100644 > > --- a/drivers/staging/most/dim2/dim2.c > > +++ b/drivers/staging/most/dim2/dim2.c > > @@ -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 > > julia > > > > > BUG_ON(!hdm_ch); > > BUG_ON(!hdm_ch->is_initialized); > > diff --git a/drivers/staging/most/dim2/hal.c b/drivers/staging/most/dim2/hal.c > > index 65282c276862..a5d40b5b138a 100644 > > --- a/drivers/staging/most/dim2/hal.c > > +++ b/drivers/staging/most/dim2/hal.c > > @@ -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; > > diff --git a/drivers/staging/most/dim2/hal.h b/drivers/staging/most/dim2/hal.h > > index 20531449acab..ef10a8741c10 100644 > > --- a/drivers/staging/most/dim2/hal.h > > +++ b/drivers/staging/most/dim2/hal.h > > @@ -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 > > > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-19 20:30 ` Deepak R Varma @ 2022-10-20 11:48 ` Deepak R Varma 2022-10-20 12:06 ` Julia Lawall 0 siblings, 1 reply; 13+ messages in thread From: Deepak R Varma @ 2022-10-20 11:48 UTC (permalink / raw) To: Julia Lawall Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > 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 <julia.lawall@inria.fr> > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > > > > Changes in v4: > > > 1. Correct patch subject and log message. Use struct type name instead of > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > 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(-) > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > index 4c1f27898a29..a69a61a69283 100644 > > > --- a/drivers/staging/most/dim2/dim2.c > > > +++ b/drivers/staging/most/dim2/dim2.c > > > @@ -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? Hi Julia, Can you please review and comment on my response? > > ./drv > > > > > julia > > > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-20 11:48 ` Deepak R Varma @ 2022-10-20 12:06 ` Julia Lawall 2022-10-20 12:56 ` Deepak R Varma 0 siblings, 1 reply; 13+ messages in thread From: Julia Lawall @ 2022-10-20 12:06 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, 20 Oct 2022, Deepak R Varma wrote: > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > 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 <julia.lawall@inria.fr> > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > --- > > > > > > > > Changes in v4: > > > > 1. Correct patch subject and log message. Use struct type name instead of > > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > > > 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(-) > > > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > > index 4c1f27898a29..a69a61a69283 100644 > > > > --- a/drivers/staging/most/dim2/dim2.c > > > > +++ b/drivers/staging/most/dim2/dim2.c > > > > @@ -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? > > 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-20 12:06 ` Julia Lawall @ 2022-10-20 12:56 ` Deepak R Varma 2022-10-20 15:04 ` Greg KH 2022-10-20 15:10 ` Julia Lawall 0 siblings, 2 replies; 13+ messages in thread From: Deepak R Varma @ 2022-10-20 12:56 UTC (permalink / raw) To: Julia Lawall Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote: > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > > > 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 <julia.lawall@inria.fr> > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > --- > > > > > > > > > > Changes in v4: > > > > > 1. Correct patch subject and log message. Use struct type name instead of > > > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > > > > > 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(-) > > > > > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > > > index 4c1f27898a29..a69a61a69283 100644 > > > > > --- a/drivers/staging/most/dim2/dim2.c > > > > > +++ b/drivers/staging/most/dim2/dim2.c > > > > > @@ -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? > > > > 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 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? ./drv > > julia ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-20 12:56 ` Deepak R Varma @ 2022-10-20 15:04 ` Greg KH 2022-10-20 15:05 ` Greg KH 2022-10-20 15:10 ` Julia Lawall 1 sibling, 1 reply; 13+ messages in thread From: Greg KH @ 2022-10-20 15:04 UTC (permalink / raw) To: Deepak R Varma Cc: Julia Lawall, outreachy, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 06:26:12PM +0530, Deepak R Varma wrote: > On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote: > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > > > > > 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 <julia.lawall@inria.fr> > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > > --- > > > > > > > > > > > > Changes in v4: > > > > > > 1. Correct patch subject and log message. Use struct type name instead of > > > > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > > > > > > > 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(-) > > > > > > > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > > > > index 4c1f27898a29..a69a61a69283 100644 > > > > > > --- a/drivers/staging/most/dim2/dim2.c > > > > > > +++ b/drivers/staging/most/dim2/dim2.c > > > > > > @@ -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? > > > > > > 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 > > 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? 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] :( ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-20 15:04 ` Greg KH @ 2022-10-20 15:05 ` Greg KH 0 siblings, 0 replies; 13+ messages in thread From: Greg KH @ 2022-10-20 15:05 UTC (permalink / raw) To: Deepak R Varma Cc: Julia Lawall, outreachy, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, Oct 20, 2022 at 05:04:52PM +0200, Greg KH wrote: > On Thu, Oct 20, 2022 at 06:26:12PM +0530, Deepak R Varma wrote: > > On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote: > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > > > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > > > > > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > > > > > > > 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 <julia.lawall@inria.fr> > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > > > --- > > > > > > > > > > > > > > Changes in v4: > > > > > > > 1. Correct patch subject and log message. Use struct type name instead of > > > > > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > > > > > > > > > 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(-) > > > > > > > > > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > > > > > index 4c1f27898a29..a69a61a69283 100644 > > > > > > > --- a/drivers/staging/most/dim2/dim2.c > > > > > > > +++ b/drivers/staging/most/dim2/dim2.c > > > > > > > @@ -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? > > > > > > > > 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 > > > > 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? > > 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] > > :( > Ah, that was because I rejected patch 1/2 here. So this one will not work as-is, my fault. Please fix up and resend only this one if you still want to see it applied. thanks, greg k-h ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name 2022-10-20 12:56 ` Deepak R Varma 2022-10-20 15:04 ` Greg KH @ 2022-10-20 15:10 ` Julia Lawall 1 sibling, 0 replies; 13+ messages in thread From: Julia Lawall @ 2022-10-20 15:10 UTC (permalink / raw) To: Deepak R Varma Cc: outreachy, gregkh, linux-staging, linux-kernel, kumarpraveen, saurabh.truth On Thu, 20 Oct 2022, Deepak R Varma wrote: > On Thu, Oct 20, 2022 at 02:06:41PM +0200, Julia Lawall wrote: > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > On Thu, Oct 20, 2022 at 02:00:29AM +0530, Deepak R Varma wrote: > > > > On Wed, Oct 19, 2022 at 10:08:53PM +0200, Julia Lawall wrote: > > > > > > > > > > > > > > > On Thu, 20 Oct 2022, Deepak R Varma wrote: > > > > > > > > > > > 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 <julia.lawall@inria.fr> > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > > --- > > > > > > > > > > > > Changes in v4: > > > > > > 1. Correct patch subject and log message. Use struct type name instead of > > > > > > variable name for the change description. Feedback from julia.lawall@inria.fr > > > > > > > > > > > > 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(-) > > > > > > > > > > > > diff --git a/drivers/staging/most/dim2/dim2.c b/drivers/staging/most/dim2/dim2.c > > > > > > index 4c1f27898a29..a69a61a69283 100644 > > > > > > --- a/drivers/staging/most/dim2/dim2.c > > > > > > +++ b/drivers/staging/most/dim2/dim2.c > > > > > > @@ -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? > > > > > > 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 > > 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? You also have two occurrences in dim2.c. julia ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-10-20 16:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-10-19 19:53 [PATCH v4 0/2] staging: most: dim2: remove unnecessary function call and variable usage Deepak R Varma 2022-10-19 19:54 ` [PATCH v4 1/2] staging: most: dim2: read done_buffers count locally from HDM channel Deepak R Varma 2022-10-20 15:03 ` Greg KH 2022-10-20 16:41 ` Deepak R Varma 2022-10-19 19:56 ` [PATCH v4 2/2] staging: most: dim2: correct misleading struct type name Deepak R Varma 2022-10-19 20:08 ` Julia Lawall 2022-10-19 20:30 ` Deepak R Varma 2022-10-20 11:48 ` Deepak R Varma 2022-10-20 12:06 ` Julia Lawall 2022-10-20 12:56 ` Deepak R Varma 2022-10-20 15:04 ` Greg KH 2022-10-20 15:05 ` Greg KH 2022-10-20 15:10 ` Julia Lawall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).