Re: [PATCHv3 3/9] mfd: twl4030-madc: Cleanup driver

[Jonathan Cameron]

On 10/03/14 17:25, Sebastian Reichel wrote:
> Some style fixes in twl4030-madc driver.
>
> Reported-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> Reported-by: Lee Jones <lee.jones@xxxxxxxxxx>
> Signed-off-by: Sebastian Reichel <sre@xxxxxxxxxx>
> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>
> Tested-by: Marek Belisko <marek@xxxxxxxxxxxxx>
Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>

One, whilst you are here request below.
> ---
>   drivers/mfd/twl4030-madc.c       | 127 +++++++++++++++++++--------------------
>   include/linux/i2c/twl4030-madc.h |   2 +-
>   2 files changed, 62 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-madc.c b/drivers/mfd/twl4030-madc.c
> index 1c5c4db..890d520 100644
> --- a/drivers/mfd/twl4030-madc.c
> +++ b/drivers/mfd/twl4030-madc.c
> @@ -49,22 +49,22 @@
>
>   #include <linux/iio/iio.h>
>
> -/*
> +/**
>    * struct twl4030_madc_data - a container for madc info
> - * @dev - pointer to device structure for madc
> - * @lock - mutex protecting this data structure
> - * @requests - Array of request struct corresponding to SW1, SW2 and RT
> - * @use_second_irq - IRQ selection (main or co-processor)
> - * @imr - Interrupt mask register of MADC
> - * @isr - Interrupt status register of MADC
> + * @dev:		Pointer to device structure for madc
> + * @lock:		Mutex protecting this data structure
> + * @requests:		Array of request struct corresponding to SW1, SW2 and RT
> + * @use_second_irq:	IRQ selection (main or co-processor)
> + * @imr:		Interrupt mask register of MADC
> + * @isr:		Interrupt status register of MADC
>    */
>   struct twl4030_madc_data {
>   	struct device *dev;
>   	struct mutex lock;	/* mutex protecting this data structure */
>   	struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
>   	bool use_second_irq;
> -	int imr;
> -	int isr;
> +	u8 imr;
> +	u8 isr;
>   };
>
>   static int twl4030_madc_read(struct iio_dev *iio_dev,
> @@ -155,17 +155,16 @@ twl4030_divider_ratios[16] = {
>   };
>
>
> -/*
> - * Conversion table from -3 to 55 degree Celcius
> - */
> -static int therm_tbl[] = {
> -30800,	29500,	28300,	27100,
> -26000,	24900,	23900,	22900,	22000,	21100,	20300,	19400,	18700,	17900,
> -17200,	16500,	15900,	15300,	14700,	14100,	13600,	13100,	12600,	12100,
> -11600,	11200,	10800,	10400,	10000,	9630,	9280,	8950,	8620,	8310,
> -8020,	7730,	7460,	7200,	6950,	6710,	6470,	6250,	6040,	5830,
> -5640,	5450,	5260,	5090,	4920,	4760,	4600,	4450,	4310,	4170,
> -4040,	3910,	3790,	3670,	3550
> +/* Conversion table from -3 to 55 degrees Celcius */
> +static int twl4030_therm_tbl[] = {
> +	30800,	29500,	28300,	27100,
> +	26000,	24900,	23900,	22900,	22000,	21100,	20300,	19400,	18700,
> +	17900,	17200,	16500,	15900,	15300,	14700,	14100,	13600,	13100,
> +	12600,	12100,	11600,	11200,	10800,	10400,	10000,	9630,	9280,
> +	8950,	8620,	8310,	8020,	7730,	7460,	7200,	6950,	6710,
> +	6470,	6250,	6040,	5830,	5640,	5450,	5260,	5090,	4920,
> +	4760,	4600,	4450,	4310,	4170,	4040,	3910,	3790,	3670,
> +	3550
>   };
>
>   /*
> @@ -197,11 +196,12 @@ const struct twl4030_madc_conversion_method twl4030_conversion_methods[] = {
>   			      },
>   };
>
> -/*
> - * Function to read a particular channel value.
> - * @madc - pointer to struct twl4030_madc_data
> - * @reg - lsb of ADC Channel
> - * If the i2c read fails it returns an error else returns 0.
> +/**
> + * twl4030_madc_channel_raw_read() - Function to read a particular channel value
> + * @madc:	pointer to struct twl4030_madc_data
> + * @reg:	lsb of ADC Channel
> + *
> + * Return: 0 on success, an error code otherwise.
>    */
>   static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
>   {
> @@ -227,7 +227,7 @@ static int twl4030_madc_channel_raw_read(struct twl4030_madc_data *madc, u8 reg)
>   }
>
>   /*
> - * Return battery temperature
> + * Return battery temperature in degrees Celsius
>    * Or < 0 on failure.
>    */
>   static int twl4030battery_temperature(int raw_volt)
> @@ -236,18 +236,18 @@ static int twl4030battery_temperature(int raw_volt)
>   	int temp, curr, volt, res, ret;
>
>   	volt = (raw_volt * TEMP_STEP_SIZE) / TEMP_PSR_R;
> -	/* Getting and calculating the supply current in micro ampers */
> +	/* Getting and calculating the supply current in micro amperes */
>   	ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE, &val,
>   		REG_BCICTL2);
>   	if (ret < 0)
>   		return ret;
> +
>   	curr = ((val & TWL4030_BCI_ITHEN) + 1) * 10;
>   	/* Getting and calculating the thermistor resistance in ohms */
>   	res = volt * 1000 / curr;
>   	/* calculating temperature */
>   	for (temp = 58; temp >= 0; temp--) {
> -		int actual = therm_tbl[temp];
> -
> +		int actual = twl4030_therm_tbl[temp];
>   		if ((actual - res) >= 0)
>   			break;
>   	}
> @@ -269,11 +269,12 @@ static int twl4030battery_current(int raw_volt)
>   	else /* slope of 0.88 mV/mA */
>   		return (raw_volt * CURR_STEP_SIZE) / CURR_PSR_R2;
>   }
> +
>   /*
>    * Function to read channel values
>    * @madc - pointer to twl4030_madc_data struct
>    * @reg_base - Base address of the first channel
> - * @Channels - 16 bit bitmap. If the bit is set, channel value is read
> + * @Channels - 16 bit bitmap. If the bit is set, channel's value is read
>    * @buf - The channel values are stored here. if read fails error
>    * @raw - Return raw values without conversion
>    * value is stored
> @@ -284,17 +285,17 @@ static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
>   				      long channels, int *buf,
>   				      bool raw)
>   {
> -	int count = 0, count_req = 0, i;
> +	int count = 0;
> +	int i;
>   	u8 reg;
>
>   	for_each_set_bit(i, &channels, TWL4030_MADC_MAX_CHANNELS) {
> -		reg = reg_base + 2 * i;
> +		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++;
> -			continue;
> +			dev_err(madc->dev, "Unable to read register 0x%X\n",
> +				reg);
> +			return buf[i];
>   		}
>   		if (raw) {
>   			count++;
> @@ -305,7 +306,7 @@ static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
>   			buf[i] = twl4030battery_current(buf[i]);
>   			if (buf[i] < 0) {
>   				dev_err(madc->dev, "err reading current\n");
> -				count_req++;
> +				return buf[i];
>   			} else {
>   				count++;
>   				buf[i] = buf[i] - 750;
> @@ -315,7 +316,7 @@ static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
>   			buf[i] = twl4030battery_temperature(buf[i]);
>   			if (buf[i] < 0) {
>   				dev_err(madc->dev, "err reading temperature\n");
> -				count_req++;
> +				return buf[i];
>   			} else {
>   				buf[i] -= 3;
>   				count++;
> @@ -336,8 +337,6 @@ static int twl4030_madc_read_channels(struct twl4030_madc_data *madc,
>   				twl4030_divider_ratios[i].numerator);
>   		}
>   	}
> -	if (count_req)
> -		dev_err(madc->dev, "%d channel conversion failed\n", count_req);
>
>   	return count;
>   }
> @@ -361,13 +360,13 @@ static int twl4030_madc_enable_irq(struct twl4030_madc_data *madc, u8 id)
>   			madc->imr);
>   		return ret;
>   	}
> +
>   	val &= ~(1 << id);
>   	ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, val, madc->imr);
>   	if (ret) {
>   		dev_err(madc->dev,
>   			"unable to write imr register 0x%X\n", madc->imr);
>   		return ret;
> -
>   	}
>
>   	return 0;
> @@ -430,7 +429,7 @@ static irqreturn_t twl4030_madc_threaded_irq_handler(int irq, void *_madc)
>   			continue;
>   		ret = twl4030_madc_disable_irq(madc, i);
>   		if (ret < 0)
> -			dev_dbg(madc->dev, "Disable interrupt failed%d\n", i);
> +			dev_dbg(madc->dev, "Disable interrupt failed %d\n", i);
>   		madc->requests[i].result_pending = 1;
>   	}
>   	for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> @@ -512,21 +511,17 @@ static int twl4030_madc_start_conversion(struct twl4030_madc_data *madc,
>   {
>   	const struct twl4030_madc_conversion_method *method;
>   	int ret = 0;
> +
> +	if (conv_method != TWL4030_MADC_SW1 && conv_method != TWL4030_MADC_SW2)
> +		return -ENOTSUPP;
> +
>   	method = &twl4030_conversion_methods[conv_method];
> -	switch (conv_method) {
> -	case TWL4030_MADC_SW1:
> -	case TWL4030_MADC_SW2:
> -		ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
> -				       TWL4030_MADC_SW_START, method->ctrl);
> -		if (ret) {
> -			dev_err(madc->dev,
> -				"unable to write ctrl register 0x%X\n",
> -				method->ctrl);
> -			return ret;
> -		}
> -		break;
> -	default:
> -		break;
> +	ret = twl_i2c_write_u8(TWL4030_MODULE_MADC, TWL4030_MADC_SW_START,
> +			       method->ctrl);
> +	if (ret) {
> +		dev_err(madc->dev, "unable to write ctrl register 0x%X\n",
> +			method->ctrl);
> +		return ret;
>   	}
>
>   	return 0;
> @@ -623,8 +618,8 @@ int twl4030_madc_conversion(struct twl4030_madc_request *req)
>   				       ch_lsb, method->avg);
>   		if (ret) {
>   			dev_err(twl4030_madc->dev,
> -				"unable to write sel reg 0x%X\n",
> -				method->sel + 1);
> +				"unable to write avg reg 0x%X\n",
> +				method->avg);
>   			goto out;
>   		}
>   	}
> @@ -665,10 +660,6 @@ out:
>   }
>   EXPORT_SYMBOL_GPL(twl4030_madc_conversion);
>
> -/*
> - * Return channel value
> - * Or < 0 on failure.
> - */
>   int twl4030_get_madc_conversion(int channel_no)
>   {
>   	struct twl4030_madc_request req;
> @@ -703,6 +694,7 @@ static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
>   					      int chan, int on)
>   {
>   	int ret;
> +	int regmask;
>   	u8 regval;
>
>   	ret = twl_i2c_read_u8(TWL_MODULE_MAIN_CHARGE,
> @@ -712,10 +704,13 @@ static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
>   			TWL4030_BCI_BCICTL1);
>   		return ret;
>   	}
> +
> +	regmask = chan ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
>   	if (on)
> -		regval |= chan ? TWL4030_BCI_ITHEN : TWL4030_BCI_TYPEN;
> +		regval |= regmask;
>   	else
> -		regval &= chan ? ~TWL4030_BCI_ITHEN : ~TWL4030_BCI_TYPEN;
> +		regval &= ~regmask;
> +
>   	ret = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
>   			       regval, TWL4030_BCI_BCICTL1);
>   	if (ret) {
> @@ -730,7 +725,7 @@ static int twl4030_madc_set_current_generator(struct twl4030_madc_data *madc,
Would be nice if this one was also in kernel-doc format :)
>   /*
>    * Function that sets MADC software power on bit to enable MADC
>    * @madc - pointer to twl4030_madc_data struct
> - * @on - Enable or disable MADC software powen on bit.
> + * @on - Enable or disable MADC software power on bit.
>    * returns error if i2c read/write fails else 0
>    */
>   static int twl4030_madc_set_power(struct twl4030_madc_data *madc, int on)
> @@ -909,7 +904,7 @@ static struct platform_driver twl4030_madc_driver = {
>   		   .name = "twl4030_madc",
>   		   .owner = THIS_MODULE,
>   		   .of_match_table = of_match_ptr(twl_madc_of_match),
> -		   },
> +	},
>   };
>
>   module_platform_driver(twl4030_madc_driver);
> diff --git a/include/linux/i2c/twl4030-madc.h b/include/linux/i2c/twl4030-madc.h
> index 01f5951..1c0134d 100644
> --- a/include/linux/i2c/twl4030-madc.h
> +++ b/include/linux/i2c/twl4030-madc.h
> @@ -44,7 +44,7 @@ struct twl4030_madc_conversion_method {
>
>   struct twl4030_madc_request {
>   	unsigned long channels;
> -	u16 do_avg;
> +	bool do_avg;
>   	u16 method;
>   	u16 type;
>   	bool active;

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/