Hi, > From: Hannu Hartikainen <hannu@xxxxxxx> > Sent: Wednesday, July 7, 2021 1:53 PM > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Antti Keränen > <detegr@rbx.email>; linux-iio@xxxxxxxxxxxxxxx > Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; Hennerich, Michael > <Michael.Hennerich@xxxxxxxxxx>; Jonathan Cameron > <jic23@xxxxxxxxxx> > Subject: RE: [RESEND PATCH] iio: adis: set GPIO reset pin direction > > 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. Yes, you're right. This is pretty much assuming that the pin is already an output one. Though you can typically make sure that the pin will be configured as an output one (through pinctrl) which is what we are doing in the devicetree overlays for the adis16475 device for example (in ADI trees). > 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.) Well, AFAIK all the ADIS devices so far have the RST pin active low so this a very fair assumption to make and use GPIOD_OUT_HIGH. It can always be changed afterwards or even add a flag to the adis_data structure... > 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. Yes, we want to make sure a reset is done on the device. So, personally, I'm fine with either approach. Either using GPIOD_OUT_HIGH or GPIO_ASIS with the extra 'gpiod_direction_output()'... - Nuno Sá
