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;
Date:   Tue, 20 Jun 2023 14:35:49 -0500
From:   Jeff LaBundy <jeff@labundy.com>
To:     Neil Armstrong <neil.armstrong@linaro.org>
Cc:     Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Bastien Nocera <hadess@hadess.net>,
        Hans de Goede <hdegoede@redhat.com>,
        Henrik Rydberg <rydberg@bitmath.org>,
        linux-input@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/4] input: touchscreen: add core support for Goodix
 Berlin Touchscreen IC
Message-ID: <ZJH/lTA0TWmdEfJN@nixie71>
References: <20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org>
 <20230606-topic-goodix-berlin-upstream-initial-v2-2-26bc8fe1e90e@linaro.org>
 <ZIvTfE5O31NKnywd@nixie71>
 <a4f36bef-7c2c-08f4-bf93-fe1f0f1d315e@linaro.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a4f36bef-7c2c-08f4-bf93-fe1f0f1d315e@linaro.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?b9aSmzld9UHsNN5wjdMR4Ug8bXDPDK0QNpeKZh98hktJhtxK3BUBXdDoQd6/?=
 =?us-ascii?Q?plN8jLM1TQL912Kkb/zF088Sq9SCW49qtXEUy04b9/61fP/Co6qdqdQbFYb/?=
 =?us-ascii?Q?cli24GDc67ML6RQD9grl3WSDqhRsN9HkxqVq6NwBvKMG3wB2s15jcj7rYRyf?=
 =?us-ascii?Q?maTgrsWMP7li7hv0xtCt6PvHXMU2XuInp1dPXnNG0MHWH005saH88qnLSryQ?=
 =?us-ascii?Q?r51k3gkeB1BoOhXeQpMD0rrd/0LVWupN6Xx9pLrearglnwx8PVS1rFeQWGOj?=
 =?us-ascii?Q?GpHTBZ1DLSEPEzitflvJoGWhAYZl2SsKWo5Va+SBOwjR3sojmPHZyExzO4/r?=
 =?us-ascii?Q?wf9I7h/8pxkMUuV7GmkfUpZM+5KzrfijqwYLxEJMnSVLEF8iRRAAwpiS5r2P?=
 =?us-ascii?Q?StuduK6RYUgE1XfmxQx+zEQ8WFH7PhcuWDTUbMkYG0ArUC7CgGoIW9IF7c8i?=
 =?us-ascii?Q?3bgTKvkUODu3TgAJSQ4vDEimjWT1MGb0ExT2vxg5yPOwW4zSiOK3L3Khrete?=
 =?us-ascii?Q?d0XXf6vTsqCsCALY9x5gWiS5ZUo+xNwt6UPPYEPLiWPZAC3gh5KtHV5IPbT6?=
 =?us-ascii?Q?qodo/6/TcYq2DWnE3Z+bniVP/D8pPJkbOTjcdghOYESI8lmB6CrykUnThsHM?=
 =?us-ascii?Q?wb6erD7LihiAGTF7cQBBzDaEjcEk7CKdwwiAWNSTa3z2E1c8lc1DssE9DLOZ?=
 =?us-ascii?Q?axxYAupYslfN+eEm4fSafF9PKkwszrNx/y5E11w52nrogKM7sNhgwHPm4TZV?=
 =?us-ascii?Q?V3iQtyypvQ1AIn4H69DO1hbRO/2VjZQn0SHwpLs7qA9f7hui8XO9gGXEennm?=
 =?us-ascii?Q?1HXOV7hMM2W6K9zQtEIDLvOIoUefV3GK1sYU+j2NXprchFLhLjRl8mhX3zFE?=
 =?us-ascii?Q?xJtd7Yd0hJsIcmEdHAHXNlAsXDrKNLulY58By1YxLaCnH+2gDr4s2IAuRODY?=
 =?us-ascii?Q?546+QPwAYnWexiImY8KuYT8Eg7akt/e2DuVEz1JmAC/m35ABcTAicA2Fsx6V?=
 =?us-ascii?Q?GKrKjDhF1qLzRHtyPpnqlGo5RP2ItlkGuNdt7AhUUXkpXZ1QfaGndMzvXxiI?=
 =?us-ascii?Q?/IXkK/d7sFDc0lcsovC48F39+HeUyhMykjKiwYWsx3DirBZUFK7ZvKtTeCXM?=
 =?us-ascii?Q?ZnpDr9K30BP2E7by6tFCSFK36N3P78wfSybu9h3MKHUAlvlapJoa8TdGyoB/?=
 =?us-ascii?Q?pcp7KeyJAsCYu2GG5UFs5PbFsq4Yq0ZY1+FfocfDSMHO7eKM9yzdNiF4RzlR?=
 =?us-ascii?Q?EoA7Py3O2hwG9MOlH6b/AFGqECJPy1BwRbH6z7eNVTH+O2+v7VVjpakXItjj?=
 =?us-ascii?Q?DUqoVYIWa57kP9P2W1xtuX72bvB0d9eZ5IC2MjXDqdZXiP6RlPQ9AR2Kwl+x?=
 =?us-ascii?Q?GrOn+y03rcuhoIg5Cx+w8JnerdwSdUS5bQ8GeH0iwxrjSSyx7+jpxIXDMKZL?=
 =?us-ascii?Q?MLM+toESI7xtKax63UNXEVBhsroVhn1345PoORH1tz50ZfxEM+gJcrJBUz1h?=
 =?us-ascii?Q?Gph/+T9N9pZcPOaovC7OiGNH9G4omV/8c2pGRm1xVC7200DlOMQFqTYpm28/?=
 =?us-ascii?Q?XMqZOMsT7Ry7wprNeE+mXp/u4r5ecikML+VpTm9i?=
