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

[Marek Vasut <marex@denx.de>]

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).

>> +	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.

[...]

>> +	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.

[...]

>> +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.

[...]