Re: [PATCH v2 3/3] Input: ili210x - add ili251x firmware update support

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, Aug 31, 2021 at 01:27:21AM +0200, Marek Vasut wrote:
> On 8/30/21 11:14 PM, Dmitry Torokhov wrote:
> 
> [...]
> 
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to request firmware %s, ret=%d\n", fwname, ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
> > > +	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
> > > +	 * once, copy them all into this buffer at the right locations, and then
> > > +	 * do all operations on this linear buffer.
> > > +	 */
> > > +	fw_buf = kcalloc(1, SZ_64K, GFP_KERNEL);
> > 
> > Why kcalloc and not kzalloc?
> 
> Because the firmware blob might be sparse (with gaps in it) and those gaps
> should be zeroed out instead of random data (see your question about the 32
> byte long memcpy() below (*) as well).

That is the exact purpose of kZalloc - to ZERO-out allocation (as
compared to kmalloc() which leaves memory uninitialized).

> 
> > > +	if (!fw_buf) {
> > > +		ret = -ENOMEM;
> > > +		goto err_alloc;
> > > +	}
> > > +
> > > +	rec = (const struct ihex_binrec *)fw->data;
> > > +	while (rec) {
> > > +		fw_addr = be32_to_cpu(rec->addr);
> > > +		fw_len = be16_to_cpu(rec->len);
> > > +
> > > +		if (fw_addr + fw_len > SZ_64K) {
> > > +			ret = -EFBIG;
> > > +			goto err_big;
> > > +		}
> > > +
> > > +		/* Find the last address before DF start address, that is AC end */
> > > +		if (fw_addr == 0xf000)
> > > +			*ac_end = fw_last_addr;
> > > +		fw_last_addr = fw_addr + fw_len;
> > > +
> > > +		memcpy(fw_buf + fw_addr, rec->data, fw_len);
> > > +		rec = ihex_next_binrec(rec);
> > > +	}
> 
> [...]
> 
> > > +static int ili251x_firmware_busy(struct i2c_client *client)
> > > +{
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	int ret, i = 0;
> > > +	u8 data;
> > > +
> > > +	do {
> > > +		/* The read_reg already contains suitable delay */
> > > +		ret = priv->chip->read_reg(client, 0x80, &data, 1);
> > 
> > Can we have symbolic name for this register (and others).
> 
> The name of this one isn't part of the example code, so ... I can call it
> something, but who knows what it really is.
> 
> I believe I did manage to find what the other registers are called already.

OK.

> 
> [...]
> 
> > > +	ret = ili251x_firmware_busy(client);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	for (fw_addr = start; fw_addr < end; fw_addr += 32) {
> > > +		fw_data[0] = REG_WRITE_DATA;
> > > +		memcpy(&(fw_data[1]), fwbuf + fw_addr, 32);
> > 
> > Is it guaranteed that we have enough data (32 bytes) and we will not
> > reach past the buffer?
> 
> Yes, see above (*). If the firmware blob entry were too short, this would
> pull in zeroes.
> 
> I tried iterating through the firmware file, but using linear buffer for the
> firmware results in far less convoluted code, and considering it is not
> performance critical, I think this is ok.

Could you drop a comment to that effect.

> 
> [...]
> 
> > > +static ssize_t ili210x_firmware_update_store(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     const char *buf, size_t count)
> > > +{
> > > +	struct i2c_client *client = to_i2c_client(dev);
> > > +	struct ili210x *priv = i2c_get_clientdata(client);
> > > +	char fwname[NAME_MAX];
> > > +	u16 ac_end, df_end;
> > > +	u8 *fwbuf;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	if (count >= NAME_MAX) {
> > > +		dev_err(dev, "File name too long\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	memcpy(fwname, buf, count);
> > > +	if (fwname[count - 1] == '\n')
> > > +		fwname[count - 1] = '\0';
> > > +	else
> > > +		fwname[count] = '\0';
> > 
> > I believe the practice is to use constant firmware name based on driver
> > or chip name. If there is desire to allow dynamic firmware name for
> > given device I think it should be handled at firmware loader core.
> 
> There are a couple of input devices which do similar thing, see e.g.:
> drivers/input/mouse/cyapa.c
> drivers/input/rmi4/rmi_f34.c
> 
> The ilitek firmware contains both firmware and calibration data, so you
> might end up with a usecase where you switch between different firmware
> blobs at runtime.
> 
> That's why it is implemented this way.

Right, and I'd rather not proliferate this any further. Can you drop the
filename support from this patch so it can be merged easily, and then we
can continue discussion on this topic separately?

Thanks.

-- 
Dmitry