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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof,

On Wed, 5 Jun 2024 12:29:06, krzk@xxxxxxxxxx wrote:
>On 05/06/2024 11:11, wangshuaijie@xxxxxxxxxx wrote:
>> From: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>> 
>> Signed-off-by: shuaijie wang <wangshuaijie@xxxxxxxxxx>
>> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
>> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>> | Reported-by: Dan Carpenter <error27@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx>
>> + *
>> + * 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?
>

The patch for v3 will fix these issues.

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

The patch for v3 will fix these issues.

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

The patch for v3 will fix these issues.

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

The register configuration file contains a chip name string, which needs
to be compared with chip_name during the validation of the register
configuration file.

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

The patch for v3 will fix these issues.

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

Kind regards,
Wang Shuaijie



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux