Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Sun, 22 Oct 2023 13:28:14 -0500
From:   Jeff LaBundy <jeff@labundy.com>
To:     Stephan Gerhold <stephan@gerhold.net>
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>,
        Henrik Rydberg <rydberg@bitmath.org>,
        linux-input@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Jonathan Albrieux <jonathan.albrieux@gmail.com>
Subject: Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
Message-ID: <ZTVpvlkORe4E41vy@nixie71>
References: <20230913-hx852x-v1-0-9c1ebff536eb@gerhold.net>
 <20230913-hx852x-v1-2-9c1ebff536eb@gerhold.net>
 <ZQYUe46/rj8jqNvg@nixie71>
 <ZRg-uZskk07jxup0@gerhold.net>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZRg-uZskk07jxup0@gerhold.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gRee0F/tdLeF68xU0B6B5vcQEr40jTr9vhu+wdPmBntVPKHDQaITWpMK2rm7?=
 =?us-ascii?Q?N+PdUILWk17/ETjcjYoDtnTOWKNlzt/6E/8hkAn4bQgfE5c1p+AIWSliD+6Z?=
 =?us-ascii?Q?nARQY8Suy7tdKO2NtnkltpKltfKqCJSIkOJNfKeWjpSYxxQ4PPEdLuLTVE5G?=
 =?us-ascii?Q?4BqSMWLDfVwn/eRbFVfWAflFXrY8gNXe3YfN6XkjIaOLW4kWJTnhs9+juhtf?=
 =?us-ascii?Q?EEsuEZfuwucAHXuSbNcHN0Hyebr9VqCR+jL56XDRx5l9EVVwrMu8LGT1z5Ne?=
 =?us-ascii?Q?kVTiBjiqvzcC1M5H31FhZn1GETJt+t2mlC/Nx6T7qF2gLR7i9SLK6GVD33VE?=
 =?us-ascii?Q?pnHWEOunCfJJkbbWLmak+EcrUfS9kOp1E7SGrpGGv9b1mHw30iy/b6NYBxSA?=
 =?us-ascii?Q?ijXXY0lt3pFVNe6QblfqHX3jrVkXZAofH4tTKfl3dqbf9iy9cPJM05hSrWhz?=
 =?us-ascii?Q?aVjRCzaVnvjoMk/DryRrBDunOZFFJOiweZskDPkqdHCJvTbc3Ne9nl/q1jIw?=
 =?us-ascii?Q?XSKrJXCjSN7u5VkU49SAvVdB7MkG6qD8BIvC204Z+I5yrDk4mFleaFtTUe3D?=
 =?us-ascii?Q?MVc6yhvN152YrS3tNg97mhV2k5fudwEh1RtFRTkwGahbSltLfRh1rrXwD2Nw?=
 =?us-ascii?Q?C4sFh1mLqDQlo+NOFr8Uz7UV1Ju2YDxsXX3PuGoOjcVbv3ynkO/cJ4c/ncn8?=
 =?us-ascii?Q?WiWFd5AxlictbVq5Un4A7xB7TgtO39w4P30CcbV3Rasatusa9cyRXcy+PD6C?=
 =?us-ascii?Q?da1kPokIxiAVpxUzrkyzk/nc/dob38Vmcca9DBaEWsvmhP/zcL7PzEN1dBS0?=
 =?us-ascii?Q?Jjh6g8ynEmM1IXPwQFLE13dXwpgwPjW2K0OdNUiixJ9wVA0HH/AmYdquHUYM?=
 =?us-ascii?Q?OKELAschCHkLexikrShoA/BJ5Z/FJMzmAsk9PWPVb6FHXJEwYuFfm9nwl2dl?=
 =?us-ascii?Q?CAw3cVbewPprpWtncbJDhiJtkWqcYmUxR/a62BRNf9qjjfNElHGnV1on2lcp?=
 =?us-ascii?Q?N5Ao0XZXlSXnp//GSKy3vyDQavdKoyOzMUyrJ6MVPnaQkasI4VKR8YF9YY96?=
 =?us-ascii?Q?YFpiPe/nd+fwR7GIlok3yq1nMkHs4zL0nlK+OkXm9WkDKmw0I7bsMQCogEFF?=
 =?us-ascii?Q?9DEgHOj5rY4bMLj4WzHpVFguicbK7B2MtLV8241jNLPx37i2mDnbQ4gCfMjy?=
 =?us-ascii?Q?5dZ4NTZy58+bAQrgYUSR+/49vVnPQXPKq++UNqdIqvY0YlOvK24cTO9n9Vjr?=
 =?us-ascii?Q?N8NidyiYzoJjUa7Njh+ahCxsRR93LL8IJFXNytlxxuLnr/7Dwr91P5Z/mJ03?=
 =?us-ascii?Q?TCFiw7aVPT2A80+9SMIJgq55al+i7GB890htNmZph1LTzdLcS/XLZU5RABIQ?=
 =?us-ascii?Q?DxdUx0NRy9i0EFewsq1eWDUhy5jydMBW0KINa2Ul5yWe6xbRby46CEbMX5Jz?=
 =?us-ascii?Q?U3nfSXOJXrxWnrqaai+q3cpTHU1GASOBPi/fu3woBGbTYarlhy2lQI9fX9Fp?=
 =?us-ascii?Q?g2K4swDeRx9fXrG1oylK58L524bzxI8G6zXTzUEUiJpAQhUrjLVBXfhbi2pS?=
 =?us-ascii?Q?XijyVODJ8DJuBuO9heGvb8m9YnEObU3ijIAq8tBv?=
