On Tue 1 Sep 2009 23:17, Song, Barry pondered: > 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. style is different than performance.... Did you think about Mike's comment - I think it was a performance issue he was talking about. On Tue 1 Sep 2009 23:09, Mike Frysinger pondered: > > 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. > >-----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 > > > _______________________________________________ > Uclinux-dist-devel mailing list > Uclinux-dist-devel@xxxxxxxxxxxxxxxxxxxx > https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel > -- 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
