From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD15FC282C7 for ; Sat, 26 Jan 2019 20:42:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 9672021872 for ; Sat, 26 Jan 2019 20:42:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1548535322; bh=V+K2tTS5n6G8J+ys+JczNogQmEH54aDiUpvRlq8CNSs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=xuVNbZD5yFot/LMeo9mQO0FsOct+FimzYRNTk75RT7JidNQl+D8e6kaN748QBOeHo nLL3j/Z6rx8u/1Sc2hYrdEjxkaUA/hfYktX9/L1O4WYFJKOtcJ9I3sBgdTNlY4ecIQ LALru6lcB7TIHeQhVnBU7fo5tY0aDwRXqPzuDcys= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726218AbfAZUmC (ORCPT ); Sat, 26 Jan 2019 15:42:02 -0500 Received: from mail.kernel.org ([198.145.29.99]:41920 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726180AbfAZUmB (ORCPT ); Sat, 26 Jan 2019 15:42:01 -0500 Received: from archlinux (cpc91196-cmbg18-2-0-cust659.5-4.cable.virginm.net [81.96.234.148]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 438882133D; Sat, 26 Jan 2019 20:41:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1548535320; bh=V+K2tTS5n6G8J+ys+JczNogQmEH54aDiUpvRlq8CNSs=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=OCpBoZVsRh5SypiX+9PLQiIBmC5mLE62GnXecvwK8wEVqPDciB4kvq7PgDUDmqhYy Fhdnwiv3JaKyvKCcVNB+QteTAlbc8nstJZYdymyXMP1WlTUk7tNa8I5WMynp8F0u7I CcbZRxztPvo4VGtlXJPLijN0Kp1DBra5qXX6o3+c= Date: Sat, 26 Jan 2019 20:41:55 +0000 From: Jonathan Cameron To: Ricardo Ribalda Delgado Cc: Alexandru Ardelean , LKML , alexandru.ardelean@analog.com, linux-iio@vger.kernel.org Subject: Re: [PATCH v2] iio:dac:ti-dac7612: Add driver for Texas Instruments DAC7612 Message-ID: <20190126204155.6fbb9512@archlinux> In-Reply-To: <20190125100055.13836-1-ricardo@ribalda.com> References: <20190125100055.13836-1-ricardo@ribalda.com> X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-iio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Fri, 25 Jan 2019 11:00:55 +0100 Ricardo Ribalda Delgado wrote: > It is a driver for Texas Instruments Dual, 12-Bit Serial Input > Digital-to-Analog Converter. > > Datasheet of this chip: > http://www.ti.com/lit/ds/sbas106/sbas106.pdf > > Signed-off-by: Ricardo Ribalda Delgado Hi Ricardo, Various bits and pieces inline. Jonathan > --- > > v2: Fix range > MAINTAINERS | 6 ++ > drivers/iio/dac/Kconfig | 10 +++ > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ti-dac7612.c | 169 +++++++++++++++++++++++++++++++++++ > 4 files changed, 186 insertions(+) > create mode 100644 drivers/iio/dac/ti-dac7612.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index d039f66a5cef..30ba5435906b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14877,6 +14877,12 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt > F: drivers/clk/keystone/sci-clk.c > F: drivers/reset/reset-ti-sci.c > > +Texas Instruments' DAC7612 DAC Driver > +M: Ricardo Ribalda > +L: linux-iio@vger.kernel.org > +S: Supported > +F: drivers/iio/dac/ti-dac7612.c > + > THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER > M: Hans Verkuil > L: linux-media@vger.kernel.org > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig > index f28daf67db6a..fbef9107acad 100644 > --- a/drivers/iio/dac/Kconfig > +++ b/drivers/iio/dac/Kconfig > @@ -375,6 +375,16 @@ config TI_DAC7311 > > If compiled as a module, it will be called ti-dac7311. > > +config TI_DAC7612 > + tristate "Texas Instruments 12-bit 2-channel DAC driver" > + depends on SPI_MASTER && GPIOLIB > + help > + Driver for the Texas Instruments DAC7612, DAC7612U, DAC7612UB > + The driver hand drive the load pin automatically, otherwise > + it needs to be toggled manually. Given the driver doesn't load without that pin, do we need to give the otherwise? I would drop this comment entirely. > + > + If compiled as a module, it will be called ti-dac7612. > + > config VF610_DAC > tristate "Vybrid vf610 DAC driver" > depends on OF > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index f0a37c93de8e..1369fa1d2f0e 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -41,4 +41,5 @@ obj-$(CONFIG_STM32_DAC) += stm32-dac.o > obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o > obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o > obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o > +obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o > obj-$(CONFIG_VF610_DAC) += vf610_dac.o > diff --git a/drivers/iio/dac/ti-dac7612.c b/drivers/iio/dac/ti-dac7612.c > new file mode 100644 > index 000000000000..278406f69e0c > --- /dev/null > +++ b/drivers/iio/dac/ti-dac7612.c > @@ -0,0 +1,169 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * DAC7612 Dual, 12-Bit Serial input Digital-to-Analog Converter > + * > + * Copyright 2019 Qtechnology A/S > + * 2019 Ricardo Ribalda > + * > + * Licensed under the GPL-2. > + */ > +#include > +#include > +#include > +#include > +#include > + > +#define NUM_CHANS 2 > +#define RESOLUTION 12 Please prefix any naming that is generic like this with the driver name. Avoids potential redefined clashes in the future. Where they are 'real numbers' rather than Magic numbers I would generally look at using the actual number rather than a define. > + > +struct dac7612 { > + struct spi_device *spi; > + struct gpio_desc *nld; > + uint16_t cache[NUM_CHANS]; > +}; > + > +static int dac7612_cmd_single(struct dac7612 *priv, int channel, u16 val) > +{ > + uint8_t buffer[2]; > + int ret; > + > + if (channel >= NUM_CHANS) Is there any way this can happen? Seems a little over paranoid. > + return -EINVAL; > + > + buffer[0] = BIT(5) | (channel << 4) | (val >> 8); BIT(5) is a magic number so should probably come from a define as should the shifts of the other parts. > + buffer[1] = val & 0xff; > + > + priv->cache[channel] = val; > + > + ret = spi_write(priv->spi, buffer, sizeof(buffer)); spi write can potentially do a dma transfer so it needs a dma safe buffer. This one isn't as it is on the stack. Given it is a moderately fast path, best option is to put the buffer in priv and use the __cacheline_aligned trick (plus the fact private data is already cacheline aligned) to get it into it's own cacheline. Wofram Sang's OSS talk on i2c dma is a good tutorial on this and is on youtube if you have time. https://www.youtube.com/watch?v=JDwaMClvV-s > + if (ret) > + return ret; > + > + gpiod_set_value(priv->nld, 0); > + gpiod_set_value(priv->nld, 1); > + > + return 0; > +} > + > +#define dac7612_CHANNEL(chan, name) { \ > + .type = IIO_VOLTAGE, \ > + .channel = (chan), \ > + .address = (chan), \ Not used, so don't set it. > + .indexed = true, \ These are bit fields, so whilst this works, = 1 is more conventional. > + .output = true, \ > + .datasheet_name = name, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = RESOLUTION, \ > + .storagebits = RESOLUTION, \ No need to provide scan_type as we don't have buffered output in mainline. Also this would be wrong as storagebits needs to be 8/16/32/64 etc. Would prefer a straight forward value like RESOLUTION was just expressed as a number. > + }, \ > +} > + > +static const struct iio_chan_spec dac7612_channels[] = { > + dac7612_CHANNEL(0, "OUTA"), > + dac7612_CHANNEL(1, "OUTB"), > +}; > + > +static int dac7612_read_raw(struct iio_dev *iio_dev, > + const struct iio_chan_spec *chan, > + int *val, int *val2, long mask) > +{ > + struct dac7612 *priv; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + priv = iio_priv(iio_dev); > + *val = priv->cache[chan->channel]; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 1000000; This makes me wonder. The units of voltage are millivolts, so is raw value * 1000000 = value in mv? That would make this a very high voltage device. See Documentation/ABI/testing/sysfs-bus-iio* for IIO ABI documentation. > + return IIO_VAL_INT_PLUS_MICRO; > + > + default: > + return -EINVAL; > + } > +} > + > +static int dac7612_write_raw(struct iio_dev *iio_dev, > + const struct iio_chan_spec *chan, > + int val, int val2, long mask) > +{ > + struct dac7612 *priv; struct dac7612 *priv = iio_priv(iio_dev); (it's just macro magic to get the right point so normally considered fine to be called like this). > + int ret; > + > + if (mask != IIO_CHAN_INFO_RAW) > + return -EINVAL; > + > + if ((val >= BIT(RESOLUTION)) || val < 0) > + return -EINVAL; As not providing a write_fmt function (which is fine) should also sanity check that val2 is 0. > + > + priv = iio_priv(iio_dev); > + if (val == priv->cache[chan->channel]) > + return 0; > + > + mutex_lock(&iio_dev->mlock); > + ret = dac7612_cmd_single(priv, chan->channel, val); > + mutex_unlock(&iio_dev->mlock); > + > + return ret; > +} > + > +static const struct iio_info dac7612_info = { > + .read_raw = dac7612_read_raw, > + .write_raw = dac7612_write_raw, > +}; > + > +static int dac7612_probe(struct spi_device *spi) > +{ > + struct iio_dev *iio_dev; > + struct dac7612 *priv; > + int i; > + int ret; > + > + iio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*priv)); > + if (!iio_dev) > + return -ENOMEM; > + > + priv = iio_priv(iio_dev); > + priv->nld = devm_gpiod_get_optional(&spi->dev, "nLD", GPIOD_OUT_HIGH); This isn't a particularly standard gpio name. Hence this driver definitely needs a DT binding doc (remember to cc dt list and maintainers). Also this should almost certainly be prefixed with a vendor prefix. It's also not the name I'm seeing on the datasheet, so needs some justifying comments. > + if (IS_ERR(priv->nld)) > + return PTR_ERR(priv->nld); > + priv->spi = spi; > + spi_set_drvdata(spi, iio_dev); > + iio_dev->dev.parent = &spi->dev; > + iio_dev->info = &dac7612_info; > + iio_dev->modes = INDIO_DIRECT_MODE; > + iio_dev->channels = dac7612_channels; > + iio_dev->num_channels = NUM_CHANS; ARRAY_SIZE. > + iio_dev->name = spi_get_device_id(spi)->name; > + > + for (i = 0; i < NUM_CHANS; i++) { ARRAY_SIZE > + ret = dac7612_cmd_single(priv, i, 0); > + if (ret) > + return ret; > + } > + > + return devm_iio_device_register(&spi->dev, iio_dev); > +} > + > +static const struct spi_device_id dac7612_id[] = { > + {"ti-dac7612"}, > + {} > +}; > +MODULE_DEVICE_TABLE(spi, dac7612_id); > + > +static struct spi_driver dac7612_driver = { > + .driver = { > + .name = "ti-dac7612", > + }, > + .probe = dac7612_probe, > + .id_table = dac7612_id, > +}; > +module_spi_driver(dac7612_driver); > + > +MODULE_AUTHOR("Ricardo Ribalda "); > +MODULE_DESCRIPTION("Texas Instruments DAC7612 DAC driver"); > +MODULE_LICENSE("GPL v2");