Received-SPF: pass (google.com: domain of linux-kernel+bounces-151171-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
Date: Fri, 19 Apr 2024 10:33:21 +0200
From: Kamel BOUHARA <kamel.bouhara@bootlin.com>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>, Rob Herring
 <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Henrik Rydberg <rydberg@bitmath.org>, linux-input@vger.kernel.org,
 linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Jeff LaBundy
 <jeff@labundy.com>, catalin.popescu@leica-geosystems.com,
 mark.satterthwaite@touchnetix.com, Thomas Petazzoni
 <thomas.petazzoni@bootlin.com>, Gregory Clement
 <gregory.clement@bootlin.com>, bsp-development.geo@leica-geosystems.com
Subject: Re: [PATCH v9 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
In-Reply-To: <20240318080533.GA35033@tpx1.home>
References: <20240301103909.167923-1-kamel.bouhara@bootlin.com>
 <20240301103909.167923-4-kamel.bouhara@bootlin.com>
 <20240313202135.2lwgtu2z67ksh2tz@pengutronix.de>
 <20240318080533.GA35033@tpx1.home>
Message-ID: <2fdb461055af669d35b20f5d994c17c6@bootlin.com>
Organization: Bootlin
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit

Le 2024-03-18 09:05, Kamel Bouhara a écrit :
> Le Wed, Mar 13, 2024 at 09:21:35PM +0100, Marco Felsch a écrit :
>> Hi Kamel,
>> 
> 
> Hello Marco,
> 
> [...]
> 

Hello,

>> > +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.
> 

I actually confirmed this case is already handled in 
acpi_subsystem_init():
                 ...
»       »       /*
»       »        * If the system is using ACPI then we can be reasonably
»       »        * confident that any regulators are managed by the 
firmware
»       »        * so tell the regulator core it has everything it needs 
to
»       »        * know.
»       »        */
»       »       regulator_has_full_constraints();

Thanks Marco for the clue :) !

Regards,

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