Message ID | 20170404123219.22040-1-lee.jones@linaro.org (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1cvNdc-0000xl-Vt; Tue, 04 Apr 2017 12:32:45 +0000 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.84_2/mailfrontend-6) with esmtp id 1cvNda-0006ne-4f; Tue, 04 Apr 2017 14:32:44 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754286AbdDDMcd (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 4 Apr 2017 08:32:33 -0400 Received: from mail-wr0-f180.google.com ([209.85.128.180]:35209 "EHLO mail-wr0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754262AbdDDMcb (ORCPT <rfc822; linux-media@vger.kernel.org>); Tue, 4 Apr 2017 08:32:31 -0400 Received: by mail-wr0-f180.google.com with SMTP id k6so208593623wre.2 for <linux-media@vger.kernel.org>; Tue, 04 Apr 2017 05:32:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:cc:subject:date:message-id; bh=9geFZIs2Nppnd+oxhYCTo9FNicTZWV2bBzjs36VWvKw=; b=UJ1cxFfewkjSmmszaF4xKNOxy4Nrxa96KPmw4TibG9mouJdhyjoaWswnyURTLmE69a tVev7waKrl2/5ndZow1Dd+PP/a/undfHmuVQPJXl4UonsY43eY56zI3WOAyU72c1mfAs QVwEJpYJxoMAiIxDraziVefwdXjBAHeb7/0Zw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=9geFZIs2Nppnd+oxhYCTo9FNicTZWV2bBzjs36VWvKw=; b=QIz/65GQJpEvo5humEhOXYUaW8+3nIaa8ys0iRiVpnmx6VQSDNfVgwo5nkGCmMKeDB vPXr6PMMWziOPT42OQ7qqasi/mWzzK4UY5Zl/VJN95cXOjD/fBZJYAJP8PEhY/tlVSMO zdBchjiHdswkqYyafUnq9L9mHndpwwksE6IN2prz7dEfAmASKUl+DubDlzfEGw2ZDGXf h3LR62W+czC3actqMqnLi2CHtAh8TDlJv1rkeRCsVaxWLOeuhBg4aaFKi/y+QdqNMNEV LuEvBFAMrBujVwIeZLYb4ejHzq0sA7xZfpAsZ/OUHZdYxPTekz+8uzbrudC0A5WYKOY7 +Dew== X-Gm-Message-State: AFeK/H0HcGy7PUoTUdtoIDNSWtbCjZvKicMHUmRr52dHeSnQdgwgKX2SMShtEl0/cqo91QjG X-Received: by 10.223.130.36 with SMTP id 33mr20523156wrb.150.1491309145286; Tue, 04 Apr 2017 05:32:25 -0700 (PDT) Received: from localhost.localdomain ([2.31.167.174]) by smtp.gmail.com with ESMTPSA id 75sm21479249wmp.2.2017.04.04.05.32.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 04 Apr 2017 05:32:24 -0700 (PDT) From: Lee Jones <lee.jones@linaro.org> To: hans.verkuil@cisco.com, mchehab@kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@stlinux.com, patrice.chotard@st.com, linux-media@vger.kernel.org, benjamin.gaignard@st.com, Lee Jones <lee.jones@linaro.org> Subject: [PATCH 1/2] [media] cec: Move capability check inside #if Date: Tue, 4 Apr 2017 13:32:18 +0100 Message-Id: <20170404123219.22040-1-lee.jones@linaro.org> X-Mailer: git-send-email 2.9.3 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2017.4.4.122715 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1000_LESS 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, BODY_SIZE_800_899 0, DKIM_SIGNATURE 0, LEGITIMATE_SIGNS 0, MULTIPLE_REAL_RCPTS 0, NO_URI_HTTPS 0, __ANY_URI 0, __CC_NAME 0, __CC_NAME_DIFF_FROM_ACC 0, __CC_REAL_NAMES 0, __FROM_DOMAIN_IN_ANY_CC2 0, __FROM_DOMAIN_IN_RCPT 0, __HAS_CC_HDR 0, __HAS_FROM 0, __HAS_LIST_ID 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MIME_TEXT_P 0, __MIME_TEXT_P1 0, __MULTIPLE_RCPTS_CC_X2 0, __NO_HTML_TAG_RAW 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS , __YOUTUBE_RCVD 0' |
Commit Message
Lee Jones
April 4, 2017, 12:32 p.m. UTC
If CONFIG_RC_CORE is not enabled then none of the RC code will be
executed anyway, so we're placing the capability check inside the
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/media/cec/cec-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 04/04/2017 02:32 PM, Lee Jones wrote: > If CONFIG_RC_CORE is not enabled then none of the RC code will be > executed anyway, so we're placing the capability check inside the > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/media/cec/cec-core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c > index 37217e2..06a312c 100644 > --- a/drivers/media/cec/cec-core.c > +++ b/drivers/media/cec/cec-core.c > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, > return ERR_PTR(res); > } > > +#if IS_REACHABLE(CONFIG_RC_CORE) > if (!(caps & CEC_CAP_RC)) > return adap; > > -#if IS_REACHABLE(CONFIG_RC_CORE) > /* Prepare the RC input device */ > adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); > if (!adap->rc) { > Not true, there is an #else further down. That said, this code is clearly a bit confusing. It would be better if at the beginning of the function we'd have this: #if !IS_REACHABLE(CONFIG_RC_CORE) caps &= ~CEC_CAP_RC; #endif and then drop the #else bit and (as you do in this patch) move the #if up. Can you make a new patch for this? Thanks! Hans
On Tue, 04 Apr 2017, Hans Verkuil wrote: > On 04/04/2017 02:32 PM, Lee Jones wrote: > > If CONFIG_RC_CORE is not enabled then none of the RC code will be > > executed anyway, so we're placing the capability check inside the > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/media/cec/cec-core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c > > index 37217e2..06a312c 100644 > > --- a/drivers/media/cec/cec-core.c > > +++ b/drivers/media/cec/cec-core.c > > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, > > return ERR_PTR(res); > > } > > > > +#if IS_REACHABLE(CONFIG_RC_CORE) > > if (!(caps & CEC_CAP_RC)) > > return adap; > > > > -#if IS_REACHABLE(CONFIG_RC_CORE) > > /* Prepare the RC input device */ > > adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); > > if (!adap->rc) { > > > > Not true, there is an #else further down. I saw the #else. It's inert code that becomes function-less. > That said, this code is clearly a bit confusing. > > It would be better if at the beginning of the function we'd have this: > > #if !IS_REACHABLE(CONFIG_RC_CORE) > caps &= ~CEC_CAP_RC; > #endif > > and then drop the #else bit and (as you do in this patch) move the #if up. > > Can you make a new patch for this? Sure.
On 04/04/2017 02:54 PM, Lee Jones wrote: > On Tue, 04 Apr 2017, Hans Verkuil wrote: > >> On 04/04/2017 02:32 PM, Lee Jones wrote: >>> If CONFIG_RC_CORE is not enabled then none of the RC code will be >>> executed anyway, so we're placing the capability check inside the >>> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>> --- >>> drivers/media/cec/cec-core.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c >>> index 37217e2..06a312c 100644 >>> --- a/drivers/media/cec/cec-core.c >>> +++ b/drivers/media/cec/cec-core.c >>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, >>> return ERR_PTR(res); >>> } >>> >>> +#if IS_REACHABLE(CONFIG_RC_CORE) >>> if (!(caps & CEC_CAP_RC)) >>> return adap; >>> >>> -#if IS_REACHABLE(CONFIG_RC_CORE) >>> /* Prepare the RC input device */ >>> adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); >>> if (!adap->rc) { >>> >> >> Not true, there is an #else further down. > > I saw the #else. It's inert code that becomes function-less. No, it isn't. It clears the CAP_RC bit so it isn't returned in the CEC_ADAP_G_CAPS ioctl. Drivers set this cap bit if they want RC support (they typically want it), but if the config option isn't there then the capability should be removed. Regards, Hans > >> That said, this code is clearly a bit confusing. >> >> It would be better if at the beginning of the function we'd have this: >> >> #if !IS_REACHABLE(CONFIG_RC_CORE) >> caps &= ~CEC_CAP_RC; >> #endif >> >> and then drop the #else bit and (as you do in this patch) move the #if up. >> >> Can you make a new patch for this? > > Sure. >
On Tue, 04 Apr 2017, Lee Jones wrote: > On Tue, 04 Apr 2017, Hans Verkuil wrote: > > > On 04/04/2017 02:32 PM, Lee Jones wrote: > > > If CONFIG_RC_CORE is not enabled then none of the RC code will be > > > executed anyway, so we're placing the capability check inside the > > > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > --- > > > drivers/media/cec/cec-core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c > > > index 37217e2..06a312c 100644 > > > --- a/drivers/media/cec/cec-core.c > > > +++ b/drivers/media/cec/cec-core.c > > > @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, > > > return ERR_PTR(res); > > > } > > > > > > +#if IS_REACHABLE(CONFIG_RC_CORE) > > > if (!(caps & CEC_CAP_RC)) > > > return adap; > > > > > > -#if IS_REACHABLE(CONFIG_RC_CORE) > > > /* Prepare the RC input device */ > > > adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); > > > if (!adap->rc) { > > > > > > > Not true, there is an #else further down. > > I saw the #else. It's inert code that becomes function-less. > > > That said, this code is clearly a bit confusing. > > > > It would be better if at the beginning of the function we'd have this: > > > > #if !IS_REACHABLE(CONFIG_RC_CORE) > > caps &= ~CEC_CAP_RC; > > #endif > > > > and then drop the #else bit and (as you do in this patch) move the #if up. > > > > Can you make a new patch for this? > > Sure. No wait, sorry! This patch is the correct fix. 'caps' is already indicating !CEC_CAP_RC, which is right. What we're trying to do here is only consider looking at the capabilities if the RC Core is enabled. If it is not enabled, the #if still does the right thing and makes sure that the caps are updated. Please take another look at the semantics.
On 04/04/2017 03:01 PM, Lee Jones wrote: > On Tue, 04 Apr 2017, Lee Jones wrote: > >> On Tue, 04 Apr 2017, Hans Verkuil wrote: >> >>> On 04/04/2017 02:32 PM, Lee Jones wrote: >>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be >>>> executed anyway, so we're placing the capability check inside the >>>> >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org> >>>> --- >>>> drivers/media/cec/cec-core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c >>>> index 37217e2..06a312c 100644 >>>> --- a/drivers/media/cec/cec-core.c >>>> +++ b/drivers/media/cec/cec-core.c >>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, >>>> return ERR_PTR(res); >>>> } >>>> >>>> +#if IS_REACHABLE(CONFIG_RC_CORE) >>>> if (!(caps & CEC_CAP_RC)) >>>> return adap; >>>> >>>> -#if IS_REACHABLE(CONFIG_RC_CORE) >>>> /* Prepare the RC input device */ >>>> adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); >>>> if (!adap->rc) { >>>> >>> >>> Not true, there is an #else further down. >> >> I saw the #else. It's inert code that becomes function-less. >> >>> That said, this code is clearly a bit confusing. >>> >>> It would be better if at the beginning of the function we'd have this: >>> >>> #if !IS_REACHABLE(CONFIG_RC_CORE) >>> caps &= ~CEC_CAP_RC; >>> #endif >>> >>> and then drop the #else bit and (as you do in this patch) move the #if up. >>> >>> Can you make a new patch for this? >> >> Sure. > > No wait, sorry! This patch is the correct fix. > > 'caps' is already indicating !CEC_CAP_RC, which is right. > > What we're trying to do here is only consider looking at the > capabilities if the RC Core is enabled. If it is not enabled, the #if > still does the right thing and makes sure that the caps are updated. > > Please take another look at the semantics. Ah, yes. You are right. But so am I: the code is just unnecessarily confusing as is seen by this discussion. I still would like to see a patch with my proposed solution. The control flow is much easier to understand that way. Regards, Hans
On Tue, 04 Apr 2017, Hans Verkuil wrote: > On 04/04/2017 03:01 PM, Lee Jones wrote: > > On Tue, 04 Apr 2017, Lee Jones wrote: > > > >> On Tue, 04 Apr 2017, Hans Verkuil wrote: > >> > >>> On 04/04/2017 02:32 PM, Lee Jones wrote: > >>>> If CONFIG_RC_CORE is not enabled then none of the RC code will be > >>>> executed anyway, so we're placing the capability check inside the > >>>> > >>>> Signed-off-by: Lee Jones <lee.jones@linaro.org> > >>>> --- > >>>> drivers/media/cec/cec-core.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c > >>>> index 37217e2..06a312c 100644 > >>>> --- a/drivers/media/cec/cec-core.c > >>>> +++ b/drivers/media/cec/cec-core.c > >>>> @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, > >>>> return ERR_PTR(res); > >>>> } > >>>> > >>>> +#if IS_REACHABLE(CONFIG_RC_CORE) > >>>> if (!(caps & CEC_CAP_RC)) > >>>> return adap; > >>>> > >>>> -#if IS_REACHABLE(CONFIG_RC_CORE) > >>>> /* Prepare the RC input device */ > >>>> adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); > >>>> if (!adap->rc) { > >>>> > >>> > >>> Not true, there is an #else further down. > >> > >> I saw the #else. It's inert code that becomes function-less. > >> > >>> That said, this code is clearly a bit confusing. > >>> > >>> It would be better if at the beginning of the function we'd have this: > >>> > >>> #if !IS_REACHABLE(CONFIG_RC_CORE) > >>> caps &= ~CEC_CAP_RC; > >>> #endif > >>> > >>> and then drop the #else bit and (as you do in this patch) move the #if up. > >>> > >>> Can you make a new patch for this? > >> > >> Sure. > > > > No wait, sorry! This patch is the correct fix. > > > > 'caps' is already indicating !CEC_CAP_RC, which is right. > > > > What we're trying to do here is only consider looking at the > > capabilities if the RC Core is enabled. If it is not enabled, the #if > > still does the right thing and makes sure that the caps are updated. > > > > Please take another look at the semantics. > > Ah, yes. You are right. But so am I: the code is just unnecessarily confusing > as is seen by this discussion. > > I still would like to see a patch with my proposed solution. The control flow > is much easier to understand that way. I have an idea. Please bear with me.
diff --git a/drivers/media/cec/cec-core.c b/drivers/media/cec/cec-core.c index 37217e2..06a312c 100644 --- a/drivers/media/cec/cec-core.c +++ b/drivers/media/cec/cec-core.c @@ -234,10 +234,10 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, return ERR_PTR(res); } +#if IS_REACHABLE(CONFIG_RC_CORE) if (!(caps & CEC_CAP_RC)) return adap; -#if IS_REACHABLE(CONFIG_RC_CORE) /* Prepare the RC input device */ adap->rc = rc_allocate_device(RC_DRIVER_SCANCODE); if (!adap->rc) {