* Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: H. Nikolaus Schaller @ 2016-10-23 19:11 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Mark Rutland, devicetree, linux-omap, Arnd Bergmann, kernel,
Tony Lindgren, linux-kernel, Mark Brown, Dmitry Torokhov,
Russell King, linux-iio, Sebastian Reichel,
Javier Martinez Canillas, Rob Herring, Mika Penttilä,
Benoît Cousson, linux-input, Michael Welling, letux-kernel,
Andrew F. Davis, Igor Grinberg
In-Reply-To: <4b6773d9-bc33-7471-ee29-01f51c5cfce2@kernel.org>
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
^ permalink raw reply
* Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: Jonathan Cameron @ 2016-10-23 19:00 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann,
kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf, Tony Lindgren,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mark Brown, Dmitry Torokhov,
Russell King, linux-iio-u79uwXL29TY76Z2rM5mHXA, Sebastian Reichel,
Javier Martinez Canillas, Rob Herring, Mika Penttilä,
Benoît Cousson, linux-input-u79uwXL29TY76Z2rM5mHXA,
Michael Welling, letux-kernel-S0jZdbWzriLCfDggNXIi3w,
Andrew F. Davis, Igor Grinberg
In-Reply-To: <F0CB8C7F-29FD-41DF-A652-9D1AF7824D5F-xXXSsgcRVICgSpxsJD1C4w@public.gmane.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-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>:
>>
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: Jonathan Cameron @ 2016-10-23 18:50 UTC (permalink / raw)
To: H. Nikolaus Schaller
Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Benoît Cousson,
Tony Lindgren, Russell King, Arnd Bergmann, Michael Welling,
Mika Penttilä, Javier Martinez Canillas, Igor Grinberg,
Sebastian Reichel, Andrew F. Davis, Mark Brown,
linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-omap-u79uwXL29TY76Z2rM5mHXA,
letux-kernel-S0jZdbWzriLCfDggNXIi3w,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
kernel-Jl6IXVxNIMRxAtABVqVhTwC/G2K4zDHf
In-Reply-To: <976C7328-A6AA-4CD5-AAAA-3B14BA3BD793-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
On 23/10/16 10:57, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>
>> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>>> Hi Jonathan,
>>>
>>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>>>>
>>>> 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 <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
>>>> 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 <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)
>>> @@ -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).
>
>>> char phys[32];
>>>
>>> struct i2c_client *client;
>>> + struct mutex mlock;
>>>
>>> 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(&ts->mlock);
>>> tsc2007_read_values(ts, &tc);
>>> + mutex_unlock(&ts->mlock);
>>>
>>> rt = tsc2007_calculate_resistance(ts, &tc);
>>>
>>> @@ -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(&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 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.
>
No you don't need to do what you are currently doing.
You need to have some means to navigate from struct iio_dev to the
struct tsc2007 - that doesn't have to be because it actually is
in iio_priv.
You can instead put a point to it in iio_priv (and only that) and allocate
it the same way in both paths (stashing a copy of the address in the
pointer in iio_priv).
> What I mean is:
>
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>>> *ts = iio_priv(indio_dev);
>
> vs.
>
>>> *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>
>
> So how can the IIO case just extend/wrap devm_kzalloc(&client->dev...) and still
> be managed as well?
Not relevant if you just allocate it the same way both times.
>
>>
>> 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).
>>
>> A couple of additions in the header to make it all work
>> (the struct tsc2007 and tsc2007_xfer() + a few of the
>> register defines..
>>
>> Nothing big and gets all the CONFIG_IIO into some really
>> obvious stubbing out in the header.
>
>
> Is there some example driver which is doing it that way to be optionally IIO
> compatible? That might be easier to understand and copy than a description.
This particular combination is unusual - but it similar to how we
do optional buffer or trigger support in various iio drivers.
Perhaps see include/linux/iio/common/st_sensors.h and look for CONFIG_IIO_TRIGGER
>
>>
>>> + if (!indio_dev) {
>>> + dev_err(&client->dev, "iio_device_alloc failed\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + *ts = iio_priv(indio_dev);
>>> +
>>> + *input_dev = devm_input_allocate_device(&client->dev);
>>> + if (!*input_dev)
>>> + return -ENOMEM;
>>> +
>>> + i2c_set_clientdata(client, *ts);
>>> + (*ts)->indio = indio_dev;
>>> +
>>> + indio_dev->name = "tsc2007";
>>> + indio_dev->dev.parent = &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(&client->dev, "iio_device_register() failed: %d\n",
>>> + err);
>>> + return err;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
>>> +
>>> +#else /* CONFIG_IIO */
>>> +
>>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>>> + struct input_dev **input_dev)
>>> +{
>>> + int err;
>>> +
>>> + *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> + if (!*ts)
>>> + return -ENOMEM;
>>> +
>>> + *input_dev = devm_input_allocate_device(&client->dev);
>>> + if (!*input_dev)
>>> + return -ENOMEM;
>>> +
>>> + i2c_set_clientdata(client, *ts);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +#define tsc2007_iio_device_unregister(ts) /* not needed */
>> That's rather ugly and fragile. I'd stub it out as an actual function
>> with no content and let the compiler drop it.
>
> Well, it is a quick and dirty draft.
> Should indeed better be a static (inline) function with empty body.
>
>>> +
>>> +#endif /* CONFIG_IIO */
>>> +
>>> #ifdef CONFIG_OF
>>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>>> {
>>> @@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
>>> I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> return -EIO;
>>>
>>> - ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>>> - if (!ts)
>>> - return -ENOMEM;
>>> -
>>> - input_dev = devm_input_allocate_device(&client->dev);
>>> - if (!input_dev)
>>> - return -ENOMEM;
>>> -
>>> - i2c_set_clientdata(client, ts);
>>> + err = tsc2007_alloc(client, &ts, &input_dev);
>>> + if (err < 0)
>>> + return err;
>>>
>>> ts->client = client;
>>> ts->irq = client->irq;
>>> ts->input = input_dev;
>>> init_waitqueue_head(&ts->wait);
>>> + mutex_init(&ts->mlock);
>>>
>>> snprintf(ts->phys, sizeof(ts->phys),
>>> "%s/input0", dev_name(&client->dev));
>>> @@ -543,6 +702,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_device_unregister(ts);
>>> return err; /* usually, chip does not respond */
>>> }
>>>
>>> @@ -556,6 +716,14 @@ static int tsc2007_probe(struct i2c_client *client,
>>> return 0;
>>> }
>>>
>>> +static int tsc2007_remove(struct i2c_client *client)
>>> +{
>>> + struct tsc2007 *ts = i2c_get_clientdata(client);
>>> + input_unregister_device(ts->input);
>>> + tsc2007_iio_device_unregister(ts);
>>> + return 0;
>>> +}
>>> +
>>> static const struct i2c_device_id tsc2007_idtable[] = {
>>> { "tsc2007", 0 },
>>> { }
>>> @@ -578,6 +746,7 @@ static struct i2c_driver tsc2007_driver = {
>>> },
>>> .id_table = tsc2007_idtable,
>>> .probe = tsc2007_probe,
>>> + .remove = tsc2007_remove,
>>> };
>>>
>>> module_i2c_driver(tsc2007_driver);
>>>
>>
>
> BR and thanks,
> Nikolaus
>
^ permalink raw reply
* Re: [PATCH v3 0/2] media: add et8ek8 camera sensor driver and documentation
From: Ivaylo Dimitrov @ 2016-10-23 18:36 UTC (permalink / raw)
To: Pavel Machek
Cc: sakari.ailus, sre, pali.rohar, linux-media, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, mchehab, devicetree,
linux-kernel
In-Reply-To: <20161023181752.GA11728@amd>
On 23.10.2016 21:17, Pavel Machek wrote:
> Hi!
>
>>>> This series adds driver for Toshiba et8ek8 camera sensor found in Nokia N900
>>>>
>>>> Changes from v2:
>>>>
>>>> - fix build when CONFIG_PM is not defined
>>>>
>>>> Changes from v1:
>>>>
>>>> - driver and documentation split into separate patches
>>>> - removed custom controls
>>>> - code changed according to the comments on v1
>>>
>>>> Ivaylo Dimitrov (2):
>>>> media: Driver for Toshiba et8ek8 5MP sensor
>>>> media: et8ek8: Add documentation
>>>
>>> Is there any progress here? Is there any way I could help?
>>>
>>
>> There were some notes I need to address, unfortunately no spare time lately
>> :( . Feel free to fix those for me and resend the patches. If not, I really
>> don't know when I will have the time needed to focus on it.
>
> So good start would be taking these two, address the comments, and try
> to merge them?
>
Yep, the whole history should be at
https://patchwork.kernel.org/patch/9171067/ and
http://www.gossamer-threads.com/lists/linux/kernel/2462501
Thanks,
Ivo
^ permalink raw reply
* Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: H. Nikolaus Schaller @ 2016-10-23 18:34 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Mark Rutland, devicetree, linux-omap, Arnd Bergmann, kernel,
Tony Lindgren, linux-kernel, Mark Brown, Dmitry Torokhov,
Russell King, linux-iio, Sebastian Reichel,
Javier Martinez Canillas, Rob Herring, Mika Penttilä,
Benoît Cousson, linux-input, Michael Welling, letux-kernel,
Andrew F. Davis, Igor Grinberg
In-Reply-To: <976C7328-A6AA-4CD5-AAAA-3B14BA3BD793@goldelico.com>
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.
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 <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);
^ permalink raw reply related
* Re: [PATCH v3 0/2] media: add et8ek8 camera sensor driver and documentation
From: Pavel Machek @ 2016-10-23 18:17 UTC (permalink / raw)
To: Ivaylo Dimitrov
Cc: sakari.ailus-X3B1VOXEql0, sre-DgEjT+Ai2ygdnm+yROfE0A,
pali.rohar-Re5JQEeQqe8AvxtiuMwx3w,
linux-media-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
galak-sgV2jX0FEOL9JmXXK+q4OQ, mchehab-JPH+aEBZ4P+UEJcrhfAQsw,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <53284bf5-9a36-fbcb-5cac-4a64823c3516-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]
Hi!
> >>This series adds driver for Toshiba et8ek8 camera sensor found in Nokia N900
> >>
> >>Changes from v2:
> >>
> >> - fix build when CONFIG_PM is not defined
> >>
> >>Changes from v1:
> >>
> >> - driver and documentation split into separate patches
> >> - removed custom controls
> >> - code changed according to the comments on v1
> >
> >>Ivaylo Dimitrov (2):
> >> media: Driver for Toshiba et8ek8 5MP sensor
> >> media: et8ek8: Add documentation
> >
> >Is there any progress here? Is there any way I could help?
> >
>
> There were some notes I need to address, unfortunately no spare time lately
> :( . Feel free to fix those for me and resend the patches. If not, I really
> don't know when I will have the time needed to focus on it.
So good start would be taking these two, address the comments, and try
to merge them?
Date: Sat, 11 Jun 2016 18:39:52 +0300
Subject: [PATCH v3 1/2] media: Driver for Toshiba et8ek8 5MP sensor
Date: Wed, 15 Jun 2016 22:24:40 +0300
Subject: Re: [PATCH v3 2/2] media: et8ek8: Add documentation
Thanks and best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/2] media: add et8ek8 camera sensor driver and documentation
From: Ivaylo Dimitrov @ 2016-10-23 18:05 UTC (permalink / raw)
To: Pavel Machek
Cc: sakari.ailus, sre, pali.rohar, linux-media, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, mchehab, devicetree,
linux-kernel
In-Reply-To: <20161023073322.GA3523@amd>
Hi,
On 23.10.2016 10:33, Pavel Machek wrote:
> Hi!
>
>> This series adds driver for Toshiba et8ek8 camera sensor found in Nokia N900
>>
>> Changes from v2:
>>
>> - fix build when CONFIG_PM is not defined
>>
>> Changes from v1:
>>
>> - driver and documentation split into separate patches
>> - removed custom controls
>> - code changed according to the comments on v1
>
>> Ivaylo Dimitrov (2):
>> media: Driver for Toshiba et8ek8 5MP sensor
>> media: et8ek8: Add documentation
>
> Is there any progress here? Is there any way I could help?
>
There were some notes I need to address, unfortunately no spare time
lately :( . Feel free to fix those for me and resend the patches. If
not, I really don't know when I will have the time needed to focus on it.
Regards,
Ivo
^ permalink raw reply
* Re: [PATCH 01/14] dma: sun6i-dma: Add burst case of 4
From: Jean-Francois Moine @ 2016-10-23 16:31 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mylène Josserand, vinod.koul-ral2JQCrhuEAvxtiuMwx3w,
wens-jdAy2FN1RRM, mturquette-rdvid1DuHRBWk0Htik3J/w,
sboyd-sgV2jX0FEOL9JmXXK+q4OQ, lgirdwood-Re5JQEeQqe8AvxtiuMwx3w,
broonie-DgEjT+Ai2ygdnm+yROfE0A, perex-/Fr2/VpizcU,
tiwai-IBi9RG/b67k, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
mark.rutland-5wv7dgnIgG8, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
devicetree-u79uwXL29TY76Z2rM5mHXA,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw,
alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
dmaengine-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20161004165553.GS5228@lukather>
On Tue, 4 Oct 2016 18:55:54 +0200
Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Tue, Oct 04, 2016 at 12:40:11PM +0200, Jean-Francois Moine wrote:
> > On Tue, 4 Oct 2016 11:46:14 +0200
> > Mylène Josserand <mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >
> > > Add the case of a burst of 4 which is handled by the SoC.
> > >
> > > Signed-off-by: Mylène Josserand <mylene.josserand-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > > drivers/dma/sun6i-dma.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > > index 8346199..0485204 100644
> > > --- a/drivers/dma/sun6i-dma.c
> > > +++ b/drivers/dma/sun6i-dma.c
> > > @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> > > switch (maxburst) {
> > > case 1:
> > > return 0;
> > > + case 4:
> > > + return 1;
> > > case 8:
> > > return 2;
> > > default:
> > > --
> > > 2.9.3
> >
> > This patch has already been rejected by Maxime in the threads
> > http://www.spinics.net/lists/dmaengine/msg08610.html
> > and
> > http://www.spinics.net/lists/dmaengine/msg08719.html
> >
> > I hope you will find the way he wants for this maxburst to be added.
>
> I was talking about something along these lines (not tested):
I wonder why you don't submit this yourself.
> -------8<---------
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 83461994e418..573ac4608293 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -240,6 +240,8 @@ static inline s8 convert_burst(u32 maxburst)
> switch (maxburst) {
> case 1:
> return 0;
> + case 4:
> + return 1;
> case 8:
> return 2;
> default:
> @@ -1110,11 +1112,19 @@ static int sun6i_dma_probe(struct platform_device *pdev)
> sdc->slave.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + sdc->slave.dst_bursts = BIT(1) | BIT(8);
> + sdc->slave.src_bursts = BIT(1) | BIT(8);
> sdc->slave.directions = BIT(DMA_DEV_TO_MEM) |
> BIT(DMA_MEM_TO_DEV);
> sdc->slave.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> sdc->slave.dev = &pdev->dev;
>
> + if (of_device_is_compatible(pdev->dev.of_node,
> + "allwinner,sun8i-h3-dma")) {
> + sdc->slave.dst_bursts |= BIT(4);
> + sdc->slave.src_bursts |= BIT(4);
> + }
> +
> sdc->pchans = devm_kcalloc(&pdev->dev, sdc->cfg->nr_max_channels,
> sizeof(struct sun6i_pchan), GFP_KERNEL);
> if (!sdc->pchans)
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index cc535a478bae..f7bbec24bb58 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -673,6 +673,8 @@ struct dma_filter {
> * each type of direction, the dma controller should fill (1 <<
> * <TYPE>) and same should be checked by controller as well
> * @max_burst: max burst capability per-transfer
> + * @dst_bursts: bitfield of the available burst sizes for the destination
> + * @src_bursts: bitfield of the available burst sizes for the source
You did not define dst_bursts nor src_bursts.
> * @residue_granularity: granularity of the transfer residue reported
> * by tx_status
> * @device_alloc_chan_resources: allocate resources and return the
> @@ -800,6 +802,14 @@ struct dma_device {
> static inline int dmaengine_slave_config(struct dma_chan *chan,
> struct dma_slave_config *config)
> {
> + if (config->src_maxburst && config->device->src_bursts &&
> + !(BIT(config->src_maxburst) & config->device->src_bursts))
The maxburst may be as big as 4Kibytes, then, I am not sure that this
code will work!
> + return -EINVAL;
> +
> + if (config->dst_maxburst && config->device->dst_bursts &&
> + !(BIT(config->dst_maxburst) & config->device->dst_bursts))
> + return -EINVAL;
> +
> if (chan->device->device_config)
> return chan->device->device_config(chan, config);
> -------8<------------
Yes, I know that the burst size is always a power of 2.
The best way to check it would be to change the {src,dts}_maxburst to a
bitmap of the possible bursts as 0x0d for 1,4 and 8 bytes. But this
asks for a lot of changes...
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: vfxxx: Add node corresponding to OCOTP
From: Shawn Guo @ 2016-10-23 13:57 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-arm-kernel, Sascha Hauer, Stefan Agner, Rob Herring,
Mark Rutland, Russell King, Fabio Estevam, devicetree,
linux-kernel
In-Reply-To: <1475371822-28879-1-git-send-email-andrew.smirnov@gmail.com>
On Sat, Oct 01, 2016 at 06:30:22PM -0700, Andrey Smirnov wrote:
> Add node corresponding to OCOTP IP block.
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] ARM: dts: vf610-zii-dev-rev-b: Remove I2C3
From: Shawn Guo @ 2016-10-23 13:54 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Sascha Hauer,
Stefan Agner, Rob Herring, Mark Rutland, Russell King,
Andrew Lunn, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1475371719-28736-1-git-send-email-andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Sat, Oct 01, 2016 at 06:28:39PM -0700, Andrey Smirnov wrote:
> I2C3 bus was only brought out in revision A1 of the board and revision
> B1 only brings out 3 I2C busses (I2C0, I2C1 and I2C2).
>
> Signed-off-by: Andrey Smirnov <andrew.smirnov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 0/3] ARM: dts: imx6qdl cleanups
From: Shawn Guo @ 2016-10-23 12:16 UTC (permalink / raw)
To: Jagan Teki
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1476437970-15800-1-git-send-email-jteki-oRp2ZoJdM/RWk0Htik3J/w@public.gmane.org>
On Fri, Oct 14, 2016 at 03:09:27PM +0530, Jagan Teki wrote:
> Patchset for imx6qdl devicetree files cleanups.
>
> Jagan Teki (3):
> arm: dts: imx6qdl: Fix "WARNING: please, no space before tabs"
> arm: dts: imx6qdl: Fix "ERROR: code indent should use tabs where
> possible"
> arm: dts: imx6qdl-wandboard-revb: Fix "ERROR: trailing whitespace"
Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2 3/3] clocksource: Add clockevent support to NPS400 driver
From: Noam Camus @ 2016-10-23 12:12 UTC (permalink / raw)
To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A
Cc: tglx-hfZtesqFncYOwBW4kG4KsQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Noam Camus
In-Reply-To: <1477224748-25223-1-git-send-email-noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Till now we used clockevent from generic ARC driver.
This was enough as long as we worked with simple multicore SoC.
When we are working with multithread SoC each HW thread can be
scheduled to receive timer interrupt using timer mask register.
This patch will provide a way to control clock events per HW thread.
The design idea is that for each core there is dedicated regirtser
(TSI) serving all 16 HW threads.
The register is a bitmask with one bit for each HW thread.
When HW thread wants that next expiration of timer interrupt will
hit it then the proper bit should be set in this dedicated register.
When timer expires all HW threads within this core which their bit
is set at the TSI register will be interrupted.
Driver can be used from device tree by:
compatible = "ezchip,nps400-timer0" <-- for clocksource
compatible = "ezchip,nps400-timer1" <-- for clockevent
Note that name convention for timer0/timer1 was taken from legacy
ARC design. This design is our base before adding HW threads.
Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Change-Id: Ib351e6fc7a6b691293040ae655f202f3cc2c1298
---
.../bindings/timer/ezchip,nps400-timer.txt | 15 --
.../bindings/timer/ezchip,nps400-timer0.txt | 17 ++
.../bindings/timer/ezchip,nps400-timer1.txt | 15 ++
drivers/clocksource/timer-nps.c | 220 +++++++++++++++++++-
include/linux/cpuhotplug.h | 1 +
5 files changed, 248 insertions(+), 20 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
deleted file mode 100644
index c8c03d7..0000000
--- a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
+++ /dev/null
@@ -1,15 +0,0 @@
-NPS Network Processor
-
-Required properties:
-
-- compatible : should be "ezchip,nps400-timer"
-
-Clocks required for compatible = "ezchip,nps400-timer":
-- clocks : Must contain a single entry describing the clock input
-
-Example:
-
-timer {
- compatible = "ezchip,nps400-timer";
- clocks = <&sysclk>;
-};
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
new file mode 100644
index 0000000..e3cfce8
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
@@ -0,0 +1,17 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible : should be "ezchip,nps400-timer0"
+
+Clocks required for compatible = "ezchip,nps400-timer0":
+- interrupts : The interrupt of the first timer
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+ compatible = "ezchip,nps400-timer0";
+ interrupts = <3>;
+ clocks = <&sysclk>;
+};
diff --git a/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
new file mode 100644
index 0000000..c0ab419
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
@@ -0,0 +1,15 @@
+NPS Network Processor
+
+Required properties:
+
+- compatible : should be "ezchip,nps400-timer1"
+
+Clocks required for compatible = "ezchip,nps400-timer1":
+- clocks : Must contain a single entry describing the clock input
+
+Example:
+
+timer {
+ compatible = "ezchip,nps400-timer1";
+ clocks = <&sysclk>;
+};
diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index 6156e54..0757328 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -46,7 +46,7 @@
/* This array is per cluster of CPUs (Each NPS400 cluster got 256 CPUs) */
static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
-static unsigned long nps_timer_rate;
+static unsigned long nps_timer1_freq;
static int nps_get_timer_clk(struct device_node *node,
unsigned long *timer_freq,
struct clk *clk)
@@ -87,10 +87,10 @@ static int __init nps_setup_clocksource(struct device_node *node)
nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
- nps_get_timer_clk(node, &nps_timer_rate, clk);
+ nps_get_timer_clk(node, &nps_timer1_freq, clk);
- ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
- nps_timer_rate, 301, 32, nps_clksrc_read);
+ ret = clocksource_mmio_init(nps_msu_reg_low_addr, "nps-tick",
+ nps_timer1_freq, 301, 32, nps_clksrc_read);
if (ret) {
pr_err("Couldn't register clock source.\n");
clk_disable_unprepare(clk);
@@ -99,5 +99,215 @@ static int __init nps_setup_clocksource(struct device_node *node)
return ret;
}
-CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer1",
nps_setup_clocksource);
+
+#ifdef CONFIG_EZNPS_MTM_EXT
+#include <soc/nps/mtm.h>
+
+/* Timer related Aux registers */
+#define AUX_REG_TIMER0_TSI 0xFFFFF850 /* timer 0 HW threads mask */
+#define NPS_REG_TIMER0_LIMIT 0x23 /* timer 0 limit */
+#define NPS_REG_TIMER0_CTRL 0x22 /* timer 0 control */
+#define NPS_REG_TIMER0_CNT 0x21 /* timer 0 count */
+
+#define TIMER0_CTRL_IE (1 << 0) /* Interrupt when Count reaches limit */
+#define TIMER0_CTRL_NH (1 << 1) /* Count only when CPU NOT halted */
+
+static unsigned long nps_timer0_freq;
+static unsigned long nps_timer0_irq;
+
+/*
+ * Arm the timer to interrupt after @cycles
+ */
+static void nps_clkevent_timer_event_setup(unsigned int cycles)
+{
+ write_aux_reg(NPS_REG_TIMER0_LIMIT, cycles);
+ write_aux_reg(NPS_REG_TIMER0_CNT, 0); /* start from 0 */
+
+ write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+}
+
+static void nps_clkevent_rm_thread(bool remove_thread)
+{
+ unsigned int cflags;
+ unsigned int enabled_threads;
+ unsigned long flags;
+ int thread;
+
+ local_irq_save(flags);
+ hw_schd_save(&cflags);
+
+ enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
+
+ /* remove thread from TSI1 */
+ if (remove_thread) {
+ thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+ enabled_threads &= ~(1 << thread);
+ write_aux_reg(AUX_REG_TIMER0_TSI, enabled_threads);
+ }
+
+ /* Re-arm the timer if needed */
+ if (!enabled_threads)
+ write_aux_reg(NPS_REG_TIMER0_CTRL, TIMER0_CTRL_NH);
+ else
+ write_aux_reg(NPS_REG_TIMER0_CTRL,
+ TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+ hw_schd_restore(cflags);
+ local_irq_restore(flags);
+}
+
+static void nps_clkevent_add_thread(bool set_event)
+{
+ int thread;
+ unsigned int cflags, enabled_threads;
+ unsigned long flags;
+
+ local_irq_save(flags);
+ hw_schd_save(&cflags);
+
+ /* add thread to TSI1 */
+ thread = read_aux_reg(CTOP_AUX_THREAD_ID);
+ enabled_threads = read_aux_reg(AUX_REG_TIMER0_TSI);
+ enabled_threads |= (1 << thread);
+ write_aux_reg(AUX_REG_TIMER0_TSI, enabled_threads);
+
+ /* set next timer event */
+ if (set_event)
+ write_aux_reg(NPS_REG_TIMER0_CTRL,
+ TIMER0_CTRL_IE | TIMER0_CTRL_NH);
+
+ hw_schd_restore(cflags);
+ local_irq_restore(flags);
+}
+
+static int nps_clkevent_set_next_event(unsigned long delta,
+ struct clock_event_device *dev)
+{
+ struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
+ struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
+
+ nps_clkevent_add_thread(true);
+ chip->irq_unmask(&desc->irq_data);
+
+ return 0;
+}
+
+/*
+ * Whenever anyone tries to change modes, we just mask interrupts
+ * and wait for the next event to get set.
+ */
+static int nps_clkevent_timer_shutdown(struct clock_event_device *dev)
+{
+ struct irq_desc *desc = irq_to_desc(nps_timer0_irq);
+ struct irq_chip *chip = irq_data_get_irq_chip(&desc->irq_data);
+
+ chip->irq_mask(&desc->irq_data);
+
+ return 0;
+}
+
+static int nps_clkevent_set_periodic(struct clock_event_device *dev)
+{
+ nps_clkevent_add_thread(false);
+ if (read_aux_reg(CTOP_AUX_THREAD_ID) == 0)
+ nps_clkevent_timer_event_setup(nps_timer0_freq / HZ);
+
+ return 0;
+}
+
+static int nps_clkevent_set_oneshot(struct clock_event_device *dev)
+{
+ nps_clkevent_rm_thread(true);
+ nps_clkevent_timer_shutdown(dev);
+
+ return 0;
+}
+
+static DEFINE_PER_CPU(struct clock_event_device, nps_clockevent_device) = {
+ .name = "NPS Timer0",
+ .features = CLOCK_EVT_FEAT_ONESHOT |
+ CLOCK_EVT_FEAT_PERIODIC,
+ .rating = 300,
+ .set_next_event = nps_clkevent_set_next_event,
+ .set_state_periodic = nps_clkevent_set_periodic,
+ .set_state_oneshot = nps_clkevent_set_oneshot,
+ .set_state_oneshot_stopped = nps_clkevent_timer_shutdown,
+ .set_state_shutdown = nps_clkevent_timer_shutdown,
+ .tick_resume = nps_clkevent_timer_shutdown,
+};
+
+static irqreturn_t timer_irq_handler(int irq, void *dev_id)
+{
+ /*
+ * Note that generic IRQ core could have passed @evt for @dev_id if
+ * irq_set_chip_and_handler() asked for handle_percpu_devid_irq()
+ */
+ struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+ int irq_reenable = clockevent_state_periodic(evt);
+
+ nps_clkevent_rm_thread(!irq_reenable);
+
+ evt->event_handler(evt);
+
+ return IRQ_HANDLED;
+}
+
+static int nps_timer_starting_cpu(unsigned int cpu)
+{
+ struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+
+ evt->cpumask = cpumask_of(smp_processor_id());
+
+ clockevents_config_and_register(evt, nps_timer0_freq, 0, ULONG_MAX);
+ enable_percpu_irq(nps_timer0_irq, 0);
+
+ return 0;
+}
+
+static int nps_timer_dying_cpu(unsigned int cpu)
+{
+ disable_percpu_irq(nps_timer0_irq);
+ return 0;
+}
+
+static int __init nps_setup_clockevent(struct device_node *node)
+{
+ struct clock_event_device *evt = this_cpu_ptr(&nps_clockevent_device);
+ struct clk *clk;
+ int ret;
+
+ nps_timer0_irq = irq_of_parse_and_map(node, 0);
+ if (nps_timer0_irq <= 0) {
+ pr_err("clockevent: missing irq");
+ return -EINVAL;
+ }
+
+ nps_get_timer_clk(node, &nps_timer0_freq, clk);
+
+ /* Needs apriori irq_set_percpu_devid() done in intc map function */
+ ret = request_percpu_irq(nps_timer0_irq, timer_irq_handler,
+ "Timer0 (per-cpu-tick)", evt);
+ if (ret) {
+ pr_err("Couldn't request irq\n");
+ clk_disable_unprepare(clk);
+ return ret;
+ }
+
+ ret = cpuhp_setup_state(CPUHP_AP_NPS_TIMER_STARTING,
+ "AP_NPS_TIMER_STARTING",
+ nps_timer_starting_cpu,
+ nps_timer_dying_cpu);
+ if (ret) {
+ pr_err("Failed to setup hotplug state");
+ clk_disable_unprepare(clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clkevt, "ezchip,nps400-timer0",
+ nps_setup_clockevent);
+#endif /* CONFIG_EZNPS_MTM_EXT */
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 34bd805..9efc1a3 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -60,6 +60,7 @@ enum cpuhp_state {
CPUHP_AP_MARCO_TIMER_STARTING,
CPUHP_AP_MIPS_GIC_TIMER_STARTING,
CPUHP_AP_ARC_TIMER_STARTING,
+ CPUHP_AP_NPS_TIMER_STARTING,
CPUHP_AP_KVM_STARTING,
CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
CPUHP_AP_KVM_ARM_VGIC_STARTING,
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 2/3] clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer
From: Noam Camus @ 2016-10-23 12:12 UTC (permalink / raw)
To: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A
Cc: tglx-hfZtesqFncYOwBW4kG4KsQ, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Noam Camus
In-Reply-To: <1477224748-25223-1-git-send-email-noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
From: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
nps_setup_clocksource() should take node as only argument i.e.:
replace
int __init nps_setup_clocksource(struct device_node *node, struct clk *clk)
with
int __init nps_setup_clocksource(struct device_node *node)
This is also serve as preperation for next patch which adds support
for clockevents to nps400.
Specifically we add new function nps_get_timer_clk() to serve clocksource
and later clockevent registration.
Signed-off-by: Noam Camus <noamca-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
drivers/clocksource/timer-nps.c | 49 ++++++++++++++++++++------------------
1 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/drivers/clocksource/timer-nps.c b/drivers/clocksource/timer-nps.c
index 70c149a..6156e54 100644
--- a/drivers/clocksource/timer-nps.c
+++ b/drivers/clocksource/timer-nps.c
@@ -47,6 +47,28 @@
static void *nps_msu_reg_low_addr[NPS_CLUSTER_NUM] __read_mostly;
static unsigned long nps_timer_rate;
+static int nps_get_timer_clk(struct device_node *node,
+ unsigned long *timer_freq,
+ struct clk *clk)
+{
+ int ret;
+
+ clk = of_clk_get(node, 0);
+ if (IS_ERR(clk)) {
+ pr_err("timer missing clk");
+ return PTR_ERR(clk);
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret) {
+ pr_err("Couldn't enable parent clk\n");
+ return ret;
+ }
+
+ *timer_freq = clk_get_rate(clk);
+
+ return 0;
+}
static cycle_t nps_clksrc_read(struct clocksource *clksrc)
{
@@ -55,23 +77,17 @@ static cycle_t nps_clksrc_read(struct clocksource *clksrc)
return (cycle_t)ioread32be(nps_msu_reg_low_addr[cluster]);
}
-static int __init nps_setup_clocksource(struct device_node *node,
- struct clk *clk)
+static int __init nps_setup_clocksource(struct device_node *node)
{
int ret, cluster;
+ struct clk *clk;
for (cluster = 0; cluster < NPS_CLUSTER_NUM; cluster++)
nps_msu_reg_low_addr[cluster] =
nps_host_reg((cluster << NPS_CLUSTER_OFFSET),
NPS_MSU_BLKID, NPS_MSU_TICK_LOW);
- ret = clk_prepare_enable(clk);
- if (ret) {
- pr_err("Couldn't enable parent clock\n");
- return ret;
- }
-
- nps_timer_rate = clk_get_rate(clk);
+ nps_get_timer_clk(node, &nps_timer_rate, clk);
ret = clocksource_mmio_init(nps_msu_reg_low_addr, "EZnps-tick",
nps_timer_rate, 301, 32, nps_clksrc_read);
@@ -83,18 +99,5 @@ static int __init nps_setup_clocksource(struct device_node *node,
return ret;
}
-static int __init nps_timer_init(struct device_node *node)
-{
- struct clk *clk;
-
- clk = of_clk_get(node, 0);
- if (IS_ERR(clk)) {
- pr_err("Can't get timer clock.\n");
- return PTR_ERR(clk);
- }
-
- return nps_setup_clocksource(node, clk);
-}
-
CLOCKSOURCE_OF_DECLARE(ezchip_nps400_clksrc, "ezchip,nps400-timer",
- nps_timer_init);
+ nps_setup_clocksource);
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v2 1/3] soc: Support for NPS HW scheduling
From: Noam Camus @ 2016-10-23 12:12 UTC (permalink / raw)
To: robh+dt, mark.rutland, daniel.lezcano
Cc: tglx, devicetree, linux-kernel, Noam Camus
In-Reply-To: <1477224748-25223-1-git-send-email-noamca@mellanox.com>
From: Noam Camus <noamca@mellanox.com>
This new header file is for NPS400 SoC (part of ARC architecture).
The header file includes macros for save/restore of HW scheduling.
The control of HW scheduling is acheived by writing core registers.
This code was moved from arc/plat-eznps so it can be used
from drivers/clocksource/, available only for CONFIG_EZNPS_MTM_EXT.
Signed-off-by: Noam Camus <noamca@mellanox.com>
---
arch/arc/plat-eznps/include/plat/ctop.h | 2 -
include/soc/nps/mtm.h | 59 +++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+), 2 deletions(-)
create mode 100644 include/soc/nps/mtm.h
diff --git a/arch/arc/plat-eznps/include/plat/ctop.h b/arch/arc/plat-eznps/include/plat/ctop.h
index 9d6718c..ee2e32d 100644
--- a/arch/arc/plat-eznps/include/plat/ctop.h
+++ b/arch/arc/plat-eznps/include/plat/ctop.h
@@ -46,9 +46,7 @@
#define CTOP_AUX_UDMC (CTOP_AUX_BASE + 0x300)
/* EZchip core instructions */
-#define CTOP_INST_HWSCHD_OFF_R3 0x3B6F00BF
#define CTOP_INST_HWSCHD_OFF_R4 0x3C6F00BF
-#define CTOP_INST_HWSCHD_RESTORE_R3 0x3E6F70C3
#define CTOP_INST_HWSCHD_RESTORE_R4 0x3E6F7103
#define CTOP_INST_SCHD_RW 0x3E6F7004
#define CTOP_INST_SCHD_RD 0x3E6F7084
diff --git a/include/soc/nps/mtm.h b/include/soc/nps/mtm.h
new file mode 100644
index 0000000..d2f5e7e
--- /dev/null
+++ b/include/soc/nps/mtm.h
@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2016, Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses. You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials
+ * provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#ifndef SOC_NPS_MTM_H
+#define SOC_NPS_MTM_H
+
+#define CTOP_INST_HWSCHD_OFF_R3 0x3B6F00BF
+#define CTOP_INST_HWSCHD_RESTORE_R3 0x3E6F70C3
+
+static inline void hw_schd_save(unsigned int *flags)
+{
+ __asm__ __volatile__(
+ " .word %1\n"
+ " st r3,[%0]\n"
+ :
+ : "r"(flags), "i"(CTOP_INST_HWSCHD_OFF_R3)
+ : "r3", "memory");
+}
+
+static inline void hw_schd_restore(unsigned int flags)
+{
+ __asm__ __volatile__(
+ " mov r3, %0\n"
+ " .word %1\n"
+ :
+ : "r"(flags), "i"(CTOP_INST_HWSCHD_RESTORE_R3)
+ : "r3");
+}
+
+#endif /* SOC_NPS_MTM_H */
--
1.7.1
^ permalink raw reply related
* [PATCH v2 0/3] Add clockevet for timer-nps driver to NPS400 SoC
From: Noam Camus @ 2016-10-23 12:12 UTC (permalink / raw)
To: robh+dt, mark.rutland, daniel.lezcano
Cc: tglx, devicetree, linux-kernel, Noam Camus
From: Noam Camus <noamca@mellanox.com>
Change log
---
V1 --> V2
Apply Daniel Lezcano comments:
CLOCKSOURCE_OF_DECLARE return value
update hotplug callbacks usage
squash of 2 first commits.
In this version I created new commit to serve as preperation for adding clockevents.
This way the last patch is more readable with clockevent content.
---
In first version of this driver we supported clocksource for the NPS400.
The support for clockevent was taken from Synopsys ARC timer driver.
This was good for working with our simulator of NPS400.
However in NPS400 ASIC the timers behave differently than simulation.
The timers in ASIC are shared between all threads whithin a core
and hence need different driver to support this behaviour.
The idea of this design is that we got 16 HW threads per core
each represented at bimask in a shared register in this core.
So when thread wants that next clockevent expiration will produce
timer interrupt to itself the correspondance bit in this register
should be set.
So theoretically if all 16 bits are set then all HW threads will get
timer interrupt on next expiration of timer 0.
Note that we use Synopsys ARC design naming convention for the timers
where:
timer0 is used for clockevents
timer1 is used for clocksource.
Noam Camus (3):
soc: Support for NPS HW scheduling
clocksource: update "fn" at CLOCKSOURCE_OF_DECLARE() of nps400 timer
clocksource: Add clockevent support to NPS400 driver
.../bindings/timer/ezchip,nps400-timer.txt | 15 --
.../bindings/timer/ezchip,nps400-timer0.txt | 17 ++
.../bindings/timer/ezchip,nps400-timer1.txt | 15 ++
arch/arc/plat-eznps/include/plat/ctop.h | 2 -
drivers/clocksource/timer-nps.c | 253 ++++++++++++++++++--
include/linux/cpuhotplug.h | 1 +
include/soc/nps/mtm.h | 59 +++++
7 files changed, 325 insertions(+), 37 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer.txt
create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer0.txt
create mode 100644 Documentation/devicetree/bindings/timer/ezchip,nps400-timer1.txt
create mode 100644 include/soc/nps/mtm.h
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx: b650v3: Calibrate USB PHY to pass eye diagram test
From: Shawn Guo @ 2016-10-23 12:11 UTC (permalink / raw)
To: Jaret Cantu
Cc: kernel-bIcnvbaLZ9MEGnE8C9+IrQ, fabio.estevam-3arQi8VN3Tc,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <1473887689-10792-1-git-send-email-jaret.cantu-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org>
On Wed, Sep 14, 2016 at 05:14:49PM -0400, Jaret Cantu wrote:
> Calibrate the USB PHY TX settings to pass the eye diagram signal
> integrity test. The settings are taken from the i.MX6 reference
> manual's recommended configuration for USB certification (66.2.6).
>
> Signed-off-by: Jaret Cantu <jaret.cantu-jEh4hwF5bVhBDgjK7y7TUQ@public.gmane.org>
Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 01/10] ethernet: add sun8i-emac driver
From: Rami Rosen @ 2016-10-23 12:08 UTC (permalink / raw)
To: Corentin Labbe
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8, wens-jdAy2FN1RRM,
linux-I+IVW8TIWO2tmTQ+vhA3Yw, David Miller, Florian Fainelli,
andrew-g2DYL2Zd6BY, Netdev, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <1475828757-926-2-git-send-email-clabbe.montjoie-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Corentin,
Trivial comment: rx_saf_fai and rx_daf_fail are not used. This implies that also
rx_saf_error and rx_daf_error should be removed:
> +static const char const estats_str[][ETH_GSTRING_LEN] = {
...
> + "rx_header_error",
> + "rx_overflow_error",
> + "rx_saf_error",
> + "rx_daf_error",
> + "rx_buf_error",
...
> +
> +struct sun8i_emac_stats {
> + u64 rx_payload_error;
...
> + u64 rx_overflow_error;
> + u64 rx_saf_fail;
> + u64 rx_daf_fail;
> + u64 tx_used_desc;
> + u64 napi_schedule;
> + u64 napi_underflow;
> +};
> +
Trivial: typo, should be: can transfer
> +/* The datasheet said that each descriptor can transfers up to 4096bytes
> + * But latter, a register documentation reduce that value to 2048
Regards,
Rami Rosen
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [v7, 1/3] nand: pl353: Add basic driver for arm pl353 smc nand interface
From: punnaiah choudary kalluri @ 2016-10-23 12:07 UTC (permalink / raw)
To: Boris Brezillon
Cc: Jason Gunthorpe, Punnaiah Choudary Kalluri, mark.rutland@arm.com,
jussi.kivilinna@iki.fi, linux-doc@vger.kernel.org,
artem.bityutskiy@linux.intel.com, linux-mtd@lists.infradead.org,
arnd@arndb.de, michal.simek@xilinx.com,
ezequiel.garcia@free-electrons.com, grant.likely@linaro.org,
devicetree@vger.kernel.org, jason@lakedaemon.net,
pawel.moll@arm.com
In-Reply-To: <20161021234611.1f737144@bbrezillon>
Hi Boris and Jason,
I am doing rework on these patches to accommodate recent changes with
respect to ooblayout. Also some of the comments that i have received
from Boris as part of the arasan nand controller upstream patches will
apply to this driver. So, i will be releasing the next set of patches for this
driver soon and request your time for reviewing those patches.
for now, please ignore these patches.
Regards,
Punnaiah
On Sat, Oct 22, 2016 at 3:16 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Fri, 21 Oct 2016 14:33:22 -0600
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>
>> On Mon, Jun 08, 2015 at 11:38:36PM +0530, Punnaiah Choudary Kalluri wrote:
>> > Add driver for arm pl353 static memory controller nand interface with
>> > HW ECC support. This controller is used in xilinx zynq soc for interfacing
>> > the nand flash memory.
>> >
>> > Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xilinx.com>
>> > Changes in v7:
>> > - Currently not implemented the memclk rate adjustments. I will
>> > look into this later and once the basic driver is accepted.
>> > - Fixed GPL licence ident
>> > Changes in v6:
>> > - Fixed the checkpatch.pl reported warnings
>> > - Using the address cycles information from the onfi param page
>> > earlier it is hardcoded to 5 in driver
>> > Changes in v5:
>> > - Configure the nand timing parameters as per the onfi spec
>> > Changes in v4:
>> > - Updated the driver to sync with pl353_smc driver APIs
>> > Changes in v3:
>> > - implemented the proper error codes
>> > - further breakdown this patch to multiple sets
>> > - added the controller and driver details to Documentation section
>> > - updated the licenece to GPLv2
>> > - reorganized the pl353_nand_ecc_init function
>> > Changes in v2:
>> > - use "depends on" rather than "select" option in kconfig
>> > - remove unused variable parts
>> > - remove dummy helper and use writel_relaxed directly
>>
>> What is the status of getting this driver merged? I am interested in
>> this driver and can provide some help if there are still TODO items.
>
> I'll try to review it next week.
>
>>
>> At the very least I will test it, is there something newer than v7 out
>> there?
>>
>> Regards,
>> Jason
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
^ permalink raw reply
* Re: [PATCH 1/3] arm64: arch_timer: Add device tree binding for hisilicon-161x01 erratum
From: Shawn Guo @ 2016-10-23 12:04 UTC (permalink / raw)
To: Ding Tianhong
Cc: Catalin Marinas, Will Deacon, Marc Zyngier, Mark Rutland,
Scott Wood, devicetree-u79uwXL29TY76Z2rM5mHXA,
stuart.yoder-3arQi8VN3Tc,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <962ea92f-870b-e1d0-5bb7-1a6d66c35122-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
On Sun, Oct 23, 2016 at 11:21:10AM +0800, Ding Tianhong wrote:
> This erratum describes a bug in logic outside the core, so MIDR can't be
> used to identify its presence, and reading an SoC-specific revision
> register from common arch timer code would be awkward. So, describe it
> in the device tree.
>
> Signed-off-by: Ding Tianhong <dingtianhong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> ---
> Documentation/devicetree/bindings/arm/arch_timer.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index ef5fbe9..26bc837 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -31,6 +31,12 @@ to deliver its interrupts via SPIs.
> This also affects writes to the tval register, due to the implicit
> counter read.
>
> +- hisilicon,erratum-161x01 : A boolean property. Indicates the presence of
> + QorIQ erratum 161201, which says that reading the counter is
QorIQ is a Freescale/NXP specific name, and shouldn't be there.
Shawn
> + unreliable unless the small range of value is returned by back-to-back reads.
> + This also affects writes to the tval register, due to the implicit
> + counter read.
> +
> ** Optional properties:
>
> - arm,cpu-registers-not-fw-configured : Firmware does not initialize
> --
> 1.9.0
>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3] pinctrl: Add SX150X GPIO Extender Pinctrl Driver
From: Linus Walleij @ 2016-10-23 10:47 UTC (permalink / raw)
To: Andrey Smirnov
Cc: Neil Armstrong, Peter Rosin, linux-kernel@vger.kernel.org,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
Roland Stigge, Vladimir Zapolskiy
In-Reply-To: <CAHQ1cqESoRdeHyvP=m=8F_VwiaYL+wbjFoNa6KvWBGfijhcRjg@mail.gmail.com>
On Sun, Oct 23, 2016 at 4:50 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> On Fri, Oct 21, 2016 at 2:09 AM, Neil Armstrong <narmstrong@baylibre.com> wrote:
>> Since the I2C sx150x GPIO expander driver uses platform_data to manage
>> the pins configurations, rewrite the driver as a pinctrl driver using
>> pinconf to get/set pin configurations from DT or debugfs.
>>
>> The pinctrl driver is functionnally equivalent as the gpio-only driver
>> and can use DT for pinconf. The platform_data confirmation is dropped.
>>
>> This patchset removed the gpio-only driver and selects the Pinctrl driver
>> config instead. This patchset also migrates the gpio dt-bindings to pinctrl
>> and add the pinctrl optional properties.
>>
>> The driver was tested with a SX1509 device on a BeagleBone black with
>> interrupt support and on an X86_64 machine over an I2C to USB converter.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
(...)
>> + ret = gpiochip_irqchip_add(&pctl->gpio,
>> + &pctl->irq_chip, 0,
>> + handle_edge_irq, IRQ_TYPE_EDGE_BOTH);
>
> Adding irqchip with IRQ_TYPE_EDGE_BOTH triggers a WARN in
> drivers/gpio/gpiolib.c:1671, on a custom Vybrid board that I have with
> the chip, so maybe it should be replaced with IRQ_TYPE_NONE? That's
> what I did for my testing and with that change
I fixed this up when applying. It is corect, it should always be IRQ_TYPE_NONE
unless it is a very old driver using boardfiles. The proper type is set up
when the driver using it requests the IRQ.
Some drivers also need to set the default handler to handle_bad_irq
and then also set that up in the irqchip .set_type() callback, especially
those hardwares that have an ACK register for edge IRQs so that a second
edge irq can come in when handling a first edge IRQ. Level IRQs don't
have this problem for obvious reasons, so we need to select between
handle_edge_irq() or handle_level_irq() on these hardwares.
Could you or Neil or both check if this applies to sx150x?
> Tested-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Thanks a lot Andrey!
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3] pinctrl: Add SX150X GPIO Extender Pinctrl Driver
From: Linus Walleij @ 2016-10-23 10:41 UTC (permalink / raw)
To: Neil Armstrong
Cc: Peter Rosin, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Roland Stigge,
Vladimir Zapolskiy, Andrey Smirnov
In-Reply-To: <1477040998-2016-1-git-send-email-narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
On Fri, Oct 21, 2016 at 11:09 AM, Neil Armstrong
<narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> wrote:
> Since the I2C sx150x GPIO expander driver uses platform_data to manage
> the pins configurations, rewrite the driver as a pinctrl driver using
> pinconf to get/set pin configurations from DT or debugfs.
Thanks! Patch applied for an immutable branch that I'll pull into
both pinctrl and gpio. All acks and test tags added too.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/5] drivers: gpio: Add support for multiple IPs
From: Linus Walleij @ 2016-10-23 10:32 UTC (permalink / raw)
To: Keerthy
Cc: Alexandre Courbot, Lokesh Vutla, Rob Herring,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux-OMAP,
Roger Quadros, Grygorii Strashko
In-Reply-To: <1476855239-32730-3-git-send-email-j-keerthy-l0cyMroinI0@public.gmane.org>
On Wed, Oct 19, 2016 at 7:33 AM, Keerthy <j-keerthy-l0cyMroinI0@public.gmane.org> wrote:
> From: Lokesh Vutla <lokeshvutla-l0cyMroinI0@public.gmane.org>
>
> Update GPIO driver to support Multiple GPIO IPs.
>
> Signed-off-by: Lokesh Vutla <lokeshvutla-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Keerthy <j-keerthy-l0cyMroinI0@public.gmane.org>
This commit message is not at all describing what the patch is doing.
What it does is bumping the GPIO pin offset in the Linux global
GPIO number space with 32 for each new controller.
> + static int bank_base;
>
> pdata = davinci_gpio_get_pdata(pdev);
> if (!pdata) {
> @@ -226,7 +227,8 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> chips[i].chip.direction_output = davinci_direction_out;
> chips[i].chip.set = davinci_gpio_set;
>
> - chips[i].chip.base = base;
> + chips[i].chip.base = bank_base;
> + bank_base += 32;
Why can you not rewrite the driver to pass -1 as base and
get a dynamic allocation of GPIO numbers instead? Then
you won't have this hairy problem.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 4/8] drivers:input:tsc2007: add iio interface to read external ADC input and temperature
From: H. Nikolaus Schaller @ 2016-10-23 9:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Benoît Cousson,
Tony Lindgren, Russell King, Arnd Bergmann, Michael Welling,
Mika Penttilä, Javier Martinez Canillas, Igor Grinberg,
Sebastian Reichel, Andrew F. Davis, Mark Brown, linux-input,
devicetree, linux-kernel, linux-omap, letux-kernel, linux-iio,
kernel
In-Reply-To: <1dd251db-7f45-78bf-c9dd-4ae29b0ed757@kernel.org>
Hi,
> Am 23.10.2016 um 11:24 schrieb Jonathan Cameron <jic23@kernel.org>:
>
> On 22/10/16 21:46, H. Nikolaus Schaller wrote:
>> Hi Jonathan,
>>
>>> Am 22.10.2016 um 20:33 schrieb Jonathan Cameron <jic23@kernel.org>:
>>>
>>> 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 <hns@goldelico.com>
>>> 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 <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)
>> @@ -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?
>> char phys[32];
>>
>> struct i2c_client *client;
>> + struct mutex mlock;
>>
>> 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(&ts->mlock);
>> tsc2007_read_values(ts, &tc);
>> + mutex_unlock(&ts->mlock);
>>
>> rt = tsc2007_calculate_resistance(ts, &tc);
>>
>> @@ -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(&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 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.
What I mean is:
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*ts));
>> *ts = iio_priv(indio_dev);
vs.
>> *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
So how can the IIO case just extend/wrap devm_kzalloc(&client->dev...) and still
be managed as well?
>
> 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).
>
> A couple of additions in the header to make it all work
> (the struct tsc2007 and tsc2007_xfer() + a few of the
> register defines..
>
> Nothing big and gets all the CONFIG_IIO into some really
> obvious stubbing out in the header.
Is there some example driver which is doing it that way to be optionally IIO
compatible? That might be easier to understand and copy than a description.
>
>> + if (!indio_dev) {
>> + dev_err(&client->dev, "iio_device_alloc failed\n");
>> + return -ENOMEM;
>> + }
>> +
>> + *ts = iio_priv(indio_dev);
>> +
>> + *input_dev = devm_input_allocate_device(&client->dev);
>> + if (!*input_dev)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, *ts);
>> + (*ts)->indio = indio_dev;
>> +
>> + indio_dev->name = "tsc2007";
>> + indio_dev->dev.parent = &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(&client->dev, "iio_device_register() failed: %d\n",
>> + err);
>> + return err;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +#define tsc2007_iio_device_unregister(ts) iio_device_unregister(ts->indio)
>> +
>> +#else /* CONFIG_IIO */
>> +
>> +static int tsc2007_alloc(struct i2c_client *client, struct tsc2007 **ts,
>> + struct input_dev **input_dev)
>> +{
>> + int err;
>> +
>> + *ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>> + if (!*ts)
>> + return -ENOMEM;
>> +
>> + *input_dev = devm_input_allocate_device(&client->dev);
>> + if (!*input_dev)
>> + return -ENOMEM;
>> +
>> + i2c_set_clientdata(client, *ts);
>> +
>> + return 0;
>> +}
>> +
>> +#define tsc2007_iio_device_unregister(ts) /* not needed */
> That's rather ugly and fragile. I'd stub it out as an actual function
> with no content and let the compiler drop it.
Well, it is a quick and dirty draft.
Should indeed better be a static (inline) function with empty body.
>> +
>> +#endif /* CONFIG_IIO */
>> +
>> #ifdef CONFIG_OF
>> static int tsc2007_get_pendown_state_gpio(struct device *dev)
>> {
>> @@ -459,20 +623,15 @@ static int tsc2007_probe(struct i2c_client *client,
>> I2C_FUNC_SMBUS_READ_WORD_DATA))
>> return -EIO;
>>
>> - ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
>> - if (!ts)
>> - return -ENOMEM;
>> -
>> - input_dev = devm_input_allocate_device(&client->dev);
>> - if (!input_dev)
>> - return -ENOMEM;
>> -
>> - i2c_set_clientdata(client, ts);
>> + err = tsc2007_alloc(client, &ts, &input_dev);
>> + if (err < 0)
>> + return err;
>>
>> ts->client = client;
>> ts->irq = client->irq;
>> ts->input = input_dev;
>> init_waitqueue_head(&ts->wait);
>> + mutex_init(&ts->mlock);
>>
>> snprintf(ts->phys, sizeof(ts->phys),
>> "%s/input0", dev_name(&client->dev));
>> @@ -543,6 +702,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_device_unregister(ts);
>> return err; /* usually, chip does not respond */
>> }
>>
>> @@ -556,6 +716,14 @@ static int tsc2007_probe(struct i2c_client *client,
>> return 0;
>> }
>>
>> +static int tsc2007_remove(struct i2c_client *client)
>> +{
>> + struct tsc2007 *ts = i2c_get_clientdata(client);
>> + input_unregister_device(ts->input);
>> + tsc2007_iio_device_unregister(ts);
>> + return 0;
>> +}
>> +
>> static const struct i2c_device_id tsc2007_idtable[] = {
>> { "tsc2007", 0 },
>> { }
>> @@ -578,6 +746,7 @@ static struct i2c_driver tsc2007_driver = {
>> },
>> .id_table = tsc2007_idtable,
>> .probe = tsc2007_probe,
>> + .remove = tsc2007_remove,
>> };
>>
>> module_i2c_driver(tsc2007_driver);
>>
>
BR and thanks,
Nikolaus
^ permalink raw reply
* Re: [PATCH v2 7/7] iio: envelope-detector: ADC driver based on a DAC and a comparator
From: Jonathan Cameron @ 2016-10-23 9:48 UTC (permalink / raw)
To: Peter Rosin, linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1477176226-10566-8-git-send-email-peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
On 22/10/16 23:43, Peter Rosin wrote:
> The DAC is used to find the peak level of an alternating voltage input
> signal by a binary search using the output of a comparator wired to
> an interrupt pin. Like so:
> _
> | \
> input +------>-------|+ \
> | \
> .-------. | }---.
> | | | / |
> | dac|-->--|- / |
> | | |_/ |
> | | |
> | | |
> | irq|------<-------'
> | |
> '-------'
>
> Signed-off-by: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
Again, just the need for opaque handling of the getting of the available
attribute.
Move your boiler plate into a core supplied function.
Looking very nice.
Jonathan
> ---
> .../testing/sysfs-bus-iio-adc-envelope-detector | 36 ++
> MAINTAINERS | 2 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/envelope-detector.c | 463 +++++++++++++++++++++
> 5 files changed, 512 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
> create mode 100644 drivers/iio/adc/envelope-detector.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
> new file mode 100644
> index 000000000000..8d890028a649
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
> @@ -0,0 +1,36 @@
> +What: /sys/bus/iio/devices/iio:deviceX/invert
> +Date: October 2016
> +KernelVersion: 4.9
> +Contact: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +Description:
> + The DAC is used to find the peak level of an alternating
> + voltage input signal by a binary search using the output
> + of a comparator wired to an interrupt pin. Like so:
> + _
> + | \
> + input +------>-------|+ \
> + | \
> + .-------. | }---.
> + | | | / |
> + | dac|-->--|- / |
> + | | |_/ |
> + | | |
> + | | |
> + | irq|------<-------'
> + | |
> + '-------'
> + The boolean invert attribute (0/1) should be set when the
> + input signal is centered around the maximum value of the
> + dac instead of zero. The envelope detector will search
> + from below in this case and will also invert the result.
> + The edge/level of the interrupt is also switched to its
> + opposite value.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/compare_interval_ms
> +Date: October 2016
> +KernelVersion: 4.9
> +Contact: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> +Description:
> + Number of milliseconds to wait for the comparator in each
> + step of the binary search for the input peak level. Needs
> + to relate to the frequency of the input signal.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4b6f6ec1b703..0d20f3561700 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6122,7 +6122,9 @@ IIO ENVELOPE DETECTOR
> M: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> L: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> S: Maintained
> +F: Documentation/ABI/testing/sysfs-bus-iio-adc-envelope-detector
> F: Documentation/devicetree/bindings/iio/adc/envelope-detector.txt
> +F: drivers/iio/adc/envelope-detector.c
>
> IIO SUBSYSTEM AND DRIVERS
> M: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 7edcf3238620..d5c4a95855c2 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -195,6 +195,16 @@ config DA9150_GPADC
> To compile this driver as a module, choose M here: the module will be
> called berlin2-adc.
>
> +config ENVELOPE_DETECTOR
> + tristate "Envelope detector using a DAC and a comparator"
> + depends on OF
> + help
> + Say yes here to build support for an envelope detector using a DAC
> + and a comparator.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called iio-envelope-detector.
> +
> config EXYNOS_ADC
> tristate "Exynos ADC driver support"
> depends on ARCH_EXYNOS || ARCH_S3C24XX || ARCH_S3C64XX || (OF && COMPILE_TEST)
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7a40c04c311f..0d773c6a0578 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_BCM_IPROC_ADC) += bcm_iproc_adc.o
> obj-$(CONFIG_BERLIN2_ADC) += berlin2-adc.o
> obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
> obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
> +obj-$(CONFIG_ENVELOPE_DETECTOR) += envelope-detector.o
> obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
> obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
> obj-$(CONFIG_HI8435) += hi8435.o
> diff --git a/drivers/iio/adc/envelope-detector.c b/drivers/iio/adc/envelope-detector.c
> new file mode 100644
> index 000000000000..3842d1a72023
> --- /dev/null
> +++ b/drivers/iio/adc/envelope-detector.c
> @@ -0,0 +1,463 @@
> +/*
> + * Driver for an envelope detector using a DAC and a comparator
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * The DAC is used to find the peak level of an alternating voltage input
> + * signal by a binary search using the output of a comparator wired to
> + * an interrupt pin. Like so:
> + * _
> + * | \
> + * input +------>-------|+ \
> + * | \
> + * .-------. | }---.
> + * | | | / |
> + * | dac|-->--|- / |
> + * | | |_/ |
> + * | | |
> + * | | |
> + * | irq|------<-------'
> + * | |
> + * '-------'
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +struct envelope {
> + spinlock_t comp_lock; /* protects comp */
> + int comp;
> +
> + struct mutex read_lock; /* protects everything else */
> +
> + int comp_irq;
> + u32 comp_irq_trigger;
> + u32 comp_irq_trigger_inv;
> +
> + struct iio_channel *dac;
> + struct delayed_work comp_timeout;
> +
> + unsigned int comp_interval;
> + bool invert;
> + u32 dac_max;
> +
> + int high;
> + int level;
> + int low;
> +
> + struct completion done;
> +};
> +
> +/*
> + * The envelope_detector_comp_latch function works together with the compare
> + * interrupt service routine below (envelope_detector_comp_isr) as a latch
> + * (one-bit memory) for if the interrupt has triggered since last calling
> + * this function.
> + * The ..._comp_isr function disables the interrupt so that the cpu does not
> + * need to service a possible interrupt flood from the comparator when no-one
> + * cares anyway, and this ..._comp_latch function reenables them again if
> + * needed.
> + */
> +static int envelope_detector_comp_latch(struct envelope *env)
> +{
> + int comp;
> +
> + spin_lock_irq(&env->comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(&env->comp_lock);
> +
> + if (!comp)
> + return 0;
> +
> + /*
> + * The irq was disabled, and is reenabled just now.
> + * But there might have been a pending irq that
> + * happened while the irq was disabled that fires
> + * just as the irq is reenabled. That is not what
> + * is desired.
> + */
> + enable_irq(env->comp_irq);
> +
> + /* So, synchronize this possibly pending irq... */
> + synchronize_irq(env->comp_irq);
> +
> + /* ...and redo the whole dance. */
> + spin_lock_irq(&env->comp_lock);
> + comp = env->comp;
> + env->comp = 0;
> + spin_unlock_irq(&env->comp_lock);
> +
> + if (comp)
> + enable_irq(env->comp_irq);
> +
> + return 1;
> +}
> +
> +static irqreturn_t envelope_detector_comp_isr(int irq, void *ctx)
> +{
> + struct envelope *env = ctx;
> +
> + spin_lock(&env->comp_lock);
> + env->comp = 1;
> + disable_irq_nosync(env->comp_irq);
> + spin_unlock(&env->comp_lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void envelope_detector_setup_compare(struct envelope *env)
> +{
> + int ret;
> +
> + /*
> + * Do a binary search for the peak input level, and stop
> + * when that level is "trapped" between two adjacent DAC
> + * values.
> + * When invert is active, use the midpoint floor so that
> + * env->level ends up as env->low when the termination
> + * criteria below is fulfilled, and use the midpoint
> + * ceiling when invert is not active so that env->level
> + * ends up as env->high in that case.
> + */
> + env->level = (env->high + env->low + !env->invert) / 2;
> +
> + if (env->high == env->low + 1) {
> + complete(&env->done);
> + return;
> + }
> +
> + /* Set a "safe" DAC level (if there is such a thing)... */
> + ret = iio_write_channel_raw(env->dac, env->invert ? 0 : env->dac_max);
> + if (ret < 0)
> + goto err;
> +
> + /* ...clear the comparison result... */
> + envelope_detector_comp_latch(env);
> +
> + /* ...set the real DAC level... */
> + ret = iio_write_channel_raw(env->dac, env->level);
> + if (ret < 0)
> + goto err;
> +
> + /* ...and wait for a bit to see if the latch catches anything. */
> + schedule_delayed_work(&env->comp_timeout,
> + msecs_to_jiffies(env->comp_interval));
> + return;
> +
> +err:
> + env->level = ret;
> + complete(&env->done);
> +}
> +
> +static void envelope_detector_timeout(struct work_struct *work)
> +{
> + struct envelope *env = container_of(work, struct envelope,
> + comp_timeout.work);
> +
> + /* Adjust low/high depending on the latch content... */
> + if (!envelope_detector_comp_latch(env) ^ !env->invert)
> + env->low = env->level;
> + else
> + env->high = env->level;
> +
> + /* ...and continue the search. */
> + envelope_detector_setup_compare(env);
> +}
> +
> +static int envelope_detector_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct envelope *env = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + /*
> + * When invert is active, start with high=max+1 and low=0
> + * since we will end up with the low value when the
> + * termination criteria is fulfilled (rounding down). And
> + * start with high=max and low=-1 when invert is not active
> + * since we will end up with the high value in that case.
> + * This ensures that the returned value in both cases are
> + * in the same range as the DAC and is a value that has not
> + * triggered the comparator.
> + */
> + mutex_lock(&env->read_lock);
> + env->high = env->dac_max + env->invert;
> + env->low = -1 + env->invert;
> + envelope_detector_setup_compare(env);
> + wait_for_completion(&env->done);
> + if (env->level < 0) {
> + ret = env->level;
> + goto err_unlock;
> + }
> + *val = env->invert ? env->dac_max - env->level : env->level;
> + mutex_unlock(&env->read_lock);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + return iio_read_channel_scale(env->dac, val, val2);
> + }
> +
> + return -EINVAL;
> +
> +err_unlock:
> + mutex_unlock(&env->read_lock);
> + return ret;
> +}
> +
> +static int envelope_detector_channel_raw_max(struct iio_channel *ch)
> +{
> + struct iio_dev *indio_dev = ch->indio_dev;
> + const int *vals;
> + int type;
> + int len;
> + int ret;
> +
Again, wrap this stuff up so that we have only
ret = iio_read_avail(ch, IIO_CHAN_INFO_RAW, &vals, &type, &len);
Could even have iio_read_avail_max returning just the max representation.
ABI wise might be cleaner to move the IIO type into the parameters as
well. It's no different from type realy in it's importance. Hmm. Not obvious
so take your pick with which ever approach feels best to you.
> + if (!ch->indio_dev->info->read_avail)
> + return -EINVAL;
> +
> + ret = ch->indio_dev->info->read_avail(indio_dev, ch->channel,
> + &vals, &type, &len,
> + IIO_CHAN_INFO_RAW);
> +
> + if (ret < 0)
> + return ret;
> + if (type != IIO_VAL_INT)
> + return -EINVAL;
> +
> + switch (ret) {
> + case IIO_AVAIL_RANGE:
> + if (len != 3)
> + return -EINVAL;
> + return vals[2];
> + }
> + return -EINVAL;
> +}
> +
> +static const struct iio_chan_spec envelope_detector_iio_channel = {
> + .type = IIO_ALTVOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + | BIT(IIO_CHAN_INFO_SCALE),
> + .indexed = 1,
> +};
> +
> +static ssize_t envelope_show_invert(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct envelope *env = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%u\n", env->invert);
> +}
> +
> +static ssize_t envelope_store_invert(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct envelope *env = iio_priv(indio_dev);
> + unsigned long invert;
> + int ret;
> + u32 trigger;
> +
> + ret = kstrtoul(buf, 0, &invert);
> + if (ret < 0)
> + return ret;
> + if (invert > 1)
> + return -EINVAL;
> +
> + trigger = invert ? env->comp_irq_trigger_inv : env->comp_irq_trigger;
> +
> + mutex_lock(&env->read_lock);
> + if (invert != env->invert)
> + ret = irq_set_irq_type(env->comp_irq, trigger);
> + if (!ret) {
> + env->invert = invert;
> + ret = len;
> + }
> + mutex_unlock(&env->read_lock);
> +
> + return ret;
> +}
> +
> +static IIO_DEVICE_ATTR(invert, 0644,
> + envelope_show_invert,
> + envelope_store_invert, 0);
> +
> +static ssize_t envelope_show_comp_interval(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct envelope *env = iio_priv(indio_dev);
> +
> + return sprintf(buf, "%u\n", env->comp_interval);
> +}
> +
> +static ssize_t envelope_store_comp_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t len)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct envelope *env = iio_priv(indio_dev);
> + unsigned long interval;
> + int ret;
> +
> + ret = kstrtoul(buf, 0, &interval);
> + if (ret < 0)
> + return ret;
> + if (interval > 1000)
> + return -EINVAL;
> +
> + mutex_lock(&env->read_lock);
> + env->comp_interval = interval;
> + mutex_unlock(&env->read_lock);
> +
> + return len;
> +}
> +
> +static IIO_DEVICE_ATTR(compare_interval_ms, 0644,
> + envelope_show_comp_interval,
> + envelope_store_comp_interval, 0);
> +
> +static struct attribute *envelope_detector_attributes[] = {
> + &iio_dev_attr_invert.dev_attr.attr,
> + &iio_dev_attr_compare_interval_ms.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group envelope_detector_attribute_group = {
> + .attrs = envelope_detector_attributes,
> +};
> +
> +static const struct iio_info envelope_detector_info = {
> + .read_raw = &envelope_detector_read_raw,
> + .driver_module = THIS_MODULE,
> + .attrs = &envelope_detector_attribute_group,
> +};
> +
> +static int envelope_detector_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct iio_dev *indio_dev;
> + struct envelope *env;
> + enum iio_chan_type type;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*env));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, indio_dev);
> + env = iio_priv(indio_dev);
> + env->comp_interval = 50; /* some sensible default? */
> +
> + spin_lock_init(&env->comp_lock);
> + mutex_init(&env->read_lock);
> + init_completion(&env->done);
> + INIT_DELAYED_WORK(&env->comp_timeout, envelope_detector_timeout);
> +
> + indio_dev->name = dev_name(dev);
> + indio_dev->dev.parent = dev;
> + indio_dev->dev.of_node = dev->of_node;
> + indio_dev->info = &envelope_detector_info;
> + indio_dev->channels = &envelope_detector_iio_channel;
> + indio_dev->num_channels = 1;
> +
> + env->dac = devm_iio_channel_get(dev, "dac");
> + if (IS_ERR(env->dac)) {
> + if (PTR_ERR(env->dac) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get dac input channel\n");
> + return PTR_ERR(env->dac);
> + }
> +
> + env->comp_irq = platform_get_irq_byname(pdev, "comp");
> + if (env->comp_irq < 0) {
> + if (env->comp_irq != -EPROBE_DEFER)
> + dev_err(dev, "failed to get compare interrupt\n");
> + return env->comp_irq;
> + }
> +
> + ret = devm_request_irq(dev, env->comp_irq, envelope_detector_comp_isr,
> + 0, "envelope-detector", env);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to request interrupt\n");
> + return ret;
> + }
> + env->comp_irq_trigger = irq_get_trigger_type(env->comp_irq);
> + if (env->comp_irq_trigger & IRQF_TRIGGER_RISING)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_FALLING;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_FALLING)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_RISING;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_HIGH)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_LOW;
> + if (env->comp_irq_trigger & IRQF_TRIGGER_LOW)
> + env->comp_irq_trigger_inv |= IRQF_TRIGGER_HIGH;
> +
> + ret = iio_get_channel_type(env->dac, &type);
> + if (ret < 0)
> + return ret;
> +
> + if (type != IIO_VOLTAGE) {
> + dev_err(dev, "dac is of the wrong type\n");
> + return -EINVAL;
> + }
> +
> + ret = envelope_detector_channel_raw_max(env->dac);
> + if (ret < 0) {
> + dev_err(dev, "dac does not indicate its raw maximum value\n");
> + return ret;
> + }
> + env->dac_max = ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct of_device_id envelope_detector_match[] = {
> + { .compatible = "axentia,tse850-envelope-detector", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, envelope_detector_match);
> +
> +static struct platform_driver envelope_detector_driver = {
> + .probe = envelope_detector_probe,
> + .driver = {
> + .name = "iio-envelope-detector",
> + .of_match_table = envelope_detector_match,
> + },
> +};
> +module_platform_driver(envelope_detector_driver);
> +
> +MODULE_DESCRIPTION("Envelope detector using a DAC and a comparator");
> +MODULE_AUTHOR("Peter Rosin <peda-koto5C5qi+TLoDKTGw+V6w@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v2 5/7] iio: dpot-dac: DAC driver based on a digital potentiometer
From: Jonathan Cameron @ 2016-10-23 9:42 UTC (permalink / raw)
To: Peter Rosin, linux-kernel
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, linux-iio, devicetree
In-Reply-To: <1477176226-10566-6-git-send-email-peda@axentia.se>
On 22/10/16 23:43, Peter Rosin wrote:
> It is assumed the that the dpot is used as a voltage divider between the
> current dpot wiper setting and the maximum resistance of the dpot. The
> divided voltage is provided by a vref regulator.
>
> .------.
> .-----------. | |
> | vref |--' .---.
> | regulator |--. | |
> '-----------' | | d |
> | | p |
> | | o | wiper
> | | t |<---------+
> | | |
> | '---' dac output voltage
> | |
> '------+------------+
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
Only suggstions is that some of the stuff for reading max value from
the channel wan't to get wrapped up in a utility function in iio/consumer.h
like the various channel value read and write functions.
As much of possible of internal IIO stuff should be opaque to consumers
(even when they happen to know about it as they are also IIO drivers ;)
Jonathan
> ---
> MAINTAINERS | 1 +
> drivers/iio/dac/Kconfig | 10 ++
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/dpot-dac.c | 298 +++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 310 insertions(+)
> create mode 100644 drivers/iio/dac/dpot-dac.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c68b72088945..8c8aae24b96b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6116,6 +6116,7 @@ M: Peter Rosin <peda@axentia.se>
> L: linux-iio@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/iio/dac/dpot-dac.txt
> +F: drivers/iio/dac/dpot-dac.c
>
> IIO SUBSYSTEM AND DRIVERS
> M: Jonathan Cameron <jic23@kernel.org>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 120b24478469..d3084028905b 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -200,6 +200,16 @@ config AD8801
> To compile this driver as a module choose M here: the module will be called
> ad8801.
>
> +config DPOT_DAC
> + tristate "DAC emulation using a DPOT"
> + depends on OF
> + help
> + Say yes here to build support for DAC emulation using a digital
> + potentiometer.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called dpot-dac.
> +
> config LPC18XX_DAC
> tristate "NXP LPC18xx DAC driver"
> depends on ARCH_LPC18XX || COMPILE_TEST
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 27642bbf75f2..f01bf4a99867 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_AD5686) += ad5686.o
> obj-$(CONFIG_AD7303) += ad7303.o
> obj-$(CONFIG_AD8801) += ad8801.o
> obj-$(CONFIG_CIO_DAC) += cio-dac.o
> +obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
> obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
> obj-$(CONFIG_M62332) += m62332.o
> obj-$(CONFIG_MAX517) += max517.o
> diff --git a/drivers/iio/dac/dpot-dac.c b/drivers/iio/dac/dpot-dac.c
> new file mode 100644
> index 000000000000..5613eae32347
> --- /dev/null
> +++ b/drivers/iio/dac/dpot-dac.c
> @@ -0,0 +1,298 @@
> +/*
> + * IIO DAC emulation driver using a digital potentiometer
> + *
> + * Copyright (C) 2016 Axentia Technologies AB
> + *
> + * Author: Peter Rosin <peda@axentia.se>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * It is assumed that the dpot is used as a voltage divider between the
> + * current dpot wiper setting and the maximum resistance of the dpot. The
> + * divided voltage is provided by a vref regulator.
> + *
> + * .------.
> + * .-----------. | |
> + * | vref |--' .---.
> + * | regulator |--. | |
> + * '-----------' | | d |
> + * | | p |
> + * | | o | wiper
> + * | | t |<---------+
> + * | | |
> + * | '---' dac output voltage
> + * | |
> + * '------+------------+
> + */
> +
> +#include <linux/err.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +struct dpot_dac {
> + struct regulator *vref;
> + struct iio_channel *dpot;
> + u32 max_ohms;
> +};
> +
> +static const struct iio_chan_spec dpot_dac_iio_channel = {
> + .type = IIO_VOLTAGE,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + | BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
> + .output = 1,
> + .indexed = 1,
> +};
> +
> +static int dpot_dac_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct dpot_dac *dac = iio_priv(indio_dev);
> + int ret;
> + unsigned long long tmp;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + return iio_read_channel_raw(dac->dpot, val);
> +
> + case IIO_CHAN_INFO_SCALE:
> + ret = iio_read_channel_scale(dac->dpot, val, val2);
> + switch (ret) {
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = *val * 1000000000LL;
> + do_div(tmp, dac->max_ohms);
> + tmp *= regulator_get_voltage(dac->vref) / 1000;
> + do_div(tmp, 1000000000LL);
> + *val = tmp;
> + return ret;
> + case IIO_VAL_INT:
> + /*
> + * Convert integer scale to fractional scale by
> + * setting the denominator (val2) to one...
> + */
> + *val2 = 1;
> + ret = IIO_VAL_FRACTIONAL;
> + /* ...and fall through. */
> + case IIO_VAL_FRACTIONAL:
> + *val *= regulator_get_voltage(dac->vref) / 1000;
> + *val2 *= dac->max_ohms;
> + break;
> + }
> +
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int dpot_dac_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + struct dpot_dac *dac = iio_priv(indio_dev);
> + struct iio_dev *dpot_dev = dac->dpot->indio_dev;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + return dpot_dev->info->read_avail(dpot_dev, dac->dpot->channel,
> + vals, type, length, mask);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int dpot_dac_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct dpot_dac *dac = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + return iio_write_channel_raw(dac->dpot, val);
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info dpot_dac_info = {
> + .read_raw = dpot_dac_read_raw,
> + .read_avail = dpot_dac_read_avail,
> + .write_raw = dpot_dac_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int dpot_dac_channel_max_ohms(struct iio_dev *indio_dev)
> +{
> + struct device *dev = &indio_dev->dev;
> + struct dpot_dac *dac = iio_priv(indio_dev);
> + struct iio_channel *ch = dac->dpot;
> + struct iio_dev *iio_ch_dev = ch->indio_dev;
> + const int *vals;
> + unsigned long long tmp;
> + int type;
> + int len;
> + int ret;
> + int val1;
> + int val2;
> + int max;
> +
Wants a wrapper in iio/consumer.h like we do for the various reads.
Otherwise, we'll end up with this as cut and paste boiler plate all over
the place.
> + if (!iio_ch_dev->info->read_avail) {
> + dev_err(dev, "dpot does not provide available raw values\n");
> + return -EINVAL;
> + }
> +
> + ret = iio_ch_dev->info->read_avail(iio_ch_dev, ch->channel,
> + &vals, &type, &len,
> + IIO_CHAN_INFO_RAW);
> +
> + if (ret < 0) {
> + dev_err(dev, "dpot failed to provide available raw values\n");
> + return ret;
> + }
(wrapper down to here..)
Worth noting that for the consumers they shouldn't need to know anything
about internal IIO structures (obviously you do here for other reasons
but try not to use that knowledge - should all be opaque outside the
subsystem.)
> + if (type != IIO_VAL_INT) {
> + dev_err(dev, "dpot provides strange raw values\n");
> + return -EINVAL;
> + }
> +
> + switch (ret) {
> + case IIO_AVAIL_RANGE:
> + if (len != 3) {
> + dev_err(dev, "need a range of available values\n");
> + return -EINVAL;
> + }
> + max = vals[2];
> + break;
> + default:
> + dev_err(dev, "dpot doesn't provide range of available values\n");
You could handle the set of values but it would be more complex so fair
enough. Any non linear dpots out there (with few enough values that we
can describe them in under page size)?
> + return -EINVAL;
> + }
> +
> + switch (iio_read_channel_scale(dac->dpot, &val1, &val2)) {
> + case IIO_VAL_INT:
> + return max * val1;
> + case IIO_VAL_FRACTIONAL:
> + tmp = (unsigned long long)max * val1;
> + do_div(tmp, val2);
> + return tmp;
> + case IIO_VAL_FRACTIONAL_LOG2:
> + tmp = (s64)vals[0] * 1000000000LL * max >> vals[1];
> + do_div(tmp, 1000000000LL);
> + return tmp;
> + default:
> + dev_err(dev, "dpot has a scale that is too weird\n");
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int dpot_dac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct iio_dev *indio_dev;
> + struct dpot_dac *dac;
> + enum iio_chan_type type;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*dac));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, indio_dev);
> + dac = iio_priv(indio_dev);
> +
> + indio_dev->name = dev_name(dev);
> + indio_dev->dev.parent = dev;
> + indio_dev->info = &dpot_dac_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &dpot_dac_iio_channel;
> + indio_dev->num_channels = 1;
> +
> + dac->vref = devm_regulator_get(dev, "vref");
> + if (IS_ERR(dac->vref)) {
> + if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "failed to get vref regulator\n");
> + return PTR_ERR(dac->vref);
> + }
> +
> + dac->dpot = devm_iio_channel_get(dev, "dpot");
> + if (IS_ERR(dac->dpot)) {
> + if (PTR_ERR(dac->dpot) != -EPROBE_DEFER)
> + dev_err(dev, "failed to get dpot input channel\n");
> + return PTR_ERR(dac->dpot);
> + }
> +
> + ret = iio_get_channel_type(dac->dpot, &type);
> + if (ret < 0)
> + return ret;
> +
> + if (type != IIO_RESISTANCE) {
> + dev_err(dev, "dpot is of the wrong type\n");
> + return -EINVAL;
> + }
> +
> + ret = dpot_dac_channel_max_ohms(indio_dev);
> + if (ret < 0)
> + return ret;
> + dac->max_ohms = ret;
> + dev_info(dev, "dpot max is %d\n", dac->max_ohms);
> +
> + ret = regulator_enable(dac->vref);
> + if (ret) {
> + dev_err(dev, "failed to enable the vref regulator\n");
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(dev, "failed to register iio device\n");
> + goto disable_reg;
> + }
> +
> + return 0;
> +
> +disable_reg:
> + regulator_disable(dac->vref);
> + return ret;
> +}
> +
> +static int dpot_dac_remove(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct dpot_dac *dac = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + regulator_disable(dac->vref);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id dpot_dac_match[] = {
> + { .compatible = "dpot-dac" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, dpot_dac_match);
> +
> +static struct platform_driver dpot_dac_driver = {
> + .probe = dpot_dac_probe,
> + .remove = dpot_dac_remove,
> + .driver = {
> + .name = "iio-dpot-dac",
> + .of_match_table = dpot_dac_match,
> + },
> +};
> +module_platform_driver(dpot_dac_driver);
> +
> +MODULE_DESCRIPTION("DAC emulation driver using a digital potentiometer");
> +MODULE_AUTHOR("Peter Rosin <peda@axentia.se>");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox