From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A7634C25B70 for ; Wed, 25 Oct 2023 04:18:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=OfFz8lcLQ8iwqi4uO49rFlTeHYPm2LqRIXrJOcdWj6c=; b=RnGOPgfn+Jg1SX ojQpv97WlpUojQ5nD+O+GIEVaS6moLIL9ax+tb5nLFKLK0BvPX4p7qNiNDOMEfRmxIxp6B9ZZB1ZO NA3znh0of3aXoZQnkbBxsMb+qdEIOAWHEItCRAeoDn9SFPMCK8d/akvK/XAuTcipy6L+porrmuIvc PQGOEGmHgk7h+iPJ1+btRAMUhTht0n615P5N97jB73K78qbMId5GQXTQ4nZOgyeRUDMsxkGUesj6F paqzDGtgw38iuYkNZ4G0tNMCEFwCevMfWbgW7pG4RiUkM2xdS6C1awLwXJFnD9+tnEn3tryZIlfO/ iyE7fl/Dp+5staR+AlZw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qvVLb-00BJAl-2l; Wed, 25 Oct 2023 04:18:23 +0000 Received: from mail-lj1-x230.google.com ([2a00:1450:4864:20::230]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qvVLY-00BJ9l-2L for linux-arm-kernel@lists.infradead.org; Wed, 25 Oct 2023 04:18:22 +0000 Received: by mail-lj1-x230.google.com with SMTP id 38308e7fff4ca-2b9d07a8d84so69802971fa.3 for ; Tue, 24 Oct 2023 21:18:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1698207497; x=1698812297; darn=lists.infradead.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=Brd8GKY25UMovr2lsG9TvdPpEmwQAUXbZxdfbbkQOpE=; b=p+rNuTkU/6BNGCBauEpblChvyRMdjU8qeIZaOQIpPDlJ6GTwrpIlpd0iEB8/nhCSDb yQr3VRNoCcfDLY6hfompThEEGkFkWPXE5EBHH++rmvFrtQwguErLcpHueJFqrSTDNf6b uxiPU9FEe7KQvxtwEeTYIF6qThGGwZihgNOUQH/PJ2MLjNp2XcmC7j2pwrgRRllG114N GanZlEui7beDvkYnOQuI7dFQFFAUTkhxeKo1ll8X1SqBfqVWbsR3o3nj4Wq2Mib1kdK7 N86dfl9E5n/Lq9CEowsIHZKYfVgB4OA4s6C++mmczYh0JvTt0wmtp26713Ku8cEORi8V 0igA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698207497; x=1698812297; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Brd8GKY25UMovr2lsG9TvdPpEmwQAUXbZxdfbbkQOpE=; b=TR3TAJOMLzOqaOoeLeOcFfXQfLj/jqstmswKQGvRiJEyGN8czH8BZUxuuxqjGEz83M 1jb7x6NGtTVZnp0tRc8QoGLslE/4XespUMtDXyoOnPlzJDBYHJ4BWl9csyeKdzmfpv/b TCgHTno7K19Y/HMYgYrycYw2Lt++VNEHzotUCC/f0IyVDFHVqxFvp1lpDDHS61ftiYGw tN+1hQS9jW9eOuH8N237g1Lb1xC0FkJ7zBv5xeoxEgysejH2SbZIdmWcg84RVeYPNXNh mVNU6RLNb2ZberMohGB9xFr6rWiDSUZdL0XTodUJluwv84Bfx4I37rcbHq4cO1Vd6Cwz JnJw== X-Gm-Message-State: AOJu0YzjGKzdVbmGUSYYL2MtpeeI6txcCFwKYwVKSue/wwvj8vX9BYYe 48ntfDyjqgTHk/eF4zcGv1dnlQ== X-Google-Smtp-Source: AGHT+IHK9MNBEXuKit4Dyq67gtZ3P+f3+x9ngQPQaqjPnQ9oZd4mGeNK5F+gV0c1lFk5HL0q/K6iHQ== X-Received: by 2002:a2e:9691:0:b0:2bf:f32a:1f64 with SMTP id q17-20020a2e9691000000b002bff32a1f64mr10173412lji.18.1698207497173; Tue, 24 Oct 2023 21:18:17 -0700 (PDT) Received: from localhost ([102.36.222.112]) by smtp.gmail.com with ESMTPSA id q16-20020adfea10000000b00326dd5486dcsm11165440wrm.107.2023.10.24.21.18.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 24 Oct 2023 21:18:16 -0700 (PDT) Date: Wed, 25 Oct 2023 07:18:13 +0300 From: Dan Carpenter To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Krzysztof Kozlowski , Alim Akhtar , Thierry Reding , linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-pwm@vger.kernel.org, kernel-janitors@vger.kernel.org Subject: Re: [PATCH] pwm: samsung: Fix a bit test Message-ID: <0d61bf0a-3aca-466c-9198-e937e81b5328@kadam.mountain> References: <917e3890-7895-4b1c-bcee-4eecb3b7fe09@moroto.mountain> <20231024211157.xv3vzqlmxmxwgvle@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231024211157.xv3vzqlmxmxwgvle@pengutronix.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231024_211820_778219_673B0496 X-CRM114-Status: GOOD ( 34.19 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 24, 2023 at 11:11:57PM +0200, Uwe Kleine-K=F6nig wrote: > Hello Dan, > = > On Tue, Oct 17, 2023 at 05:04:08PM +0300, Dan Carpenter wrote: > > This code has two problems. First, it passes the wrong bit parameter to > > test_bit(). Second, it mixes using PWMF_REQUESTED in test_bit() and in > > open coded bit tests. > > = > > The test_bit() function takes a bit number. In other words, > > "if (test_bit(0, &flags))" is the equivalent of "if (flags & (1 << 0))". > > Passing (1 << 0) to test_bit() is like writing BIT(BIT(0)). It's a > > double shift bug. > > = > > In pwm_samsung_resume() these issues mean that the flag is never set and > > the function is essentially a no-op. > > = > > Fixes: 4c9548d24c0d ("pwm: samsung: Put per-channel data into driver da= ta") > > Signed-off-by: Dan Carpenter > > --- > > From static analysis and not tested. > > = > > drivers/pwm/pwm-samsung.c | 2 +- > > include/linux/pwm.h | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > = > > diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c > > index 10fe2c13cd80..acf4a0d8d990 100644 > > --- a/drivers/pwm/pwm-samsung.c > > +++ b/drivers/pwm/pwm-samsung.c > > @@ -630,7 +630,7 @@ static int pwm_samsung_resume(struct device *dev) > > struct pwm_device *pwm =3D &chip->pwms[i]; > > struct samsung_pwm_channel *chan =3D &our_chip->channel[i]; > > = > > - if (!(pwm->flags & PWMF_REQUESTED)) > > + if (!test_bit(PWMF_REQUESTED, &pwm->flags)) > > continue; > > = > > if (our_chip->variant.output_mask & BIT(i)) > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > index e3b437587b32..3eee5bf367fb 100644 > > --- a/include/linux/pwm.h > > +++ b/include/linux/pwm.h > > @@ -41,8 +41,8 @@ struct pwm_args { > > }; > > = > > enum { > > - PWMF_REQUESTED =3D 1 << 0, > > - PWMF_EXPORTED =3D 1 << 1, > > + PWMF_REQUESTED =3D 0, > > + PWMF_EXPORTED =3D 1, > = > I'd want s/ / / here. Or even not assign explicit values at all? > = I feel like the 0 and 1 add value. But sure, I can remove the extra space. You're right that trying to align stuff is potentially going to cause pain in the future. > > }; > > = > > /* > = > I'd say these are two separate issues, with the one in pwm-samsung being > bad and the one in "only" ugly. > = > I wonder how I could get the samsung part wrong. All current usages of > PMWF_REQUESTED (and also PWMF_EXPORTED) use test_bit (et al). Grepping > through history pwm-pca9685.c got this wrong in a similar way for some > time, but otherwise it was always used correctly. > = > The definition of the flags in is ugly since = > f051c466cf69 ("pwm: Allow chips to support multiple PWMs") from 2011! > = > @Dan: Would you split the patch in two please? Sure. regards, dan carpenter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel