Re: [PATCH v3 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Jeff LaBundy <jeff@labundy.com>]

Hi Kamel,

On Mon, Nov 13, 2023 at 02:32:12PM +0100, kamel.bouhara@bootlin.com wrote:
> Le 2023-10-22 23:54, Jeff LaBundy a écrit :
> > Hi Kamel,
> 
> Hi Jeff,
> 
> > 
> > On Thu, Oct 12, 2023 at 09:40:34AM +0200, Kamel Bouhara wrote:
> > > Add a new driver for the TouchNetix's axiom family of
> > > touchscreen controllers. This driver only supports i2c
> > > and can be later adapted for SPI and USB support.
> > > 
> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > ---
> > >  MAINTAINERS                                   |   1 +
> > >  drivers/input/touchscreen/Kconfig             |  13 +
> > >  drivers/input/touchscreen/Makefile            |   1 +
> > >  .../input/touchscreen/touchnetix_axiom_i2c.c  | 740
> > > ++++++++++++++++++
> > >  4 files changed, 755 insertions(+)
> > >  create mode 100644 drivers/input/touchscreen/touchnetix_axiom_i2c.c
> > 
> > Please do not include 'i2c' in the filename. If the driver is expanded
> > in
> > the future to support SPI, it would make sense to have
> > touchnetix_axiom.c,
> > touchnetix_axiom_i2c.c and touchnetix_axiom_spi.c. To prevent this
> > driver
> > from having to be renamed in that case, just call it touchnetix_axiom.c.
> > 
> 
> Sure but the generic part of the code could also be moved to
> touchnetix_axiom.c.

Right; I'm simply saying to do this now as opposed to having a giant patch
later when SPI support comes along. In case I have misunderstood your reply,
please let me know.

[...]

> > > +#include <linux/crc16.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/input.h>
> > > +#include <linux/input/mt.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > 
> > Please #include mod_devicetable.h as well.
> > 
> 
> OK is this only for the sake of clarity ? As mod_devicetable.h is already
> included in linux/of.h ?

I haphazardly wrote this comment while in the process of reviewing
dbce1a7d5dce ("Input: Explicitly include correct DT includes"); however
you are correct. That being said, do you really need the entire of.h
for this driver?

> 
> > > +#include <linux/of.h>
> > > +#include <linux/pm.h>

[...]

> > > +static int
> > > +axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8
> > > *buf, u16 len)
> > > +{
> > > +	struct axiom_data *ts = i2c_get_clientdata(client);
> > > +	struct axiom_cmd_header cmd_header;
> > > +	struct i2c_msg msg[2];
> > > +	int ret;
> > > +
> > > +	cmd_header.target_address =
> > > cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > > +	cmd_header.length = cpu_to_le16(len);
> > > +	cmd_header.read = 1;
> > > +
> > > +	msg[0].addr = client->addr;
> > > +	msg[0].flags = 0;
> > > +	msg[0].len = sizeof(cmd_header);
> > > +	msg[0].buf = (u8 *)&cmd_header;
> > > +
> > > +	msg[1].addr = client->addr;
> > > +	msg[1].flags = I2C_M_RD;
> > > +	msg[1].len = len;
> > > +	msg[1].buf = (char *)buf;
> > 
> > Again, please use u8 in place of char, as was done for the first
> > element.
> 
> OK.
> 
> > 
> > > +
> > > +	ret = i2c_transfer(client->adapter, msg, 2);
> > 
> > Please use ARRAY_SIZE(msg) above as you do below.
> > 
> > > +	if (ret != ARRAY_SIZE(msg)) {
> > > +		dev_err(&client->dev,
> > > +			"Failed reading usage %#x page %#x, error=%d\n",
> > > +			usage, page, ret);
> > > +		return -EIO;
> > > +	}
> > 
> > This check papers over negative error codes that may have been returned
> > by
> > i2c_transfer(). For ret < 0 you should return ret, and only return -EIO
> > for
> > 0 <= ret < ARRAY_SIZE(msg).
> > 
> > More importantly, however, if this device supports multiple transports
> > and
> > you expect SPI support can be added in the future, you really should use
> > regmap throughout in order to avoid ripping up this driver later.
> > 
> 
> I have a doubt on wether or not regmap can be used for SPI as there is some
> specific padding required for SPI.

You can still define your own read and write callbacks in the small SPI
driver, leaving generic regmap calls in the core driver.

> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8
> > > *buf, u16 len)
> > > +{
> > > +	struct axiom_data *ts = i2c_get_clientdata(client);
> > > +	struct axiom_cmd_header cmd_header;
> > > +	struct i2c_msg msg[2];
> > > +	int ret;
> > > +
> > > +	cmd_header.target_address =
> > > cpu_to_le16(usage_to_target_address(ts, usage, page, 0));
> > > +	cmd_header.length = cpu_to_le16(len);
> > > +	cmd_header.read = 0;
> > > +
> > > +	msg[0].addr = client->addr;
> > > +	msg[0].flags = 0;
> > > +	msg[0].len = sizeof(cmd_header);
> > > +	msg[0].buf = (u8 *)&cmd_header;
> > > +
> > > +	msg[1].addr = client->addr;
> > > +	msg[1].flags = 0;
> > > +	msg[1].len = len;
> > > +	msg[1].buf = (char *)buf;
> > > +
> > > +	ret = i2c_transfer(client->adapter, msg, 2);
> > > +	if (ret < 0) {
> > > +		dev_err(&client->dev,
> > > +			"Failed to write usage %#x page %#x, error=%d\n", usage,
> > > +			page, ret);
> > > +		return ret;
> > > +	}
> > 
> > The error handling between your read and write wrappers is inconsistent;
> > please see my comment above.
> > 
> > Is there any reason i2c_master_send() cannot work here? I'm guessing the
> > controller needs a repeated start in between the two messages?
> > 
> 
> Yes reads requires repeated starts between each messages.
> 
> For writes I could still use i2c_master_send() but what makes it more
> relevant here ?

The code can be a bit smaller in terms of lines with the header and payload
copied into a small buffer and written with i2c_master_send(), but this is
fine too.

[...]

> 
> > For these kind of special requirements, it's helpful to add some
> > comments
> > as to why the HW calls for additional housekeeping.
> > 
> 
> OK.
> 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Decodes and populates the local Usage Table.
> > > + * Given a buffer of data read from page 1 onwards of u31 from an
> > > aXiom
> > > + * device.
> > > + */
> > 
> > What is a usage table? These comments aren't helpful unless some of the
> > underlying concepts are defined as well.
> 
> It's a set of registers regrouped in categories (data, configuration, device
> and report).
> 
> I'll try to clarify it.

