| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240725124811.890843-1-wangshuaijie@awinic.com>
Date: Thu, 25 Jul 2024 12:48:11 +0000
From: wangshuaijie@...nic.com
To: krzk@...nel.org
Cc: conor+dt@...nel.org,
devicetree@...r.kernel.org,
jic23@...nel.org,
kangjiajun@...nic.com,
krzk+dt@...nel.org,
lars@...afoo.de,
linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org,
liweilei@...nic.com,
robh@...nel.org,
wangshuaijie@...nic.com,
waqar.hameed@...s.com
Subject: Re: [PATCH V3 2/2] Add support for Awinic proximity sensor
Hi Krzysztof,
On Fri, 12 Jul 2024 14:01:07 +0200, krzk@...nel.org wrote:
>On 12/07/2024 13:32, wangshuaijie@...nic.com wrote:
>> From: shuaijie wang <wangshuaijie@...nic.com>
>>
>> 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@...nic.com>
>> ---
>
>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
Powered by blists - more mailing lists