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
