Hi Jonathan,

Am 23.10.2016 um 21:00 schrieb Jonathan Cameron [off-list ref]:

On 23/10/16 19:34, H. Nikolaus Schaller wrote:

quoted

Hi Jonathan,

quoted

Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller [off-list ref]:

Hi,

quoted

quoted

+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(&client->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 &client->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!

quoted

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!

quoted

quoted

quoted

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).

quoted

quoted

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 

quoted

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

quoted

quoted

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.

quoted

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

Yes a few of those as well.

quoted

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.

quoted

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

quoted

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 <linux/of.h>
#include <linux/of_gpio.h>
#include <linux/input/touchscreen.h>
+#include <linux/iio/iio.h>

Should not need this after introducing the new file.  Will only be
needed in the iio specific .c file.

quoted

#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:

obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o
obj-$(CONFIG_IIO)			+= tsc2007_iio.o

We have configured CONFIG_TOUCHSCREEN_TSC2007=m and CONFIG_IIO=y.

This obviously compiles tsc2007_iio.o into the kernel.

This means that tsc2007_iio.o references tsc2007_xfer which is part of
the module.

I would like to get both linked into the module, but the iio part
obviously only if CONFIG_IIO is defined (either -y or -m).

How can I define this?

Or can I define

obj-$(CONFIG_TOUCHSCREEN_TSC2007)	+= tsc2007.o tsc2007_iio.o

and embrace all code in tsc2007_iio with a big #ifdef CONFIG_IIO
so that it is compiled into an empty object file in the non-iio case?

BR and thanks,
Nikolaus