From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v2] iio: dac: Add support for the AD5592R/AD5593R ADCs/DACs Date: Fri, 11 Mar 2016 23:28:32 +0700 Message-ID: References: <1456407412-16218-1-git-send-email-michael.hennerich@analog.com> <56D1E1CB.6070704@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: Received: from mail-oi0-f43.google.com ([209.85.218.43]:34803 "EHLO mail-oi0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932775AbcCKQ2d (ORCPT ); Fri, 11 Mar 2016 11:28:33 -0500 Received: by mail-oi0-f43.google.com with SMTP id m82so89522403oif.1 for ; Fri, 11 Mar 2016 08:28:33 -0800 (PST) In-Reply-To: <56D1E1CB.6070704@kernel.org> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Jonathan Cameron Cc: Michael Hennerich , Lars-Peter Clausen , Hartmut Knaack , Paul Cercueil , Rob Herring , =?UTF-8?Q?Pawe=C5=82_Moll?= , Mark Rutland , "ijc+devicetree@hellion.org.uk" , "linux-iio@vger.kernel.org" , "devicetree@vger.kernel.org" , Alexandre Courbot , "linux-gpio@vger.kernel.org" On Sun, Feb 28, 2016 at 12:50 AM, Jonathan Cameron 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