On Sun, Feb 28, 2016 at 12:50 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>> +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.
I haven't seen the original patch, please mail the next version
to me so I can take a look.
>> +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
>> +#ifdef CONFIG_GPIOLIB
Naaaaah really? Just select GPIOLIB and get rid of ifdeffery.
It's cool to have gpios available.
>> +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.
>> + return -ENODEV;
>> + }
>> +
>> + if (offset >= chip->ngpio)
>> + return -EINVAL;
gpiolib already guards against this I think.
>> +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.
>> + 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?
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
