On 03/11/2016 05:28 PM, Linus Walleij wrote:

On Sun, Feb 28, 2016 at 12:50 AM, Jonathan Cameron [off-list ref] wrote:

quoted

quoted

+Analog Devices AD5592R/AD5593R DAC/ADC device driver
+
+Required properties for the AD5592R:
+     - compatible: Must be "adi,ad5592r"
+     - reg: SPI chip select number for the device
+     - spi-max-frequency: Max SPI frequency to use (< 30000000)
+     - spi-cpol: The AD5592R requires inverse clock polarity (CPOL) mode

If this should be a gpiochip, this shall also be reflected in the
device tree bindings and the example in the bindings by
stating gpio-controller; and #gpio-cells, referring to
the binding in gpio/gpio.txt so consumers can pick a GPIO
from this device.

Hi Linus,

Thanks for the feedback.

- Will add this to the documentation.

I haven't seen the original patch, please mail the next version
to me so I can take a look.

quoted

quoted

+config AD5592R_BASE
+     tristate
+
+config AD5592R
+     tristate "Analog Devices AD5592R ADC/DAC driver"
+     depends on SPI_MASTER
+     depends on OF
+     select AD5592R_BASE
+     help
+       Say yes here to build support for Analog Devices AD5592R
+       Digital to Analog / Analog to Digital Converter.
+
+       To compile this driver as a module, choose M here: the
+       module will be called ad5592r.
+
+config AD5593R
+     tristate "Analog Devices AD5593R ADC/DAC driver"
+     depends on I2C
+     depends on OF
+     select AD5592R_BASE
+     help
+       Say yes here to build support for Analog Devices AD5593R
+       Digital to Analog / Analog to Digital Converter.
+
+       To compile this driver as a module, choose M here: the
+       module will be called ad5593r.
+

I guess something here should select GPIOLIB
and depend of OF_GPIO

That reminds me - On my dev tree I removed to the depends on OF from 
kconfig. But I haven't rebased this commit into my for_upstream branch.

I tried to avoid any OF dependency in the driver by using the 
linux/property API.

I'll add select GPIOLIB - and gpio/Kconfig will select OF_GPIO if OF.

quoted

quoted

+#ifdef CONFIG_GPIOLIB

Naaaaah really? Just select GPIOLIB and get rid of ifdeffery.
It's cool to have gpios available.

Convinced me.

quoted

quoted

+static int ad5592r_gpio_request(struct gpio_chip *chip, unsigned offset)
+{
+     struct ad5592r_state *st = gpiochip_get_data(chip);
+
+     if (!(st->gpio_map & BIT(offset))) {
+             dev_err(st->dev, "GPIO %d is reserved by alternate function\n",
+                     offset);

This gpio_map looks a bit like pin control.

It might be overkill to use all of the pinctrl subsystem,
we have circumvented it in other places.

I think it's overkill too.

quoted

quoted

+             return -ENODEV;
+     }
+
+     if (offset >= chip->ngpio)
+             return -EINVAL;

gpiolib already guards against this I think.

Then I drop it

quoted

quoted

+static int ad5592r_gpio_init(struct ad5592r_state *st)
+{
+     st->gpiochip.label = dev_name(st->dev);
+     st->gpiochip.base = -1;
+     st->gpiochip.ngpio = 8;
+     st->gpiochip.parent = st->dev;
+     st->gpiochip.can_sleep = true;
+     st->gpiochip.direction_input = ad5592r_gpio_direction_input;
+     st->gpiochip.direction_output = ad5592r_gpio_direction_output;
+     st->gpiochip.get = ad5592r_gpio_get;
+     st->gpiochip.set = ad5592r_gpio_set;
+     st->gpiochip.request = ad5592r_gpio_request;
+     st->gpiochip.owner = THIS_MODULE;
+
+     mutex_init(&st->gpio_lock);
+
+     return gpiochip_add_data(&st->gpiochip, st);

The gpiolib should be fine with the of_node from the parent so
looks fine.

quoted

quoted

+             mutex_lock(&iio_dev->mlock);
+             st->ops->reg_write(st, AD5592R_REG_RESET, 0xdac);
+             mutex_unlock(&iio_dev->mlock);

What's that? (0xdac)? Clever magic?

Clever magic - to avoid accidental resets.

quoted

quoted

+             case CH_MODE_DAC_AND_ADC:
+                     dac |= BIT(i);
+                     adc |= BIT(i);
+                     break;
+
+             case CH_MODE_UNUSED_PULL_DOWN:
+                     pulldown |= BIT(i);
+                     break;
+
+             case CH_MODE_UNUSED_OUT_TRISTATE:
+                     tristate |= BIT(i);
+                     break;
+
+             case CH_MODE_UNUSED_OUT_LOW:
+                     st->gpio_out |= BIT(i);
+                     break;
+
+             case CH_MODE_UNUSED_OUT_HIGH:
+                     st->gpio_out |= BIT(i);
+                     st->gpio_val |= BIT(i);
+                     break;
+
+             case CH_MODE_GPIO_OPEN_DRAIN:
+                     open_drain |= BIT(i);

Deja-vu with include/linux/pinctrl/pinconf-generic.h

We call tristate "bias high impedance".

Yours,
Linus Walleij

-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif