Re: [Uclinux-dist-devel] [PATCH 2/2] add ad714x input driverforbutton/scrollwhell/slider/touchpad

[Robin Getz <rgetz@blackfin.uclinux.org>]

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@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
> >
> _______________________________________________
> Uclinux-dist-devel mailing list
> Uclinux-dist-devel@blackfin.uclinux.org
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html