No. I can't agree other coding style issues you said. I will keep my original codes for these issues except adding a BUS_SPI. >-----Original Message----- >From: Mike Frysinger [mailto:vapier.adi@xxxxxxxxx] >Sent: Wednesday, September 02, 2009 11:09 AM >To: Song, Barry >Cc: Barry Song; dbrownell@xxxxxxxxxxxxxxxxxxxxx; dtor@xxxxxxx; >dmitry.torokhov@xxxxxxxxx; >spi-devel-general@xxxxxxxxxxxxxxxxxxxxx; >linux-input@xxxxxxxxxxxxxxx; uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx >Subject: Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input >driver forbutton/scrollwhell/slider/touchpad > >On Tue, Sep 1, 2009 at 22:46, Song, Barry wrote: >>>From: Mike Frysinger >>>> +struct ad714x_chip { >>>> + unsigned short h_state; >>>> + unsigned short l_state; >>>> + unsigned short c_state; >>>> + unsigned short adc_reg[STAGE_NUM]; >>>> + unsigned short amb_reg[STAGE_NUM]; >>>> + unsigned short sensor_val[STAGE_NUM]; >>>> + >>>> + struct ad714x_platform_data *hw; >>>> + struct ad714x_driver_data *sw; >>>> + >>>> + bus_device *bus; >>>> + int (*read) (bus_device *, unsigned short, >unsigned short *); >>>> + int (*write) (bus_device *, unsigned short, >unsigned short); >>> >>>using function pointers is pure overhead in your current >>>implementation. create a ad714x_read macro that expands to >either the >>>spi or the i2c version depending on which is enabled. >> >> Using macro can be a choice. But I don't think it can save >much overhead here. >> Function pointers encapsulates the object better. > >did you look at the generated code ? function pointers requires >pointer loads and indirect calls. telling gcc that the function is >completely static allows for direct calls (and thus less register >pressure) and if gcc gets clever enough, avoid call overhead. > >>>> +static int stage_cal_abs_pos(struct ad714x_chip *ad714x, >>>>>int start_stage, int end_stage, int highest_stage, int max_coord) >>>> +{ >>>> + int a_param, b_param; >>>> + >>>> + if (highest_stage == start_stage) { >>>> + a_param = ad714x->sensor_val[start_stage + 1]; >>>> + b_param = ad714x->sensor_val[start_stage] + >>>> + ad714x->sensor_val[start_stage + 1]; >>>> + } else if (highest_stage == end_stage) { >>>> + a_param = ad714x->sensor_val[end_stage] * >>>> + (end_stage - start_stage) + >>>> + ad714x->sensor_val[end_stage - 1] * >>>> + (end_stage - start_stage - 1); >>>> + b_param = ad714x->sensor_val[end_stage] + >>>> + ad714x->sensor_val[end_stage - 1]; >>>> + } else { >>>> + a_param = ad714x->sensor_val[highest_stage] * >>>> + (highest_stage - start_stage) + >>>> + ad714x->sensor_val[highest_stage - 1] * >>>> + (highest_stage - start_stage - 1) + >>>> + ad714x->sensor_val[highest_stage + 1] * >>>> + (highest_stage - start_stage + 1); >>>> + b_param = ad714x->sensor_val[highest_stage] + >>>> + ad714x->sensor_val[highest_stage - 1] + >>>> + ad714x->sensor_val[highest_stage + 1]; >>>> + } >>>> + >>>> + return (max_coord / (end_stage - start_stage)) * >>>a_param / b_param; >>>> +} >>> >>>do the local vars really need "_stage" suffix ? if you trimmed that, >>>i imagine it'd make the code a bit easier to digest. >> >> Yes. It need the _stage suffix. Otherwise, nobody know >what's started, what's ended. > >that doesnt make sense. if it's labeled "start" and "end" in the >local function, it's pretty obvious. > >>>> + input[alloc_idx-1]->id.bustype = BUS_I2C; >>> >>>you set bustype to I2C in common code even though this will be >>>executed for both SPI and I2C interfaces >> >> There is a BUS_I2C, but no BUS_SPI. So people may use other >serial type to imply. >> Do you think we should add a macro in input.h? I am really >suprised why the macro hasn't been added until now. > >yes, a new BUS_SPI should be added > >>>> +{ >>>> + int ret; >>>> + unsigned short tx[2]; >>>> + unsigned short rx[2]; >>>> + struct spi_transfer t = { >>>> + .tx_buf = tx, >>>> + .rx_buf = rx, >>>> + .len = 4, >>>> + }; >>>> + struct spi_message m; >>>> + >>>> + tx[0] = (AD714x_SPI_ADDR << AD714x_SPI_ADDR_SHFT) | >>>> + (AD714x_SPI_READ << AD714x_SPI_READ_SHFT) | reg; >>>> + >>>> + spi_message_init(&m); >>>> + spi_message_add_tail(&t, &m); >>>> + ret = spi_sync(spi, &m); >>> >>>cant you use spi_write_then_read() ? dont let the u8* >prototype scare >>>you, it should work with writing 16bits and then reading 16bits. >> >> I have never been scared by any u8* or something else. I >only prefer to use raw spi API, which can show the bottom >level timing and SPI bus feature better. >> In fact, I prefer to use raw i2c API too. > >that doesnt make sense. the entire purpose of writing these helper >write/read functions is because people often do just that -- write a >few then read a few. your concerns are purely superficial. >-mike > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
