Re: [PATCH] TSC2007: private data for platform callbacks.

Re: [PATCH] TSC2007: private data for platform callbacks.

[Trilok Soni]

Hi Manuel,

CCing linux-input mailing list, so not deleting any code from this
e-mail while replying.

On Thu, May 14, 2009 at 3:39 PM, Manuel Lauss
<mano@roarinelk.homelinux.net> wrote:
> Add a private data field to the tsc2007 platform data and pass this
> to the callbacks.

I hope that you have created this patch on top of two recently posted
patches by Kwangwoo Lee
to merge changes from Thierry.

>
> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> ---
>  drivers/input/touchscreen/tsc2007.c |   52 +++++++++++++++++++++++-----------
>  include/linux/i2c/tsc2007.h         |   10 ++++---
>  2 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 948e167..5c4242d 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -83,10 +83,35 @@ struct tsc2007 {
>        unsigned                pendown;
>        int                     irq;
>
> -       int                     (*get_pendown_state)(void);
> -       void                    (*clear_penirq)(void);
> +       struct tsc2007_platform_data *pd;
>  };
>
> +/* ask platform for pendown state */
> +static inline int ts_get_pendown_state(struct tsc2007 *ts)
> +{
> +       return ts->pd->get_pendown_state(ts->pd->priv);

So, we don't need check here, like this:

if (ts->pd->get_pendown_state)
          ret =  ts->pd->get_pendown_state(ts->pd->priv);

> +}
> +
> +/* ask platform to clear pendown irq if available */
> +static inline void ts_clear_penirq(struct tsc2007 *ts)
> +{
> +       if (ts->pd->clear_penirq)
> +               ts->pd->clear_penirq(ts->pd->priv);
> +}
> +
> +static inline void ts_init_platform_hw(struct tsc2007 *ts)
> +{
> +       if (ts->pd->init_platform_hw)
> +               ts->pd->init_platform_hw(ts->pd->priv);
> +}
> +
> +static inline void ts_exit_platform_hw(struct tsc2007 *ts)
> +{
> +       if (ts->pd->exit_platform_hw)
> +               ts->pd->exit_platform_hw(ts->pd->priv);
> +}
> +
> +
>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>  {
>        s32 data;
> @@ -129,7 +154,7 @@ static void tsc2007_send_event(void *tsc)
>                rt = z2;
>                rt -= z1;
>                rt *= x;
> -               rt *= ts->x_plate_ohms;
> +               rt *= ts->pd->x_plate_ohms;
>                rt /= z1;
>                rt = (rt + 2047) >> 12;
>        } else
> @@ -204,7 +229,7 @@ static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
>
>        spin_lock_irqsave(&ts->lock, flags);
>
> -       if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
> +       if (unlikely(!ts_get_pendown_state(ts) && ts->pendown)) {
>                struct input_dev *input = ts->input;
>
>                dev_dbg(&ts->client->dev, "UP\n");
> @@ -235,14 +260,13 @@ static irqreturn_t tsc2007_irq(int irq, void *handle)
>
>        spin_lock_irqsave(&ts->lock, flags);
>
> -       if (likely(ts->get_pendown_state())) {
> +       if (likely(ts_get_pendown_state(ts))) {
>                disable_irq_nosync(ts->irq);
>                hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
>                                        HRTIMER_MODE_REL);
>        }
>
> -       if (ts->clear_penirq)
> -               ts->clear_penirq();
> +       ts_clear_penirq(ts);
>
>        spin_unlock_irqrestore(&ts->lock, flags);
>
> @@ -253,7 +277,7 @@ static int tsc2007_probe(struct i2c_client *client,
>                        const struct i2c_device_id *id)
>  {
>        struct tsc2007 *ts;
> -       struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data;
> +       struct tsc2007_platform_data *pdata = client->dev.platform_data;

This change shows that you need to rebase based on two patches
submitted by Lee recently.

>        struct input_dev *input_dev;
>        int err;
>
> @@ -283,12 +307,8 @@ static int tsc2007_probe(struct i2c_client *client,
>
>        spin_lock_init(&ts->lock);
>
> -       ts->model             = pdata->model;
> -       ts->x_plate_ohms      = pdata->x_plate_ohms;
> -       ts->get_pendown_state = pdata->get_pendown_state;
> -       ts->clear_penirq      = pdata->clear_penirq;
> -
> -       pdata->init_platform_hw();
> +       ts->pd = pdata;
> +       ts_init_platform_hw(ts);
>
>        snprintf(ts->phys, sizeof(ts->phys),
>                 "%s/input0", dev_name(&client->dev));
> @@ -335,10 +355,8 @@ static int tsc2007_probe(struct i2c_client *client,
>  static int tsc2007_remove(struct i2c_client *client)
>  {
>        struct tsc2007  *ts = i2c_get_clientdata(client);
> -       struct tsc2007_platform_data *pdata;
>
> -       pdata = client->dev.platform_data;
> -       pdata->exit_platform_hw();
> +       ts_exit_platform_hw(ts);
>
>        free_irq(ts->irq, ts);
>        hrtimer_cancel(&ts->timer);
> diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
> index c6361fb..45582b3 100644
> --- a/include/linux/i2c/tsc2007.h
> +++ b/include/linux/i2c/tsc2007.h
> @@ -7,11 +7,13 @@ struct tsc2007_platform_data {
>        u16     model;                          /* 2007. */
>        u16     x_plate_ohms;
>
> -       int     (*get_pendown_state)(void);
> -       void    (*clear_penirq)(void);          /* If needed, clear 2nd level
> +       int     (*get_pendown_state)(void *priv);
> +       void    (*clear_penirq)(void *priv);    /* If needed, clear 2nd level
>                                                   interrupt source */
> -       int     (*init_platform_hw)(void);
> -       void    (*exit_platform_hw)(void);
> +       int     (*init_platform_hw)(void *priv);
> +       void    (*exit_platform_hw)(void *priv);
> +
> +       void    *priv;                          /* passed to callbacks */
>  };
>
>  #endif
> --
> 1.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
---Trilok Soni
http://triloksoni.wordpress.com
http://www.linkedin.com/in/triloksoni
--
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

Re: [PATCH] TSC2007: private data for platform callbacks.

[Manuel Lauss]

Hi Trilok,

On Thu, May 14, 2009 at 03:58:19PM +0530, Trilok Soni wrote:
> CCing linux-input mailing list, so not deleting any code from this
> e-mail while replying.
> 
> On Thu, May 14, 2009 at 3:39 PM, Manuel Lauss
> <mano@roarinelk.homelinux.net> wrote:
> > Add a private data field to the tsc2007 platform data and pass this
> > to the callbacks.
> 
> I hope that you have created this patch on top of two recently posted
> patches by Kwangwoo Lee
> to merge changes from Thierry.

No; it's against Linux's git. Will rediff.


 
> > - ? ? ? int ? ? ? ? ? ? ? ? ? ? (*get_pendown_state)(void);
> > - ? ? ? void ? ? ? ? ? ? ? ? ? ?(*clear_penirq)(void);
> > + ? ? ? struct tsc2007_platform_data *pd;
> > ?};
> >
> > +/* ask platform for pendown state */
> > +static inline int ts_get_pendown_state(struct tsc2007 *ts)
> > +{
> > + ? ? ? return ts->pd->get_pendown_state(ts->pd->priv);
> 
> So, we don't need check here, like this:
> 
> if (ts->pd->get_pendown_state)
>           ret =  ts->pd->get_pendown_state(ts->pd->priv);

The driver doesn't work right without that callback so the check
in here is kind of pointless. Better to check for it in the
probe function and refuse to probe if it isn't there.  All other
callbacks seem optional to me.

 
> > ? ? ? ?struct tsc2007 *ts;
> > - ? ? ? struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data;
> > + ? ? ? struct tsc2007_platform_data *pdata = client->dev.platform_data;
> 
> This change shows that you need to rebase based on two patches
> submitted by Lee recently.

Ah, that shouldn't be in there in the first place.

Thanks!
	Manuel Lauss

Re: [PATCH] TSC2007: private data for platform callbacks.

[Kwangwoo Lee]

Hi Manual,

On Thu, May 14, 2009 at 7:28 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> Hi Manuel,
>
> CCing linux-input mailing list, so not deleting any code from this
> e-mail while replying.
>
> On Thu, May 14, 2009 at 3:39 PM, Manuel Lauss
> <mano@roarinelk.homelinux.net> wrote:
>> Add a private data field to the tsc2007 platform data and pass this
>> to the callbacks.
>
> I hope that you have created this patch on top of two recently posted
> patches by Kwangwoo Lee
> to merge changes from Thierry.
>
>>
>> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
>> ---
>>  drivers/input/touchscreen/tsc2007.c |   52 +++++++++++++++++++++++-----------
>>  include/linux/i2c/tsc2007.h         |   10 ++++---
>>  2 files changed, 41 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
>> index 948e167..5c4242d 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -83,10 +83,35 @@ struct tsc2007 {
>>        unsigned                pendown;
>>        int                     irq;
>>
>> -       int                     (*get_pendown_state)(void);
>> -       void                    (*clear_penirq)(void);
>> +       struct tsc2007_platform_data *pd;

Why is this pd is contained in struct tsc2007?

>>  };
>>
>> +/* ask platform for pendown state */
>> +static inline int ts_get_pendown_state(struct tsc2007 *ts)
>> +{
>> +       return ts->pd->get_pendown_state(ts->pd->priv);

Is it required in get_pendown_state() function in your platform?
The function just return true/false indicating the pendown state.

>
> So, we don't need check here, like this:
>
> if (ts->pd->get_pendown_state)
>          ret =  ts->pd->get_pendown_state(ts->pd->priv);
>
>> +}
>> +
>> +/* ask platform to clear pendown irq if available */
>> +static inline void ts_clear_penirq(struct tsc2007 *ts)
>> +{
>> +       if (ts->pd->clear_penirq)
>> +               ts->pd->clear_penirq(ts->pd->priv);
>> +}
>> +
>> +static inline void ts_init_platform_hw(struct tsc2007 *ts)
>> +{
>> +       if (ts->pd->init_platform_hw)
>> +               ts->pd->init_platform_hw(ts->pd->priv);
>> +}
>> +
>> +static inline void ts_exit_platform_hw(struct tsc2007 *ts)
>> +{
>> +       if (ts->pd->exit_platform_hw)
>> +               ts->pd->exit_platform_hw(ts->pd->priv);
>> +}
>> +
>> +
>>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>>  {
>>        s32 data;
>> @@ -129,7 +154,7 @@ static void tsc2007_send_event(void *tsc)
>>                rt = z2;
>>                rt -= z1;
>>                rt *= x;
>> -               rt *= ts->x_plate_ohms;
>> +               rt *= ts->pd->x_plate_ohms;
>>                rt /= z1;
>>                rt = (rt + 2047) >> 12;
>>        } else
>> @@ -204,7 +229,7 @@ static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
>>
>>        spin_lock_irqsave(&ts->lock, flags);
>>
>> -       if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
>> +       if (unlikely(!ts_get_pendown_state(ts) && ts->pendown)) {
>>                struct input_dev *input = ts->input;
>>
>>                dev_dbg(&ts->client->dev, "UP\n");
>> @@ -235,14 +260,13 @@ static irqreturn_t tsc2007_irq(int irq, void *handle)
>>
>>        spin_lock_irqsave(&ts->lock, flags);
>>
>> -       if (likely(ts->get_pendown_state())) {
>> +       if (likely(ts_get_pendown_state(ts))) {
>>                disable_irq_nosync(ts->irq);
>>                hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
>>                                        HRTIMER_MODE_REL);
>>        }
>>
>> -       if (ts->clear_penirq)
>> -               ts->clear_penirq();
>> +       ts_clear_penirq(ts);
>>
>>        spin_unlock_irqrestore(&ts->lock, flags);
>>
>> @@ -253,7 +277,7 @@ static int tsc2007_probe(struct i2c_client *client,
>>                        const struct i2c_device_id *id)
>>  {
>>        struct tsc2007 *ts;
>> -       struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data;
>> +       struct tsc2007_platform_data *pdata = client->dev.platform_data;
>
> This change shows that you need to rebase based on two patches
> submitted by Lee recently.
>
>>        struct input_dev *input_dev;
>>        int err;
>>
>> @@ -283,12 +307,8 @@ static int tsc2007_probe(struct i2c_client *client,
>>
>>        spin_lock_init(&ts->lock);
>>
>> -       ts->model             = pdata->model;
>> -       ts->x_plate_ohms      = pdata->x_plate_ohms;
>> -       ts->get_pendown_state = pdata->get_pendown_state;
>> -       ts->clear_penirq      = pdata->clear_penirq;
>> -
>> -       pdata->init_platform_hw();
>> +       ts->pd = pdata;
>> +       ts_init_platform_hw(ts);
>>
>>        snprintf(ts->phys, sizeof(ts->phys),
>>                 "%s/input0", dev_name(&client->dev));
>> @@ -335,10 +355,8 @@ static int tsc2007_probe(struct i2c_client *client,
>>  static int tsc2007_remove(struct i2c_client *client)
>>  {
>>        struct tsc2007  *ts = i2c_get_clientdata(client);
>> -       struct tsc2007_platform_data *pdata;
>>
>> -       pdata = client->dev.platform_data;
>> -       pdata->exit_platform_hw();
>> +       ts_exit_platform_hw(ts);
>>
>>        free_irq(ts->irq, ts);
>>        hrtimer_cancel(&ts->timer);
>> diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
>> index c6361fb..45582b3 100644
>> --- a/include/linux/i2c/tsc2007.h
>> +++ b/include/linux/i2c/tsc2007.h
>> @@ -7,11 +7,13 @@ struct tsc2007_platform_data {
>>        u16     model;                          /* 2007. */
>>        u16     x_plate_ohms;
>>
>> -       int     (*get_pendown_state)(void);
>> -       void    (*clear_penirq)(void);          /* If needed, clear 2nd level
>> +       int     (*get_pendown_state)(void *priv);
>> +       void    (*clear_penirq)(void *priv);    /* If needed, clear 2nd level
>>                                                   interrupt source */
>> -       int     (*init_platform_hw)(void);
>> -       void    (*exit_platform_hw)(void);
>> +       int     (*init_platform_hw)(void *priv);
>> +       void    (*exit_platform_hw)(void *priv);
>> +
>> +       void    *priv;                          /* passed to callbacks */
>>  };
>>
>>  #endif
>> --
>> 1.6.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>
>
>
> --
> ---Trilok Soni
> http://triloksoni.wordpress.com
> http://www.linkedin.com/in/triloksoni
> --
> 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
>

Regards,

-- 
Kwangwoo Lee <kwangwoo.lee@gmail.com>
--
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

Re: [PATCH] TSC2007: private data for platform callbacks.

[Kwangwoo Lee]

On Thu, May 14, 2009 at 7:37 PM, Manuel Lauss
<mano@roarinelk.homelinux.net> wrote:
> Hi Trilok,
>
> On Thu, May 14, 2009 at 03:58:19PM +0530, Trilok Soni wrote:
>> CCing linux-input mailing list, so not deleting any code from this
>> e-mail while replying.
>>
>> On Thu, May 14, 2009 at 3:39 PM, Manuel Lauss
>> <mano@roarinelk.homelinux.net> wrote:
>> > Add a private data field to the tsc2007 platform data and pass this
>> > to the callbacks.
>>
>> I hope that you have created this patch on top of two recently posted
>> patches by Kwangwoo Lee
>> to merge changes from Thierry.
>
> No; it's against Linux's git. Will rediff.
>
>
>
>> > - ? ? ? int ? ? ? ? ? ? ? ? ? ? (*get_pendown_state)(void);
>> > - ? ? ? void ? ? ? ? ? ? ? ? ? ?(*clear_penirq)(void);
>> > + ? ? ? struct tsc2007_platform_data *pd;
>> > ?};
>> >
>> > +/* ask platform for pendown state */
>> > +static inline int ts_get_pendown_state(struct tsc2007 *ts)
>> > +{
>> > + ? ? ? return ts->pd->get_pendown_state(ts->pd->priv);
>>
>> So, we don't need check here, like this:
>>
>> if (ts->pd->get_pendown_state)
>>           ret =  ts->pd->get_pendown_state(ts->pd->priv);
>
> The driver doesn't work right without that callback so the check
> in here is kind of pointless. Better to check for it in the
> probe function and refuse to probe if it isn't there.  All other
> callbacks seem optional to me.

I agree. This function should be checked in the probe one.

>
>
>> > ? ? ? ?struct tsc2007 *ts;
>> > - ? ? ? struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data;
>> > + ? ? ? struct tsc2007_platform_data *pdata = client->dev.platform_data;
>>
>> This change shows that you need to rebase based on two patches
>> submitted by Lee recently.
>
> Ah, that shouldn't be in there in the first place.

This should be fixed.

>
> Thanks!
>        Manuel Lauss
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Thanks,

-- 
Kwangwoo Lee <kwangwoo.lee@gmail.com>

Re: [PATCH] TSC2007: private data for platform callbacks.

[Manuel Lauss]

On Fri, May 15, 2009 at 01:25:41PM +0900, Kwangwoo Lee wrote:
> Hi Manual,
> 
> On Thu, May 14, 2009 at 7:28 PM, Trilok Soni <soni.trilok@gmail.com> wrote:
> > Hi Manuel,
> >
> > CCing linux-input mailing list, so not deleting any code from this
> > e-mail while replying.
> >
> > On Thu, May 14, 2009 at 3:39 PM, Manuel Lauss
> > <mano@roarinelk.homelinux.net> wrote:
> >> Add a private data field to the tsc2007 platform data and pass this
> >> to the callbacks.
> >
> > I hope that you have created this patch on top of two recently posted
> > patches by Kwangwoo Lee
> > to merge changes from Thierry.
> >
> >>
> >> Signed-off-by: Manuel Lauss <mano@roarinelk.homelinux.net>
> >> ---
> >> ?drivers/input/touchscreen/tsc2007.c | ? 52 +++++++++++++++++++++++-----------
> >> ?include/linux/i2c/tsc2007.h ? ? ? ? | ? 10 ++++---
> >> ?2 files changed, 41 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> >> index 948e167..5c4242d 100644
> >> --- a/drivers/input/touchscreen/tsc2007.c
> >> +++ b/drivers/input/touchscreen/tsc2007.c
> >> @@ -83,10 +83,35 @@ struct tsc2007 {
> >> ? ? ? ?unsigned ? ? ? ? ? ? ? ?pendown;
> >> ? ? ? ?int ? ? ? ? ? ? ? ? ? ? irq;
> >>
> >> - ? ? ? int ? ? ? ? ? ? ? ? ? ? (*get_pendown_state)(void);
> >> - ? ? ? void ? ? ? ? ? ? ? ? ? ?(*clear_penirq)(void);
> >> + ? ? ? struct tsc2007_platform_data *pd;
> 
> Why is this pd is contained in struct tsc2007?

It just seemed nicer to me to store a pointer to the platform data
instead of duplicating its members.

 
> >> +/* ask platform for pendown state */
> >> +static inline int ts_get_pendown_state(struct tsc2007 *ts)
> >> +{
> >> + ? ? ? return ts->pd->get_pendown_state(ts->pd->priv);
> 
> Is it required in get_pendown_state() function in your platform?
> The function just return true/false indicating the pendown state.

It enables me to reuse the same callbacks if more than one tsc2007 is
soldered on (different bits to fiddle with in external registers).

Thanks,
	Manuel Lauss