X-OriginatorOrg: labundy.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0ad96ee7-ebef-49d4-6eb3-08dbd32ca8c4
X-MS-Exchange-CrossTenant-AuthSource: DM5PR0801MB3767.namprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Oct 2023 18:28:16.7659
 (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: uULBDG4KONP44Lza3E23F1TCurD24iUUCmuI/6NFXukgKY1OeUiIGlmPwVb6Xu+faFnDrPAeS7qhwhT/b5mNMg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR08MB7831
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,
	SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Sun, 22 Oct 2023 11:28:42 -0700 (PDT)

Hi Stephan,

On Sat, Sep 30, 2023 at 05:28:57PM +0200, Stephan Gerhold wrote:

[...]

> In v2 I have added linux/of.h and linux/mod_devicetable.h, since I'm
> actually using definitions from these two only. Seems like including
> of_device.h is discouraged nowadays, see commit dbce1a7d5dce ("Input:
> Explicitly include correct DT includes").

Apologies for the delayed response and some confusion from my side. What
you have added in v2 is correct, and what I should have suggested in the
first place.

[...]

> > Nit: it would still be nice to preserve as many return values as possible, perhaps
> > as follows:
> > 
> > +exit_test_mode:
> > 	error = i2c_smbus_write_byte_data(...) ? : error;
> > 
> > > +power_off:
> > > +	hx852x_power_off(hx);
> > > +	return error;
> > 
> > Similarly, with hx852x_power_off() being promoted to int as suggested above,
> > this could be:
> > 
> > 	return hx852x_power_off(...) ? : error;
> > 
> > There are other idiomatic ways to do the same thing based on your preference.
> > Another (perhaps more clear) option would be to move some of these test mode
> > functions into a helper, which would also avoid some goto statements.
> > 
> 
> I played with this for a bit. A problem of the "? : error" approach is
> that it hides the original error in case the new calls error again.

That's correct; good catch.

> 
> Let's assume
> 
> 	error = hx852x_start(hx);
> 	if (error)
> 		goto power_off;
> 
> fails with error = -ENXIO. We jump to power_off:
> 
> power_off:
> 	return hx852x_power_off(hx) ? : error;
> 
> Let's say for whatever reason hx852x_power_off() fails too but returns
> -EINVAL. Then the final return value will be -EINVAL, while with the
> current approach in this patch it would return the original cause
> (-ENXIO). I think that's more clear.
> 
> I also played with moving code to a separate function to avoid the
> gotos, but I feel like that makes the fairly focused logic of this
> function (reading the configuration by temporarily entering the test
> mode) just more confusing.
> 
> To still fix the error handling I ended up with duplicating the
> "success" code path and the "error" code path (it's just two function
> calls), i.e.:
> 
> 	error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> 	if (error)
> 		goto err_power_off;
> 
> 	return hx852x_power_off(hx);
> 
> err_test_mode:
> 	i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
> err_power_off:
> 	hx852x_power_off(hx);
> 	return error;
> 
> I hope that's fine too. A bit ugly maybe but in this case I would prefer
> having the main code path (reading the configuration) clearly readable.
> 
> Let me know if you have a better suggestion for these (I'll send v2 in a
> bit so that you can see the full diff there).

Maybe we can massage this just a bit more; I have followed up with another
suggestion in v2.

> 
> Thanks!
> Stephan

Kind regards,
Jeff LaBundy