| Message ID | 20240526-nvt-ts-devicetree-regulator-support-v3-3-aa88d10ccd9a@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | novatek-nvt-ts: add support for NT36672A touchscreen | expand |
Hi Joel, On 5/27/24 5:26 AM, 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 | 78 +++++++++++++++++++++++++++--- > 1 file changed, 72 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c > index 224fd112b25a9..7a82a1b09f9d5 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]; > }; > @@ -139,9 +143,23 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static void nvt_ts_disable_regulators(void *_data) > +{ > + struct nvt_ts_data *data = _data; > + > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > +} > + > 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; > + } > This is weird, you already enable the regulators in probe() and those get disabled again on remove() by the devm action you add. So there is no need to enable / disable the regulators on start/stop . If you want the regulators to only be enabled when the touchscreen is on then you should disable the regulators again in probe() after the nvt_ts_read_data() call there (and drop the devm action). Or if you want to leave the regulators on all the time then you can drop the enable + disable calls here. > enable_irq(data->client->irq); > gpiod_set_value_cansleep(data->reset_gpio, 0); > @@ -155,6 +173,7 @@ static void nvt_ts_stop(struct input_dev *dev) > > disable_irq(data->client->irq); > gpiod_set_value_cansleep(data->reset_gpio, 1); > + nvt_ts_disable_regulators(data); > } > > static int nvt_ts_suspend(struct device *dev) > @@ -199,9 +218,37 @@ 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\n"); > + return error; > + } > + > + error = devm_add_action_or_reset(dev, nvt_ts_disable_regulators, data); > + if (error) { > + dev_err(dev, "failed to install regulator disable handler\n"); > + return error; > + } > + If you are going to keep this devm action (see discussion above) then please switch from devm_regulator_bulk_get() + regulator_bulk_enable() to devm_regulator_bulk_get_enable() which gets and enables the regulators for you in one call. You can then drop the regulator_bulk_enable() + devm_add_action_or_reset() calls since devm_regulator_bulk_get_enable() takes care of both for you. > data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); > error = PTR_ERR_OR_ZERO(data->reset_gpio); > if (error) { > @@ -225,8 +272,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 +324,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 }, The i2c-subsystem will also match of compatible strings to i2c_device_ids by looking at the partof the compatible after the ',', so for a compatible of e.g. "novatek,nt36672a-ts" will match an i2c_device_id of "nt36672a-ts". So if you change these to lower-case: { "nt11205-ts", (unsigned long) &nvt_nt11205_ts_data }, { "nt36672a-ts", (unsigned long) &nvt_nt36672a_ts_data }, Then you can drop the nvt_ts_of_match table since that is not necessary then. Hmm I just realized that this will break module auto-loading though since that does require of modaliases . So maybe this is not such a good idea after all. Still switching to lowercase i2c_device_id-s would be good for consistency and you need to respin the patch-set for the regulator issue anyways. Regards, Hans > { } > }; > MODULE_DEVICE_TABLE(i2c, nvt_ts_i2c_id); > @@ -287,6 +352,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 de Goede, On 5/27/24 03:42, Hans de Goede wrote: > Hi Joel, > > On 5/27/24 5:26 AM, Joel Selvaraj via B4 Relay wrote: >> From: Joel Selvaraj <joelselvaraj.oss@gmail.com> >> >> --- >> drivers/input/touchscreen/novatek-nvt-ts.c | 78 +++++++++++++++++++++++++++--- >> 1 file changed, 72 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c >> index 224fd112b25a9..7a82a1b09f9d5 100644 >> --- a/drivers/input/touchscreen/novatek-nvt-ts.c >> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c >> @@ -139,9 +143,23 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id) >> return IRQ_HANDLED; >> } >> >> +static void nvt_ts_disable_regulators(void *_data) >> +{ >> + struct nvt_ts_data *data = _data; >> + >> + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); >> +} >> + >> 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; >> + } >> > > This is weird, you already enable the regulators in probe() and > those get disabled again on remove() by the devm action you add. > > So there is no need to enable / disable the regulators on start/stop . > > If you want the regulators to only be enabled when the touchscreen > is on then you should disable the regulators again in probe() > after the nvt_ts_read_data() call there (and drop the devm action). Yes, I want the regulators to be enabled only when the touchscreen is on/active. I will disable the regulators in probe and remove the devm action in v4. >> @@ -277,8 +324,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 }, > > The i2c-subsystem will also match of compatible strings to i2c_device_ids > by looking at the partof the compatible after the ',', so for a compatible > of e.g. "novatek,nt36672a-ts" will match an i2c_device_id of "nt36672a-ts". > > So if you change these to lower-case: > > { "nt11205-ts", (unsigned long) &nvt_nt11205_ts_data }, > { "nt36672a-ts", (unsigned long) &nvt_nt36672a_ts_data }, > > Then you can drop the nvt_ts_of_match table since that is not necessary > then. > > Hmm I just realized that this will break module auto-loading though since that > does require of modaliases . > > So maybe this is not such a good idea after all. Still switching to lowercase > i2c_device_id-s would be good for consistency and you need to respin > the patch-set for the regulator issue anyways. Ok. I will change it to lowercase i2c device id in v4. Regards, Joel Selvaraj
Hi, On 6/1/24 2:10 AM, Joel Selvaraj wrote: > Hi Hans de Goede, > > On 5/27/24 03:42, Hans de Goede wrote: >> Hi Joel, >> >> On 5/27/24 5:26 AM, Joel Selvaraj via B4 Relay wrote: >>> From: Joel Selvaraj <joelselvaraj.oss@gmail.com> >>> >>> --- >>> drivers/input/touchscreen/novatek-nvt-ts.c | 78 +++++++++++++++++++++++++++--- >>> 1 file changed, 72 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c >>> index 224fd112b25a9..7a82a1b09f9d5 100644 >>> --- a/drivers/input/touchscreen/novatek-nvt-ts.c >>> +++ b/drivers/input/touchscreen/novatek-nvt-ts.c >>> @@ -139,9 +143,23 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id) >>> return IRQ_HANDLED; >>> } >>> +static void nvt_ts_disable_regulators(void *_data) >>> +{ >>> + struct nvt_ts_data *data = _data; >>> + >>> + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); >>> +} >>> + >>> 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; >>> + } >>> >> >> This is weird, you already enable the regulators in probe() and >> those get disabled again on remove() by the devm action you add. >> >> So there is no need to enable / disable the regulators on start/stop . >> >> If you want the regulators to only be enabled when the touchscreen >> is on then you should disable the regulators again in probe() >> after the nvt_ts_read_data() call there (and drop the devm action). > > Yes, I want the regulators to be enabled only when the touchscreen is on/active. I will disable the regulators in probe and remove the devm action in v4. Sounds good. It is great to see people working on getting mainline kernels to work on phones like the work you are doing here. Thank you for doing this work! Regards, Hans
Hi Hans de Goede, On 6/1/24 09:07, Hans de Goede wrote: > Sounds good. > > It is great to see people working on getting mainline > kernels to work on phones like the work you are doing here. > Thank you for doing this work! Thanks! :) Regards, Joel Selvaraj
diff --git a/drivers/input/touchscreen/novatek-nvt-ts.c b/drivers/input/touchscreen/novatek-nvt-ts.c index 224fd112b25a9..7a82a1b09f9d5 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]; }; @@ -139,9 +143,23 @@ static irqreturn_t nvt_ts_irq(int irq, void *dev_id) return IRQ_HANDLED; } +static void nvt_ts_disable_regulators(void *_data) +{ + struct nvt_ts_data *data = _data; + + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); +} + 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 +173,7 @@ static void nvt_ts_stop(struct input_dev *dev) disable_irq(data->client->irq); gpiod_set_value_cansleep(data->reset_gpio, 1); + nvt_ts_disable_regulators(data); } static int nvt_ts_suspend(struct device *dev) @@ -199,9 +218,37 @@ 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\n"); + return error; + } + + error = devm_add_action_or_reset(dev, nvt_ts_disable_regulators, data); + if (error) { + dev_err(dev, "failed to install regulator disable handler\n"); + return error; + } + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); error = PTR_ERR_OR_ZERO(data->reset_gpio); if (error) { @@ -225,8 +272,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 +324,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 +352,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,