On Thu, Sep 13, 2012 at 18:21:19, Lars-Peter Clausen wrote:
> On 09/13/2012 12:40 PM, 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.
> >
>
> Hi,
>
> couple of minor issues inline.
Hi,
Please find my comments inline.
>
> > Signed-off-by: Patil, Rachna <rachna@xxxxxx>
> > ---
> > 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
> >
> [...]
> > +
> > +struct adc_device {
>
> Not really an issue, but I'd use a consistent function/struct prefix.
> Currently you use both "adc" and "tiadc"
Ok. I will update this.
>
> > + struct ti_tscadc_dev *mfd_tscadc;
> > + struct iio_dev *idev;
>
> idev is used only once in the remove callback. But you can get a pointer to it easily using platform_get_drvdata. So I'd remove it from the adc_device struct.
Ok. I will remove this.
>
> > + 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;
>
> extra whitespace
Ok. I will remove this.
>
> > + 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 const struct iio_info tiadc_info = {
> > + .read_raw = &tiadc_read_raw,
>
> .driver_module = THIS_MODULE,
>
> > +};
> > +
> > +static int __devinit tiadc_probe(struct platform_device *pdev) {
> > + struct iio_dev *idev;
>
> For consistency with other drivers please rename idev to indio_dev throughout the driver.
Ok. I will rename this.
>
> > + int err;
> > + struct adc_device *adc_dev;
> > + struct ti_tscadc_dev *tscadc_dev = pdev->dev.platform_data;
> > + struct mfd_tscadc_board *pdata;
> > +
> > + pdata = (struct mfd_tscadc_board *)tscadc_dev->dev->platform_data;
>
> The cast should not be necessary.
True,
I will correct this.
>
> > + if (!pdata || !pdata->adc_init) {
> > + dev_err(tscadc_dev->dev, "Could not find platform data\n");
>
> I'd still use pdev->dev for the device parameter here. Seeing a message printed by this driver for another device might be confusing.
Yes, I will correct this.
>
> > + return -EINVAL;
> > + }
> > +
> > + idev = iio_device_alloc(sizeof(struct adc_device));
> > + if (idev == NULL) {
> > + dev_err(&pdev->dev, "failed to allocate iio device.\n");
> > + err = -ENOMEM;
> > + goto err_ret;
> > + }
> > + adc_dev = iio_priv(idev);
> > +
> > + tscadc_dev->adc = adc_dev;
>
> This there any reason why you need to store a pointer to the adc struct in the mfd struct? Is it going to be used outside of the adc driver? Currently it is, as far as I can see, only used in the remove callback and suspend/resume handlers. But there you can use iio_priv just as easily to get the pointer to the adc device struct and it certainly will be also be cleaner to do it that way.
Yes, currently it is used only in suspend/resume.
I will drop this line.
>
> > + adc_dev->mfd_tscadc = tscadc_dev;
> > + adc_dev->idev = idev;
> > + adc_dev->channels = pdata->adc_init->adc_channels;
> > +
> > + idev->dev.parent = &pdev->dev;
> > + idev->name = dev_name(&pdev->dev);
> > + idev->modes = INDIO_DIRECT_MODE;
> > + idev->info = &tiadc_info;
> > +
> > + adc_step_config(adc_dev);
> > +
> > + err = tiadc_channel_init(idev, adc_dev->channels);
> > + if (err < 0)
> > + goto err_free_device;
> > +
> > + err = iio_device_register(idev);
> > + if (err)
> > + goto err_free_channels;
> > +
> > + dev_info(&pdev->dev, "attached adc driver\n");
>
> I'd remove that line, it's just noise.
Ok. I will drop this.
>
> > + platform_set_drvdata(pdev, idev);
> > +
> > + return 0;
> > +
> > +err_free_channels:
> > + tiadc_channels_remove(idev);
> > +err_free_device:
> > + iio_device_free(idev);
> > +err_ret:
> > + return err;
> > +}
> > +
>
> [...]
> > diff --git a/include/linux/platform_data/ti_adc.h
> > b/include/linux/platform_data/ti_adc.h
> > new file mode 100644
> > index 0000000..5a89f1d
> > --- /dev/null
> > +++ b/include/linux/platform_data/ti_adc.h
> > @@ -0,0 +1,14 @@
> > +#ifndef __LINUX_TI_ADC_H
> > +#define __LINUX_TI_ADC_H
> > +
> > +/**
> > + * struct adc_data ADC Input information
> > + * @adc_channels: Number of analog inputs
> > + * available for ADC.
> > + */
> > +
> > +struct adc_data {
>
> The struct name might be a bit to generic.
>
> > + int adc_channels;
>
> It does not really matter, but considering that there hardly ever going to be a negative number of channels I'd make this unsigned.
Ok. I will update this.
Regards,
Rachna
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
