lkml.org 
[lkml]   [2023]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From
Le 13/09/2023 à 15:25, Stephan Gerhold a écrit :
> From: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Co-developed-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> ---

...

> +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> +{
> + struct hx852x *hx = ptr;
> + int error;
> +
> + error = hx852x_handle_events(hx);
> + if (error) {
> + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);

Should dev_err_ratelimited() be preferred?

> + return IRQ_NONE;
> + }
> +
> + return IRQ_HANDLED;
> +}

...

> +static int hx852x_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct hx852x *hx;
> + int error, i;

Nit: err or ret is shorter and maybe more "standard".

> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_WRITE_BYTE |
> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> + dev_err(dev, "not all i2c functionality supported\n");
> + return -ENXIO;
> + }
> +
> + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> + if (!hx)
> + return -ENOMEM;
> +
> + hx->client = client;
> + hx->input_dev = devm_input_allocate_device(dev);
> + if (!hx->input_dev)
> + return -ENOMEM;
> +
> + hx->input_dev->name = "Himax HX852x";
> + hx->input_dev->id.bustype = BUS_I2C;
> + hx->input_dev->open = hx852x_input_open;
> + hx->input_dev->close = hx852x_input_close;
> +
> + i2c_set_clientdata(client, hx);
> + input_set_drvdata(hx->input_dev, hx);
> +
> + hx->supplies[0].supply = "vcca";
> + hx->supplies[1].supply = "vccd";
> + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> + if (error < 0)
> + return dev_err_probe(dev, error, "failed to get regulators");
> +
> + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(hx->reset_gpiod))
> + return dev_err_probe(dev, error, "failed to get reset gpio");
> +
> + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> + if (error) {
> + dev_err(dev, "failed to request irq %d: %d\n", client->irq, error);

dev_err_probe() could be used to be consistent with above code.
Same for below dev_err() calls.

> + return error;
> + }
> +
> + error = hx852x_read_config(hx);
> + if (error)
> + return error;
> +
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> + error = hx852x_parse_properties(hx);
> + if (error)
> + return error;
> +
> + hx->input_dev->keycode = hx->keycodes;
> + hx->input_dev->keycodemax = hx->keycount;
> + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> + for (i = 0; i < hx->keycount; i++)
> + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> +
> + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error) {
> + dev_err(dev, "failed to init MT slots: %d\n", error);
> + return error;
> + }
> +
> + error = input_register_device(hx->input_dev);
> + if (error) {

input_mt_destroy_slots() should be called here, or in an error handling
path below, or via a devm_add_action_or_reset().

It should also be called in a .remove function (unless
devm_add_action_or_reset is prefered)

CJ

> + dev_err(dev, "failed to register input device: %d\n", error);
> + return error;
> + }
> +
> + return 0;
> +}

...

\
 
 \ /
  Last update: 2023-09-17 08:06    [W:0.130 / U:2.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site