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, ®); > + 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, ®); > + return 0; > + } > + mdelay(1); > + } > + > + aw_sar_i2c_read(p_sar->i2c, REG_FWVER, ®); > + > + 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, ®_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