On 19/12/2024 19:32, Vladimir Zapolskiy wrote:

quoted

quoted

quoted

+        rst-pins {
+            pins = "gpio78";
+            function = "gpio";
+            drive-strength = <2>;
+            bias-pull-down;
+            output-low;
+        };

I have doubts that it's proper to embed a reset gpio into driver's
pinctrl suspend/resume power management.

Konrad, can you please confirm that it's really accepted?

I'd rather ask to remove this reset pin control.

There's certainly some appearances of this in the tree.

You could make the argument that it makes sense to prevent 
misconfiguration
(i.e. the bootloader may set the pin in input mode), but then the counter
argument is that the (Linux) gpiod APIs request OUT_LOW/HIGH, and we 
would
expect that the driver uses that if the GPIO is requested through
e.g. reset-gpios.

I'm not particularly sure what to recommend here. Krzysztof?

I'm worried by a possibility that a device reset/shutdown control GPIO 
could
be turned off by entering the "sleep" pinctrl setup. If a particular 
GPIO/pin
is off, is it still continuously functional as a control GPIO of some 
device?
I believe it is not anymore in general, this is my concern here.

I agree for this particular case that rst-pin should be excised.

- RST is an active low signal, which is typically _pulsed_ for a period
   when the sensor is powered to trigger a reset in the state machine of
   the sensor

- What is the use-case of pulling RST down the GPIO in suspend ?
   I'd remove the output-low though it should make no difference as
   the sensor regulators will be off.

- MCLK I think should have a suspend state specified or at least
   I can't think of a good reason right now why what I see here is wrong.

For the default state this patch disables the GPIO pull down bias, which 
to me seems logical and correct.

TBH I don't have a big concern about the RST pin in reset because the 
regulators will be off.

---
bod