ACK, thanks.

[...]

> > > +/* Rebaseline the touchscreen, effectively zero-ing it */
> > 
> > What does it mean to rebaseline the touchscreen? I'm guessing it means
> > to null out or normalize pressure? Please consider a less colloquialized
> > function name.
> > 
> > Out of curiousity, what happens if the user's hand happens to be on the
> > touch surface at the time you call axiom_rebaseline()? Does the device
> > recover on its own?
> 
> This indeed force the controller to measure a new capacitance by zeoring it,
> I don't really know if it's harmful, yet the documentation says rebaseline
> is
> for tuning or debug purpose.
> 
> I believe this is done for testing the communication.

ACK, thanks.

> 
> > 
> > > +static int axiom_rebaseline(struct axiom_data *ts)
> > > +{
> > > +	char buffer[8] = {};
> > 
> > Are you expecting each element to be initialized to zero?
> 
> Yes.

ACK, I merely had not seen this method before.

> 
> > 
> > > +
> > > +	buffer[0] = AXIOM_REBASELINE_CMD;
> > > +
> > > +	return axiom_i2c_write(ts->client, AXIOM_REPORT_USAGE_ID, 0,
> > > buffer, sizeof(buffer));
> > > +}
> > > +
> > > +static int axiom_i2c_probe(struct i2c_client *client)
> > > +{
> > > +	struct device *dev = &client->dev;
> > > +	struct input_dev *input_dev;
> > > +	struct axiom_data *ts;
> > > +	int ret;
> > > +	int target;
> > > +
> > > +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > > +	if (!ts)
> > > +		return -ENOMEM;
> > > +
> > > +	ts->client = client;
> > > +	i2c_set_clientdata(client, ts);
> > > +	ts->dev = dev;
> > > +
> > > +	ts->irq_gpio = devm_gpiod_get_optional(dev, "irq", GPIOD_IN);
> > > +	if (IS_ERR(ts->irq_gpio))
> > > +		return dev_err_probe(dev, PTR_ERR(ts->irq_gpio), "failed to get
> > > irq GPIO");
> > > +
> > > +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > > GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(ts->reset_gpio))
> > > +		return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get
> > > reset GPIO\n");
> > > +
> > > +	axiom_reset(ts->reset_gpio);
> > 
> > We shouldn't call axiom_reset() if reset_gpio is NULL. Even though the
> > calls to gpiod_set_value_cansleep() will bail safely, there is no reason
> > to make the CPU sleep for 100 ms if the device was not actually reset.
> > 
> > > +
> > > +	if (ts->irq_gpio) {
> > > +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > > +						axiom_irq, 0, dev_name(dev), ts);
> > 
> > Did you mean to set IRQF_ONESHOT?
> 
> No

OK, why not? This is a threaded handler and it's obviously not meant to be
reentrant. Why is this implementation different than any other driver with
a threaded handler? In case I have misunderstood, please let me know.

Kind regards,
Jeff LaBundy