Message ID | 20240601-nvt-ts-devicetree-regulator-support-v4-3-e0c0174464c4@gmail.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | novatek-nvt-ts: add support for NT36672A touchscreen | expand |
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. > 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, >
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, >> >
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; 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; + 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) { @@ -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); + if (error) { + dev_err(dev, "failed to disable regulators: %d\n", error); + return error; + } width = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_WIDTH]); height = get_unaligned_be16(&data->buf[NVT_TS_PARAMS_HEIGHT]); @@ -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,