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
