Hi! Thanks for reviewing the patch. I'll also chime in. We were working on an ADIS device together with Antti so I've read some of the relevant code, documentation and datasheets. On Wed, 7 Jul 2021 08:36:47 +0000, "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > Thanks for the patch. Forcing the device reset was intentional > (thus the GPIO_ASIS). But what Lars is suggesting is a good idea > and a neat improvement here. I don't understand what you mean. The GPIO consumer documentation[0] states that if GPIO_ASIS is used, the pin direction must be set in driver code later. AFAICT that doesn't happen. If a pin was defined as input by default by the manufacturer, I don't think there's a way to make an ADIS device work with RST on it without patching the driver. Device trees couldn't be used to do that IIRC. I.e. this patch is needed so the device reset works. On Wed, 7 Jul 2021 10:18:57 +0200, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > How about requesting it as GPIOD_OUT_HIGH and removing the > gpiod_set_value_cansleep(gpio, 1) to avoid unnecessary toggling of the pin. GPIOD_OUT_LOW and GPIOD_OUT_HIGH have different semantics.[1] Is setting the pin to use wrong semantics to save one line of code and possibly toggle the pin one time less worth it? (The ADIS devices whose datasheets I've read have the RST pin as active low, ie. GPIOD_OUT_LOW is semantically correct.) If we really want that, I think the better choice is to use GPIO_ASIS, gpiod_direction_output and gpiod_set_raw_value_cansleep and skip semantically describing the pin altogether. Hannu [0]: https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html > GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must > be set later with one of the dedicated functions. [1]: https://www.kernel.org/doc/html/latest/driver-api/gpio/consumer.html#the-active-low-and-open-drain-semantics
