linux-kernel - Re: [PATCH v9 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 18 Mar 2024 09:05:33 +0100
From: Kamel Bouhara <kamel.bouhara@...tlin.com>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Henrik Rydberg <rydberg@...math.org>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	Jeff LaBundy <jeff@...undy.com>,
	catalin.popescu@...ca-geosystems.com,
	mark.satterthwaite@...chnetix.com,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	Gregory Clement <gregory.clement@...tlin.com>,
	bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH v9 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

Le Wed, Mar 13, 2024 at 09:21:35PM +0100, Marco Felsch a écrit :
> Hi Kamel,
>

Hello Marco,

[...]

> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct input_dev *input_dev;
> > +	struct axiom_data *ts;
> > +	u32 poll_interval;
> > +	int target;
> > +	int error;
> > +
> > +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> > +	if (!ts)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, ts);
> > +	ts->client = client;
> > +	ts->dev = dev;
> > +
> > +	ts->regmap = devm_regmap_init_i2c(client, &axiom_i2c_regmap_config);
> > +	error = PTR_ERR_OR_ZERO(ts->regmap);
> > +	if (error) {
> > +		dev_err(dev, "Failed to initialize regmap: %d\n", error);
> > +		return error;
> > +	}
> > +
> > +	ts->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(ts->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(ts->reset_gpio), "failed to get reset GPIO\n");
> > +
> > +	if (ts->reset_gpio)
> > +		axiom_reset(ts->reset_gpio);
>
> This seems useless, since you doing an reset without enabling the power
> supply (below). I know there are systems which do have the supply always
> connected or for ACPI the supply is managed via firmware, but the driver
> should implement the correct logic and for DT/OF case this is not
> correct.
>
> > +
> > +	ts->vddi = devm_regulator_get_optional(dev, "vddi");
> > +	if (!IS_ERR(ts->vddi)) {
> > +		error = devm_regulator_get_enable(dev, "vddi");
>
> Regulators are ref counted and now you request the regulator twice. Also
> the regulator is not optional, it is required for the device to work.
> Same applies to the vdda below.
>

While it is true most of the time, it occurs that for x86 based boards,
adding a regulator entirely is not always possible.

In our particular case, the I2C controller for this touchscreen is
behind a CPLD (aka embedded controller) so I have no direct access to
the I2C controller and it isn't described in the ACPI table.

In a normal case, I would use ACPI override to pass regulator
properties, but here it's not possible.

Having a CPLD exposing this kind of controller is quite common on x86
based boards. So, we need a way to support the case when a regulator
can't be described. The optional regulator looked like a good option,
but if you have a better alternative, I am open to considering it.

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com