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 <rac...@ti.com>
> > ---
> > 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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to