Re: [PATCH 04/11] PMIC: Add TSI/misc input Driver for Dialog DA9052

On 06/10/2011 04:50 AM, Somebody in the thread at some point said:

+static ssize_t write_da9052_reg(struct da9052 *da9052, u8 reg_addr, u8 data) +{

• ssize_t ret = 0;
• struct da9052_ssc_msg ssc_msg;
• ssc_msg.addr = reg_addr;
• ssc_msg.data = data;
• ret = da9052->write(da9052,&ssc_msg);
• if (ret) {

• DA9052_DEBUG("%s: ", __func__);
  

• DA9052_DEBUG("da9052_ssc_write Failed %d\n", ret);
  

• }
• return ret;

+}

+static ssize_t read_da9052_reg(struct da9052 *da9052, u8 reg_addr) +{

• ssize_t ret = 0;
  

• struct da9052_ssc_msg ssc_msg;
  

• ssc_msg.addr = reg_addr;
• ssc_msg.data = 0;
• ret = da9052->read(da9052,&ssc_msg);
• if (ret) {

• DA9052_DEBUG("%s: ", __func__);
  

• DA9052_DEBUG("da9052_ssc_read Failed =>  %d\n", ret);
  

• return -ret;
  

Huh... you return -ret on error here, but on write_da9052_reg() you return ret on error... is that right?

• }
• return ssc_msg.data;

+}

+static struct da9052_tsi_info *get_tsi_drvdata(void) +{

• return&gda9052_tsi_info;

+}

+static ssize_t da9052_tsi_config_measure_seq(struct da9052_ts_priv *priv,

• 				enum TSI_MEASURE_SEQ seq)
  

