linux-kernel - Re: [PATCH 2/3] Input: add support for the STMicroelectronics FingerTip touchscreen

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 18 Jan 2017 10:58:04 +0900
From:   Andi Shyti <andi.shyti@...sung.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Krzysztof Kozlowski <krzk@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Javier Martinez Canillas <javier@....samsung.com>,
        linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
        Andi Shyti <andi@...zian.org>
Subject: Re: [PATCH 2/3] Input: add support for the STMicroelectronics
 FingerTip touchscreen

Hi Dmitry,

thanks for looking into the driver and the review!
I agree with almost everything, some comments below.

> > +	/* optional tuning */
> > +	err = stmfts_write_and_wait(sdata, STMFTS_MS_CX_TUNING);
> > +	if (err)
> > +		dev_warn(&sdata->client->dev, "failed to perform mutual auto tune\n");
> > +
> > +	/* optional tuning */
> > +	err = stmfts_write_and_wait(sdata, STMFTS_SS_CX_TUNING);
> > +	if (err)
> > +		dev_warn(&sdata->client->dev, "failed to perform self auto tune\n");
> > +
> > +	err = stmfts_write_and_wait(sdata, STMFTS_FULL_FORCE_CALIBRATION);
> > +	if (err)
> > +		return err;
> 
> Hmm, do you need to calibrate on boot, or maybe wait for the device to
> be opened first?

The datasheet says that I need to calibrate during boot. Anyway,
the calibration requires some time (it can be up to a couple of
seconds), therefore I wouldn't like to calibrate everytime
someone opens the device.

> > +static int stmfts_enable_key(struct stmfts_data *sdata)
> > +{
> > +	int err;
> > +
> > +	sdata->input_key = devm_input_allocate_device(&sdata->client->dev);
> > +	if (!sdata->input_key)
> > +		return -ENOMEM;
> > +
> > +	sdata->input_key->name = "stmfts_key";
> > +	sdata->input_key->id.bustype = BUS_I2C;
> > +	sdata->input_key->open = stmfts_input_key_open;
> > +	sdata->input_key->close = stmfts_input_key_close;
> 
> If you want separate input device for keys I'd recommend setting ->phys
> on them. Another option is to add keys to teh touchscreen device. Do you
> expect different actors listening to these devices? Even with different
> actors you can program event masks so that if process is not interested
> in some events (like ABS_MT_* events) it is not woken up when dveice
> generates them.

Are you suggesting to drop one event interface?

The reason why I chose to generate two interfaces is indeed
because there might be be different processes completely
unrelated to each other that will interact with the device (for
example in Jaechul's driver that you just reviewed, touchscreen
and touchkey are different devices).

I chose it for coherency with the common userspace architecture.
But of course, I can do as you recommend.

I have one more question. This touchscreen has also a proximity
functionality that works exactly as a proximity sensor and I
planned to add it in the future. Would it make sense to use the
iio framework (as it behaves as a proximity sensor) or use the
current interface? For instance, I was going to use the iio.

> > +	/* get the regulator for powering the leds on */
> > +	sdata->ledvdd = devm_regulator_get(&sdata->client->dev, "ledvdd");
> > +	if (IS_ERR(sdata->ledvdd))
> > +		/* there is no LED connected to the touch key */
> > +		sdata->ledvdd = NULL;
> 
> As I mentioned, is it truly regulator, or just a GPIO?

As I wrote in the previous mail, this is truly a regulator. If it
was a gpio, I would still prefer to use the regulator_* functions
as those would be managed by regulator-gpio and I don't need to
change the driver for that.

Thanks a lot,
Andi