Re: [PATCH V2 4/5] Add aw963xx series related interfaces to the aw_sar driver.

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/06/2024 11:11, wangshuaijie@awinic.com wrote:
> From: shuaijie wang <wangshuaijie@awinic.com>
> 
> Signed-off-by: shuaijie wang <wangshuaijie@awinic.com>
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Reported-by: Dan Carpenter <error27@gmail.com>
> ---
>  drivers/input/misc/aw_sar/aw963xx/aw963xx.c | 974 ++++++++++++++++++++
>  drivers/input/misc/aw_sar/aw963xx/aw963xx.h | 753 +++++++++++++++
>  2 files changed, 1727 insertions(+)
>  create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.c
>  create mode 100644 drivers/input/misc/aw_sar/aw963xx/aw963xx.h
> 
> diff --git a/drivers/input/misc/aw_sar/aw963xx/aw963xx.c b/drivers/input/misc/aw_sar/aw963xx/aw963xx.c
> new file mode 100644
> index 000000000000..7ce40174a089
> --- /dev/null
> +++ b/drivers/input/misc/aw_sar/aw963xx/aw963xx.c
> @@ -0,0 +1,974 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AWINIC sar sensor driver (aw963xx)
> + *
> + * Author: Shuaijie Wang<wangshuaijie@awinic.com>
> + *
> + * Copyright (c) 2024 awinic Technology CO., LTD
> + */
> +#include "aw963xx.h"
> +#include "../aw_sar.h"
> +
> +#define AW963XX_I2C_NAME "aw963xx_sar"
> +
> +static void aw963xx_set_cs_as_irq(struct aw_sar *p_sar, int flag);
> +static void aw963xx_get_ref_ch_enable(struct aw_sar *p_sar);
> +
> +static int32_t aw963xx_read_init_over_irq(void *load_bin_para)
> +{
> +	struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
> +	uint32_t cnt = 1000;
> +	uint32_t reg;
> +	int32_t ret;
> +
> +	while (cnt--) {
> +		ret = aw_sar_i2c_read(p_sar->i2c, REG_IRQSRC, &reg);
> +		if (ret != 0) {
> +			dev_err(p_sar->dev, "i2c error %d", ret);
> +			return ret;
> +		}
> +		if ((reg & 0x01) == 0x01) {
> +			aw_sar_i2c_read(p_sar->i2c, REG_FWVER, &reg);
> +			return 0;
> +		}
> +		mdelay(1);
> +	}
> +
> +	aw_sar_i2c_read(p_sar->i2c, REG_FWVER, &reg);
> +
> +	return -EINVAL;
> +}
> +
> +static void aw963xx_convert_little_endian_2_big_endian(struct aw_bin *aw_bin)
> +{
> +	uint32_t start_index = aw_bin->header_info[0].valid_data_addr;
> +	uint32_t fw_len = aw_bin->header_info[0].reg_num;
> +	uint32_t uints = fw_len / AW963XX_SRAM_UPDATE_ONE_UINT_SIZE;
> +	uint8_t tmp1;
> +	uint8_t tmp2;
> +	uint8_t tmp3;
> +	uint8_t tmp4;
> +	int i;
> +
> +	for (i = 0; i < uints; i++) {
> +		tmp1 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 3];
> +		tmp2 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 2];
> +		tmp3 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 1];
> +		tmp4 = aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE];
> +		aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE]     = tmp1;
> +		aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 1] = tmp2;
> +		aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 2] = tmp3;
> +		aw_bin->info.data[start_index + i * AW963XX_SRAM_UPDATE_ONE_UINT_SIZE + 3] = tmp4;
> +	}
> +}
> +
> +/**
> + * @aw963xx_sram_fill_not_wrote_area()
> + *         |----------------code ram-----------------|
> + *       0x2000                                    0x4fff
> + *         |--- app wrote here ---|--fill with 0xff--|
> + *
> + *         if the size of app is less than the size of code ram, the rest of the
> + *         ram is filled with 0xff.
> + * @load_bin_para
> + * @offset the rear addr of app
> + * @return int32_t
> + */
> +static int32_t aw963xx_sram_fill_not_wrote_area(void *load_bin_para, uint32_t offset)
> +{
> +	uint32_t last_pack_len = (AW963XX_SRAM_END_ADDR - offset) %
> +						AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
> +	uint32_t pack_cnt = last_pack_len == 0 ?
> +			((AW963XX_SRAM_END_ADDR - offset) / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) :
> +			((AW963XX_SRAM_END_ADDR - offset) / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) + 1;
> +	uint8_t buf[AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2] = { 0 };
> +	struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
> +	uint32_t download_addr_with_ofst;
> +	uint8_t *r_buf;
> +	int32_t ret;
> +	uint32_t i;
> +
> +	r_buf = devm_kzalloc(p_sar->dev, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE, GFP_KERNEL);
> +	if (!r_buf)
> +		return -ENOMEM;
> +
> +	memset(buf, 0xff, sizeof(buf));
> +	for (i = 0; i < pack_cnt; i++) {
> +		memset(r_buf, 0, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
> +		download_addr_with_ofst = offset + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
> +		buf[0] = (uint8_t)(download_addr_with_ofst >> OFFSET_BIT_8);
> +		buf[1] = (uint8_t)(download_addr_with_ofst);
> +		if (i != (pack_cnt - 1)) {
> +			ret = aw_sar_i2c_write_seq(p_sar->i2c, buf,
> +					AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
> +				devm_kfree(p_sar->dev, r_buf);
> +				return ret;
> +			}
> +			ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf,
> +					AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
> +				devm_kfree(p_sar->dev, r_buf);
> +				return ret;
> +			}
> +			if (memcmp(&buf[2], r_buf, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) != 0) {
> +				dev_err(p_sar->dev, "read is not equal to write ");
> +				devm_kfree(p_sar->dev, r_buf);
> +				return -EINVAL;
> +			}
> +		} else {
> +			ret = aw_sar_i2c_write_seq(p_sar->i2c, buf, last_pack_len + 2);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
> +				devm_kfree(p_sar->dev, r_buf);
> +				return ret;
> +			}
> +			ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf, last_pack_len);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
> +				devm_kfree(p_sar->dev, r_buf);
> +				return ret;
> +			}
> +			if (memcmp(&buf[2], r_buf, last_pack_len) != 0) {
> +				dev_err(p_sar->dev, "read is not equal to write ");
> +				devm_kfree(p_sar->dev, r_buf);
> +				return -EINVAL;
> +			}
> +		}
> +	}
> +
> +	devm_kfree(p_sar->dev, r_buf);
> +
> +	return 0;
> +}
> +
> +static int32_t aw963xx_sram_data_write(struct aw_bin *aw_bin, void *load_bin_para)
> +{
> +	uint8_t buf[AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2] = { 0 };
> +	uint32_t start_index = aw_bin->header_info[0].valid_data_addr;
> +	uint32_t fw_bin_version = aw_bin->header_info[0].app_version;
> +	uint32_t download_addr = AW963XX_RAM_START_ADDR;
> +	uint32_t fw_len = aw_bin->header_info[0].reg_num;
> +	uint32_t last_pack_len = fw_len % AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
> +	struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
> +	uint32_t download_addr_with_ofst = 0;
> +	uint32_t pack_cnt;
> +	uint8_t *r_buf;
> +	int32_t ret = -EINVAL;
> +	uint32_t i;
> +
> +	r_buf = devm_kzalloc(p_sar->dev, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE, GFP_KERNEL);
> +	if (!r_buf)
> +		return -ENOMEM;
> +
> +	pack_cnt = ((fw_len % AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) == 0) ?
> +			(fw_len / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) :
> +			(fw_len / AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) + 1;
> +
> +	dev_info(p_sar->dev, "fw_bin_version = 0x%x", fw_bin_version);
> +	for (i = 0; i < pack_cnt; i++) {
> +		memset(r_buf, 0, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
> +		download_addr_with_ofst = download_addr + i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE;
> +		buf[0] = (uint8_t)(download_addr_with_ofst >> OFFSET_BIT_8);
> +		buf[1] = (uint8_t)(download_addr_with_ofst);
> +		if (i != (pack_cnt - 1)) {
> +			memcpy(&buf[2], &aw_bin->info.data[start_index +
> +					i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE],
> +					AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
> +			ret = aw_sar_i2c_write_seq(p_sar->i2c, buf,
> +					AW963XX_SRAM_UPDATE_ONE_PACK_SIZE + 2);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
> +				goto err_out;
> +			}
> +			ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf,
> +					AW963XX_SRAM_UPDATE_ONE_PACK_SIZE);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
> +				goto err_out;
> +			}
> +			if (memcmp(&buf[2], r_buf, AW963XX_SRAM_UPDATE_ONE_PACK_SIZE) != 0) {
> +				dev_err(p_sar->dev, "read is not equal to write ");
> +				ret = -EIO;
> +				goto err_out;
> +			}
> +		} else { // last pack process
> +			memcpy(&buf[2], &aw_bin->info.data[start_index +
> +					i * AW963XX_SRAM_UPDATE_ONE_PACK_SIZE], last_pack_len);
> +			ret = aw_sar_i2c_write_seq(p_sar->i2c, buf, last_pack_len + 2);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, write_seq error!", i);
> +				goto err_out;
> +			}
> +			ret = aw_sar_i2c_read_seq(p_sar->i2c, buf, 2, r_buf, last_pack_len);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, read_seq error!", i);
> +				goto err_out;
> +			}
> +			if (memcmp(&buf[2], r_buf, last_pack_len) != 0) {
> +				dev_err(p_sar->dev, "read is not equal to write ");
> +				ret = -EIO;
> +				goto err_out;
> +			}
> +			/* fill 0xff in the area that not worte. */
> +			ret = aw963xx_sram_fill_not_wrote_area(load_bin_para,
> +					download_addr_with_ofst + last_pack_len);
> +			if (ret != 0) {
> +				dev_err(p_sar->dev, "cnt%d, sram_fill_not_wrote_area error!", i);
> +				goto err_out;
> +			}
> +		}
> +	}
> +
> +err_out:
> +	devm_kfree(p_sar->dev, r_buf);

