Hi, On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@xxxxxxxxxx> wrote: > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client) > ihid_elan->ops.power_up = elan_i2c_hid_power_up; > ihid_elan->ops.power_down = elan_i2c_hid_power_down; > > - /* Start out with reset asserted */ > - ihid_elan->reset_gpio = > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH); > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", > + GPIOD_ASIS); I'm not a huge fan of this part of the change. It feels like the GPIO state should be initialized by the probe function. Right before we call i2c_hid_core_probe() we should be in the state of "powered off" and the reset line should be in a consistent state. If "no_reset_on_power_off" then it should be de-asserted. Else it should be asserted. I think GPIOD_ASIS doesn't actually do anything useful for you, right? i2c_hid_core_probe() will power on and the first thing that'll happen there is that the reset line will be unconditionally asserted. Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow able to maintain continuity of this GPIO line from the BIOS state to the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS" property before because I've always wanted the ability to have GPIOs that could more seamlessly transition their firmware state to their kernel state. I don't think the API actually allows it. The fact that GPIO regulators don't support this seamless transition (even though it would be an obvious feature to add) supports my theory that the API doesn't currently allow it. It may be possible to make something work on some implementations but I think it's not guaranteed. Specifically, the docs say: * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set later with one of the dedicated functions. So that means that you can't read the pin without making it an input (which might change the state if it was previously driving a value) and you can't write the pin without making it an output and choosing a value to set it to. Basically grabbing a pin with "asis" doesn't allow you to do anything with it--it just claims it and doesn't let anyone else have it. -Doug