X-OriginatorOrg: labundy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 371c3e5f-8c36-47d9-1acd-08db71c5905a
X-MS-Exchange-CrossTenant-AuthSource: SN4PR0801MB3774.namprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2023 19:35:54.8840
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 00b69d09-acab-4585-aca7-8fb7c6323e6f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: addZM3+N6KehIuBPSgBLBg1P2eEYo4aWg4i5uk2S5aK3hgFleGVwIravsRYWiDEICm+orSkLIrTjduIDlEZohQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR08MB7165
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,
        T_SCC_BODY_TEXT_LINE 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

Hi Neil,

On Tue, Jun 20, 2023 at 08:38:20PM +0200, Neil Armstrong wrote:

[...]

> > > +static int goodix_berlin_power_on(struct goodix_berlin_core *cd, bool on)
> > > +{
> > > +	int error = 0;
> > 
> > No need to initialize 'error' here.
> 
> Th refactor I did needs it to be initialized at 0 because the if() always calls return,
> but yeah it's kind of ugly.

Ah, you're correct; I see now.

> 
> > 
> > > +
> > > +	if (on) {
> > > +		error = regulator_enable(cd->iovdd);
> > > +		if (error < 0) {
> > > +			dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
> > > +			return error;
> > > +		}
> > > +
> > > +		/* Vendor waits 3ms for IOVDD to settle */
> > > +		usleep_range(3000, 3100);
> > > +
> > > +		error = regulator_enable(cd->avdd);
> > > +		if (error < 0) {
> > > +			dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
> > > +			goto power_off_iovdd;
> > > +		}
> > > +
> > > +		/* Vendor waits 15ms for IOVDD to settle */
> > > +		usleep_range(15000, 15100);
> > > +
> > > +		gpiod_set_value(cd->reset_gpio, 0);
> > > +
> > > +		/* Vendor waits 4ms for Firmware to initialize */
> > > +		usleep_range(4000, 4100);
> > > +
> > > +		error = goodix_berlin_dev_confirm(cd);
> > > +		if (error < 0)
> > > +			goto power_off_gpio;
> > 
> > All of this cleaned up nicely. The following comment is idiomatic, but I feel
> > the goto can be easily eliminated as follows:
> > 
> > 		error = goodix_berlin_dev_confirm(cd);
> > 		if (error)
> > 			break;
> 
> Break ? in an if ?

Ignore my comment; I lost my place and thought we were inside a loop :)

Kind regards,
Jeff LaBundy