Re: [PATCH v4 3/3] Input: novatek-nvt-ts: add support for NT36672A touchscreen

[Joel Selvaraj <joelselvaraj.oss@gmail.com>]

Hi Hans,

On 6/1/24 12:07, Hans de Goede wrote:
> Hi Joel,
> 
> Thank you for the new version.
> 
> On 6/1/24 5:30 PM, Joel Selvaraj via B4 Relay wrote:
>> From: Joel Selvaraj <joelselvaraj.oss@gmail.com>
>>
>> Extend the novatek touchscreen driver to support NT36672A chip which
>> is found in phones like qcom/sdm845-xiaomi-beryllium-tianma.dts.
>> Added devicetree support for the driver and used i2c chip data to handle
>> the variation in chip id and wake type. Also added vcc and iovcc
>> regulators which are used to power the touchscreen hardware.
>>
>> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> ---
>>   drivers/input/touchscreen/novatek-nvt-ts.c | 70 +++++++++++++++++++++++++++---
>>   1 file changed, 64 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c
>> index 9bee3a0c122fb..c24c33f609eb8 100644
>> --- a/drivers/input/touchscreen/novatek-nvt-ts.c
>> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c
>> @@ -31,9 +31,6 @@
>>   #define NVT_TS_PARAMS_CHIP_ID		0x0e
>>   #define NVT_TS_PARAMS_SIZE		0x0f
>>   
>> -#define NVT_TS_SUPPORTED_WAKE_TYPE	0x05
>> -#define NVT_TS_SUPPORTED_CHIP_ID	0x05
>> -
>>   #define NVT_TS_MAX_TOUCHES		10
>>   #define NVT_TS_MAX_SIZE			4096
>>   
>> @@ -51,11 +48,18 @@ static const int nvt_ts_irq_type[4] = {
>>   	IRQF_TRIGGER_HIGH
>>   };
>>   
>> +struct nvt_ts_i2c_chip_data {
>> +	u8 wake_type;
>> +	u8 chip_id;
>> +};
>> +
>>   struct nvt_ts_data {
>>   	struct i2c_client *client;
>>   	struct input_dev *input;
>>   	struct gpio_desc *reset_gpio;
>> +	struct regulator_bulk_data regulators[2];
>>   	struct touchscreen_properties prop;
>> +	const struct nvt_ts_i2c_chip_data *chip;
> 
> Almost there. I have one remark which requires fixing below,
> so since a v5 will be necessary anyways I also spotted another
> small possible improvement:
> 
> Since you only use chip->wake_type and chip->chip_id
> inside probe() you can make this chip pointer a local
> variable in probe(). This saves having this stored
> on the kernel heap even though it is never used again.
> 

Thanks for your detailed explanation here and below. I will fix them in v5.

Regards,
Joel

>>   	int max_touches;
>>   	u8 buf[NVT_TS_TOUCH_SIZE * NVT_TS_MAX_TOUCHES];
>>   };
>> @@ -142,6 +146,13 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id)
>>   static int nvt_ts_start(struct input_dev *dev)
>>   {
>>   	struct nvt_ts_data *data = input_get_drvdata(dev);
>> +	int error;
>> +
>> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
>> +	if (error) {
>> +		dev_err(&data->client->dev, "failed to enable regulators\n");
>> +		return error;
>> +	}
>>   
>>   	enable_irq(data->client->irq);
>>   	gpiod_set_value_cansleep(data->reset_gpio, 0);
>> @@ -155,6 +166,7 @@ static void nvt_ts_stop(struct input_dev *dev)
>>   
>>   	disable_irq(data->client->irq);
>>   	gpiod_set_value_cansleep(data->reset_gpio, 1);
>> +	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>>   }
>>   
>>   static int nvt_ts_suspend(struct device *dev)
>> @@ -199,9 +211,31 @@ static int nvt_ts_probe(struct i2c_client *client)
>>   	if (!data)
>>   		return -ENOMEM;
>>   
>> +	data->chip = device_get_match_data(&client->dev);
>> +	if (!data->chip)
>> +		return -EINVAL;
>> +
> 
> As mentioned above instead of data->chip you can use a local
> "chip" variable here.
> 
>>   	data->client = client;
>>   	i2c_set_clientdata(client, data);
>>   
>> +	/*
>> +	 * VCC is the analog voltage supply
>> +	 * IOVCC is the digital voltage supply
>> +	 */
>> +	data->regulators[0].supply = "vcc";
>> +	data->regulators[1].supply = "iovcc";
>> +	error = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators), data->regulators);
>> +	if (error) {
>> +		dev_err(dev, "cannot get regulators: %d\n", error);
>> +		return error;
>> +	}
>> +
>> +	error = regulator_bulk_enable(ARRAY_SIZE(data->regulators), data->regulators);
>> +	if (error) {
>> +		dev_err(dev, "failed to enable regulators: %d\n", error);
>> +		return error;
>> +	}
>> +
>>   	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>>   	error = PTR_ERR_OR_ZERO(data->reset_gpio);
>>   	if (error) {
> 
> Almost there. You need to disable the regulators when probe fails to
> avoid an error from the regulator core about unbalanced enable/disable
> of the regulators when the devm framework releases them.
> 
> So you need to add a regulator_bulk_disable() call in
> the "if (error) {" branch here:
> 
>    	data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>    	error = PTR_ERR_OR_ZERO(data->reset_gpio);
>    	if (error) {
> 		regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> 		dev_err(dev, "failed to request reset GPIO: %d\n", error);
> 		return error;
> 	}
> 
> And ... (continued below)
> 
>> @@ -216,6 +250,11 @@ static int nvt_ts_probe(struct i2c_client *client)
>>   	gpiod_set_value_cansleep(data->reset_gpio, 1); /* Put back in reset */
>>   	if (error)
>>   		return error;
>> +	error = regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> 
> I would not error check this, just like how it is not error checked
> in void nvt_ts_stop() and then I would move it to above the error
> checking for the nvt_ts_read_data(...NVT_TS_PARAMETERS...), to avoid
> the need for an extra regulator_bulk_disable() call in the if (error)
> path for the nvt_ts_read_data() call.
> 
> So make the code look like this:
> 
>          error = nvt_ts_read_data(data->client, NVT_TS_PARAMETERS_START,
>                                   data->buf, NVT_TS_PARAMS_SIZE);
>          gpiod_set_value_cansleep(data->reset_gpio, 1); /* Put back in reset */
> 	regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>          if (error)
>                  return error;
> 
>          width  = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_WIDTH]);
>          height = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_HEIGHT]);
> 	...
> 
> This way you only need one extra regulator_bulk_disable() call for
> error-exit paths in the case of devm_gpiod_get(dev, "reset", ...)
> failing.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
>> @@ -225,8 +264,8 @@ static int nvt_ts_probe(struct i2c_client *client)
>>   	if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE ||
>>   	    data->max_touches > NVT_TS_MAX_TOUCHES ||
>>   	    irq_type >= ARRAY_SIZE(nvt_ts_irq_type) ||
>> -	    data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE ||
>> -	    data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) {
>> +	    data->buf[NVT_TS_PARAMS_WAKE_TYPE] != data->chip->wake_type ||
>> +	    data->buf[NVT_TS_PARAMS_CHIP_ID] != data->chip->chip_id) {
>>   		dev_err(dev, "Unsupported touchscreen parameters: %*ph\n",
>>   			NVT_TS_PARAMS_SIZE, data->buf);
>>   		return -EIO;
>> @@ -277,8 +316,26 @@ static int nvt_ts_probe(struct i2c_client *client)
>>   	return 0;
>>   }
>>   
>> +static const struct nvt_ts_i2c_chip_data nvt_nt11205_ts_data = {
>> +	.wake_type = 0x05,
>> +	.chip_id = 0x05,
>> +};
>> +
>> +static const struct nvt_ts_i2c_chip_data nvt_nt36672a_ts_data = {
>> +	.wake_type = 0x01,
>> +	.chip_id = 0x08,
>> +};
>> +
>> +static const struct of_device_id nvt_ts_of_match[] = {
>> +	{ .compatible = "novatek,nt11205-ts", .data = &nvt_nt11205_ts_data },
>> +	{ .compatible = "novatek,nt36672a-ts", .data = &nvt_nt36672a_ts_data },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, nvt_ts_of_match);
>> +
>>   static const struct i2c_device_id nvt_ts_i2c_id[] = {
>> -	{ "nt11205-ts" },
>> +	{ "nt11205-ts", (unsigned long) &nvt_nt11205_ts_data },
>> +	{ "nt36672a-ts", (unsigned long) &nvt_nt36672a_ts_data },
>>   	{ }
>>   };
>>   MODULE_DEVICE_TABLE(i2c, nvt_ts_i2c_id);
>> @@ -287,6 +344,7 @@ static struct i2c_driver nvt_ts_driver = {
>>   	.driver = {
>>   		.name	= "novatek-nvt-ts",
>>   		.pm	= pm_sleep_ptr(&nvt_ts_pm_ops),
>> +		.of_match_table = nvt_ts_of_match,
>>   	},
>>   	.probe = nvt_ts_probe,
>>   	.id_table = nvt_ts_i2c_id,
>>
>