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

[Joel Selvaraj]

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@xxxxxxxxx>
> >
> > ---
> >   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