On Wed, Apr 24, 2024 at 09:24:33AM -0700, Doug Anderson wrote: > 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. > > 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. That's not really true. The device is often in an undefined power state and we try to make sure that the hand over is as smooth as possible to avoid resetting displays and similar unnecessarily. The power-on sequence is what brings the device into a defined power state. > > 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. Even for GPIO regulators we have the "regulator-boot-on" devicetree property which is supposed to be set if the boot firmware has left a regulator on so that the regulator initialisation can preserve the state. > > 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. Agreed. > 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? As I pointed out above we already do this for any error paths before requesting the reset line. And I also don't think we need to worry too much about power consumption in case of errors. But there is one case I had not considered before, and that is your gpio regulator example but where the boot-on flag does not match the actual regulator state. If the supply is on and reset deasserted, but the regulator-boot-on flag is not set, then we want to make sure that reset is asserted before disabling the supply when requesting the regulator. > 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. You're right. It will work in my case because of the gpiolib open-drain implementation, but not generally. I'll add back the reset during early probe and add error handling for deasserting reset on machines like the X13s. On these, the touchscreen may now be reset a couple of times in case of probe deferrals, but device links should generally prevent that. Johan
