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