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

[Jeff LaBundy]

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