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

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,
	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: Mon, 23 Oct 2023 19:28:28 -0500	[thread overview]
Message-ID: <ZTcPrC+0K1unNPIv@nixie71> (raw)
In-Reply-To: <ZTVuqW7oU5BmPzTS@gerhold.net>

Hi Stephan,

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.

> Do you mind if keep this as-is? I don't have a strong opinion about
> this, but a slight preference for my current solution.

I think any of these three solutions gets the job done; please choose
the one you feel is easiest to read and maintain.

> > > +}

Kind regards,
Jeff LaBundy