All of lore.kernel.org
 help / color / mirror / Atom feed
From: Samuel Ortiz <sameo@linux.intel.com>
To: Keerthy <j-keerthy@ti.com>
Cc: lm-sensors@lm-sensors.org, guenter.roeck@ericsson.com,
	khali@linux-fr.org, mikko.k.ylinen@nokia.com,
	amit.kucheria@canonical.com, linux-omap@vger.kernel.org
Subject: Re: [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 madc module
Date: Mon, 28 Feb 2011 02:00:46 +0100	[thread overview]
Message-ID: <20110228010045.GD2749@sortiz-mobl> (raw)
In-Reply-To: <1298560713-18790-1-git-send-email-j-keerthy@ti.com>

Hi Keerthy,

On Thu, Feb 24, 2011 at 08:48:33PM +0530, Keerthy wrote:
> Introducing a driver for MADC on TWL4030 powerIC. MADC stands for monitoring
> ADC. This driver monitors the real time conversion of analog signals like
> battery temperature, battery cuurent etc.
Looks mostly good to me, one minor comment though:

> +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> +				      u8 reg_base, u16 channels, int *buf)
> +{
> +	int count = 0, count_req = 0;
> +	u8 reg, i;
> +
> +	for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> +		if (channels & (1 << i)) {
You may want to use for_each_set_bit() here.


> +			reg = reg_base + 2 * i;
> +			buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> +			if (buf[i] < 0) {
> +				dev_err(madc->dev,
> +				"Unable to read register 0x%X\n", reg);
> +				count_req++;
For a clearer code, just call continue here, and the following blck will look
nicer.

> +static irqreturn_t twl4030_madc_threaded_irq_handler(int irq, void *_madc)
> +{
> +	struct twl4030_madc_data *madc = _madc;
> +	const struct twl4030_madc_conversion_method *method;
> +	u8 isr_val, imr_val;
> +	int i, len, ret;
> +	struct twl4030_madc_request *r;

Don't you want to take the madc lock around here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

WARNING: multiple messages have this Message-ID (diff)
From: Samuel Ortiz <sameo@linux.intel.com>
To: Keerthy <j-keerthy@ti.com>
Cc: lm-sensors@lm-sensors.org, guenter.roeck@ericsson.com,
	khali@linux-fr.org, mikko.k.ylinen@nokia.com,
	amit.kucheria@canonical.com, linux-omap@vger.kernel.org
Subject: Re: [lm-sensors] [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030
Date: Mon, 28 Feb 2011 01:00:46 +0000	[thread overview]
Message-ID: <20110228010045.GD2749@sortiz-mobl> (raw)
In-Reply-To: <1298560713-18790-1-git-send-email-j-keerthy@ti.com>

Hi Keerthy,

On Thu, Feb 24, 2011 at 08:48:33PM +0530, Keerthy wrote:
> Introducing a driver for MADC on TWL4030 powerIC. MADC stands for monitoring
> ADC. This driver monitors the real time conversion of analog signals like
> battery temperature, battery cuurent etc.
Looks mostly good to me, one minor comment though:

> +static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
> +				      u8 reg_base, u16 channels, int *buf)
> +{
> +	int count = 0, count_req = 0;
> +	u8 reg, i;
> +
> +	for (i = 0; i < TWL4030_MADC_MAX_CHANNELS; i++) {
> +		if (channels & (1 << i)) {
You may want to use for_each_set_bit() here.


> +			reg = reg_base + 2 * i;
> +			buf[i] = twl4030_madc_channel_raw_read(madc, reg);
> +			if (buf[i] < 0) {
> +				dev_err(madc->dev,
> +				"Unable to read register 0x%X\n", reg);
> +				count_req++;
For a clearer code, just call continue here, and the following blck will look
nicer.

> +static irqreturn_t twl4030_madc_threaded_irq_handler(int irq, void *_madc)
> +{
> +	struct twl4030_madc_data *madc = _madc;
> +	const struct twl4030_madc_conversion_method *method;
> +	u8 isr_val, imr_val;
> +	int i, len, ret;
> +	struct twl4030_madc_request *r;

Don't you want to take the madc lock around here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

  reply	other threads:[~2011-02-28  1:00 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-24 15:18 [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 madc module Keerthy
2011-02-24 15:30 ` [lm-sensors] [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 madc Keerthy
2011-02-28  1:00 ` Samuel Ortiz [this message]
2011-02-28  1:00   ` [lm-sensors] [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 Samuel Ortiz
2011-02-28  4:54   ` [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 madc module J, KEERTHY
2011-02-28  4:57     ` [lm-sensors] [PATCH 1/2 v2] mfd: twl4030: Driver for twl4030 J, KEERTHY

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110228010045.GD2749@sortiz-mobl \
    --to=sameo@linux.intel.com \
    --cc=amit.kucheria@canonical.com \
    --cc=guenter.roeck@ericsson.com \
    --cc=j-keerthy@ti.com \
    --cc=khali@linux-fr.org \
    --cc=linux-omap@vger.kernel.org \
    --cc=lm-sensors@lm-sensors.org \
    --cc=mikko.k.ylinen@nokia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.