| Message ID | 6b772a1ac06ae3b0d63e198e7238c1590b14703a.1666208065.git.drv@mailo.com |
|---|---|
| State | New |
| Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org>
Delivered-To: ouuuleilei@gmail.com
Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp516369wrs;
Wed, 19 Oct 2022 13:15:34 -0700 (PDT)
X-Google-Smtp-Source:
AMsMyM7L7jfppe2deJ5qlLGByUVDkDKkqBQiW8TsASGc1kgYUxBJBNGYV+eAq5ZHkUjvid5bUnqQ
X-Received: by 2002:a17:907:968d:b0:78e:1a4:131 with SMTP id
hd13-20020a170907968d00b0078e01a40131mr8098485ejc.439.1666210534158;
Wed, 19 Oct 2022 13:15:34 -0700 (PDT)
ARC-Seal: i=1; a=rsa-sha256; t=1666210534; cv=none;
d=google.com; s=arc-20160816;
b=B/6SW8j2l0/dlCI31GZNNTiOYLuFf74DlerWaBxQPYTipaSBtU8j5/xzLqSbPHdQUj
6WBeOmhkQu/9kQOZi9hvoBfFJwz9bVKqR212icQ49LGQP87SD35bXN9Ci+j58wONgWzc
d86y9UJeih814RQUWE/8T2RB1l6W4eo7whDD0Z2H5vMmNiDUSP0WeBfVl65vWdF1MpSQ
p1ztTIx2zyAos4VNWrQOeg+pdVtbrkdG8LswA7Rsy76U22G5WHudQDmaFijPvZMwsOwe
1SB8PvB7n6Nx273sWkfj2KSDkAdqNiYvpM+HWlBjjH4TylCRtp6Qoc6Uxb4yum9i/2In
BDOA==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;
s=arc-20160816;
h=list-id:precedence:in-reply-to:content-disposition:mime-version
:references:message-id:subject:cc:to:from:date:dkim-signature;
bh=HQMRW4/R8uzKfDdKyJuwPp1NGRbjkZsJ8ogF8q2SN98=;
b=bombZ3z2HwUKE6ubH490i55m8HHCKN0nSmekUFJzAy0JTl6mAwnyMkQCgSlvy1tMON
rmSYg6I4uyfUhDUpS+eIw47bDPbpTR+BWdDy5/4uTnEHC4rzINip7iyI4NE8CJOPfBtm
S+yQTyuGk4lKEC/jle5ZTuj1EfovxRp8CnImxxusLzZ/oHkv5KlDRrCSnH1bd+OI3gCo
9AoCz0ftNCk9YPaeYFE8FYh2H2iJ4bOAnCo+8X7mRiv8UpwB1ZjSao1va5f5l81QawLq
13k9pr/JV9Dn0YaHvja43uxqjx+ghCAOxhIeRtAXIN1ryTodnPBy7xtxHZyL2La9UHpE
lqDQ==
ARC-Authentication-Results: i=1; mx.google.com;
dkim=fail header.i=@mailo.com header.s=mailo header.b=b5pPwo8k;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com
Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20])
by mx.google.com with ESMTP id
y10-20020a056402270a00b0045dc9b4c034si6643896edd.582.2022.10.19.13.15.06;
Wed, 19 Oct 2022 13:15:34 -0700 (PDT)
Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
client-ip=2620:137:e000::1:20;
Authentication-Results: mx.google.com;
dkim=fail header.i=@mailo.com header.s=mailo header.b=b5pPwo8k;
spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org
designates 2620:137:e000::1:20 as permitted sender)
smtp.mailfrom=linux-kernel-owner@vger.kernel.org;
dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
id S230476AbiJST4s (ORCPT <rfc822;samuel.l.nystrom@gmail.com>
+ 99 others); Wed, 19 Oct 2022 15:56:48 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56058 "EHLO
lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
with ESMTP id S231287AbiJST4o (ORCPT
<rfc822;linux-kernel@vger.kernel.org>);
Wed, 19 Oct 2022 15:56:44 -0400
Received: from msg-1.mailo.com (msg-1.mailo.com [213.182.54.11])
by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 77F861C77D5
for <linux-kernel@vger.kernel.org>;
Wed, 19 Oct 2022 12:56:41 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo;
t=1666209386; bh=T3f8Wh/SL8v/3/j9kA5MFUnw8WYI2ZeJHwNe8d+A6nY=;
h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:References:
MIME-Version:Content-Type:In-Reply-To;
b=b5pPwo8kCguNHoG7iP4tFI1A0U7wSrBmBb77BV0RDrjFy16TIUzBBsVBXrZSO39xW
bsXKIZsg+HTeqoXhd+JMKll4Oy0rw5flB75eRIKkNlUMAwxKV847/Yv0tSx0YlKX1O
5U99LN5RvQchoUuUohCrywWvDbzd677IRx6fVlpA=
Received: by b-6.in.mailobj.net [192.168.90.16] with ESMTP
via [213.182.55.206]
Wed, 19 Oct 2022 21:56:26 +0200 (CEST)
X-EA-Auth:
4xMhWhHPuopDUeHFk/sD5rYY7mMTCudS9R7qM7M/KAyHJoX2lkMGoxsmSQGPWaMy191iulbBmV7zXkhLlWCH2/TiKY8LI8Ve
Date: Thu, 20 Oct 2022 01:26:21 +0530
From: Deepak R Varma <drv@mailo.com>
To: outreachy@lists.linux.dev, gregkh@linuxfoundation.org,
linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Cc: kumarpraveen@linux.microsoft.com, saurabh.truth@gmail.com
Subject: [PATCH v4 2/2] staging: most: dim2: correct misleading struct type
name
Message-ID:
<6b772a1ac06ae3b0d63e198e7238c1590b14703a.1666208065.git.drv@mailo.com>
References: <cover.1666208065.git.drv@mailo.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cover.1666208065.git.drv@mailo.com>
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS
autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?=
X-GMAIL-THRID: =?utf-8?q?1747148377282815988?=
X-GMAIL-MSGID: =?utf-8?q?1747148377282815988?=
|
| Series |
staging: most: dim2: remove unnecessary function call and variable usage
|
|
Commit Message
Deepak R Varma
Oct. 19, 2022, 7:56 p.m. UTC
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(-)
--
2.30.2
Comments
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 > > > > >
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 > > > > > > > > > >
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 > > > > > > > >
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
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
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] :(
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
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
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);