RE: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driver forbutton/scrollwhell/slider/touchpad

["Song, Barry" <Barry.Song@analog.com>]

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@gmail.com] 
>Sent: Wednesday, September 02, 2009 11:09 AM
>To: Song, Barry
>Cc: Barry Song; dbrownell@users.sourceforge.net; dtor@mail.ru; 
>dmitry.torokhov@gmail.com; 
>spi-devel-general@lists.sourceforge.net; 
>linux-input@vger.kernel.org; uclinux-dist-devel@blackfin.uclinux.org
>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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html