Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 24/10/16 20:14, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
>>
>> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
 Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :

 Hi,

>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>> +  struct input_dev **input_dev)
>> +{
>> +   int err;
>> +   struct iio_dev *indio_dev;
>> +
>> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
> Instead of doing this to reduce the delta between versions make 
> iio_priv a struct tsc2007 **
>
> That is have a single pointer in there and do your allocation of struct
> tsc2007 separately.

 Sorry, but I think I do not completely understand what you mean here.

 The problem is that we need to allocate some struct tsc2007 in both cases.
 But in one case managed directly by >dev and in the other managed
 indirectly. This is why I use the private area of struct iio_dev to store
 the full struct tsc2007 and not just a pointer.
>>>
>>> Ok, I think I finally did understand how you mean this and have started to
>>> implement something.
>>>
>> oops. Didn't look on in my emails to get to this one!
>>> The idea is to have one alloc function to return a struct tsc2007. This
>>> can be part of the probe function, like it is in the unpatched driver.
>>>
>>> In case of iio this struct tsc2007 is also allocated explicitly so that
>>> a pointer can be stored in iio_priv.
>>>
>>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>>>
>>> I have added that approach to my inlined patch and it seems to work 
>>> (attached).
>>>
>>> Sorry if I do not use the wording you would use and sometimes overlook
>>> something you have said. I feel here like moving on thin ice and doing
>>> guesswork about unspoken assumptions...
>> That's fine.  Stuff that can appear obvious to one person is not
>> necessarily obvious to another!
>>>

>
> Having doing that, you can have this CONFIG_IIO block as just
> doing the iio stuff with the input elements pulled back into the main
> probe function.
>
> Then define something like
>
> iio_configure (stubbed to nothing if no IIO)
> and
> iio_unconfigure (also stubbed to nothing if no IIO).
>>>
>>> This seems to work (draft attached).
>>>
>
> A couple of additions in the header
>>>
>>> I think you mean tsc2007.h?
>> Nope. A local header alongside the driver is what you want for this stuff.
>> driver/input/tsc2007.h 
>>>
>>> This currently contains only platform data and could IMHO be eliminated
>>> if everything becomes DT.
>>>
> to make it all work
> (the struct tsc2007 and tsc2007_xfer() + a few of the
> register defines..
>>>
>>> Here it appears to me that I have to make a lot of so far private static
>>> and even static inline functions public so that I can make them stubs and
>>> call them from tsc2007_iio.c.
>> There will be a few.
>>>
>>> And for having proper parameter types I have to make most private structs
>>> also public.
>> Yes a few of those as well.
>>>
>>> I really like the idea to have the optional iio feature in a separate source
>>> file, but when really starting to write code, I get the impression that
>>> it introduces more problems than it solves.
>>>
>>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>>> as well. There are also two static function in some #ifdef #else # endif
>>> and not going through stubs.
>> Usually it is only done once a certain volume of code exists.
>>>
>>> So is this intended to give up some static definitions?
>> Yes, that happens the moment you have multiple source files.
>>
>> Some losses but generally end up with clean code separation. Always a trade
>> off unfortunately.  Pity we can't just insist IIO is available! Rather large
>> to pull in for what is probable a niche use case.
>>
>> Below is definitely heading in the right direction. I remember vaguely being
>> convinced of the worth of doing this when optional code is involved!
>> (was a good while ago now)
>>
>> Jonathan
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>>> b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..92da8f6 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>> Should not need this after introducing the new file.  Will only be
>> needed in the iio specific .c file.
>>>
>>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>>> @@ -98,6 +99,9 @@ struct tsc2007 {
>> This will definitely need to go in the header though.
> 
> Now I have split the code into:
> 
> tsc2007.h (constants, 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 24/10/16 20:14, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
>>
>> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
 Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :

 Hi,

>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>> +  struct input_dev **input_dev)
>> +{
>> +   int err;
>> +   struct iio_dev *indio_dev;
>> +
>> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
> Instead of doing this to reduce the delta between versions make 
> iio_priv a struct tsc2007 **
>
> That is have a single pointer in there and do your allocation of struct
> tsc2007 separately.

 Sorry, but I think I do not completely understand what you mean here.

 The problem is that we need to allocate some struct tsc2007 in both cases.
 But in one case managed directly by >dev and in the other managed
 indirectly. This is why I use the private area of struct iio_dev to store
 the full struct tsc2007 and not just a pointer.
>>>
>>> Ok, I think I finally did understand how you mean this and have started to
>>> implement something.
>>>
>> oops. Didn't look on in my emails to get to this one!
>>> The idea is to have one alloc function to return a struct tsc2007. This
>>> can be part of the probe function, like it is in the unpatched driver.
>>>
>>> In case of iio this struct tsc2007 is also allocated explicitly so that
>>> a pointer can be stored in iio_priv.
>>>
>>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>>>
>>> I have added that approach to my inlined patch and it seems to work 
>>> (attached).
>>>
>>> Sorry if I do not use the wording you would use and sometimes overlook
>>> something you have said. I feel here like moving on thin ice and doing
>>> guesswork about unspoken assumptions...
>> That's fine.  Stuff that can appear obvious to one person is not
>> necessarily obvious to another!
>>>

>
> Having doing that, you can have this CONFIG_IIO block as just
> doing the iio stuff with the input elements pulled back into the main
> probe function.
>
> Then define something like
>
> iio_configure (stubbed to nothing if no IIO)
> and
> iio_unconfigure (also stubbed to nothing if no IIO).
>>>
>>> This seems to work (draft attached).
>>>
>
> A couple of additions in the header
>>>
>>> I think you mean tsc2007.h?
>> Nope. A local header alongside the driver is what you want for this stuff.
>> driver/input/tsc2007.h 
>>>
>>> This currently contains only platform data and could IMHO be eliminated
>>> if everything becomes DT.
>>>
> to make it all work
> (the struct tsc2007 and tsc2007_xfer() + a few of the
> register defines..
>>>
>>> Here it appears to me that I have to make a lot of so far private static
>>> and even static inline functions public so that I can make them stubs and
>>> call them from tsc2007_iio.c.
>> There will be a few.
>>>
>>> And for having proper parameter types I have to make most private structs
>>> also public.
>> Yes a few of those as well.
>>>
>>> I really like the idea to have the optional iio feature in a separate source
>>> file, but when really starting to write code, I get the impression that
>>> it introduces more problems than it solves.
>>>
>>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>>> as well. There are also two static function in some #ifdef #else # endif
>>> and not going through stubs.
>> Usually it is only done once a certain volume of code exists.
>>>
>>> So is this intended to give up some static definitions?
>> Yes, that happens the moment you have multiple source files.
>>
>> Some losses but generally end up with clean code separation. Always a trade
>> off unfortunately.  Pity we can't just insist IIO is available! Rather large
>> to pull in for what is probable a niche use case.
>>
>> Below is definitely heading in the right direction. I remember vaguely being
>> convinced of the worth of doing this when optional code is involved!
>> (was a good while ago now)
>>
>> Jonathan
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>>> b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..92da8f6 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>> Should not need this after introducing the new file.  Will only be
>> needed in the iio specific .c file.
>>>
>>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>>> @@ -98,6 +99,9 @@ struct tsc2007 {
>> This will definitely need to go in the header though.
> 
> Now I have split the code into:
> 
> tsc2007.h (constants, structs and stubs)
> tsc2007_iio.c 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>> 
>>> Hi,
>>> 
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +  struct input_dev **input_dev)
> +{
> +   int err;
> +   struct iio_dev *indio_dev;
> +
> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
 Instead of doing this to reduce the delta between versions make 
 iio_priv a struct tsc2007 **
 
 That is have a single pointer in there and do your allocation of struct
 tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by >dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work 
>> (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
 
 Having doing that, you can have this CONFIG_IIO block as just
 doing the iio stuff with the input elements pulled back into the main
 probe function.
 
 Then define something like
 
 iio_configure (stubbed to nothing if no IIO)
 and
 iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
 
 A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 
>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
 to make it all work
 (the struct tsc2007 and tsc2007_xfer() + a few of the
 register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)
> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.

Now I have split the code into:

tsc2007.h (constants, structs and stubs)
tsc2007_iio.c (the iio stuff)
tsc2007.c (most parts of the original driver)

but I have a problem of correctly modifying the Makefile.

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>> 
>>> Hi,
>>> 
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +  struct input_dev **input_dev)
> +{
> +   int err;
> +   struct iio_dev *indio_dev;
> +
> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
 Instead of doing this to reduce the delta between versions make 
 iio_priv a struct tsc2007 **
 
 That is have a single pointer in there and do your allocation of struct
 tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by >dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work 
>> (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
 
 Having doing that, you can have this CONFIG_IIO block as just
 doing the iio stuff with the input elements pulled back into the main
 probe function.
 
 Then define something like
 
 iio_configure (stubbed to nothing if no IIO)
 and
 iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
 
 A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 
>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
 to make it all work
 (the struct tsc2007 and tsc2007_xfer() + a few of the
 register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)
> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.

Now I have split the code into:

tsc2007.h (constants, structs and stubs)
tsc2007_iio.c (the iio stuff)
tsc2007.c (most parts of the original driver)

but I have a problem of correctly modifying the Makefile.

It currently looks like:

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>> 
>>> Hi,
>>> 
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +  struct input_dev **input_dev)
> +{
> +   int err;
> +   struct iio_dev *indio_dev;
> +
> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
 Instead of doing this to reduce the delta between versions make 
 iio_priv a struct tsc2007 **
 
 That is have a single pointer in there and do your allocation of struct
 tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by >dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work 
>> (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
 
 Having doing that, you can have this CONFIG_IIO block as just
 doing the iio stuff with the input elements pulled back into the main
 probe function.
 
 Then define something like
 
 iio_configure (stubbed to nothing if no IIO)
 and
 iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
 
 A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 

Ah, ok. This makes sense.

>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
 to make it all work
 (the struct tsc2007 and tsc2007_xfer() + a few of the
 register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)

Ok. I think I can now integrate it and then post as PATCH v5 (together with
some other fixes for the patch set).

> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.
>> 
>>int (*get_pendown_state)(struct device 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron :
> 
> On 23/10/16 19:34, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>> 
>>> Hi,
>>> 
> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
> +  struct input_dev **input_dev)
> +{
> +   int err;
> +   struct iio_dev *indio_dev;
> +
> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
 Instead of doing this to reduce the delta between versions make 
 iio_priv a struct tsc2007 **
 
 That is have a single pointer in there and do your allocation of struct
 tsc2007 separately.
>>> 
>>> Sorry, but I think I do not completely understand what you mean here.
>>> 
>>> The problem is that we need to allocate some struct tsc2007 in both cases.
>>> But in one case managed directly by >dev and in the other managed
>>> indirectly. This is why I use the private area of struct iio_dev to store
>>> the full struct tsc2007 and not just a pointer.
>> 
>> Ok, I think I finally did understand how you mean this and have started to
>> implement something.
>> 
> oops. Didn't look on in my emails to get to this one!
>> The idea is to have one alloc function to return a struct tsc2007. This
>> can be part of the probe function, like it is in the unpatched driver.
>> 
>> In case of iio this struct tsc2007 is also allocated explicitly so that
>> a pointer can be stored in iio_priv.
>> 
>> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
>> 
>> I have added that approach to my inlined patch and it seems to work 
>> (attached).
>> 
>> Sorry if I do not use the wording you would use and sometimes overlook
>> something you have said. I feel here like moving on thin ice and doing
>> guesswork about unspoken assumptions...
> That's fine.  Stuff that can appear obvious to one person is not
> necessarily obvious to another!
>> 
>>> 
 
 Having doing that, you can have this CONFIG_IIO block as just
 doing the iio stuff with the input elements pulled back into the main
 probe function.
 
 Then define something like
 
 iio_configure (stubbed to nothing if no IIO)
 and
 iio_unconfigure (also stubbed to nothing if no IIO).
>> 
>> This seems to work (draft attached).
>> 
 
 A couple of additions in the header
>> 
>> I think you mean tsc2007.h?
> Nope. A local header alongside the driver is what you want for this stuff.
> driver/input/tsc2007.h 

Ah, ok. This makes sense.

>> 
>> This currently contains only platform data and could IMHO be eliminated
>> if everything becomes DT.
>> 
 to make it all work
 (the struct tsc2007 and tsc2007_xfer() + a few of the
 register defines..
>> 
>> Here it appears to me that I have to make a lot of so far private static
>> and even static inline functions public so that I can make them stubs and
>> call them from tsc2007_iio.c.
> There will be a few.
>> 
>> And for having proper parameter types I have to make most private structs
>> also public.
> Yes a few of those as well.
>> 
>> I really like the idea to have the optional iio feature in a separate source
>> file, but when really starting to write code, I get the impression that
>> it introduces more problems than it solves.
>> 
>> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
>> as well. There are also two static function in some #ifdef #else # endif
>> and not going through stubs.
> Usually it is only done once a certain volume of code exists.
>> 
>> So is this intended to give up some static definitions?
> Yes, that happens the moment you have multiple source files.
> 
> Some losses but generally end up with clean code separation. Always a trade
> off unfortunately.  Pity we can't just insist IIO is available! Rather large
> to pull in for what is probable a niche use case.
> 
> Below is definitely heading in the right direction. I remember vaguely being
> convinced of the worth of doing this when optional code is involved!
> (was a good while ago now)

Ok. I think I can now integrate it and then post as PATCH v5 (together with
some other fixes for the patch set).

> 
> Jonathan
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..92da8f6 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
> Should not need this after introducing the new file.  Will only be
> needed in the iio specific .c file.
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -98,6 +99,9 @@ struct tsc2007 {
> This will definitely need to go in the header though.
>> 
>>int (*get_pendown_state)(struct device *);
>>void

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 19:34, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>
>> Hi,
>>
 +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
 +  struct input_dev **input_dev)
 +{
 +   int err;
 +   struct iio_dev *indio_dev;
 +
 +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
>>> Instead of doing this to reduce the delta between versions make 
>>> iio_priv a struct tsc2007 **
>>>
>>> That is have a single pointer in there and do your allocation of struct
>>> tsc2007 separately.
>>
>> Sorry, but I think I do not completely understand what you mean here.
>>
>> The problem is that we need to allocate some struct tsc2007 in both cases.
>> But in one case managed directly by >dev and in the other managed
>> indirectly. This is why I use the private area of struct iio_dev to store
>> the full struct tsc2007 and not just a pointer.
> 
> Ok, I think I finally did understand how you mean this and have started to
> implement something.
> 
oops. Didn't look on in my emails to get to this one!
> The idea is to have one alloc function to return a struct tsc2007. This
> can be part of the probe function, like it is in the unpatched driver.
> 
> In case of iio this struct tsc2007 is also allocated explicitly so that
> a pointer can be stored in iio_priv.
> 
> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
> 
> I have added that approach to my inlined patch and it seems to work 
> (attached).
> 
> Sorry if I do not use the wording you would use and sometimes overlook
> something you have said. I feel here like moving on thin ice and doing
> guesswork about unspoken assumptions...
That's fine.  Stuff that can appear obvious to one person is not
necessarily obvious to another!
> 
>>
>>>
>>> Having doing that, you can have this CONFIG_IIO block as just
>>> doing the iio stuff with the input elements pulled back into the main
>>> probe function.
>>>
>>> Then define something like
>>>
>>> iio_configure (stubbed to nothing if no IIO)
>>> and
>>> iio_unconfigure (also stubbed to nothing if no IIO).
> 
> This seems to work (draft attached).
> 
>>>
>>> A couple of additions in the header
> 
> I think you mean tsc2007.h?
Nope. A local header alongside the driver is what you want for this stuff.
driver/input/tsc2007.h 
> 
> This currently contains only platform data and could IMHO be eliminated
> if everything becomes DT.
> 
>>> to make it all work
>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>> register defines..
> 
> Here it appears to me that I have to make a lot of so far private static
> and even static inline functions public so that I can make them stubs and
> call them from tsc2007_iio.c.
There will be a few.
> 
> And for having proper parameter types I have to make most private structs
> also public.
Yes a few of those as well.
> 
> I really like the idea to have the optional iio feature in a separate source
> file, but when really starting to write code, I get the impression that
> it introduces more problems than it solves.
> 
> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
> as well. There are also two static function in some #ifdef #else # endif
> and not going through stubs.
Usually it is only done once a certain volume of code exists.
> 
> So is this intended to give up some static definitions?
Yes, that happens the moment you have multiple source files.

Some losses but generally end up with clean code separation. Always a trade
off unfortunately.  Pity we can't just insist IIO is available! Rather large
to pull in for what is probable a niche use case.

Below is definitely heading in the right direction. I remember vaguely being
convinced of the worth of doing this when optional code is involved!
(was a good while ago now)

Jonathan
> 
> BR and thanks,
> Nikolaus
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..92da8f6 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
Should not need this after introducing the new file.  Will only be
needed in the iio specific .c file.
>  
>  #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>  #define TSC2007_MEASURE_AUX(0x2 << 4)
> @@ -98,6 +99,9 @@ struct tsc2007 {
This will definitely need to go in the header though.
>  
> int (*get_pendown_state)(struct device *);
> void(*clear_penirq)(void);
> +
> +   struct mutexmlock;
> +   void*private;
>  };
>  
>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
> @@ -192,7 +196,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
> 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 19:34, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
>>
>> Hi,
>>
 +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
 +  struct input_dev **input_dev)
 +{
 +   int err;
 +   struct iio_dev *indio_dev;
 +
 +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
>>> Instead of doing this to reduce the delta between versions make 
>>> iio_priv a struct tsc2007 **
>>>
>>> That is have a single pointer in there and do your allocation of struct
>>> tsc2007 separately.
>>
>> Sorry, but I think I do not completely understand what you mean here.
>>
>> The problem is that we need to allocate some struct tsc2007 in both cases.
>> But in one case managed directly by >dev and in the other managed
>> indirectly. This is why I use the private area of struct iio_dev to store
>> the full struct tsc2007 and not just a pointer.
> 
> Ok, I think I finally did understand how you mean this and have started to
> implement something.
> 
oops. Didn't look on in my emails to get to this one!
> The idea is to have one alloc function to return a struct tsc2007. This
> can be part of the probe function, like it is in the unpatched driver.
> 
> In case of iio this struct tsc2007 is also allocated explicitly so that
> a pointer can be stored in iio_priv.
> 
> This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.
> 
> I have added that approach to my inlined patch and it seems to work 
> (attached).
> 
> Sorry if I do not use the wording you would use and sometimes overlook
> something you have said. I feel here like moving on thin ice and doing
> guesswork about unspoken assumptions...
That's fine.  Stuff that can appear obvious to one person is not
necessarily obvious to another!
> 
>>
>>>
>>> Having doing that, you can have this CONFIG_IIO block as just
>>> doing the iio stuff with the input elements pulled back into the main
>>> probe function.
>>>
>>> Then define something like
>>>
>>> iio_configure (stubbed to nothing if no IIO)
>>> and
>>> iio_unconfigure (also stubbed to nothing if no IIO).
> 
> This seems to work (draft attached).
> 
>>>
>>> A couple of additions in the header
> 
> I think you mean tsc2007.h?
Nope. A local header alongside the driver is what you want for this stuff.
driver/input/tsc2007.h 
> 
> This currently contains only platform data and could IMHO be eliminated
> if everything becomes DT.
> 
>>> to make it all work
>>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>>> register defines..
> 
> Here it appears to me that I have to make a lot of so far private static
> and even static inline functions public so that I can make them stubs and
> call them from tsc2007_iio.c.
There will be a few.
> 
> And for having proper parameter types I have to make most private structs
> also public.
Yes a few of those as well.
> 
> I really like the idea to have the optional iio feature in a separate source
> file, but when really starting to write code, I get the impression that
> it introduces more problems than it solves.
> 
> And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
> as well. There are also two static function in some #ifdef #else # endif
> and not going through stubs.
Usually it is only done once a certain volume of code exists.
> 
> So is this intended to give up some static definitions?
Yes, that happens the moment you have multiple source files.

Some losses but generally end up with clean code separation. Always a trade
off unfortunately.  Pity we can't just insist IIO is available! Rather large
to pull in for what is probable a niche use case.

Below is definitely heading in the right direction. I remember vaguely being
convinced of the worth of doing this when optional code is involved!
(was a good while ago now)

Jonathan
> 
> BR and thanks,
> Nikolaus
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..92da8f6 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
Should not need this after introducing the new file.  Will only be
needed in the iio specific .c file.
>  
>  #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>  #define TSC2007_MEASURE_AUX(0x2 << 4)
> @@ -98,6 +99,9 @@ struct tsc2007 {
This will definitely need to go in the header though.
>  
> int (*get_pendown_state)(struct device *);
> void(*clear_penirq)(void);
> +
> +   struct mutexmlock;
> +   void*private;
>  };
>  
>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
> @@ -192,7 +196,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
> /* pen 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 10:57, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron :
>>
>> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
 Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :

 On 17/10/16 14:57, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
>
> Additionally it can measure the chip temperature.
>
> This extension provides an iio interface for these adc channels.
>
> Since it is not wasting much resources and is very straightforward,
> we simply provide all other adc channels as optional iio interfaces
> as weel. This can be used for debugging or special applications.
 well
>
> Signed-off-by: H. Nikolaus Schaller 
 This could be cleaner done perhaps by factoring out the IIO stuff into a 
 separate
 file and using a header with stubs to deal with the no available case.

 There will only be a handful of stubs and it'll give you a lot cleaner code
 in here.

 If def fun in .c files is always harder to deal with than in a header
 where stubs are really obvious.
>>>
>>> Yes, it became a lot of #ifdefs spread over the source file.
>>>
>>> The easiest thing would be to require IIO to be enabled :)
>>>
>>> With your proposal to consider refactoring, I think the crucial part
>>> is the conditional allocation either through devm_iio_device_alloc()
>>> or devm_kzalloc(). This can be refactored into some conditional
>>> tsc2007_alloc().
>>>
>>> I have tried some draft (not tested and not tidied up) to check if the
>>> direction is good.
>>>
>>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>>> seem to be very complex now in comparison. And the patch itself has only a
>>> handful of hunks (8).
>>>
>>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move 
>>> around
>>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
>>> understand
>>> because it is not really iio specific and one has to switch between two 
>>> source
>>> files. And I would have to touch the Makefile as well.
>>>
>>> What do you think?
>> I'd still split it.  The only bit of the IIO block that isn't specific is
>> a tiny chunk of the allocation code (as you've highlighted).
>>
>> Even that can be avoided by adding a tiny bit more indirection than would
>> otherwise be needed (it's not pretty but it would give a clean separation).
> 
> I hope I understand what you mean (which is an indication that the result
> may be much easier to read for you but not me...).
> 
>> It's pretty much the way this sort of optional functionality should always
>> be done - means that if you don't care (i.e. it's not enabled) you don't
>> even have to see the code.
> 
>>
>> Jonathan
>>>
>>> If generally ok, I can include that in [PATCH v5].
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>>> b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..691e79f 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>>
>>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>>> @@ -69,9 +70,13 @@ struct ts_event {
>>>
>>> struct tsc2007 {
>>>struct input_dev*input;
>>> +#ifdef CONFIG_IIO
>>> +   struct iio_dev  *indio;
>>> +#endif
>> I wouldn't bother with this one.  Just have 
>> struct iio_dev; before this and it'll waste a whole
>> one pointer (+ you shouldn't need to have iio.h included
>> in here once you have spit the files).
> 
> Looks as if I have to make a knot in my brain before I start to understand...
> 
> How can I use struct iio_dev here w/o including iio.h?
you aren't using it.  You have a pointer to it.

So it (before this definition) you have a line that says
struct iio_dev;  you let the compiler know such a structure exists.
At that point you don't actually have to provide a definition of
what is in it as long as all you use is a pointer (they are always
the same size).
> 
>>>charphys[32];
>>>
>>>struct i2c_client   *client;
>>> +   struct mutexmlock;
>>>
>>>u16 model;
>>>u16 x_plate_ohms;
>>> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
>>> *handle)
>>>while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>
>>>/* pen is down, continue with the measurement */
>>> +
>>> +   mutex_lock(>mlock);

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 23/10/16 10:57, H. Nikolaus Schaller wrote:
> Hi,
> 
>> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron :
>>
>> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
 Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :

 On 17/10/16 14:57, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
>
> Additionally it can measure the chip temperature.
>
> This extension provides an iio interface for these adc channels.
>
> Since it is not wasting much resources and is very straightforward,
> we simply provide all other adc channels as optional iio interfaces
> as weel. This can be used for debugging or special applications.
 well
>
> Signed-off-by: H. Nikolaus Schaller 
 This could be cleaner done perhaps by factoring out the IIO stuff into a 
 separate
 file and using a header with stubs to deal with the no available case.

 There will only be a handful of stubs and it'll give you a lot cleaner code
 in here.

 If def fun in .c files is always harder to deal with than in a header
 where stubs are really obvious.
>>>
>>> Yes, it became a lot of #ifdefs spread over the source file.
>>>
>>> The easiest thing would be to require IIO to be enabled :)
>>>
>>> With your proposal to consider refactoring, I think the crucial part
>>> is the conditional allocation either through devm_iio_device_alloc()
>>> or devm_kzalloc(). This can be refactored into some conditional
>>> tsc2007_alloc().
>>>
>>> I have tried some draft (not tested and not tidied up) to check if the
>>> direction is good.
>>>
>>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>>> seem to be very complex now in comparison. And the patch itself has only a
>>> handful of hunks (8).
>>>
>>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move 
>>> around
>>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
>>> understand
>>> because it is not really iio specific and one has to switch between two 
>>> source
>>> files. And I would have to touch the Makefile as well.
>>>
>>> What do you think?
>> I'd still split it.  The only bit of the IIO block that isn't specific is
>> a tiny chunk of the allocation code (as you've highlighted).
>>
>> Even that can be avoided by adding a tiny bit more indirection than would
>> otherwise be needed (it's not pretty but it would give a clean separation).
> 
> I hope I understand what you mean (which is an indication that the result
> may be much easier to read for you but not me...).
> 
>> It's pretty much the way this sort of optional functionality should always
>> be done - means that if you don't care (i.e. it's not enabled) you don't
>> even have to see the code.
> 
>>
>> Jonathan
>>>
>>> If generally ok, I can include that in [PATCH v5].
>>>
>>> BR and thanks,
>>> Nikolaus
>>>
>>>
>>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>>> b/drivers/input/touchscreen/tsc2007.c
>>> index 5e3c4bf..691e79f 100644
>>> --- a/drivers/input/touchscreen/tsc2007.c
>>> +++ b/drivers/input/touchscreen/tsc2007.c
>>> @@ -30,6 +30,7 @@
>>> #include 
>>> #include 
>>> #include 
>>> +#include 
>>>
>>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>>> @@ -69,9 +70,13 @@ struct ts_event {
>>>
>>> struct tsc2007 {
>>>struct input_dev*input;
>>> +#ifdef CONFIG_IIO
>>> +   struct iio_dev  *indio;
>>> +#endif
>> I wouldn't bother with this one.  Just have 
>> struct iio_dev; before this and it'll waste a whole
>> one pointer (+ you shouldn't need to have iio.h included
>> in here once you have spit the files).
> 
> Looks as if I have to make a knot in my brain before I start to understand...
> 
> How can I use struct iio_dev here w/o including iio.h?
you aren't using it.  You have a pointer to it.

So it (before this definition) you have a line that says
struct iio_dev;  you let the compiler know such a structure exists.
At that point you don't actually have to provide a definition of
what is in it as long as all you use is a pointer (they are always
the same size).
> 
>>>charphys[32];
>>>
>>>struct i2c_client   *client;
>>> +   struct mutexmlock;
>>>
>>>u16 model;
>>>u16 x_plate_ohms;
>>> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
>>> *handle)
>>>while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>>>
>>>/* pen is down, continue with the measurement */
>>> +
>>> +   mutex_lock(>mlock);
>>>tsc2007_read_values(ts, );
>>> +

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
> 
> Hi,
> 
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> +  struct input_dev **input_dev)
>>> +{
>>> +   int err;
>>> +   struct iio_dev *indio_dev;
>>> +
>>> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
>> Instead of doing this to reduce the delta between versions make 
>> iio_priv a struct tsc2007 **
>> 
>> That is have a single pointer in there and do your allocation of struct
>> tsc2007 separately.
> 
> Sorry, but I think I do not completely understand what you mean here.
> 
> The problem is that we need to allocate some struct tsc2007 in both cases.
> But in one case managed directly by >dev and in the other managed
> indirectly. This is why I use the private area of struct iio_dev to store
> the full struct tsc2007 and not just a pointer.

Ok, I think I finally did understand how you mean this and have started to
implement something.

The idea is to have one alloc function to return a struct tsc2007. This
can be part of the probe function, like it is in the unpatched driver.

In case of iio this struct tsc2007 is also allocated explicitly so that
a pointer can be stored in iio_priv.

This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.

I have added that approach to my inlined patch and it seems to work (attached).

Sorry if I do not use the wording you would use and sometimes overlook
something you have said. I feel here like moving on thin ice and doing
guesswork about unspoken assumptions...

> 
>> 
>> Having doing that, you can have this CONFIG_IIO block as just
>> doing the iio stuff with the input elements pulled back into the main
>> probe function.
>> 
>> Then define something like
>> 
>> iio_configure (stubbed to nothing if no IIO)
>> and
>> iio_unconfigure (also stubbed to nothing if no IIO).

This seems to work (draft attached).

>> 
>> A couple of additions in the header

I think you mean tsc2007.h?

This currently contains only platform data and could IMHO be eliminated
if everything becomes DT.

>> to make it all work
>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>> register defines..

Here it appears to me that I have to make a lot of so far private static
and even static inline functions public so that I can make them stubs and
call them from tsc2007_iio.c.

And for having proper parameter types I have to make most private structs
also public.

I really like the idea to have the optional iio feature in a separate source
file, but when really starting to write code, I get the impression that
it introduces more problems than it solves.

And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
as well. There are also two static function in some #ifdef #else # endif
and not going through stubs.

So is this intended to give up some static definitions?

BR and thanks,
Nikolaus

diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..92da8f6 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -98,6 +99,9 @@ struct tsc2007 {
 
int (*get_pendown_state)(struct device *);
void(*clear_penirq)(void);
+
+   struct mutexmlock;
+   void*private;
 };
 
 static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
@@ -192,7 +196,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +326,157 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+struct tsc2007_iio {
+   struct tsc2007 *ts;
+};
+
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller :
> 
> Hi,
> 
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> +  struct input_dev **input_dev)
>>> +{
>>> +   int err;
>>> +   struct iio_dev *indio_dev;
>>> +
>>> +   indio_dev = devm_iio_device_alloc(>dev, sizeof(*ts));
>> Instead of doing this to reduce the delta between versions make 
>> iio_priv a struct tsc2007 **
>> 
>> That is have a single pointer in there and do your allocation of struct
>> tsc2007 separately.
> 
> Sorry, but I think I do not completely understand what you mean here.
> 
> The problem is that we need to allocate some struct tsc2007 in both cases.
> But in one case managed directly by >dev and in the other managed
> indirectly. This is why I use the private area of struct iio_dev to store
> the full struct tsc2007 and not just a pointer.

Ok, I think I finally did understand how you mean this and have started to
implement something.

The idea is to have one alloc function to return a struct tsc2007. This
can be part of the probe function, like it is in the unpatched driver.

In case of iio this struct tsc2007 is also allocated explicitly so that
a pointer can be stored in iio_priv.

This just means an additional iio_priv->ts = devm_kzalloc() in case of iio.

I have added that approach to my inlined patch and it seems to work (attached).

Sorry if I do not use the wording you would use and sometimes overlook
something you have said. I feel here like moving on thin ice and doing
guesswork about unspoken assumptions...

> 
>> 
>> Having doing that, you can have this CONFIG_IIO block as just
>> doing the iio stuff with the input elements pulled back into the main
>> probe function.
>> 
>> Then define something like
>> 
>> iio_configure (stubbed to nothing if no IIO)
>> and
>> iio_unconfigure (also stubbed to nothing if no IIO).

This seems to work (draft attached).

>> 
>> A couple of additions in the header

I think you mean tsc2007.h?

This currently contains only platform data and could IMHO be eliminated
if everything becomes DT.

>> to make it all work
>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>> register defines..

Here it appears to me that I have to make a lot of so far private static
and even static inline functions public so that I can make them stubs and
call them from tsc2007_iio.c.

And for having proper parameter types I have to make most private structs
also public.

I really like the idea to have the optional iio feature in a separate source
file, but when really starting to write code, I get the impression that
it introduces more problems than it solves.

And I wonder a little why it is not done for #ifdef CONFIG_OF in tsc2007.c
as well. There are also two static function in some #ifdef #else # endif
and not going through stubs.

So is this intended to give up some static definitions?

BR and thanks,
Nikolaus

diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..92da8f6 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -98,6 +99,9 @@ struct tsc2007 {
 
int (*get_pendown_state)(struct device *);
void(*clear_penirq)(void);
+
+   struct mutexmlock;
+   void*private;
 };
 
 static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
@@ -192,7 +196,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +326,157 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+struct tsc2007_iio {
+   struct tsc2007 *ts;
+};
+
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   TSC2007_CHAN_IIO(6, "pen", 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron :
> 
> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
>>> 
>>> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
 The tsc2007 chip not only has a resistive touch screen controller but
 also an external AUX adc imput which can be used for an ambient
 light sensor, battery voltage monitoring or any general purpose.
 
 Additionally it can measure the chip temperature.
 
 This extension provides an iio interface for these adc channels.
 
 Since it is not wasting much resources and is very straightforward,
 we simply provide all other adc channels as optional iio interfaces
 as weel. This can be used for debugging or special applications.
>>> well
 
 Signed-off-by: H. Nikolaus Schaller 
>>> This could be cleaner done perhaps by factoring out the IIO stuff into a 
>>> separate
>>> file and using a header with stubs to deal with the no available case.
>>> 
>>> There will only be a handful of stubs and it'll give you a lot cleaner code
>>> in here.
>>> 
>>> If def fun in .c files is always harder to deal with than in a header
>>> where stubs are really obvious.
>> 
>> Yes, it became a lot of #ifdefs spread over the source file.
>> 
>> The easiest thing would be to require IIO to be enabled :)
>> 
>> With your proposal to consider refactoring, I think the crucial part
>> is the conditional allocation either through devm_iio_device_alloc()
>> or devm_kzalloc(). This can be refactored into some conditional
>> tsc2007_alloc().
>> 
>> I have tried some draft (not tested and not tidied up) to check if the
>> direction is good.
>> 
>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>> seem to be very complex now in comparison. And the patch itself has only a
>> handful of hunks (8).
>> 
>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move 
>> around
>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
>> understand
>> because it is not really iio specific and one has to switch between two 
>> source
>> files. And I would have to touch the Makefile as well.
>> 
>> What do you think?
> I'd still split it.  The only bit of the IIO block that isn't specific is
> a tiny chunk of the allocation code (as you've highlighted).
> 
> Even that can be avoided by adding a tiny bit more indirection than would
> otherwise be needed (it's not pretty but it would give a clean separation).

I hope I understand what you mean (which is an indication that the result
may be much easier to read for you but not me...).

> It's pretty much the way this sort of optional functionality should always
> be done - means that if you don't care (i.e. it's not enabled) you don't
> even have to see the code.

> 
> Jonathan
>> 
>> If generally ok, I can include that in [PATCH v5].
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..691e79f 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -69,9 +70,13 @@ struct ts_event {
>> 
>> struct tsc2007 {
>>struct input_dev*input;
>> +#ifdef CONFIG_IIO
>> +   struct iio_dev  *indio;
>> +#endif
> I wouldn't bother with this one.  Just have 
> struct iio_dev; before this and it'll waste a whole
> one pointer (+ you shouldn't need to have iio.h included
> in here once you have spit the files).

Looks as if I have to make a knot in my brain before I start to understand...

How can I use struct iio_dev here w/o including iio.h?

>>charphys[32];
>> 
>>struct i2c_client   *client;
>> +   struct mutexmlock;
>> 
>>u16 model;
>>u16 x_plate_ohms;
>> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
>> *handle)
>>while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>> 
>>/* pen is down, continue with the measurement */
>> +
>> +   mutex_lock(>mlock);
>>tsc2007_read_values(ts, );
>> +   mutex_unlock(>mlock);
>> 
>>rt = tsc2007_calculate_resistance(ts, );
>> 
>> @@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
>>tsc2007_stop(ts);
>> }
>> 
>> +#ifdef CONFIG_IIO
>> +
>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>> +{ \
>> +   .datasheet_name = _name, \
>> +   .type = _type, \
>> +   .info_mask_separate = 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi,

> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron :
> 
> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>> 
>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
>>> 
>>> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
 The tsc2007 chip not only has a resistive touch screen controller but
 also an external AUX adc imput which can be used for an ambient
 light sensor, battery voltage monitoring or any general purpose.
 
 Additionally it can measure the chip temperature.
 
 This extension provides an iio interface for these adc channels.
 
 Since it is not wasting much resources and is very straightforward,
 we simply provide all other adc channels as optional iio interfaces
 as weel. This can be used for debugging or special applications.
>>> well
 
 Signed-off-by: H. Nikolaus Schaller 
>>> This could be cleaner done perhaps by factoring out the IIO stuff into a 
>>> separate
>>> file and using a header with stubs to deal with the no available case.
>>> 
>>> There will only be a handful of stubs and it'll give you a lot cleaner code
>>> in here.
>>> 
>>> If def fun in .c files is always harder to deal with than in a header
>>> where stubs are really obvious.
>> 
>> Yes, it became a lot of #ifdefs spread over the source file.
>> 
>> The easiest thing would be to require IIO to be enabled :)
>> 
>> With your proposal to consider refactoring, I think the crucial part
>> is the conditional allocation either through devm_iio_device_alloc()
>> or devm_kzalloc(). This can be refactored into some conditional
>> tsc2007_alloc().
>> 
>> I have tried some draft (not tested and not tidied up) to check if the
>> direction is good.
>> 
>> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
>> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
>> seem to be very complex now in comparison. And the patch itself has only a
>> handful of hunks (8).
>> 
>> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move 
>> around
>> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
>> understand
>> because it is not really iio specific and one has to switch between two 
>> source
>> files. And I would have to touch the Makefile as well.
>> 
>> What do you think?
> I'd still split it.  The only bit of the IIO block that isn't specific is
> a tiny chunk of the allocation code (as you've highlighted).
> 
> Even that can be avoided by adding a tiny bit more indirection than would
> otherwise be needed (it's not pretty but it would give a clean separation).

I hope I understand what you mean (which is an indication that the result
may be much easier to read for you but not me...).

> It's pretty much the way this sort of optional functionality should always
> be done - means that if you don't care (i.e. it's not enabled) you don't
> even have to see the code.

> 
> Jonathan
>> 
>> If generally ok, I can include that in [PATCH v5].
>> 
>> BR and thanks,
>> Nikolaus
>> 
>> 
>> diff --git a/drivers/input/touchscreen/tsc2007.c 
>> b/drivers/input/touchscreen/tsc2007.c
>> index 5e3c4bf..691e79f 100644
>> --- a/drivers/input/touchscreen/tsc2007.c
>> +++ b/drivers/input/touchscreen/tsc2007.c
>> @@ -30,6 +30,7 @@
>> #include 
>> #include 
>> #include 
>> +#include 
>> 
>> #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>> #define TSC2007_MEASURE_AUX(0x2 << 4)
>> @@ -69,9 +70,13 @@ struct ts_event {
>> 
>> struct tsc2007 {
>>struct input_dev*input;
>> +#ifdef CONFIG_IIO
>> +   struct iio_dev  *indio;
>> +#endif
> I wouldn't bother with this one.  Just have 
> struct iio_dev; before this and it'll waste a whole
> one pointer (+ you shouldn't need to have iio.h included
> in here once you have spit the files).

Looks as if I have to make a knot in my brain before I start to understand...

How can I use struct iio_dev here w/o including iio.h?

>>charphys[32];
>> 
>>struct i2c_client   *client;
>> +   struct mutexmlock;
>> 
>>u16 model;
>>u16 x_plate_ohms;
>> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
>> *handle)
>>while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>> 
>>/* pen is down, continue with the measurement */
>> +
>> +   mutex_lock(>mlock);
>>tsc2007_read_values(ts, );
>> +   mutex_unlock(>mlock);
>> 
>>rt = tsc2007_calculate_resistance(ts, );
>> 
>> @@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
>>tsc2007_stop(ts);
>> }
>> 
>> +#ifdef CONFIG_IIO
>> +
>> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
>> +{ \
>> +   .datasheet_name = _name, \
>> +   .type = _type, \
>> +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
>> +   

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 22/10/16 21:46, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
>>
>> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels.
>>>
>>> Since it is not wasting much resources and is very straightforward,
>>> we simply provide all other adc channels as optional iio interfaces
>>> as weel. This can be used for debugging or special applications.
>> well
>>>
>>> Signed-off-by: H. Nikolaus Schaller 
>> This could be cleaner done perhaps by factoring out the IIO stuff into a 
>> separate
>> file and using a header with stubs to deal with the no available case.
>>
>> There will only be a handful of stubs and it'll give you a lot cleaner code
>> in here.
>>
>> If def fun in .c files is always harder to deal with than in a header
>> where stubs are really obvious.
> 
> Yes, it became a lot of #ifdefs spread over the source file.
> 
> The easiest thing would be to require IIO to be enabled :)
> 
> With your proposal to consider refactoring, I think the crucial part
> is the conditional allocation either through devm_iio_device_alloc()
> or devm_kzalloc(). This can be refactored into some conditional
> tsc2007_alloc().
> 
> I have tried some draft (not tested and not tidied up) to check if the
> direction is good.
> 
> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
> seem to be very complex now in comparison. And the patch itself has only a
> handful of hunks (8).
> 
> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
> understand
> because it is not really iio specific and one has to switch between two source
> files. And I would have to touch the Makefile as well.
> 
> What do you think?
I'd still split it.  The only bit of the IIO block that isn't specific is
a tiny chunk of the allocation code (as you've highlighted).

Even that can be avoided by adding a tiny bit more indirection than would
otherwise be needed (it's not pretty but it would give a clean separation).

It's pretty much the way this sort of optional functionality should always
be done - means that if you don't care (i.e. it's not enabled) you don't
even have to see the code.

Jonathan
> 
> If generally ok, I can include that in [PATCH v5].
> 
> BR and thanks,
> Nikolaus
> 
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..691e79f 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>  #define TSC2007_MEASURE_AUX(0x2 << 4)
> @@ -69,9 +70,13 @@ struct ts_event {
>  
>  struct tsc2007 {
> struct input_dev*input;
> +#ifdef CONFIG_IIO
> +   struct iio_dev  *indio;
> +#endif
I wouldn't bother with this one.  Just have 
struct iio_dev; before this and it'll waste a whole
one pointer (+ you shouldn't need to have iio.h included
in here once you have spit the files).
> charphys[32];
>  
> struct i2c_client   *client;
> +   struct mutexmlock;
>  
> u16 model;
> u16 x_plate_ohms;
> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
> /* pen is down, continue with the measurement */
> +
> +   mutex_lock(>mlock);
> tsc2007_read_values(ts, );
> +   mutex_unlock(>mlock);
>  
> rt = tsc2007_calculate_resistance(ts, );
>  
> @@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
> tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> +   .datasheet_name = _name, \
> +   .type = _type, \
> +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
> +   BIT(_chan_info), \
> +   .indexed = 1, \
> +   .channel = _chan, \
> +}
> +
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> +   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 22/10/16 21:46, H. Nikolaus Schaller wrote:
> Hi Jonathan,
> 
>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
>>
>> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
>>> The tsc2007 chip not only has a resistive touch screen controller but
>>> also an external AUX adc imput which can be used for an ambient
>>> light sensor, battery voltage monitoring or any general purpose.
>>>
>>> Additionally it can measure the chip temperature.
>>>
>>> This extension provides an iio interface for these adc channels.
>>>
>>> Since it is not wasting much resources and is very straightforward,
>>> we simply provide all other adc channels as optional iio interfaces
>>> as weel. This can be used for debugging or special applications.
>> well
>>>
>>> Signed-off-by: H. Nikolaus Schaller 
>> This could be cleaner done perhaps by factoring out the IIO stuff into a 
>> separate
>> file and using a header with stubs to deal with the no available case.
>>
>> There will only be a handful of stubs and it'll give you a lot cleaner code
>> in here.
>>
>> If def fun in .c files is always harder to deal with than in a header
>> where stubs are really obvious.
> 
> Yes, it became a lot of #ifdefs spread over the source file.
> 
> The easiest thing would be to require IIO to be enabled :)
> 
> With your proposal to consider refactoring, I think the crucial part
> is the conditional allocation either through devm_iio_device_alloc()
> or devm_kzalloc(). This can be refactored into some conditional
> tsc2007_alloc().
> 
> I have tried some draft (not tested and not tidied up) to check if the
> direction is good.
> 
> This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
> new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
> seem to be very complex now in comparison. And the patch itself has only a
> handful of hunks (8).
> 
> Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
> one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
> understand
> because it is not really iio specific and one has to switch between two source
> files. And I would have to touch the Makefile as well.
> 
> What do you think?
I'd still split it.  The only bit of the IIO block that isn't specific is
a tiny chunk of the allocation code (as you've highlighted).

Even that can be avoided by adding a tiny bit more indirection than would
otherwise be needed (it's not pretty but it would give a clean separation).

It's pretty much the way this sort of optional functionality should always
be done - means that if you don't care (i.e. it's not enabled) you don't
even have to see the code.

Jonathan
> 
> If generally ok, I can include that in [PATCH v5].
> 
> BR and thanks,
> Nikolaus
> 
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index 5e3c4bf..691e79f 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
>  #define TSC2007_MEASURE_AUX(0x2 << 4)
> @@ -69,9 +70,13 @@ struct ts_event {
>  
>  struct tsc2007 {
> struct input_dev*input;
> +#ifdef CONFIG_IIO
> +   struct iio_dev  *indio;
> +#endif
I wouldn't bother with this one.  Just have 
struct iio_dev; before this and it'll waste a whole
one pointer (+ you shouldn't need to have iio.h included
in here once you have spit the files).
> charphys[32];
>  
> struct i2c_client   *client;
> +   struct mutexmlock;
>  
> u16 model;
> u16 x_plate_ohms;
> @@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
> while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
> /* pen is down, continue with the measurement */
> +
> +   mutex_lock(>mlock);
> tsc2007_read_values(ts, );
> +   mutex_unlock(>mlock);
>  
> rt = tsc2007_calculate_resistance(ts, );
>  
> @@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
> tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> +   .datasheet_name = _name, \
> +   .type = _type, \
> +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
> +   BIT(_chan_info), \
> +   .indexed = 1, \
> +   .channel = _chan, \
> +}
> +
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> +   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> +   TSC2007_CHAN_IIO(4, "adc", 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
> 
> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
>> The tsc2007 chip not only has a resistive touch screen controller but
>> also an external AUX adc imput which can be used for an ambient
>> light sensor, battery voltage monitoring or any general purpose.
>> 
>> Additionally it can measure the chip temperature.
>> 
>> This extension provides an iio interface for these adc channels.
>> 
>> Since it is not wasting much resources and is very straightforward,
>> we simply provide all other adc channels as optional iio interfaces
>> as weel. This can be used for debugging or special applications.
> well
>> 
>> Signed-off-by: H. Nikolaus Schaller 
> This could be cleaner done perhaps by factoring out the IIO stuff into a 
> separate
> file and using a header with stubs to deal with the no available case.
> 
> There will only be a handful of stubs and it'll give you a lot cleaner code
> in here.
> 
> If def fun in .c files is always harder to deal with than in a header
> where stubs are really obvious.

Yes, it became a lot of #ifdefs spread over the source file.

The easiest thing would be to require IIO to be enabled :)

With your proposal to consider refactoring, I think the crucial part
is the conditional allocation either through devm_iio_device_alloc()
or devm_kzalloc(). This can be refactored into some conditional
tsc2007_alloc().

I have tried some draft (not tested and not tidied up) to check if the
direction is good.

This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
seem to be very complex now in comparison. And the patch itself has only a
handful of hunks (8).

Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
understand
because it is not really iio specific and one has to switch between two source
files. And I would have to touch the Makefile as well.

What do you think?

If generally ok, I can include that in [PATCH v5].

BR and thanks,
Nikolaus


diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..691e79f 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -69,9 +70,13 @@ struct ts_event {
 
 struct tsc2007 {
struct input_dev*input;
+#ifdef CONFIG_IIO
+   struct iio_dev  *indio;
+#endif
charphys[32];
 
struct i2c_client   *client;
+   struct mutexmlock;
 
u16 model;
u16 x_plate_ohms;
@@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+   struct  tsc2007 *tsc = iio_priv(indio_dev);
+   int adc_chan = chan->channel;
+   int ret = 0;
+
+   if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+   return -EINVAL;
+
+   if (mask != IIO_CHAN_INFO_RAW)
+   return -EINVAL;
+
+   mutex_lock(>mlock);
+
+   switch (chan->channel) {
+   case 0:
+   *val = tsc2007_xfer(tsc, READ_X);
+   break;
+   

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

Hi Jonathan,

> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron :
> 
> On 17/10/16 14:57, H. Nikolaus Schaller wrote:
>> The tsc2007 chip not only has a resistive touch screen controller but
>> also an external AUX adc imput which can be used for an ambient
>> light sensor, battery voltage monitoring or any general purpose.
>> 
>> Additionally it can measure the chip temperature.
>> 
>> This extension provides an iio interface for these adc channels.
>> 
>> Since it is not wasting much resources and is very straightforward,
>> we simply provide all other adc channels as optional iio interfaces
>> as weel. This can be used for debugging or special applications.
> well
>> 
>> Signed-off-by: H. Nikolaus Schaller 
> This could be cleaner done perhaps by factoring out the IIO stuff into a 
> separate
> file and using a header with stubs to deal with the no available case.
> 
> There will only be a handful of stubs and it'll give you a lot cleaner code
> in here.
> 
> If def fun in .c files is always harder to deal with than in a header
> where stubs are really obvious.

Yes, it became a lot of #ifdefs spread over the source file.

The easiest thing would be to require IIO to be enabled :)

With your proposal to consider refactoring, I think the crucial part
is the conditional allocation either through devm_iio_device_alloc()
or devm_kzalloc(). This can be refactored into some conditional
tsc2007_alloc().

I have tried some draft (not tested and not tidied up) to check if the
direction is good.

This reduces the number of #ifdef CONFIG_IIO from 7 to 2 without introducing
new files or includes. There are also 2 other #ifdef CONFIG_OF so it doesn't
seem to be very complex now in comparison. And the patch itself has only a
handful of hunks (8).

Moving tsc2007_alloc into a separate file tsc2007_iio.c would only move around
one #ifdef CONFIG_OF from tsc2007.c but IMHO makes it more difficult to 
understand
because it is not really iio specific and one has to switch between two source
files. And I would have to touch the Makefile as well.

What do you think?

If generally ok, I can include that in [PATCH v5].

BR and thanks,
Nikolaus


diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index 5e3c4bf..691e79f 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -69,9 +70,13 @@ struct ts_event {
 
 struct tsc2007 {
struct input_dev*input;
+#ifdef CONFIG_IIO
+   struct iio_dev  *indio;
+#endif
charphys[32];
 
struct i2c_client   *client;
+   struct mutexmlock;
 
u16 model;
u16 x_plate_ohms;
@@ -192,7 +197,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +327,162 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+   struct  tsc2007 *tsc = iio_priv(indio_dev);
+   int adc_chan = chan->channel;
+   int ret = 0;
+
+   if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+   return -EINVAL;
+
+   if (mask != IIO_CHAN_INFO_RAW)
+   return -EINVAL;
+
+   mutex_lock(>mlock);
+
+   switch (chan->channel) {
+   case 0:
+   *val = tsc2007_xfer(tsc, READ_X);
+   break;
+   case 1:
+   *val = 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 17/10/16 14:57, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
> 
> Additionally it can measure the chip temperature.
> 
> This extension provides an iio interface for these adc channels.
> 
> Since it is not wasting much resources and is very straightforward,
> we simply provide all other adc channels as optional iio interfaces
> as weel. This can be used for debugging or special applications.
well
> 
> Signed-off-by: H. Nikolaus Schaller 
This could be cleaner done perhaps by factoring out the IIO stuff into a 
separate
file and using a header with stubs to deal with the no available case.

There will only be a handful of stubs and it'll give you a lot cleaner code
in here.

If def fun in .c files is always harder to deal with than in a header
where stubs are really obvious.

J
> ---
>  drivers/input/touchscreen/tsc2007.c | 151 
> +++-
>  1 file changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index c314331..b4a9504 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TSC2007_MEASURE_TEMP0(0x0 << 4)
>  #define TSC2007_MEASURE_AUX  (0x2 << 4)
> @@ -61,6 +62,16 @@
>  #define READ_X   (ADC_ON_12BIT | TSC2007_MEASURE_X)
>  #define PWRDOWN  (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>  
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> + .datasheet_name = _name, \
> + .type = _type, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
> + BIT(_chan_info), \
> + .indexed = 1, \
> + .channel = _chan, \
> +}
> +
>  struct ts_event {
>   u16 x;
>   u16 y;
> @@ -69,9 +80,13 @@ struct ts_event {
>  
>  struct tsc2007 {
>   struct input_dev*input;
> +#ifdef CONFIG_IIO
> + struct iio_dev  *indio;
> +#endif
>   charphys[32];
>  
>   struct i2c_client   *client;
> + struct mutexmlock;
>  
>   u16 model;
>   u16 x_plate_ohms;
> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
>   while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>   /* pen is down, continue with the measurement */
> +
> + mutex_lock(>mlock);
>   tsc2007_read_values(ts, );
> + mutex_unlock(>mlock);
>  
>   rt = tsc2007_calculate_resistance(ts, );
>  
> @@ -319,6 +337,89 @@ static void tsc2007_close(struct input_dev *input_dev)
>   tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> + TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
> + TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
> +};
> +
> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> +{
> + struct  tsc2007 *tsc = iio_priv(indio_dev);
> + int adc_chan = chan->channel;
> + int ret = 0;
> +
> + if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
> + return -EINVAL;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + mutex_lock(>mlock);
> +
> + switch (chan->channel) {
> + case 0:
> + *val = tsc2007_xfer(tsc, READ_X);
> + break;
> + case 1:
> + *val = tsc2007_xfer(tsc, READ_Y);
> + break;
> + case 2:
> + *val = tsc2007_xfer(tsc, READ_Z1);
> + break;
> + case 3:
> + *val = tsc2007_xfer(tsc, READ_Z2);
> + break;
> + case 4:
> + *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
> + break;
> + case 5: {
> + struct ts_event tc;
> +
> + tc.x = tsc2007_xfer(tsc, READ_X);
> + tc.z1 = tsc2007_xfer(tsc, READ_Z1);
> + tc.z2 = tsc2007_xfer(tsc, READ_Z2);
> + *val = tsc2007_calculate_resistance(tsc, );
> + break;
> + }
> + case 6:
> + *val = 

Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[Jonathan Cameron]

On 17/10/16 14:57, H. Nikolaus Schaller wrote:
> The tsc2007 chip not only has a resistive touch screen controller but
> also an external AUX adc imput which can be used for an ambient
> light sensor, battery voltage monitoring or any general purpose.
> 
> Additionally it can measure the chip temperature.
> 
> This extension provides an iio interface for these adc channels.
> 
> Since it is not wasting much resources and is very straightforward,
> we simply provide all other adc channels as optional iio interfaces
> as weel. This can be used for debugging or special applications.
well
> 
> Signed-off-by: H. Nikolaus Schaller 
This could be cleaner done perhaps by factoring out the IIO stuff into a 
separate
file and using a header with stubs to deal with the no available case.

There will only be a handful of stubs and it'll give you a lot cleaner code
in here.

If def fun in .c files is always harder to deal with than in a header
where stubs are really obvious.

J
> ---
>  drivers/input/touchscreen/tsc2007.c | 151 
> +++-
>  1 file changed, 150 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/touchscreen/tsc2007.c 
> b/drivers/input/touchscreen/tsc2007.c
> index c314331..b4a9504 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define TSC2007_MEASURE_TEMP0(0x0 << 4)
>  #define TSC2007_MEASURE_AUX  (0x2 << 4)
> @@ -61,6 +62,16 @@
>  #define READ_X   (ADC_ON_12BIT | TSC2007_MEASURE_X)
>  #define PWRDOWN  (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
>  
> +#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
> +{ \
> + .datasheet_name = _name, \
> + .type = _type, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
> + BIT(_chan_info), \
> + .indexed = 1, \
> + .channel = _chan, \
> +}
> +
>  struct ts_event {
>   u16 x;
>   u16 y;
> @@ -69,9 +80,13 @@ struct ts_event {
>  
>  struct tsc2007 {
>   struct input_dev*input;
> +#ifdef CONFIG_IIO
> + struct iio_dev  *indio;
> +#endif
>   charphys[32];
>  
>   struct i2c_client   *client;
> + struct mutexmlock;
>  
>   u16 model;
>   u16 x_plate_ohms;
> @@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void 
> *handle)
>   while (!ts->stopped && tsc2007_is_pen_down(ts)) {
>  
>   /* pen is down, continue with the measurement */
> +
> + mutex_lock(>mlock);
>   tsc2007_read_values(ts, );
> + mutex_unlock(>mlock);
>  
>   rt = tsc2007_calculate_resistance(ts, );
>  
> @@ -319,6 +337,89 @@ static void tsc2007_close(struct input_dev *input_dev)
>   tsc2007_stop(ts);
>  }
>  
> +#ifdef CONFIG_IIO
> +
> +static const struct iio_chan_spec tsc2007_iio_channel[] = {
> + TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
> + TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
> + TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
> +};
> +
> +static int tsc2007_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> +{
> + struct  tsc2007 *tsc = iio_priv(indio_dev);
> + int adc_chan = chan->channel;
> + int ret = 0;
> +
> + if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
> + return -EINVAL;
> +
> + if (mask != IIO_CHAN_INFO_RAW)
> + return -EINVAL;
> +
> + mutex_lock(>mlock);
> +
> + switch (chan->channel) {
> + case 0:
> + *val = tsc2007_xfer(tsc, READ_X);
> + break;
> + case 1:
> + *val = tsc2007_xfer(tsc, READ_Y);
> + break;
> + case 2:
> + *val = tsc2007_xfer(tsc, READ_Z1);
> + break;
> + case 3:
> + *val = tsc2007_xfer(tsc, READ_Z2);
> + break;
> + case 4:
> + *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
> + break;
> + case 5: {
> + struct ts_event tc;
> +
> + tc.x = tsc2007_xfer(tsc, READ_X);
> + tc.z1 = tsc2007_xfer(tsc, READ_Z1);
> + tc.z2 = tsc2007_xfer(tsc, READ_Z2);
> + *val = tsc2007_calculate_resistance(tsc, );
> + break;
> + }
> + case 6:
> + *val = tsc2007_is_pen_down(tsc);
> +

[PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

The tsc2007 chip not only has a resistive touch screen controller but
also an external AUX adc imput which can be used for an ambient
light sensor, battery voltage monitoring or any general purpose.

Additionally it can measure the chip temperature.

This extension provides an iio interface for these adc channels.

Since it is not wasting much resources and is very straightforward,
we simply provide all other adc channels as optional iio interfaces
as weel. This can be used for debugging or special applications.

Signed-off-by: H. Nikolaus Schaller 
---
 drivers/input/touchscreen/tsc2007.c | 151 +++-
 1 file changed, 150 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index c314331..b4a9504 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -61,6 +62,16 @@
 #define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
 struct ts_event {
u16 x;
u16 y;
@@ -69,9 +80,13 @@ struct ts_event {
 
 struct tsc2007 {
struct input_dev*input;
+#ifdef CONFIG_IIO
+   struct iio_dev  *indio;
+#endif
charphys[32];
 
struct i2c_client   *client;
+   struct mutexmlock;
 
u16 model;
u16 x_plate_ohms;
@@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +337,89 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+   struct  tsc2007 *tsc = iio_priv(indio_dev);
+   int adc_chan = chan->channel;
+   int ret = 0;
+
+   if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+   return -EINVAL;
+
+   if (mask != IIO_CHAN_INFO_RAW)
+   return -EINVAL;
+
+   mutex_lock(>mlock);
+
+   switch (chan->channel) {
+   case 0:
+   *val = tsc2007_xfer(tsc, READ_X);
+   break;
+   case 1:
+   *val = tsc2007_xfer(tsc, READ_Y);
+   break;
+   case 2:
+   *val = tsc2007_xfer(tsc, READ_Z1);
+   break;
+   case 3:
+   *val = tsc2007_xfer(tsc, READ_Z2);
+   break;
+   case 4:
+   *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
+   break;
+   case 5: {
+   struct ts_event tc;
+
+   tc.x = tsc2007_xfer(tsc, READ_X);
+   tc.z1 = tsc2007_xfer(tsc, READ_Z1);
+   tc.z2 = tsc2007_xfer(tsc, READ_Z2);
+   *val = tsc2007_calculate_resistance(tsc, );
+   break;
+   }
+   case 6:
+   *val = tsc2007_is_pen_down(tsc);
+   break;
+   case 7:
+   *val = tsc2007_xfer(tsc,
+   (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
+   break;
+   case 8:
+   *val = tsc2007_xfer(tsc,
+   (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
+   break;
+   }
+
+   /* Prepare for next touch reading - power down ADC, enable PENIRQ */
+   tsc2007_xfer(tsc, PWRDOWN);
+
+   mutex_unlock(>mlock);
+
+   ret = IIO_VAL_INT;
+
+   

[PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature

[H. Nikolaus Schaller]

The tsc2007 chip not only has a resistive touch screen controller but
also an external AUX adc imput which can be used for an ambient
light sensor, battery voltage monitoring or any general purpose.

Additionally it can measure the chip temperature.

This extension provides an iio interface for these adc channels.

Since it is not wasting much resources and is very straightforward,
we simply provide all other adc channels as optional iio interfaces
as weel. This can be used for debugging or special applications.

Signed-off-by: H. Nikolaus Schaller 
---
 drivers/input/touchscreen/tsc2007.c | 151 +++-
 1 file changed, 150 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/tsc2007.c 
b/drivers/input/touchscreen/tsc2007.c
index c314331..b4a9504 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -30,6 +30,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #define TSC2007_MEASURE_TEMP0  (0x0 << 4)
 #define TSC2007_MEASURE_AUX(0x2 << 4)
@@ -61,6 +62,16 @@
 #define READ_X (ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define TSC2007_CHAN_IIO(_chan, _name, _type, _chan_info) \
+{ \
+   .datasheet_name = _name, \
+   .type = _type, \
+   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |  \
+   BIT(_chan_info), \
+   .indexed = 1, \
+   .channel = _chan, \
+}
+
 struct ts_event {
u16 x;
u16 y;
@@ -69,9 +80,13 @@ struct ts_event {
 
 struct tsc2007 {
struct input_dev*input;
+#ifdef CONFIG_IIO
+   struct iio_dev  *indio;
+#endif
charphys[32];
 
struct i2c_client   *client;
+   struct mutexmlock;
 
u16 model;
u16 x_plate_ohms;
@@ -192,7 +207,10 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
while (!ts->stopped && tsc2007_is_pen_down(ts)) {
 
/* pen is down, continue with the measurement */
+
+   mutex_lock(>mlock);
tsc2007_read_values(ts, );
+   mutex_unlock(>mlock);
 
rt = tsc2007_calculate_resistance(ts, );
 
@@ -319,6 +337,89 @@ static void tsc2007_close(struct input_dev *input_dev)
tsc2007_stop(ts);
 }
 
+#ifdef CONFIG_IIO
+
+static const struct iio_chan_spec tsc2007_iio_channel[] = {
+   TSC2007_CHAN_IIO(0, "x", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(1, "y", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(2, "z1", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(3, "z2", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(4, "adc", IIO_VOLTAGE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(5, "rt", IIO_VOLTAGE, IIO_CHAN_INFO_RAW), /* Ohms? */
+   TSC2007_CHAN_IIO(6, "pen", IIO_PRESSURE, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(7, "temp0", IIO_TEMP, IIO_CHAN_INFO_RAW),
+   TSC2007_CHAN_IIO(8, "temp1", IIO_TEMP, IIO_CHAN_INFO_RAW),
+};
+
+static int tsc2007_read_raw(struct iio_dev *indio_dev,
+   struct iio_chan_spec const *chan, int *val, int *val2, long mask)
+{
+   struct  tsc2007 *tsc = iio_priv(indio_dev);
+   int adc_chan = chan->channel;
+   int ret = 0;
+
+   if (adc_chan >= ARRAY_SIZE(tsc2007_iio_channel))
+   return -EINVAL;
+
+   if (mask != IIO_CHAN_INFO_RAW)
+   return -EINVAL;
+
+   mutex_lock(>mlock);
+
+   switch (chan->channel) {
+   case 0:
+   *val = tsc2007_xfer(tsc, READ_X);
+   break;
+   case 1:
+   *val = tsc2007_xfer(tsc, READ_Y);
+   break;
+   case 2:
+   *val = tsc2007_xfer(tsc, READ_Z1);
+   break;
+   case 3:
+   *val = tsc2007_xfer(tsc, READ_Z2);
+   break;
+   case 4:
+   *val = tsc2007_xfer(tsc, (ADC_ON_12BIT | TSC2007_MEASURE_AUX));
+   break;
+   case 5: {
+   struct ts_event tc;
+
+   tc.x = tsc2007_xfer(tsc, READ_X);
+   tc.z1 = tsc2007_xfer(tsc, READ_Z1);
+   tc.z2 = tsc2007_xfer(tsc, READ_Z2);
+   *val = tsc2007_calculate_resistance(tsc, );
+   break;
+   }
+   case 6:
+   *val = tsc2007_is_pen_down(tsc);
+   break;
+   case 7:
+   *val = tsc2007_xfer(tsc,
+   (ADC_ON_12BIT | TSC2007_MEASURE_TEMP0));
+   break;
+   case 8:
+   *val = tsc2007_xfer(tsc,
+   (ADC_ON_12BIT | TSC2007_MEASURE_TEMP1));
+   break;
+   }
+
+   /* Prepare for next touch reading - power down ADC, enable PENIRQ */
+   tsc2007_xfer(tsc, PWRDOWN);
+
+   mutex_unlock(>mlock);
+
+   ret = IIO_VAL_INT;
+
+   return ret;
+}
+