Message-ID: <20091127193636.GA16866@sortiz.org>
Date:	Fri, 27 Nov 2009 20:36:38 +0100
From:	Samuel Ortiz <sameo@...ux.intel.com>
To:	Amit Kucheria <amit.kucheria@...durent.com>
Cc:	List Linux Kernel <linux-kernel@...r.kernel.org>,
	linux-omap@...r.kernel.org, Mikko Ylinen <mikko.k.ylinen@...ia.com>
Subject: Re: [PATCH] mfd: twl4030: Driver for twl4030 madc module

Hi Amit,

On Wed, Nov 25, 2009 at 12:47:51PM +0200, Amit Kucheria wrote:
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index af0fc90..df1897b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -25,8 +25,9 @@ obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_MENELAUS)		+= menelaus.o
>  
>  obj-$(CONFIG_TWL4030_CORE)	+= twl4030-core.o twl4030-irq.o
> -obj-$(CONFIG_TWL4030_POWER)    += twl4030-power.o
> +obj-$(CONFIG_TWL4030_POWER)	+= twl4030-power.o
>  obj-$(CONFIG_TWL4030_CODEC)	+= twl4030-codec.o
> +obj-$(CONFIG_TWL4030_MADC)	+= twl4030-madc.o
>  
>  obj-$(CONFIG_MFD_MC13783)	+= mc13783-core.o
>  
> diff --git a/drivers/mfd/twl4030-madc.c b/drivers/mfd/twl4030-madc.c
> new file mode 100644
> index 0000000..90ce99a
> --- /dev/null
> +++ b/drivers/mfd/twl4030-madc.c
> @@ -0,0 +1,548 @@
> +/*
> + * drivers/i2c/chips/twl4030-madc.c
drivers/mfd/twl4030-madc.c
By the way, have you considered moving this one to drivers/hwmon ?


> + * TWL4030 MADC module driver
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Mikko Ylinen <mikko.k.ylinen@...ia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/i2c/twl4030.h>
> +#include <linux/i2c/twl4030-madc.h>
> +
> +#include <asm/uaccess.h>
> +
> +#define TWL4030_MADC_PFX	"twl4030-madc: "
> +
> +struct twl4030_madc_data {
> +	struct device		*dev;
> +	struct mutex		lock;
> +	struct work_struct	ws;
> +	struct twl4030_madc_request	requests[TWL4030_MADC_NUM_METHODS];
Typically, I'd expect to have a list (limited in length) of requests here, but
I guess you're happy with that array .



> +	int imr;
> +	int isr;
> +};
> +
> +static struct twl4030_madc_data *the_madc;
I dont particularly enjoy that. All of the twl4030 drivers have this bad habit
of assuming they will be alone on a platform. Although it's certainly very
likely, I still think having this kind of design is not so nice.


> +static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc, int chan, int on);
> +
> +static
> +const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
> +	[TWL4030_MADC_RT] = {
> +		.sel	= TWL4030_MADC_RTSELECT_LSB,
> +		.avg	= TWL4030_MADC_RTAVERAGE_LSB,
> +		.rbase	= TWL4030_MADC_RTCH0_LSB,
> +	},
> +	[TWL4030_MADC_SW1] = {
> +		.sel	= TWL4030_MADC_SW1SELECT_LSB,
> +		.avg	= TWL4030_MADC_SW1AVERAGE_LSB,
> +		.rbase	= TWL4030_MADC_GPCH0_LSB,
> +		.ctrl	= TWL4030_MADC_CTRL_SW1,
> +	},
> +	[TWL4030_MADC_SW2] = {
> +		.sel	= TWL4030_MADC_SW2SELECT_LSB,
> +		.avg	= TWL4030_MADC_SW2AVERAGE_LSB,
> +		.rbase	= TWL4030_MADC_GPCH0_LSB,
> +		.ctrl	= TWL4030_MADC_CTRL_SW2,
> +	},
> +};
> +
> +static int twl4030_madc_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MADC, &val, reg);
> +	if (ret) {
> +		dev_dbg(madc->dev, "unable to read register 0x%X\n", reg);
> +		return ret;
> +	}
> +
> +	return val;
> +}
FWIW, you're not checking the return value on any of the madc_read calls
below.


> +static void twl4030_madc_write(struct twl4030_madc_data *madc, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	ret = twl4030_i2c_write_u8(TWL4030_MODULE_MADC, val, reg);
> +	if (ret)
> +		dev_err(madc->dev, "unable to write register 0x%X\n", reg);
> +}
> +
> +static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
> +{
> +	u8 msb, lsb;
> +
> +	/* For each ADC channel, we have MSB and LSB register pair. MSB address
> +	 * is always LSB address+1. reg parameter is the addr of LSB register */
> +	msb = twl4030_madc_read(madc, reg + 1);
> +	lsb = twl4030_madc_read(madc, reg);
> +
> +	return (int)(((msb << 8) | lsb) >> 6);
> +}
> +
> +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> +		u8 reg_base, u16 channels, int *buf)
> +{
> +	int count = 0;
> +	u8 reg, i;
> +
> +	if (unlikely(!buf))
> +		return 0;
> +
> +	for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> +		if (channels & (1<<i)) {
> +			reg = reg_base + 2*i;
> +			buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> +			count++;
> +		}
> +	}
> +	return count;
> +}
> +
> +static void twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
> +{
> +	u8 val;
> +
> +	val = twl4030_madc_read(madc, madc->imr);
> +	val &= ~(1 << id);
> +	twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static void twl4030_madc_disable_irq(struct twl4030_madc_data *madc, int id)
> +{
> +	u8 val;
> +
> +	val = twl4030_madc_read(madc, madc->imr);
> +	val |= (1 << id);
> +	twl4030_madc_write(madc, madc->imr, val);
> +}
> +
> +static irqreturn_t twl4030_madc_irq_handler(int irq, void *_madc)
> +{
> +	struct twl4030_madc_data *madc = _madc;
> +	u8 isr_val, imr_val;
> +	int i;
> +
> +#ifdef CONFIG_LOCKDEP
> +	/* WORKAROUND for lockdep forcing IRQF_DISABLED on us, which
> +	 * we don't want and can't tolerate.  Although it might be
> +	 * friendlier not to borrow this thread context...
> +	 */
> +	local_irq_enable();
> +#endif
I'm curious, what's special about madc so that it can't tolerate that ?


> +	/* Use COR to ack interrupts since we have no shared IRQs in ISRx */
> +	isr_val = twl4030_madc_read(madc, madc->isr);
> +	imr_val = twl4030_madc_read(madc, madc->imr);
> +
> +	isr_val &= ~imr_val;
> +
> +	for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> +		if (!(isr_val & (1<<i)))
> +			continue;
> +
> +		twl4030_madc_disable_irq(madc, i);
> +		madc->requests[i].result_pending = 1;
> +	}
> +
> +	schedule_work(&madc->ws);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void twl4030_madc_work(struct work_struct *ws)
> +{
> +	const struct twl4030_madc_conversion_method *method;
> +	struct twl4030_madc_data *madc;
> +	struct twl4030_madc_request *r;
> +	int len, i;
> +
> +	madc = container_of(ws, struct twl4030_madc_data, ws);
> +	mutex_lock(&madc->lock);
> +
> +	for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> +		r = &madc->requests[i];
> +
> +		/* No pending results for this method, move to next one */
> +		if (!r->result_pending)
> +			continue;
> +
> +		method = &twl4030_conversion_methods[r->method];
> +
> +		/* Read results */
> +		len = twl4030_madc_read_channels(madc, method->rbase,
> +						 r->channels, r->rbuf);
> +
> +		/* Return results to caller */
> +		if (r->func_cb != NULL) {
> +			r->func_cb(len, r->channels, r->rbuf);
> +			r->func_cb = NULL;
> +		}
> +
> +		/* Free request */
> +		r->result_pending = 0;
> +		r->active	  = 0;
> +	}
> +
> +	mutex_unlock(&madc->lock);
> +}
I think you may want to consider using a threaded irq here, see
request_threaded_irq().


