Hi, On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@xxxxxxxxxx> wrote: > > On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote: > > 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. > > First, the reset gpio will be set before probe() returns, just not > immediately when it is requested. > > [ Sure, your panel follower implementation may defer the actual probe of > the touchscreen even further but I think that's a design flaw in the > current implementation. ] > > Second, the device is not necessarily in the "powered off" state Logically, the driver treats it as being in "powered off" state, though. That's why the i2c-hid core makes the call to power it on. IMO we should strive to make it more of a consistent state, not less of one. > as the > driver leaves the power supplies in whatever state that the boot > firmware left them in. I guess it depends on the regulator. ;-) For GPIO-regulators they aren't in whatever state the boot firmware left them in. For non-GPIO regulators we (usually) do preserve the state that the boot firmware left them in. > Not immediately asserting reset and instead leaving it in the state that > the boot firmware left it in is also no different from what happens when > a probe function bails out before requesting the reset line. > > > 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. > > It avoids asserting reset before we need to and thus also avoid the need > to deassert it on early probe failures (e.g. if one of the regulator > lookups fails). I guess so, though I'm of the opinion that we should be robust against the state that firmware left things in. The firmware's job is to boot the kernel and make sure that the system is running in a safe/reliable way, not to optimize the power consumption of the board. If the firmware left the line configured as "output low" then you'd let that stand. If it's important for the line to be left in a certain state, isn't it better to make that explicit? Also note: if we really end up keeping GPIOD_ASIS, which I'm still not convinced is the right move, the docs seem to imply that you need to explicitly set a direction before using it. Your current patch doesn't do that. -Doug
