| 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
| ||
|
Date: Fri, 14 Sep 2012 09:20:57 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Patil, Rachna" <rachna@...com>
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>
Subject: Re: [PATCH v3 4/5] IIO : ADC: tiadc: Add support of TI's ADC driver
On 14/09/12 07:00, Patil, Rachna wrote:
> 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.
I'd be in favour of doing this now rather than when the problem presents
itself. Anyone mind?
>
>>
>> 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