Re: [PATCH v2 5/8] input: goodix: write configuration data to device

[Dmitry Torokhov]

On Tue, Jun 23, 2015 at 01:27:01PM +0000, Tirdea, Irina wrote:
> 
> 
> > -----Original Message-----
> > From: linux-input-owner@xxxxxxxxxxxxxxx [mailto:linux-input-owner@xxxxxxxxxxxxxxx] On Behalf Of Dmitry Torokhov
> > Sent: 09 June, 2015 21:14
> > To: Tirdea, Irina
> > Cc: Bastien Nocera; Mark Rutland; linux-input@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rob
> > Herring; Pawel Moll; Ian Campbell; Kumar Gala; Purdila, Octavian
> > Subject: Re: [PATCH v2 5/8] input: goodix: write configuration data to device
> > 
> > Hi Irina,
> > 
> 
> Hi Dmitry,
> 
> > On Mon, Jun 08, 2015 at 05:37:50PM +0300, Irina Tirdea wrote:
> > > Goodix devices can be configured by writing custom data to the device at
> > > init. The configuration data is read with request_firmware from
> > > "goodix_<id>_cfg.bin", where <id> is the product id read from the device
> > > (e.g.: goodix_911_cfg.bin for Goodix GT911, goodix_9271_cfg.bin for
> > > GT9271).
> > >
> > > The configuration information has a specific format described in the Goodix
> > > datasheet. It includes X/Y resolution, maximum supported touch points,
> > > interrupt flags, various sesitivity factors and settings for advanced
> > > features (like gesture recognition).
> > >
> > > This is based on Goodix datasheets for GT911 and GT9271 and on Goodix
> > > driver gt9xx.c for Android (publicly available in Android kernel
> > > trees for various devices).
> > 
> > I am afraid that just using request_firmware() in probe() is not the
> > best option as it may cause either errors or delays in
> > initialization of the driver is compiled into the kernel and tries to
> > initialize before filesystem with configuration file is mounted (and
> > firmware is not built into the kernel). We can either try switch to
> > request_firmware_nowait() or at least document that we need firmware at
> > boot.
> > 
> 
> The reason I did not use request_firmware_nowait() is that this configuration data
> includes information needed by probe (x/y resolution, interrupt trigger type, max
> touch num), used in goodix_read_config, goodix_ts_report_touch  and for irq flags
> when requesting the interrupt. I will document that we need this configuration
> firmware at boot in the commit message and add a comment in the code.
> Is there any other documentation I need to update?
> 
> > >
> > > Signed-off-by: Octavian Purdila <octavian.purdila@xxxxxxxxx>
> > > Signed-off-by: Irina Tirdea <irina.tirdea@xxxxxxxxx>
> > > ---
> > >  drivers/input/touchscreen/goodix.c | 128 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 128 insertions(+)
> > >
> > > diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> > > index c345eb7..1ce9278 100644
> > > --- a/drivers/input/touchscreen/goodix.c
> > > +++ b/drivers/input/touchscreen/goodix.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/interrupt.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/acpi.h>
> > > +#include <linux/firmware.h>
> > >  #include <linux/gpio.h>
> > >  #include <linux/of.h>
> > >  #include <asm/unaligned.h>
> > > @@ -95,6 +96,39 @@ static int goodix_i2c_read(struct i2c_client *client,
> > >  	return ret < 0 ? ret : (ret != ARRAY_SIZE(msgs) ? -EIO : 0);
> > >  }
> > >
> > > +/**
> > > + * goodix_i2c_write - write data to a register of the i2c slave device.
> > > + *
> > > + * @client: i2c device.
> > > + * @reg: the register to write to.
> > > + * @buf: raw data buffer to write.
> > > + * @len: length of the buffer to write
> > > + */
> > > +static int goodix_i2c_write(struct i2c_client *client, u16 reg, const u8 *buf,
> > > +			    unsigned len)
> > > +{
> > > +	u8 *addr_buf;
> > > +	struct i2c_msg msg;
> > > +	int ret;
> > > +
> > > +	addr_buf = kmalloc(len + 2, GFP_KERNEL);
> > > +	if (!addr_buf)
> > > +		return -ENOMEM;
> > > +
> > > +	addr_buf[0] = reg >> 8;
> > > +	addr_buf[1] = reg & 0xFF;
> > > +	memcpy(&addr_buf[2], buf, len);
> > > +
> > > +	msg.flags = 0;
> > > +	msg.addr = client->addr;
> > > +	msg.buf = addr_buf;
> > > +	msg.len = len + 2;
> > > +
> > > +	ret = i2c_transfer(client->adapter, &msg, 1);
> > > +	kfree(addr_buf);
> > > +	return ret < 0 ? ret : (ret != 1 ? -EIO : 0);
> > > +}
> > > +
> > >  static int goodix_ts_read_input_report(struct goodix_ts_data *ts, u8 *data)
> > >  {
> > >  	int touch_num;
> > > @@ -192,6 +226,95 @@ static irqreturn_t goodix_ts_irq_handler(int irq, void *dev_id)
> > >  	return IRQ_HANDLED;
> > >  }
> > >
> > > +/**
> > > + * goodix_check_cfg - Checks if config buffer is valid
> > > + *
> > > + * @ts: goodix_ts_data pointer
> > > + * @fw: firmware config data
> > > + */
> > > +static int goodix_check_cfg(struct goodix_ts_data *ts,
> > > +			    const struct firmware *fw)
> > > +{
> > > +	int i, raw_cfg_len;
> > > +	u8 check_sum = 0;
> > > +
> > > +	if (fw->size > GOODIX_CONFIG_MAX_LENGTH) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The length of the config buffer array is not correct");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	raw_cfg_len = fw->size - 2;
> > > +	for (i = 0; i < raw_cfg_len; i++)
> > > +		check_sum += fw->data[i];
> > > +	check_sum = (~check_sum) + 1;
> > > +	if (check_sum != fw->data[raw_cfg_len]) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The checksum of the config buffer array is not correct");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (fw->data[raw_cfg_len + 1] != 1) {
> > > +		dev_err(&ts->client->dev,
> > > +			"The Config_Fresh register needs to be set");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * goodix_send_cfg - Write device config
> > > + *
> > > + * @ts: goodix_ts_data pointer
> > > + * @id: product id read from device
> > > + */
> > > +static int goodix_send_cfg(struct goodix_ts_data *ts, u16 id)
> > > +{
> > > +	const struct firmware *fw = NULL;
> > > +	char *fw_name;
> > > +	int ret;
> > > +
> > > +	fw_name = kasprintf(GFP_KERNEL, "goodix_%d_cfg.bin", id);
> > > +	if (!fw_name)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = request_firmware(&fw, fw_name, &ts->client->dev);
> > > +	if (ret) {
> > > +		dev_err(&ts->client->dev, "Unable to open firmware %s\n",
> > > +			fw_name);
> > > +		goto err_free_fw_name;
> > 
> > That I think will cause driver to abort binding if config is not there.
> > Do we always need to load the config? Is configuration stored in NVRAM?
> > Maybe configuration should be only loaded when userspace requests it?
> > 
> 
> The device has a default configuration, but usually this does not match the platform
> needs. It is stored in volatile RAM, so we need to rewrite it after power off.
> From my tests, the default values are invalid if I do not write any config data
> (goodix_read_config will print "Invalid config, using defaults").
> 
> However, we should still be able to use the device with these defaults even
> if the config firmware is not there. I will just return 0 if firmware is not found.
> Since configuration needs to be written before configuring the input device and
> the interrupts, I don't think we can allow userspace to control it.

Well, my concern is that we currently have devices and DTSes that work
just fine without special config. How do they manage to do that? We need
to make sure that new changes do not break existing users.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/