+{

• ssize_t ret = 0;
• u8 data = 0;
• struct da9052_tsi_info *ts = get_tsi_drvdata();
• if (seq> 1)

• return -EINVAL;
  

• da9052_lock(priv->da9052);
• ret = read_da9052_reg(priv->da9052, DA9052_TSICONTA_REG);
• if (ret< 0) {

• DA9052_DEBUG("DA9052_TSI: %s:", __func__);
  

• DA9052_DEBUG("read_da9052_reg Failed\n");
  

• da9052_unlock(priv->da9052);
  

• return ret;
  

• }
• data = (u8)ret;
• if (seq == XYZP_MODE)

• data = enable_xyzp_mode(data);
  

• else if (seq == XP_MODE)

• data = enable_xp_mode(data);
  

• else {

• DA9052_DEBUG("DA9052_TSI: %s:", __func__);
  

• DA9052_DEBUG("Invalid Value passed\n");
  

• da9052_unlock(priv->da9052);
  

• return -EINVAL;
  

• }

When I see else if I wonder if I should be looking at switch()

switch (seq) { case XYZP_MODE: data = enable_xyzp_mode(data); break; case XP_MODE: data = enable_xp_mode(data); break; default: DA9052_DEBUG("DA9052_TSI: %s:", __func__); DA9052_DEBUG("Invalid Value passed\n"); ret = -EINVAL; goto bail; }

• switch (mode) {
• case ECONOMY_MODE:

Hey you like switch too... good!

• da9052_lock(priv->da9052);
• ret = read_da9052_reg(priv->da9052, DA9052_TSICONTA_REG);

Just an idea... some places in the kernel use a nice convention, if a function requires a lock they prepend it with __, eg, __read_da9052_reg. That makes it real clear at the usage if you should check for lock, or rename the function using it also to use __ since it depends on the lock. Just an idea to make it easier to see lock requirement.

• if ((DA9052_GPIO_PIN_3 != DA9052_GPIO_CONFIG_TSI) ||

• (DA9052_GPIO_PIN_4 != DA9052_GPIO_CONFIG_TSI) ||
  

• (DA9052_GPIO_PIN_5 != DA9052_GPIO_CONFIG_TSI) ||
  

• (DA9052_GPIO_PIN_6 != DA9052_GPIO_CONFIG_TSI) ||
  

• (DA9052_GPIO_PIN_7 != DA9052_GPIO_CONFIG_TSI)) {
  

• printk(KERN_ERR"DA9052_TSI: Configure DA9052 GPIO ");
  

• printk(KERN_ERR"pins for TSI\n");
  

• return -EINVAL;
  

• }

Dunno what I am looking at there... it's all preprocessor defines known at compiletime? Better to use #if and #error then?

• ret = da9052_tsi_config_gpio(priv);

Can it fail? If not lose the ret = otherwise test it.

• ret = da9052_tsi_config_state(priv, DEFAULT_TSI_STATE);
• if (ret) {

• for (cnt = 0; cnt<  NUM_INPUT_DEVS; cnt++) {
  

• 	if (ts->input_devs[cnt] != NULL)
  

• 		input_free_device(ts->input_devs[cnt]);
  

• }
  

• }

None of the { } are needed.

+static ssize_t da9052_tsi_config_gpio(struct da9052_ts_priv *priv) +{

• u8 idx = 0;
• ssize_t ret = 0;
• struct da9052_ssc_msg ssc_msg[priv->tsi_pdata->num_gpio_tsi_register];

I am not sure if dynamic local array sizing is good news in kernel code, I don't recall seeing it anywhere else in kernel. Consider just defining it to be the maximum extent?

• idx = 0;
• ssc_msg[idx].data = clear_bits(ssc_msg[idx].data,

• 				DA9052_GPIO0203_GPIO3PIN);
  

• idx++;

Simplify to ssg_msg[idx++].data...

• if (state == ENABLE)

• data = set_auto_tsi_en(data);
  

• else if (state == DISABLE)

• data = reset_auto_tsi_en(data);
  

• else {

• DA9052_DEBUG("DA9052_TSI: %s:", __func__);
  

• DA9052_DEBUG("Invalid Parameter Passed\n");
  

• da9052_unlock(priv->da9052);
  

• return -EINVAL;
  

• }

Maybe switch? I guess it's just matter of taste.

You can change the code structure a bit to get away from excessive indentation -->

+static u32 da9052_tsi_get_reg_data(struct da9052_ts_priv *priv) +{

• u32 free_cnt, copy_cnt, cnt;
• if (down_interruptible(&priv->tsi_reg_fifo.lock))

• return 0;
  

• copy_cnt = 0;
• if ((priv->tsi_reg_fifo.head - priv->tsi_reg_fifo.tail)> 1) {

• free_cnt = get_reg_free_space_cnt(priv);
  

• if (free_cnt>  TSI_POLL_SAMPLE_CNT)
  

• 	free_cnt = TSI_POLL_SAMPLE_CNT;
  

• cnt = da9052_tsi_get_rawdata(
  

• 	&priv->tsi_reg_fifo.data[priv->tsi_reg_fifo.tail],
  

• 	free_cnt);
  

• if (cnt>  free_cnt) {
  

• 	DA9052_DEBUG("EH copied more data");
  

• 	return -EINVAL;
  

• }
  

• copy_cnt = cnt;
  

• while (cnt--)
  

• 	incr_with_wrap_reg_fifo(priv->tsi_reg_fifo.tail);
  

goto bail;

} if ((priv->tsi_reg_fifo.head - priv->tsi_reg_fifo.tail) > 0) {

copy_cnt = 0; goto bail; }

[ loses one level of tabbing ]

• free_cnt = (TSI_REG_DATA_BUF_SIZE - priv->tsi_reg_fifo.tail);
  

• if (free_cnt>  TSI_POLL_SAMPLE_CNT) {
  

• 	free_cnt = TSI_POLL_SAMPLE_CNT;
  

• 	cnt = da9052_tsi_get_rawdata(
  

• 	&priv->tsi_reg_fifo.data[priv->tsi_reg_fifo.tail],
  

• 	free_cnt);
  

• 	if (cnt>  free_cnt) {
  

• 		DA9052_DEBUG("EH copied more data");
  

• 		return -EINVAL;
  

• 	}
  

• 	copy_cnt = cnt;
  

• 	while (cnt--)
  

• 		incr_with_wrap_reg_fifo(
  

• 				priv->tsi_reg_fifo.tail);
  

goto bail; }

[ loses second level of tabbing ]

• 	if (free_cnt) {
  

• 		cnt = da9052_tsi_get_rawdata(
  

• 			&priv->
  

• 			tsi_reg_fifo.data[priv->
  

• 			tsi_reg_fifo.tail],
  

• 			free_cnt
  

• 			);
  

• 		if (cnt>  free_cnt) {
  

• 			DA9052_DEBUG("EH copied more data");
  

• 			return -EINVAL;
  

• 		}
  

• 		copy_cnt = cnt;
  

• 		while (cnt--)
  

• 			incr_with_wrap_reg_fifo(
  

• 				priv->tsi_reg_fifo.tail);
  

• 	}
  

• 	free_cnt = priv->tsi_reg_fifo.head;
  

• 	if (free_cnt>  TSI_POLL_SAMPLE_CNT - copy_cnt)
  

• 		free_cnt = TSI_POLL_SAMPLE_CNT - copy_cnt;
  

• 	if (free_cnt) {
  

• 		cnt = da9052_tsi_get_rawdata(
  

• 			&priv->tsi_reg_fifo.data[priv->
  

• 			tsi_reg_fifo.tail], free_cnt
  

• 			);
  

• 		if (cnt>  free_cnt) {
  

• 			DA9052_DEBUG("EH copied more data");
  

• 			return -EINVAL;
  

• 		}
  

• 		copy_cnt += cnt;
  

• 		while (cnt--)
  

• 			incr_with_wrap_reg_fifo(
  

• 				priv->tsi_reg_fifo.tail);
  

• 	}
  

bail:

• if (data_cnt)
  

• 	ts->tsi_zero_data_cnt = 0;
  

• else {
  

Don't need { } on else

• } else if (tsi_reg.tsi_fifo_start> tsi_reg.tsi_fifo_end) {

• data_cnt = ((TSI_FIFO_SIZE - tsi_reg.tsi_fifo_start)
  

• + tsi_reg.tsi_fifo_end);
  

Trailing operator + should go at end of previous line

• xform.Divider = ((screenPtr[0].x - screenPtr[1].x)

• 	* (screenPtr[1].y - screenPtr[2].y))
  

• 	- ((screenPtr[1].x - screenPtr[2].x)
  

• 	* (screenPtr[0].y - screenPtr[1].y));
  

Trailing operators again need to go at end of previous line... several more instances here.

+#if (ENABLE_AVERAGE_FILTER)

Dude I feel your pain.

I tried a couple of years ago to add a touchscreen driver for Openmoko which had generic awesome median filtering code I wrote.

The median filter effectiveness for resistive touchscreen noise (low probability excursions) was really amazing.

Maybe the opinion changed, or this is small enough it won't get noticed -- my patches established a filter chain class at that time so it was pretty easy to hate on -- but while some people liked it, mainly there were many people following what they thought was the received wisdom that filtering should be done in userspace. Never mind the issue of decimation vs userspace wakeups.

So good luck.

• if (priv->tsi_reg_fifo.head<= priv->tsi_reg_fifo.tail) {

• reg_data_cnt = (priv->tsi_reg_fifo.tail -
  

• 		priv->tsi_reg_fifo.head);
  

• } else {

• reg_data_cnt = (priv->tsi_reg_fifo.tail +
  

• 		(TSI_REG_DATA_BUF_SIZE -
  

• 		priv->tsi_reg_fifo.head));
  

• }

None of the {} are needed.

• if (priv->tsi_reg_fifo.head<= priv->tsi_reg_fifo.tail) {

• free_cnt = ((TSI_REG_DATA_BUF_SIZE - 1) -
  

• 	(priv->tsi_reg_fifo.tail - priv->tsi_reg_fifo.head));
  

• } else

{} not needed

+# if (ENABLE_WINDOW_FILTER)

• ret = da9052_tsi_window_filter(priv,&tmp_raw_data);
  

• if (ret != SUCCESS)
  

• 	continue;
  

+#endif

It might make sense to eliminate #if here by always defining the filter functions, and giving them empty body via #if just in there if not enabled.

• priv->early_data_flag = FALSE;
  

• if (down_interruptible(&priv->tsi_raw_fifo.lock)) {
  

• 	DA9052_DEBUG("%s: Failed to ", __func__);
  

• 	DA9052_DEBUG("acquire RAW FIFO Lock!\n");
  

• 	up(&priv->tsi_reg_fifo.lock);
  

• 	return;
  

• }
  

• priv->tsi_raw_fifo.data[priv->tsi_raw_fifo.tail] = tmp_raw_data;
  

• incr_with_wrap_raw_fifo(priv->tsi_raw_fifo.tail);
  

• up(&priv->tsi_raw_fifo.lock);
  

• }

Can lose a blank line.

• up(&priv->tsi_reg_fifo.lock);
• return;

Lose the "return" on void function, does nothing.

+}

+static u32 get_raw_data_cnt(struct da9052_ts_priv *priv) +{

• u32 raw_data_cnt;
• if (priv->tsi_raw_fifo.head<= priv->tsi_raw_fifo.tail)

• raw_data_cnt =
  

• 	(priv->tsi_raw_fifo.tail - priv->tsi_raw_fifo.head);
  

• else

• raw_data_cnt =
  

• 	(priv->tsi_raw_fifo.tail + (TSI_RAW_DATA_BUF_SIZE -
  

• 	priv->tsi_raw_fifo.head));
  

• return raw_data_cnt;

+}

+static void da9052_tsi_convert_reg_to_coord(struct da9052_ts_priv *priv,

• 			struct da9052_tsi_data *raw_data)
  

+{

• struct da9052_tsi_reg *src;
• struct da9052_tsi_data *dst = raw_data;
• src =&priv->tsi_reg_fifo.data[priv->tsi_reg_fifo.head];
• dst->x = (src->x_msb<< X_MSB_SHIFT);
• dst->x |= (src->lsb& X_LSB_MASK)>> X_LSB_SHIFT;

Unify into one expression with |

• tsi_avg_data->x /= TSI_AVERAGE_FILTER_SIZE;
• tsi_avg_data->y /= TSI_AVERAGE_FILTER_SIZE;
• tsi_avg_data->z /= TSI_AVERAGE_FILTER_SIZE;

Snif so many memories...

+#if DA9052_TSI_AVG_FLT_DATA_PROFILING

• printk(KERN_DEBUG "A\tX\t%4d\tY\t%4d\tZ\t%4d\n",

• 	(u16)tsi_avg_data->x,
  

• 	(u16)tsi_avg_data->y,
  

• 	(u16)tsi_avg_data->z);
  

+#endif

• return;

Lose the return.

+static s32 diff_within_window(struct da9052_tsi_data *prev_raw_data,

• 		struct da9052_tsi_data *cur_raw_data)
  

+{

• s32 ret = -EINVAL;
• s32 x, y;
• x = ((cur_raw_data->x) - (prev_raw_data->x));
• y = ((cur_raw_data->y) - (prev_raw_data->y));

No need for any ()

• if (priv->win_reference_valid == TRUE) {

Do you really need to have == TRUE? How about

if (priv->win_reference_valid) {

• 	prev_raw_data = (*raw_data);
  

• } else {

• priv->tsi_raw_fifo.data[priv->tsi_raw_fifo.tail] = (*raw_data);
  

Lose the () in both cases

• incr_with_wrap_raw_fifo(priv->tsi_raw_fifo.tail);
  

• if (get_raw_data_cnt(priv) == SAMPLE_CNT_FOR_WIN_REF) {
  

• 	ref_found = FALSE;
  

I think you can use 1 or 0 or true or false in this kind of situation.

• 	if (ref_found == FALSE)
  

if (!ref_found)

• 		priv->tsi_raw_fifo.tail =
  

• 			priv->tsi_raw_fifo.head;
  

• 	else {
  

• 		prev_raw_data = priv->tsi_raw_fifo.data[cur];
  

• 		prev_raw_data.x +=
  

• 			priv->tsi_raw_fifo.data[next].x;
  

• 		prev_raw_data.y +=
  

• 			priv->tsi_raw_fifo.data[next].y;
  

• 		prev_raw_data.z +=
  

• 			priv->tsi_raw_fifo.data[next].z;
  

• 		prev_raw_data.x = prev_raw_data.x / 2;
  

• 		prev_raw_data.y = prev_raw_data.y / 2;
  

• 		prev_raw_data.z = prev_raw_data.z / 2;
  

prev_raw_data.z /= 2;

• 		(*raw_data) = prev_raw_data;
  

Lose the ()

• 		priv->tsi_raw_fifo.tail =
  

• 				priv->tsi_raw_fifo.head;
  

• 		priv->win_reference_valid = TRUE;
  

• 		ret = SUCCESS;
  

Just ret = 0 is fine

• 	}
  

• }
  

• }
• return ret;

+} +#endif +static inline void clean_tsi_reg_fifo(struct da9052_ts_priv *priv) +{

• priv->tsi_reg_fifo.head = FIRST_SAMPLE;
• priv->tsi_reg_fifo.tail = FIRST_SAMPLE;

+}

+static inline void clean_tsi_raw_fifo(struct da9052_ts_priv *priv) +{

• priv->tsi_raw_fifo.head = FIRST_SAMPLE;
• priv->tsi_raw_fifo.tail = FIRST_SAMPLE;

+}

No need for inline