lkml.org 
[lkml]   [2024]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v9 3/3] Input: Add TouchNetix axiom i2c touchscreen driver
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

\
 
 \ /
  Last update: 2024-04-19 10:33    [W:0.065 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site