| Message ID | F0CB8C7F-29FD-41DF-A652-9D1AF7824D5F@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/10/16 19:34, H. Nikolaus Schaller wrote: > Hi Jonathan, > >> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >> >> 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(&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! > 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 <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. > > #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 mutex mlock; > + 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(&ts->mlock); > tsc2007_read_values(ts, &tc); > + mutex_unlock(&ts->mlock); > > rt = tsc2007_calculate_resistance(ts, &tc); > > @@ -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; Spot on. > +}; > + > +#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_iio *iio = iio_priv(indio_dev); > + struct tsc2007 *tsc = iio->ts; > + 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(&tsc->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, &tc); > + 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(&tsc->mlock); > + > + ret = IIO_VAL_INT; > + > + return ret; > +} > + > +static const struct iio_info tsc2007_iio_info = { > + .read_raw = tsc2007_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static inline int tsc2007_iio_configure(struct tsc2007 *ts) > +{ > + int err; > + struct iio_dev *indio_dev; > + struct tsc2007_iio *iio; > + > + indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio)); > + if (!indio_dev) { > + dev_err(&ts->client->dev, "iio_device_alloc failed\n"); > + return -ENOMEM; > + } > + > + iio = iio_priv(indio_dev); > + iio->ts = ts; > + ts->private = (void *) indio_dev; > + > + indio_dev->name = "tsc2007"; > + indio_dev->dev.parent = &ts->client->dev; > + indio_dev->info = &tsc2007_iio_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = tsc2007_iio_channel; > + indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel); > + > + err = iio_device_register(indio_dev); > + if (err < 0) { > + dev_err(&ts->client->dev, "iio_device_register() failed: %d\n", > + err); > + return err; > + } > + > + return 0; > +} > + > +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) > +{ > + struct iio_dev *indio_dev = ts->private; > + > + iio_device_unregister(indio_dev); > +} > + > +#else /* CONFIG_IIO */ > + > +static inline int tsc2007_iio_configure(struct tsc2007 *ts) > +{ > + /* not needed */ > +} > + > +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) > +{ > + /* not needed */ > +} > + > +#endif /* CONFIG_IIO */ > + > #ifdef CONFIG_OF > static int tsc2007_get_pendown_state_gpio(struct device *dev) > { > @@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client, > ts->client = client; > ts->irq = client->irq; > ts->input = input_dev; > + > + err = tsc2007_iio_configure(ts); > + if (err < 0) > + return err; > + > init_waitqueue_head(&ts->wait); > + mutex_init(&ts->mlock); > > snprintf(ts->phys, sizeof(ts->phys), > "%s/input0", dev_name(&client->dev)); > @@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client, > ts->fuzzz, 0); > } else { > err = tsc2007_probe_dt(client, ts); > - if (err) > + if (err) { > + tsc2007_iio_unconfigure(ts); > return err; > + } > } > > if (pdata) { > @@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client, > dev_err(&client->dev, > "Failed to register exit_platform_hw action, %d\n", > err); > + tsc2007_iio_unconfigure(ts); > return err; > } > } > @@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client, > if (err) { > dev_err(&client->dev, "Failed to request irq %d: %d\n", > ts->irq, err); > + tsc2007_iio_unconfigure(ts); Maybe need a common unwind and use a goto to get to it. > return err; > } > > @@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client, > if (err < 0) { > dev_err(&client->dev, > "Failed to setup chip: %d\n", err); > + tsc2007_iio_unconfigure(ts); > return err; /* usually, chip does not respond */ > } > > @@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client, > if (err) { > dev_err(&client->dev, > "Failed to register input device: %d\n", err); > + tsc2007_iio_unconfigure(ts); > return err; > } > > return 0; > } > > +static int tsc2007_remove(struct i2c_client *client) > +{ > + struct tsc2007 *ts = i2c_get_clientdata(client); > + tsc2007_iio_unconfigure(ts); > + input_unregister_device(ts->input); > + return 0; > +} > + > static const struct i2c_device_id tsc2007_idtable[] = { > { "tsc2007", 0 }, > { } > @@ -578,6 +756,7 @@ static struct i2c_driver tsc2007_driver = { > }, > .id_table = tsc2007_idtable, > .probe = tsc2007_probe, > + .remove = tsc2007_remove, > }; > > module_i2c_driver(tsc2007_driver);-- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>: > > On 23/10/16 19:34, H. Nikolaus Schaller wrote: >> Hi Jonathan, >> >>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>> >>> 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(&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! >> 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 <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. >> >> #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 mutex mlock; >> + 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(&ts->mlock); >> tsc2007_read_values(ts, &tc); >> + mutex_unlock(&ts->mlock); >> >> rt = tsc2007_calculate_resistance(ts, &tc); >> >> @@ -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; > Spot on. >> +}; >> + >> +#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_iio *iio = iio_priv(indio_dev); >> + struct tsc2007 *tsc = iio->ts; >> + 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(&tsc->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, &tc); >> + 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(&tsc->mlock); >> + >> + ret = IIO_VAL_INT; >> + >> + return ret; >> +} >> + >> +static const struct iio_info tsc2007_iio_info = { >> + .read_raw = tsc2007_read_raw, >> + .driver_module = THIS_MODULE, >> +}; >> + >> +static inline int tsc2007_iio_configure(struct tsc2007 *ts) >> +{ >> + int err; >> + struct iio_dev *indio_dev; >> + struct tsc2007_iio *iio; >> + >> + indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio)); >> + if (!indio_dev) { >> + dev_err(&ts->client->dev, "iio_device_alloc failed\n"); >> + return -ENOMEM; >> + } >> + >> + iio = iio_priv(indio_dev); >> + iio->ts = ts; >> + ts->private = (void *) indio_dev; >> + >> + indio_dev->name = "tsc2007"; >> + indio_dev->dev.parent = &ts->client->dev; >> + indio_dev->info = &tsc2007_iio_info; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->channels = tsc2007_iio_channel; >> + indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel); >> + >> + err = iio_device_register(indio_dev); >> + if (err < 0) { >> + dev_err(&ts->client->dev, "iio_device_register() failed: %d\n", >> + err); >> + return err; >> + } >> + >> + return 0; >> +} >> + >> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) >> +{ >> + struct iio_dev *indio_dev = ts->private; >> + >> + iio_device_unregister(indio_dev); >> +} >> + >> +#else /* CONFIG_IIO */ >> + >> +static inline int tsc2007_iio_configure(struct tsc2007 *ts) >> +{ >> + /* not needed */ >> +} >> + >> +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) >> +{ >> + /* not needed */ >> +} >> + >> +#endif /* CONFIG_IIO */ >> + >> #ifdef CONFIG_OF >> static int tsc2007_get_pendown_state_gpio(struct device *dev) >> { >> @@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client, >> ts->client = client; >> ts->irq = client->irq; >> ts->input = input_dev; >> + >> + err = tsc2007_iio_configure(ts); >> + if (err < 0) >> + return err; >> + >> init_waitqueue_head(&ts->wait); >> + mutex_init(&ts->mlock); >> >> snprintf(ts->phys, sizeof(ts->phys), >> "%s/input0", dev_name(&client->dev)); >> @@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client, >> ts->fuzzz, 0); >> } else { >> err = tsc2007_probe_dt(client, ts); >> - if (err) >> + if (err) { >> + tsc2007_iio_unconfigure(ts); >> return err; >> + } >> } >> >> if (pdata) { >> @@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client, >> dev_err(&client->dev, >> "Failed to register exit_platform_hw action, %d\n", >> err); >> + tsc2007_iio_unconfigure(ts); >> return err; >> } >> } >> @@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client, >> if (err) { >> dev_err(&client->dev, "Failed to request irq %d: %d\n", >> ts->irq, err); >> + tsc2007_iio_unconfigure(ts); > Maybe need a common unwind and use a goto to get to it. Yes, makes sense. >> return err; >> } >> >> @@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client, >> if (err < 0) { >> dev_err(&client->dev, >> "Failed to setup chip: %d\n", err); >> + tsc2007_iio_unconfigure(ts); >> return err; /* usually, chip does not respond */ >> } >> >> @@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client, >> if (err) { >> dev_err(&client->dev, >> "Failed to register input device: %d\n", err); >> + tsc2007_iio_unconfigure(ts); >> return err; >> } >> >> return 0; >> } >> >> +static int tsc2007_remove(struct i2c_client *client) >> +{ >> + struct tsc2007 *ts = i2c_get_clientdata(client); >> + tsc2007_iio_unconfigure(ts); >> + input_unregister_device(ts->input); >> + return 0; >> +} >> + >> static const struct i2c_device_id tsc2007_idtable[] = { >> { "tsc2007", 0 }, >> { } >> @@ -578,6 +756,7 @@ static struct i2c_driver tsc2007_driver = { >> }, >> .id_table = tsc2007_idtable, >> .probe = tsc2007_probe, >> + .remove = tsc2007_remove, >> }; >> >> module_i2c_driver(tsc2007_driver);-- >> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html BR and thanks, Nikolaus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jonathan, > Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>: > > On 23/10/16 19:34, H. Nikolaus Schaller wrote: >> Hi Jonathan, >> >>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>> >>> 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(&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! >> 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 <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. >> >> #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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24/10/16 20:14, H. Nikolaus Schaller wrote: > Hi Jonathan, > >> Am 23.10.2016 um 21:00 schrieb Jonathan Cameron <jic23@kernel.org>: >> >> On 23/10/16 19:34, H. Nikolaus Schaller wrote: >>> Hi Jonathan, >>> >>>> Am 23.10.2016 um 11:57 schrieb H. Nikolaus Schaller <hns@goldelico.com>: >>>> >>>> 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(&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! >>> 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 <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. >>> >>> #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 This is a common problem with optional support. Various ways of handling it. A simple one would be: #Define the stuff that always forms part of the .o file magic_tsc2007-y := tsc2007.o #Define the optional bit to build the resulting magic_tsc2007.o file magic_tsc2007-$(CONFIG_IIO) += tsc2007_iio.o #Use this magic combined file obj-$(CONFIG_TOUCHSCREEN_TSC2007) += magic_tsc2007.o With sensible naming of course ;) Jonathan > > 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 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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> #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 mutex mlock; + 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(&ts->mlock); tsc2007_read_values(ts, &tc); + mutex_unlock(&ts->mlock); rt = tsc2007_calculate_resistance(ts, &tc); @@ -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", 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_iio *iio = iio_priv(indio_dev); + struct tsc2007 *tsc = iio->ts; + 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(&tsc->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, &tc); + 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(&tsc->mlock); + + ret = IIO_VAL_INT; + + return ret; +} + +static const struct iio_info tsc2007_iio_info = { + .read_raw = tsc2007_read_raw, + .driver_module = THIS_MODULE, +}; + +static inline int tsc2007_iio_configure(struct tsc2007 *ts) +{ + int err; + struct iio_dev *indio_dev; + struct tsc2007_iio *iio; + + indio_dev = devm_iio_device_alloc(&ts->client->dev, sizeof(struct tsc2007_iio)); + if (!indio_dev) { + dev_err(&ts->client->dev, "iio_device_alloc failed\n"); + return -ENOMEM; + } + + iio = iio_priv(indio_dev); + iio->ts = ts; + ts->private = (void *) indio_dev; + + indio_dev->name = "tsc2007"; + indio_dev->dev.parent = &ts->client->dev; + indio_dev->info = &tsc2007_iio_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = tsc2007_iio_channel; + indio_dev->num_channels = ARRAY_SIZE(tsc2007_iio_channel); + + err = iio_device_register(indio_dev); + if (err < 0) { + dev_err(&ts->client->dev, "iio_device_register() failed: %d\n", + err); + return err; + } + + return 0; +} + +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) +{ + struct iio_dev *indio_dev = ts->private; + + iio_device_unregister(indio_dev); +} + +#else /* CONFIG_IIO */ + +static inline int tsc2007_iio_configure(struct tsc2007 *ts) +{ + /* not needed */ +} + +static inline void tsc2007_iio_unconfigure(struct tsc2007 *ts) +{ + /* not needed */ +} + +#endif /* CONFIG_IIO */ + #ifdef CONFIG_OF static int tsc2007_get_pendown_state_gpio(struct device *dev) { @@ -472,7 +630,13 @@ static int tsc2007_probe(struct i2c_client *client, ts->client = client; ts->irq = client->irq; ts->input = input_dev; + + err = tsc2007_iio_configure(ts); + if (err < 0) + return err; + init_waitqueue_head(&ts->wait); + mutex_init(&ts->mlock); snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&client->dev)); @@ -503,8 +667,10 @@ static int tsc2007_probe(struct i2c_client *client, ts->fuzzz, 0); } else { err = tsc2007_probe_dt(client, ts); - if (err) + if (err) { + tsc2007_iio_unconfigure(ts); return err; + } } if (pdata) { @@ -516,6 +682,7 @@ static int tsc2007_probe(struct i2c_client *client, dev_err(&client->dev, "Failed to register exit_platform_hw action, %d\n", err); + tsc2007_iio_unconfigure(ts); return err; } } @@ -533,6 +700,7 @@ static int tsc2007_probe(struct i2c_client *client, if (err) { dev_err(&client->dev, "Failed to request irq %d: %d\n", ts->irq, err); + tsc2007_iio_unconfigure(ts); return err; } @@ -543,6 +711,7 @@ static int tsc2007_probe(struct i2c_client *client, if (err < 0) { dev_err(&client->dev, "Failed to setup chip: %d\n", err); + tsc2007_iio_unconfigure(ts); return err; /* usually, chip does not respond */ } @@ -550,12 +719,21 @@ static int tsc2007_probe(struct i2c_client *client, if (err) { dev_err(&client->dev, "Failed to register input device: %d\n", err); + tsc2007_iio_unconfigure(ts); return err; } return 0; } +static int tsc2007_remove(struct i2c_client *client) +{ + struct tsc2007 *ts = i2c_get_clientdata(client); + tsc2007_iio_unconfigure(ts); + input_unregister_device(ts->input); + return 0; +} + static const struct i2c_device_id tsc2007_idtable[] = { { "tsc2007", 0 }, { } @@ -578,6 +756,7 @@ static struct i2c_driver tsc2007_driver = { }, .id_table = tsc2007_idtable, .probe = tsc2007_probe, + .remove = tsc2007_remove, }; module_i2c_driver(tsc2007_driver);--