Hi Krzysztof, On Fri, 12 Jul 2024 14:01:07 +0200, krzk@xxxxxxxxxx wrote: >On 12/07/2024 13:32, wangshuaijie@xxxxxxxxxx wrote: >> From: shuaijie wang <wangshuaijie@xxxxxxxxxx> >> >> 1. Modify the structure of the driver. >> 2. Change the style of the driver's comments. >> 3. Remove unnecessary log printing. >> 4. Modify the function used for memory allocation. >> 5. Modify the driver registration process. >> 6. Remove the functionality related to updating firmware. >> 7. Change the input subsystem in the driver to the iio subsystem. >> 8. Modify the usage of the interrupt pin. > >I don't understand why do you put some sort of changelog into commit >msg. Please read submitting patches. > The patch for v4 will fix these issues. >> >> Signed-off-by: shuaijie wang <wangshuaijie@xxxxxxxxxx> >> --- > >Please use subject prefixes matching the subsystem. You can get them for >example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory >your patch is touching. For bindings, the preferred subjects are >explained here: >https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters > >> drivers/iio/proximity/Kconfig | 10 + >> drivers/iio/proximity/Makefile | 2 + >> drivers/iio/proximity/aw9610x.c | 1150 ++++++++++ >> drivers/iio/proximity/aw963xx.c | 1371 ++++++++++++ >> drivers/iio/proximity/aw_sar.c | 1850 +++++++++++++++++ >> drivers/iio/proximity/aw_sar.h | 23 + >> drivers/iio/proximity/aw_sar_comm_interface.c | 550 +++++ >> drivers/iio/proximity/aw_sar_comm_interface.h | 172 ++ >> drivers/iio/proximity/aw_sar_type.h | 371 ++++ >> 9 files changed, 5499 insertions(+) >> create mode 100644 drivers/iio/proximity/aw9610x.c >> create mode 100644 drivers/iio/proximity/aw963xx.c >> create mode 100644 drivers/iio/proximity/aw_sar.c >> create mode 100644 drivers/iio/proximity/aw_sar.h >> create mode 100644 drivers/iio/proximity/aw_sar_comm_interface.c >> create mode 100644 drivers/iio/proximity/aw_sar_comm_interface.h >> create mode 100644 drivers/iio/proximity/aw_sar_type.h >> >> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig >> index 2ca3b0bc5eba..a60d3dc955b3 100644 >> --- a/drivers/iio/proximity/Kconfig >> +++ b/drivers/iio/proximity/Kconfig >> @@ -219,4 +219,14 @@ config VL53L0X_I2C >> To compile this driver as a module, choose M here: the >> module will be called vl53l0x-i2c. >> >> +config AWINIC_SAR >> + tristate "Awinic AW96XXX proximity sensor" >> + depends on I2C >> + help >> + Say Y here to build a driver for Awinic's AW96XXX capacitive >> + proximity sensor. >> + >> + To compile this driver as a module, choose M here: the >> + module will be called awinic_sar. >> + >> endmenu >> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile >> index f36598380446..d4bd9edd8362 100644 >> --- a/drivers/iio/proximity/Makefile >> +++ b/drivers/iio/proximity/Makefile >> @@ -21,4 +21,6 @@ obj-$(CONFIG_SX_COMMON) += sx_common.o >> obj-$(CONFIG_SX9500) += sx9500.o >> obj-$(CONFIG_VCNL3020) += vcnl3020.o >> obj-$(CONFIG_VL53L0X_I2C) += vl53l0x-i2c.o >> +obj-$(CONFIG_AWINIC_SAR) += awinic_sar.o >> +awinic_sar-objs := aw_sar_comm_interface.o aw_sar.o aw9610x.o aw963xx.o >> > > > >> + >> +static void aw_sar_power_deinit(struct aw_sar *p_sar) >> +{ >> + if (p_sar->power_enable) { >> + /* >> + * Turn off the power output. However, >> + * it may not be turned off immediately >> + * There are scenes where power sharing can exist >> + */ >> + regulator_disable(p_sar->vcc); >> + regulator_put(p_sar->vcc); >> + } >> +} >> + >> +static void aw_sar_power_enable(struct aw_sar *p_sar, bool on) >> +{ >> + int rc; >> + >> + if (on) { >> + rc = regulator_enable(p_sar->vcc); >> + if (rc) { >> + dev_err(p_sar->dev, "regulator_enable vol failed rc = %d", rc); > >Again example of ugly code. > The v4 version patch will delete related code. >> + } else { >> + p_sar->power_enable = AW_TRUE; > >NAK. > >All this driver is some ancient, downstream or user-space-generic-code. >Sorry, you already got such comment. > >First, your control of power seems like entire code is spaghetti. >Basically, your control flow is random, no functions know when they are >called. To solve this, you introduce "power_enable" so the functions can >figure out if they are called with power enabled or not. > >That's just crappy and spaghetti design. > >This redefinition of true and false is a cherry on top. DO NOT EVER send >such code. NEVER. > >You must clean up all such user-space/Windows/whatever you have there stuff. > The patch for v4 will fix these issues. >> + msleep(20); >> + } >> + } else { >> + rc = regulator_disable(p_sar->vcc); >> + if (rc) >> + dev_err(p_sar->dev, "regulator_disable vol failed rc = %d", rc); >> + else >> + p_sar->power_enable = AW_FALSE; >> + } >> +} >> + >> +static int regulator_is_get_voltage(struct aw_sar *p_sar) >> +{ >> + unsigned int cnt = 10; >> + int voltage_val; >> + >> + while (cnt--) { >> + voltage_val = regulator_get_voltage(p_sar->vcc); > >What is that? > >Did you just forgot to set proper ramp delays? > The v4 version patch will delete related code. >> + if (voltage_val >= AW_SAR_VCC_MIN_UV) >> + return 0; >> + mdelay(1); >> + } >> + /* Ensure that the chip initialization is completed */ >> + msleep(20); >> + >> + return -EINVAL; >> +} >> +/* AW_SAR_REGULATOR_POWER_ON end */ > > >... > >> +static int aw_sar_regulator_power(struct aw_sar *p_sar) >> +{ >> + struct aw_sar_dts_info *p_dts_info = &p_sar->dts_info; >> + int ret = 0; >> + >> + p_dts_info->use_regulator_flag = >> + of_property_read_bool(p_sar->i2c->dev.of_node, "awinic,regulator-power-supply"); >> + >> + /* Configure the use of regulator power supply in DTS */ >> + if (p_sar->dts_info.use_regulator_flag == true) { >> + ret = aw_sar_regulator_power_init(p_sar); >> + if (ret != 0) { >> + dev_err(p_sar->dev, "power init failed"); >> + return ret; >> + } >> + aw_sar_power_enable(p_sar, AW_TRUE); >> + ret = regulator_is_get_voltage(p_sar); >> + if (ret != 0) { >> + dev_err(p_sar->dev, "get_voltage failed"); >> + aw_sar_power_deinit(p_sar); >> + } >> + } >> + >> + return ret; >> +} >> + >> +static int aw_sar_get_chip_info(struct aw_sar *p_sar) >> +{ >> + int ret; >> + unsigned char i; >> + >> + for (i = 0; i < AW_SAR_DRIVER_MAX; i++) { >> + if (g_aw_sar_driver_list[i].p_who_am_i != NULL) { > >Sorry, this overall code is just ugly and with poor readability. >Variables like "g_aw_sar_driver_list" are just not helping. > >The driver is really huge for a "simple" proximity sensor, so I wonder >if this was somehow over-engineered or is not really simple, but quite >complex sensor. > >Anyway, huge driver with poor code is not helping to review. > > Thank you for your suggestion. I agree with you that the software architecture is indeed too complex for proximity sensors. I will remove the compatibility code in the v4 version patch and only support the aw9610x series. >> + >> + >> +/* Drive logic entry */ >> +static int aw_sar_i2c_probe(struct i2c_client *i2c) >> +{ >> + struct iio_dev *sar_iio_dev; >> + struct aw_sar *p_sar; >> + int ret; >> + >> + if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_I2C)) { >> + pr_err("check_functionality failed!\n"); >> + return -EIO; >> + } >> + >> + sar_iio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*p_sar)); >> + if (!sar_iio_dev) >> + return -ENOMEM; >> + p_sar = iio_priv(sar_iio_dev); >> + p_sar->aw_iio_dev = sar_iio_dev; >> + p_sar->dev = &i2c->dev; >> + p_sar->i2c = i2c; >> + i2c_set_clientdata(i2c, p_sar); >> + >> + /* 1.Judge whether to use regular power supply. If yes, supply power */ >> + ret = aw_sar_regulator_power(p_sar); >> + if (ret != 0) { >> + dev_err(&i2c->dev, "regulator_power error!"); >> + return ret; >> + } >> + >> + /* 2.Get chip initialization resources */ >> + ret = aw_sar_get_chip_info(p_sar); >> + if (ret != 0) { >> + dev_err(&i2c->dev, "chip_init error!"); > >Not much improved. > > ><form letter> >This is a friendly reminder during the review process. > >It seems my or other reviewer's previous comments were not fully >addressed. Maybe the feedback got lost between the quotes, maybe you >just forgot to apply it. Please go back to the previous discussion and >either implement all requested changes or keep discussing them. > >Thank you. ></form letter> > The v4 version patch will delete related code. >> + >> +static const struct dev_pm_ops aw_sar_pm_ops = { >> + .suspend = aw_sar_suspend, >> + .resume = aw_sar_resume, >> +}; >> + >> +static const struct of_device_id aw_sar_dt_match[] = { >> + { .compatible = "awinic,aw96103" }, >> + { .compatible = "awinic,aw96105" }, >> + { .compatible = "awinic,aw96303" }, >> + { .compatible = "awinic,aw96305" }, >> + { .compatible = "awinic,aw96308" }, >> + { }, >> +}; >> + >> +static const struct i2c_device_id aw_sar_i2c_id[] = { >> + { AW_SAR_I2C_NAME, 0 }, > >Having device_id tables not in sync is usually bad sign. Why do you need >i2c_device_id in the first place? > The patch for v4 will fix these issues. >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(i2c, aw_sar_i2c_id); >> + >> +static struct i2c_driver aw_sar_i2c_driver = { >> + .driver = { >> + .name = AW_SAR_I2C_NAME, >> + .of_match_table = aw_sar_dt_match, >> + .pm = &aw_sar_pm_ops, >> + }, >> + .probe = aw_sar_i2c_probe, >> + .remove = aw_sar_i2c_remove, >> + .shutdown = aw_sar_i2c_shutdown, >> + .id_table = aw_sar_i2c_id, >> +}; >> +module_i2c_driver(aw_sar_i2c_driver); >> + >> +MODULE_DESCRIPTION("AWINIC SAR Driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_IMPORT_NS(AWINIC_PROX); > > > >Best regards, >Krzysztof Thank you for your response. I will optimize the code in the next version to make it look more concise. Kind regards, Wang Shuaijie