> +static int twl4030_madc_set_irq(struct twl4030_madc_data *madc,
> +		struct twl4030_madc_request *req)
> +{
> +	struct twl4030_madc_request *p;
> +
> +	p = &madc->requests[req->method];
> +
> +	memcpy(p, req, sizeof *req);
> +
> +	twl4030_madc_enable_irq(madc, req->method);
> +
> +	return 0;
> +}
> +
> +static inline void twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
> +		int conv_method)
> +{
> +	const struct twl4030_madc_conversion_method *method;
> +
> +	method = &twl4030_conversion_methods[conv_method];
> +
> +	switch (conv_method) {
> +	case TWL4030_MADC_SW1:
> +	case TWL4030_MADC_SW2:
> +		twl4030_madc_write(madc, method->ctrl, TWL4030_MADC_SW_START);
> +		break;
> +	case TWL4030_MADC_RT:
> +	default:
> +		break;
> +	}
> +}
> +
> +static int twl4030_madc_wait_conversion_ready(
> +		struct twl4030_madc_data *madc,
> +		unsigned int timeout_ms, u8 status_reg)
> +{
> +	unsigned long timeout;
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	do {
> +		int reg;
> +
> +		reg = twl4030_madc_read(madc, status_reg);
> +		if (unlikely(reg < 0))
> +			return reg;
> +		if (!(reg & TWL4030_MADC_BUSY) && (reg & TWL4030_MADC_EOC_SW))
> +			return 0;
> +	} while (!time_after(jiffies, timeout));
> +
> +	return -EAGAIN;
> +}
> +
> +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on);
> +int twl4030_madc_conversion(struct twl4030_madc_request *req)
> +{
> +	const struct twl4030_madc_conversion_method *method;
> +	u8 ch_msb, ch_lsb;
> +	int ret;
> +
> +	if (unlikely(!req))
> +		return -EINVAL;
> +
> +	mutex_lock(&the_madc->lock);
> +
> +	twl4030_madc_set_power(the_madc, 1);
> +
> +	/* Do we have a conversion request ongoing */
> +	if (the_madc->requests[req->method].active) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ch_msb = (req->channels >> 8) & 0xff;
> +	ch_lsb = req->channels & 0xff;
> +
> +	method = &twl4030_conversion_methods[req->method];
> +
> +	/* Select channels to be converted */
> +	twl4030_madc_write(the_madc, method->sel + 1, ch_msb);
> +	twl4030_madc_write(the_madc, method->sel, ch_lsb);
> +
> +	/* Select averaging for all channels if do_avg is set */
> +	if (req->do_avg) {
> +		twl4030_madc_write(the_madc, method->avg + 1, ch_msb);
> +		twl4030_madc_write(the_madc, method->avg, ch_lsb);
> +	}
> +
> +	if ((req->type == TWL4030_MADC_IRQ_ONESHOT) && (req->func_cb != NULL)) {
> +		twl4030_madc_set_irq(the_madc, req);
> +		twl4030_madc_start_conversion(the_madc, req->method);
> +		the_madc->requests[req->method].active = 1;
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	/* With RT method we should not be here anymore */
> +	if (req->method == TWL4030_MADC_RT) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	twl4030_madc_start_conversion(the_madc, req->method);
> +	the_madc->requests[req->method].active = 1;
> +
> +	/* Wait until conversion is ready (ctrl register returns EOC) */
> +	ret = twl4030_madc_wait_conversion_ready(the_madc, 5, method->ctrl);
So, you're busy looping with all conversions ? I have no idea how often this
is called, but putting the caller to sleep on e.g. a waitqueue would be more
elegant.


> +	if (ret) {
> +		dev_dbg(the_madc->dev, "conversion timeout!\n");
> +		the_madc->requests[req->method].active = 0;
> +		goto out;
> +	}
> +
> +	ret = twl4030_madc_read_channels(the_madc, method->rbase, req->channels,
> +					 req->rbuf);
> +
> +	the_madc->requests[req->method].active = 0;
> +
> +	twl4030_madc_set_power(the_madc, 0);
> +
> +out:
> +	mutex_unlock(&the_madc->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(twl4030_madc_conversion);
Who's supposed to use this one ?


> +static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
> +		int chan, int on)
> +{
> +	int ret;
> +	u8 regval;
> +
> +	/* Current generator is only available for ADCIN0 and ADCIN1. NB:
> +	 * ADCIN1 current generator only works when AC or VBUS is present */
> +	if (chan > 1)
> +		return EINVAL;
> +
> +	ret = twl4030_i2c_read_u8(TWL4030_MODULE_MAIN_CHARGE,
> +				  &regval, TWL4030_BCI_BCICTL1);
> +	if (ret) {
> +		dev_dbg(madc->dev, "unable to read register 0x%X\n", TWL4030_BCI_BCICTL1);
> +		return ret;
> +	}
> +
> +	if (on) {
> +		regval |= (chan) ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
> +		regval |= TWL4030_BCI_MESBAT;
> +	}
> +	else {
> +		regval &= (chan) ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
> +		regval &= ~TWL4030_BCI_MESBAT;
> +	}
> +
> +	ret = twl4030_i2c_write_u8(TWL4030_MODULE_MAIN_CHARGE,
> +				   regval, TWL4030_BCI_BCICTL1);
> +	if (ret) {
> +		dev_dbg(madc->dev, "unable to write register 0x%X\n", TWL4030_BCI_BCICTL1);
> +	}
> +	return ret;
> +}
> +
> +static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
> +{
> +	int ret = 0;
> +	u8 regval;
> +
> +	if (on) {
> +		regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> +		regval |= TWL4030_MADC_MADCON;
> +		twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> +
> +		ret |= twl4030_madc_set_current_generator(madc, 0, 1);
> +
> +	} else {
> +		ret |= twl4030_madc_set_current_generator(madc, 0, 0);
> +
> +		regval = twl4030_madc_read(madc, TWL4030_MADC_CTRL1);
> +		regval &= ~TWL4030_MADC_MADCON;
> +		twl4030_madc_write(madc, TWL4030_MADC_CTRL1, regval);
> +	}
> +	return ret;
> +}
> +
> +static long twl4030_madc_ioctl(struct file *filp, unsigned int cmd,
> +			       unsigned long arg)
> +{
> +	struct twl4030_madc_user_parms par;
> +	int val, ret;
> +
> +	ret = copy_from_user(&par, (void __user *) arg, sizeof(par));
> +	if (ret) {
> +		dev_dbg(the_madc->dev, "copy_from_user: %d\n", ret);
> +		return -EACCES;
> +	}
> +
> +	switch (cmd) {
> +	case TWL4030_MADC_IOCX_ADC_RAW_READ: {
> +		struct twl4030_madc_request req;
> +		if (par.channel >= TWL4030_MADC_MAX_CHANNELS)
> +			return -EINVAL;
> +
> +		req.channels = (1 << par.channel);
> +		req.do_avg	= par.average;
> +		req.method	= TWL4030_MADC_SW1;
> +		req.func_cb	= NULL;
> +		req.type	= TWL4030_MADC_WAIT;
> +
> +		val = twl4030_madc_conversion(&req);
> +		if (likely(val > 0)) {
> +			par.status = 0;
> +			par.result = (u16)req.rbuf[par.channel];
> +		} else if (val == 0) {
> +			par.status = -ENODATA;
> +		} else {
> +			par.status = val;
> +		}
> +		break;
> +					     }
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = copy_to_user((void __user *) arg, &par, sizeof(par));
> +	if (ret) {
> +		dev_dbg(the_madc->dev, "copy_to_user: %d\n", ret);
> +		return -EACCES;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct file_operations twl4030_madc_fileops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = twl4030_madc_ioctl
> +};
> +
> +static struct miscdevice twl4030_madc_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "twl4030-adc",
> +	.fops = &twl4030_madc_fileops
> +};
> +
> +static int __init twl4030_madc_probe(struct platform_device *pdev)
> +{
> +	struct twl4030_madc_data *madc;
> +	struct twl4030_madc_platform_data *pdata = pdev->dev.platform_data;
> +
> +	int ret;
> +	u8 regval;
> +
> +	madc = kzalloc(sizeof *madc, GFP_KERNEL);
> +	if (!madc)
> +		return -ENOMEM;
> +
> +	if (!pdata) {
> +		dev_dbg(&pdev->dev, "platform_data not available\n");
> +		ret = -EINVAL;
> +		goto err_pdata;
> +	}
> +
> +	madc->imr = (pdata->irq_line == 1) ? TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> +	madc->isr = (pdata->irq_line == 1) ? TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
> +
> +	ret = misc_register(&twl4030_madc_device);
> +	if (ret) {
> +		dev_dbg(&pdev->dev, "could not register misc_device\n");
> +		goto err_misc;
> +	}
> +
> +	ret = request_irq(platform_get_irq(pdev, 0), twl4030_madc_irq_handler,
> +			  0, "twl4030_madc", madc);
> +	if (ret) {
> +		dev_dbg(&pdev->dev, "could not request irq\n");
> +		goto err_irq;
> +	}
> +
> +	platform_set_drvdata(pdev, madc);
> +	mutex_init(&madc->lock);
> +	INIT_WORK(&madc->ws, twl4030_madc_work);
> +
> +	the_madc = madc;
> +
> +	return 0;
> +
> +err_irq:
> +	misc_deregister(&twl4030_madc_device);
> +
> +err_misc:
> +err_pdata:
> +	kfree(madc);
> +
> +	return ret;
> +}
> +
> +static int __exit twl4030_madc_remove(struct platform_device *pdev)
> +{
> +	struct twl4030_madc_data *madc = platform_get_drvdata(pdev);
> +
> +	free_irq(platform_get_irq(pdev, 0), madc);
> +	cancel_work_sync(&madc->ws);
> +	misc_deregister(&twl4030_madc_device);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver twl4030_madc_driver = {
> +	.probe		= twl4030_madc_probe,
> +	.remove		= __exit_p(twl4030_madc_remove),
> +	.driver		= {
> +		.name	= "twl4030_madc",
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static int __init twl4030_madc_init(void)
> +{
> +	return platform_driver_register(&twl4030_madc_driver);
> +}
> +module_init(twl4030_madc_init);
> +
> +static void __exit twl4030_madc_exit(void)
> +{
> +	platform_driver_unregister(&twl4030_madc_driver);
> +}
> +module_exit(twl4030_madc_exit);
> +
> +MODULE_ALIAS("platform:twl4030-madc");
> +MODULE_AUTHOR("Nokia Corporation");
> +MODULE_DESCRIPTION("twl4030 ADC driver");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
> new file mode 100644
> index 0000000..24523b5
> --- /dev/null
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -0,0 +1,126 @@
> +/*
> + * include/linux/i2c/twl4030-madc.h
> + *
> + * TWL4030 MADC module driver header
> + *
> + * Copyright (C) 2008 Nokia Corporation
> + * Mikko Ylinen <mikko.k.ylinen@...ia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#ifndef _TWL4030_MADC_H
> +#define _TWL4030_MADC_H
> +
> +struct twl4030_madc_conversion_method {
> +	u8 sel;
> +	u8 avg;
> +	u8 rbase;
> +	u8 ctrl;
> +};
> +
> +#define TWL4030_MADC_MAX_CHANNELS 16
> +
> +struct twl4030_madc_request {
> +	u16 channels;
> +	u16 do_avg;
> +	u16 method;
> +	u16 type;
> +	int active;
> +	int result_pending;
active and result_pending can be bool.

Cheers,
Samuel. 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/
--
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