Hi Jeff,
On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@xxxxxxxxx>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@xxxxxxxxx>
> > Co-developed-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > ---
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 10 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/himax_hx852x.c | 491 +++++++++++++++++++++++++++++++
> > 4 files changed, 509 insertions(+)
> >
> > [...]
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..f42a87faa86c 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > new file mode 100644
> > index 000000000000..31616dcfc5ab
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > @@ -0,0 +1,491 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Himax HX852x(ES) Touchscreen Driver
> > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@xxxxxxxxxxx>
> > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@xxxxxxxxx>
> > + *
> > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > + * Copyright (c) 2014 Himax Corporation.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> Please explicitly #include of_device.h.
>
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").
> > [...]
> > +static void hx852x_stop(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
>
> Granted the function is of void type, should we not still return if there
> is an error?
>
> Actually, I would still keep this function as an int for future re-use, even
> though hx852x_input_close() simply ignores the value. This way, the return
> value can be propagated to the return value of hx852x_suspend() and elsewhere.
>
> > +
> > + msleep(20);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
>
> Same here; no need to sleep following a register write that seemingly did
> not happen.
>
> > +
> > + msleep(30);
> > +}
> > +
> > +static void hx852x_power_off(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > +}
>
> Same comment with regard to function type; it's nice for internal helpers
> to be of type int, even if the core callback using it is void.
>
> > +
> > +static int hx852x_read_config(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + struct hx852x_config conf = {0};
> > + 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 power_off;
> > +
> > + hx852x_stop(hx);
>
> See my earlier comment about promoting this function's type to int.
>
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > + HX852X_SRAM_SWITCH_TEST_MODE);
> > + if (error)
> > + goto power_off;
> > +
> > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > + HX852X_SRAM_ADDR_CONFIG);
> > + if (error)
> > + goto exit_test_mode;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > + if (error)
> > + goto exit_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: %d, y res: %d, max fingers: %d\n",
> > + x_res, y_res, hx->max_fingers);
> > +
> > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > + dev_err(dev, "max supported fingers: %d, found: %d\n",
> > + HX852X_MAX_FINGERS, hx->max_fingers);
> > + error = -EINVAL;
> > + goto exit_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);
> > + }
> > +
> > +exit_test_mode:
> > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
>
> 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.
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).
Thanks!
Stephan
