Hi Christophe and Jeff,
Thanks for your comments!
On Sun, Sep 17, 2023 at 11:37:20AM -0500, Jeff LaBundy wrote:
> On Sun, Sep 17, 2023 at 08:03:48AM +0200, Christophe JAILLET wrote:
> > Le 13/09/2023 à 15:25, Stephan Gerhold a écrit :
> > > From: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxxx>
> > > Co-developed-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@xxxxxxxxxxxxxxxx>
> > > ---
> >
> > ...
> >
> > > +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?
> >
I haven't ever seen this but I guess you're right. It could spam
potentially. :-) I will change it in v2.
> > > + 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".
>
> For what it's worth, 'error' tends to be more common in input.
>
Yep, this is the only reason why we used it. I usually use "ret" but got
the feeling "error" is preferred for the input subsystem.
> >
> > > +
> > > + 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.
> >
Right, will change it!
> > > + 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().
>
> This seems like a memory leak in every touchscreen driver; maybe it is more
> practical to have the input core handle this clean-up.
>
> Other drivers can and do insert other return paths between input_mt_init_slots()
> and input_register_device(), so it seems that we cannot solve this by calling
> input_mt_destroy_slots() from the error path within input_register_device().
>
> Maybe a better option is to update input_mt_init_slots() to use device-managed
> allocation instead?
>
Hmm, it would be fairly easy to add the input_mt_destroy_slots() call as
part of the single if statement I have here, but yeah, someone would
need to make a patch for literally all of the other touchscreen drivers.
Both options (add call or some devm magic) would be fine for me. :-)
> >
> > It should also be called in a .remove function (unless
> > devm_add_action_or_reset is prefered)
>
> I think the remove path is OK, as input_dev_release() handles this for us. In
> case I have misunderstood, please let me know.
>
Yep, I think so too!
Thanks,
Stephan
