Re: [PATCH V3 2/2] Add support for Awinic proximity sensor

[wangshuaijie]

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