| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4CE347531D4CA947960AF71FF095B9323E953D32@DBDE01.ent.ti.com>
Date: Fri, 14 Sep 2012 06:00:41 +0000
From: "Patil, Rachna" <rachna@...com>
To: Jonathan Cameron <jic23@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Dmitry Torokhov <dtor@...l.ru>,
Jonathan Cameron <jic23@....ac.uk>
Subject: RE: [PATCH v3 4/5] IIO : ADC: tiadc: Add support of TI's ADC driver
On Thu, Sep 13, 2012 at 17:43:30, Jonathan Cameron wrote:
> On 13/09/12 11:40, Patil, Rachna wrote:
> > This patch adds support for TI's ADC driver.
> > This is a multifunctional device.
> > Analog input lines are provided on which voltage measurements can be
> > carried out.
> > You can have upto 8 input lines.
> >
> > Signed-off-by: Patil, Rachna <rachna@...com>
>
> There's a little fuzz in applying this due to other drivers that have gone in recently.
>
> Actually this is going to be 'interesting' to merge. Dmitry, Samuel thoughts on who takes this one and how? Maybe this is a case for a 'special' branch pulled into more than one tree?
>
>
> One minor thing inline. I have an aversion to dynamic allocation of
> things that are then constant.
>
> Also the module name is simply ti_adc. Does seem a little 'vague'
> given the range of ADC's TI makes :) Perhaps keep the reference
> to the tsc in there? Personally I'd have preferred the whole thing
> being named after a particular part number (any one it support would
> do) to avoid a clash in future with a new touch screen adc from TI.
> Bit late for that though I guess ;)
Yes, true.
TI definitely might come up with more IP's of this type.
This IP(TSC / ADC) is present on AM335x. If necessary we can rename the driver to ti_am335x_XXX.
>
> Jonathan
> > ---
> > Changes in v2:
> > Addressed review comments from Matthias Kaehlcke
> >
> > Changes in v3:
> > Addressed review comments from Jonathan Cameron.
> > Added comments, new line appropriately.
> >
> > drivers/iio/adc/Kconfig | 7 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti_adc.c | 223 ++++++++++++++++++++++++++++++++++
> > drivers/mfd/ti_tscadc.c | 18 +++-
> > include/linux/mfd/ti_tscadc.h | 9 ++-
> > include/linux/platform_data/ti_adc.h | 14 ++
> > 6 files changed, 270 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/iio/adc/ti_adc.c
> > create mode 100644 include/linux/platform_data/ti_adc.h
> >
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 8a78b4f..ad32df8 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -22,4 +22,11 @@ config AT91_ADC
> > help
> > Say yes here to build support for Atmel AT91 ADC.
> >
> > +config TI_ADC
> > + tristate "TI's ADC driver"
> > + depends on ARCH_OMAP2PLUS
> > + help
> > + Say yes here to build support for Texas Instruments ADC
> > + driver which is also a MFD client.
> > +
> > endmenu
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index 52eec25..a930cee 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -4,3 +4,4 @@
> >
> > obj-$(CONFIG_AD7266) += ad7266.o
> > obj-$(CONFIG_AT91_ADC) += at91_adc.o
> > +obj-$(CONFIG_TI_ADC) += ti_adc.o
> > diff --git a/drivers/iio/adc/ti_adc.c b/drivers/iio/adc/ti_adc.c
> > new file mode 100644
> > index 0000000..56f8af2
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti_adc.c
> > @@ -0,0 +1,223 @@
> > +/*
> > + * TI ADC MFD driver
> > + *
> > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/iio/iio.h>
> > +
> > +#include <linux/mfd/ti_tscadc.h>
> > +#include <linux/platform_data/ti_adc.h>
> > +
> > +struct adc_device {
> > + struct ti_tscadc_dev *mfd_tscadc;
> > + struct iio_dev *idev;
> > + int channels;
> > +};
> > +
> > +static unsigned int adc_readl(struct adc_device *adc, unsigned int reg)
> > +{
> > + return readl(adc->mfd_tscadc->tscadc_base + reg);
> > +}
> > +
> > +static void adc_writel(struct adc_device *adc, unsigned int reg,
> > + unsigned int val)
> > +{
> > + writel(val, adc->mfd_tscadc->tscadc_base + reg);
> > +}
> > +
> > +static void adc_step_config(struct adc_device *adc_dev)
> > +{
> > + unsigned int stepconfig;
> > + int i, channels = 0, steps;
> > +
> > + /*
> > + * There are 16 configurable steps and 8 analog input
> > + * lines available which are shared between Touchscreen and ADC.
> > + *
> > + * Steps backwards i.e. from 16 towards 0 are used by ADC
> > + * depending on number of input lines needed.
> > + * Channel would represent which analog input
> > + * needs to be given to ADC to digitalize data.
> > + */
> > +
> > + steps = TOTAL_STEPS - adc_dev->channels;
> > + channels = TOTAL_CHANNELS - adc_dev->channels;
> > +
> > + stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
> > +
> > + for (i = (steps + 1); i <= TOTAL_STEPS; i++) {
> > + adc_writel(adc_dev, REG_STEPCONFIG(i),
> > + stepconfig | STEPCONFIG_INP(channels));
> > + adc_writel(adc_dev, REG_STEPDELAY(i),
> > + STEPCONFIG_OPENDLY);
> > + channels++;
> > + }
> > + adc_writel(adc_dev, REG_SE, STPENB_STEPENB);
> > +}
> > +
> > +static int tiadc_channel_init(struct iio_dev *idev, int channels)
> > +{
> > + struct iio_chan_spec *chan_array;
> > + int i;
> > +
> > + idev->num_channels = channels;
> > + chan_array = kcalloc(idev->num_channels, sizeof(struct iio_chan_spec),
> > + GFP_KERNEL);
> > +
> > + if (chan_array == NULL)
> > + return -ENOMEM;
> > +
> Minor point, but I'd be tempted to do this as a static const array and
> then just set num_channels appropriately. Still it's such a simple
> driver that I'm not that fussed.
I looked at some other drivers, they seem to be doing the same way.
I would like to go with the existing convention.
> > + for (i = 0; i < (idev->num_channels); i++) {
> > + struct iio_chan_spec *chan = chan_array + i;
> > + chan->type = IIO_VOLTAGE;
> > + chan->indexed = 1;
> > + chan->channel = i;
> > + chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
> > + }
> > +
> > + idev->channels = chan_array;
> > +
> > + return idev->num_channels;
> > +}
> > +
Regards,
Rachna
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists