Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver

From: Stephan Gerhold <stephan@gerhold.net>
To: Jeff LaBundy <jeff@labundy.com>
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,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	Jonathan Albrieux <jonathan.albrieux@gmail.com>
Subject: Re: [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
Date: Tue, 24 Oct 2023 20:35:29 +0200	[thread overview]
Message-ID: <ZTgOcXMKVY2o_ikx@gerhold.net> (raw)
In-Reply-To: <ZTcPrC+0K1unNPIv@nixie71>

Hi Jeff,

On Mon, Oct 23, 2023 at 07:28:28PM -0500, Jeff LaBundy wrote:
> On Sun, Oct 22, 2023 at 08:49:13PM +0200, Stephan Gerhold wrote:
> > > > +static int hx852x_read_config(struct hx852x *hx)
> > > > +{
> > > > +	struct device *dev = &hx->client->dev;
> > > > +	struct hx852x_config conf;
> > > > +	int x_res, y_res;
> > > > +	int error;
> > > > +
> > > > +	error = hx852x_power_on(hx);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	/* Sensing must be turned on briefly to load the config */
> > > > +	error = hx852x_start(hx);
> > > > +	if (error)
> > > > +		goto err_power_off;
> > > > +
> > > > +	error = hx852x_stop(hx);
> > > > +	if (error)
> > > > +		goto err_power_off;
> > > > +
> > > > +	error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > > > +					  HX852X_SRAM_SWITCH_TEST_MODE);
> > > > +	if (error)
> > > > +		goto err_power_off;
> > > > +
> > > > +	error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > > > +					  HX852X_SRAM_ADDR_CONFIG);
> > > > +	if (error)
> > > > +		goto err_test_mode;
> > > > +
> > > > +	error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > > > +	if (error)
> > > > +		goto err_test_mode;
> > > > +
> > > > +	x_res = be16_to_cpu(conf.x_res);
> > > > +	y_res = be16_to_cpu(conf.y_res);
> > > > +	hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > > > +	dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
> > > > +		x_res, y_res, hx->max_fingers);
> > > > +
> > > > +	if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > > > +		dev_err(dev, "max supported fingers: %u, found: %u\n",
> > > > +			HX852X_MAX_FINGERS, hx->max_fingers);
> > > > +		error = -EINVAL;
> > > > +		goto err_test_mode;
> > > > +	}
> > > > +
> > > > +	if (x_res && y_res) {
> > > > +		input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > > > +		input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > > > +	}
> > > > +
> > > > +	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;
> > > 
> > > Your new version is an improvement, but maybe we can remove duplicate
> > > code by introducing some helper variables:
> > > 
> > > 	int error, error2 = 0, error3;
> > > 
> > > 	/* ... */
> > > 
> > > err_test_mode:
> > > 	error2 = i2c_smbus_write_byte_data(...);
> > > 
> > > err_power_off:
> > > 	error3 = hx852x_power_off(...);
> > > 
> > > 	if (error)
> > > 		return error;
> > > 
> > > 	return error2 ? : error3;
> > > 
> > > In this case we achieve our goal of attempting to return the device to a
> > > safe state in both passing and failing cases. In the event of multiple
> > > errors, we return the first to occur.
> > > 
> > 
> > Right, this would work as well. Personally I think my solution is
> > slightly easier to read though. In your version my eyes somewhat
> > "stumble" over the multiple "error" variables and then about the purpose
> > of the "? : " construction. This is probably highly subjective. :-)
> 
> Agreed, my suggestion is a bit unwieldy, and prone to uninitialized
> variable bugs. However, I feel that duplicate code, especially side
> by side like this, is also confusing and prone to bugs in case the
> sequence must be updated in the future. As a compromise, how about
> something closer to my first idea:
> 
> err_test_mode:
> 	error = i2c_smbus_write_byte_data(...) ? : error;
> 
> err_power_off:
> 	return hx852x_power_off(...) ? : error;
> 
> This is nice and compact, and ensures that errors returned by the two
> functions are reported no matter the flow. The only functional change
> is that the _last_ error takes priority; but in practice this does not
> really matter. Normally if one I2C write fails, all subsequent writes
> will fail with the same return code until the hardware is recovered
> somehow.
> 
> For the corner case where the code jumps to exit_test_mode with error
> equal to -EINVAL, and i2c_smbus_write_byte_data() then fails and changes
> error to something like -EIO, I don't think we care. After the HW issue
> is fixed and all I2C writes succeed, the developer will then see that
> the number of contacts reported by the FW is invalid anyway :)
> 
> Side note: the '? :' syntax is just a shortcut that sets error equal
> to the return value of i2c_smbus_write_byte_data() if true (failure)
> without having to declare a temporary variable. If false (no failure),
> error is assigned to itself. It is perfectly legal.
> 

Thanks a lot for your detailed review and explanations!

In v3 I have changed the code to your suggestion above and also
addressed your other comments in the initial reply. :)

Stephan