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

From: Krzysztof Kozlowski
Date: Wed Jun 05 2024 - 06:29:19 EST

Next message: Chengming Zhou: "Re: [PATCH] block: fix request.queuelist usage in flush"
Previous message: Jesper Dangaard Brouer: "Re: [PATCH v3 net-next 14/15] net: Reference bpf_redirect_info via task_struct on PREEMPT_RT."
In reply to: wangshuaijie: "[PATCH V2 4/5] Add aw963xx series related interfaces to the aw_sar driver."
Next in thread: wangshuaijie: "[PATCH V2 2/5] Add universal interface for the aw_sar driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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?

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

Next message: Chengming Zhou: "Re: [PATCH] block: fix request.queuelist usage in flush"
Previous message: Jesper Dangaard Brouer: "Re: [PATCH v3 net-next 14/15] net: Reference bpf_redirect_info via task_struct on PREEMPT_RT."
In reply to: wangshuaijie: "[PATCH V2 4/5] Add aw963xx series related interfaces to the aw_sar driver."
Next in thread: wangshuaijie: "[PATCH V2 2/5] Add universal interface for the aw_sar driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]