Why do you use managed interface?

> +
> +	return ret;
> +}
> +
> +static int32_t aw963xx_update_firmware(struct aw_bin *aw_bin, void *load_bin_para)
> +{
> +	struct aw_sar *p_sar = (struct aw_sar *)load_bin_para;
> +	struct aw963xx *aw963xx = (struct aw963xx *)p_sar->priv_data;
> +	struct i2c_client *i2c = p_sar->i2c;
> +	int32_t ret;
> +
> +	if (aw963xx->start_mode == AW963XX_ROM_MODE) {
> +		dev_info(p_sar->dev, "no need to update fw.");
> +		return 0;
> +	}
> +
> +	//step1: close coderam shutdown mode

Plaese fix your style to be consistent. There is a space after //.
Always, so fix all your patches.



...

> +
> +int32_t aw963xx_check_chipid(void *data)
> +{
> +	struct aw_sar *p_sar = (struct aw_sar *)data;
> +	uint32_t reg_val;
> +	int32_t ret;
> +
> +	if (!p_sar)
> +		return -EINVAL;
> +
> +	ret = aw_sar_i2c_read(p_sar->i2c, REG_CHIP_ID0, &reg_val);
> +	if (ret < 0) {
> +		dev_err(p_sar->dev, "read CHIP ID failed: %d", ret);
> +		return ret;
> +	}
> +
> +	switch (reg_val) {
> +	case AW96303_CHIP_ID:
> +		dev_info(p_sar->dev, "aw96303 detected, 0x%04x", reg_val);

Your driver is quite noisy. Reduce the severity of informational
messages, because driver should be quiet on success.

I don't understand why even having dev_info in 5 places instead of one
place.

> +		memcpy(p_sar->chip_name, AW96303, 8);
> +		ret = 0;
> +		break;
> +	case AW96305_CHIP_ID:
> +		dev_info(p_sar->dev, "aw96305 detected, 0x%04x", reg_val);
> +		memcpy(p_sar->chip_name, AW96305, 8);
> +		ret = 0;
> +		break;
> +	case AW96305BFOR_CHIP_ID:
> +		dev_info(p_sar->dev, "aw96305bfor detected, 0x%04x", reg_val);
> +		memcpy(p_sar->chip_name, AW96305BFOR, 8);
> +		ret = 0;
> +		break;
> +	case AW96308_CHIP_ID:
> +		dev_info(p_sar->dev, "aw96308 detected, 0x%04x", reg_val);
> +		memcpy(p_sar->chip_name, AW96308, 8);
> +		ret = 0;
> +		break;
> +	case AW96310_CHIP_ID:
> +		dev_info(p_sar->dev, "aw96310 detected, 0x%04x", reg_val);
> +		memcpy(p_sar->chip_name, AW96310, 8);

No, all these memcpy are just silly. You later compare strings instead
of comparing the detected chip id (integer).

> +		ret = 0;
> +		break;
> +	default:
> +		dev_info(p_sar->dev, "chip id error, 0x%04x", reg_val);
> +		ret =  -EIO;

Fix your style, just one space after =. This applies in multiple places.

> +		break;
> +	}
> +
> +	return ret;
> +}
> +

There are so many trivial issues in this driver that I think you should
start from huge cleanup from all these trivialities before sending to
review. You try to upstream a downstream, poor quality code. This is
always a pain. Instead you should take moderately recent driver, which
passed review, as a template and work on top of it with Linux coding
uniformed style.

Best regards,
Krzysztof