On Wed, Dec 01, 2021 at 10:27:46PM +1000, Alistair Francis wrote: > On Sat, Nov 6, 2021 at 4:47 PM Dmitry Torokhov > <dmitry.torokhov@xxxxxxxxx> wrote: > > > > Hi Alistair, > > > > On Wed, Nov 03, 2021 at 09:48:27PM +1000, Alistair Francis wrote: > > > From: Mylčne Josserand <mylene.josserand@xxxxxxxxxxx> > > > ... > > > + si->tch_hdr.max, > > > + si->xy_mode + 3 + si->tch_hdr.ofs, > > > + si->tch_hdr.bofs); > > > + > > > + num_cur_tch = tch.hdr; > > > + if (num_cur_tch > max_tch) { > > > + dev_err(dev, "Num touch err detected (n=%d)\n", num_cur_tch); > > > + num_cur_tch = max_tch; > > > + } > > > + > > > + if (num_cur_tch == 0 && ts->num_prv_rec == 0) > > > + return 0; > > > + > > > + /* extract xy_data for all currently reported touches */ > > > + if (num_cur_tch) > > > + cyttsp5_get_mt_touches(ts, &tch, num_cur_tch); > > > + else > > > + cyttsp5_mt_lift_all(ts); > > > > Not needed with INPUT_MT_DROP_UNUSED. > > I tried INPUT_MT_DROP_UNUSED and I still need this function I am curious why that is. Probably call to input_mt_sync_frame() was in a wrong place? ... > > > + > > > +static int cyttsp5_deassert_int(struct cyttsp5 *ts) > > > +{ > > > + u16 size; > > > + u8 buf[2]; > > > + int rc; > > > + > > > + rc = regmap_bulk_read(ts->regmap, HID_INPUT_REG, buf, 2); > > > + if (rc < 0) > > > + return rc; > > > + > > > + size = get_unaligned_le16(&buf[0]); > > > + if (size == 2 || size == 0) > > > + return 0; > > > > If you were to use level interrupts this probably would not be needed. > > I have tried with/without this and I still can't get level interrupts working. Does the platform support level interrupts? ... > > > +static int cyttsp5_probe(struct device *dev, struct regmap *regmap, int irq, > > > + const char *name) > > > +{ > > > + struct cyttsp5 *ts; > > > + struct cyttsp5_sysinfo *si; > > > + int rc = 0, i; > > > + > > > + ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL); > > > + if (!ts) > > > + return -ENOMEM; > > > + > > > + /* Initialize device info */ > > > + ts->regmap = regmap; > > > + ts->dev = dev; > > > + si = &ts->sysinfo; > > > + dev_set_drvdata(dev, ts); > > > + > > > + /* Initialize mutexes and spinlocks */ > > > + mutex_init(&ts->system_lock); > > > + > > > + /* Initialize wait queue */ > > > + init_waitqueue_head(&ts->wait_q); > > > + > > > + /* Power up the device */ > > > + ts->vdd = regulator_get(dev, "vdd"); > > > > Do not mix managed an unmanaged resources. You are leaking this > > regulator. > > I'm not clear what I should do differently here. devm_regulator_get(). > > > > > > + if (IS_ERR(ts->vdd)) { > > > + rc = PTR_ERR(ts->vdd); > > > + dev_set_drvdata(dev, NULL); > > > + kfree(ts); You can't call kfree() for memory allocated with devm_kzalloc(). > > > + return rc; > > > + } > > > + > > > + rc = regulator_enable(ts->vdd); > > > + if (rc) { > > > + regulator_put(ts->vdd); > > > + dev_set_drvdata(dev, NULL); > > > + kfree(ts); > > > + return rc; > > > + } > > > + > > > + /* Reset the gpio to be in a reset state */ > > > + ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > > > + if (IS_ERR(ts->reset_gpio)) { > > > + rc = PTR_ERR(ts->reset_gpio); > > > + dev_err(dev, "Failed to request reset gpio, error %d\n", rc); > > > + return rc; > > > + } > > > + gpiod_set_value(ts->reset_gpio, 1); > > > + > > > + /* Need a delay to have device up */ > > > + msleep(20); > > > + > > > + rc = devm_request_threaded_irq(dev, irq, NULL, cyttsp5_handle_irq, > > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > > > Please do not override platform setup with hardcoded triggers. Also, it > > is strongly recommended to use level interrupts for these peripherals. > > > > This is also likely unsafe if controller is not completely shut off and > > is capable of generating interrupts given input device is not yet > > allocated. > > I have dropped the `IRQF_TRIGGER_FALLING |` > > I have tried to use level interrupts, but I can't get the device > working with them. That is weird, does the interrupt controller support level interrupts? Thanks. -- Dmitry