On Mon, Feb 28, 2011 at 6:30 AM, Samuel Ortiz <[email protected]> wrote:
> 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.
>
Ok.
>
>> + 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.
>
I will call continue.
>> +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 ?
Ok.
>
> Cheers,
> Samuel.
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
>
--
Regards and Thanks,
Keerthy
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
