On Mon, 8 Jan 2018 15:12:52 +0100 Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx> wrote: > Hi Jonathan, Eugen, > > On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote: > > On Thu, 4 Jan 2018 17:17:54 +0200 > > Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> wrote: > > > > > On 29.12.2017 19:02, Jonathan Cameron wrote: > > > > On Fri, 22 Dec 2017 17:07:19 +0200 > > > > Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> wrote: > > > > > > > >> The ADC IP supports position and pressure measurements for a touchpad > > > >> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure > > > >> measurement support. > > > >> Using the inkern API, a driver can request a trigger and read the > > > >> channel values from the ADC. > > > >> The implementation provides a trigger named "touch" which can be > > > >> connected to a consumer driver. > > > >> Once a driver connects and attaches a pollfunc to this trigger, the > > > >> configure trigger callback is called, and then the ADC driver will > > > >> initialize pad measurement. > > > >> First step is to enable touchscreen 4wire support and enable > > > >> pen detect IRQ. > > > >> Once a pen is detected, a periodic trigger is setup to trigger every > > > >> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll > > > >> is called, and the consumer driver is then woke up, and it can read the > > > >> respective channels for the values : X, and Y for position and pressure > > > >> channel. > > > >> Because only one trigger can be active in hardware in the same time, > > > >> while touching the pad, the ADC will block any attempt to use the > > > >> triggered buffer. Same, conversions using the software trigger are also > > > >> impossible (since the periodic trigger is setup). > > > >> If some driver wants to attach while the trigger is in use, it will > > > >> also fail. > > > >> Once the pen is not detected anymore, the trigger is free for use (hardware > > > >> or software trigger, with or without DMA). > > > >> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled. > > > >> > > > >> Some parts of this patch are based on initial original work by > > > >> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy > > > >> > > > > OK, so comments inline. > > > > > > > > What I'm missing currently though is an explanation of why the slightly > > > > more standard arrangement of using a callback buffer doesn't work here. > > > > The only addition I think you need to do that is to allow a consumer to > > > > request a particular trigger. I also think some of the other provisions > > > > could be handled using standard features and slightly reducing the flexibility. > > > > I don't know for example if it's useful to allow other channels to be > > > > read when touch is not in progress or not. > > > > > > > > So restrictions: > > > > > > > > 1. Touch screen channels can only be read when touch is enabled. > > > > - use the available_scan_masks to control this. Or the callback that lets > > > > you do the same dynamically. > > > > 2. You need to push these channels to your consumer driver. > > > > - register a callback buffer rather than jumping through the hoops to > > > > insert your own pollfunc. That will call a function in your > > > > consumer, providing the data from the 3 channels directly. > > > > 3. You need to make sure it is using the right driver. For that you > > > > will I think need a new interface. > > > > > > > > Various other comments inline. I may well be missing something as this is > > > > a fair bit of complex code to read - if so then next version should have > > > > a clear cover letter describing why this more standard approach can't be > > > > used. > > > > > > Hello Jonathan and thanks for the review of my patch series, > > > > > > before starting and working over the required modifications and > > > suggestions that you sent me, I want to be a little more explicit about > > > the design of my implementation. > > > Hope this will clarify some things, and maybe I can as well understand > > > better what you have in mind to support this feature set. > > > > > > Why have I picked a pollfunction: We discussed a while back on the > > > mailing list that you do not have an inkern mechanism to expose the > > > triggers to other drivers, and that it may be a good idea to have it for > > > such kind of actually multi function device, instead of having a MFD > > > driver, an ADC driver, and an Input driver, all sharing the same > > > register map, the same IRQ , etc, with some kind of synchronization to > > > avoid stepping on each other for the hardware resource. > > > > No disagreement with that principle. > > > > > So I considered to expose the trigger by attaching and detaching > > > pollfunctions to it. Which is the main thing what we use a trigger for. > > > > Hmm. It's definitely one approach. But we do already have other drivers > > where the trigger is controlled by a consumer and to my mind that > > is a cleaner approach as it doesn't short cut the equivalent of > > doing it from userspace. > > > > drivers/iio/potentiostat/lmp91000.c does something similar though > > for a rather different use. You need your consumer interface > > to get the handle to the trigger in this case > > (the lmp91000 is actually providing the trigger rather than > > consuming it). > > > > > > > > > > So, what I had in mind, was to create a consumer driver that will > > > request triggers from the IIO device just like other drivers request > > > channels (part which is already done in IIO). > > > In order to do this I had to somehow wake up the consumer driver when > > > new data was available from the touchscreen. So, having the IRQ only in > > > the ADC device, and then on Pen detect and No pen detect just start or > > > stop the periodic trigger, which needs to be polled. The magic part is > > > that the consumer driver has a poll function already attached to this > > > trigger, so the poll function is just called every time we have new > > > data. The poll function is attached as an irq handler, and then we can > > > reuse all the read_raw data by using a scheduled work from the consumer > > > driver, to read the channels. > > > > If you had done this via a callback buffer the only difference is that > > the pollfunc would have been a standard one pulling the relevant channels > > and passing them on down to the buffer interface which could then decide > > what to do with them. > > > > > To do this, the ADC registers a special trigger named "touch trigger" > > > which is never enabled by the ADC driver. Instead, when a pollfunc is > > > attached to it, the attach function will also configure it with enabled > > > state. > > > > Whilst it might not make sense to enable it in the touch screen driver > > I'm not sure there is strictly any reason to prevent it being so used. > > > > > In the ADC, this means to start the touchscreen functionality. If > > > the touch is requested, it will standby and wait for pen detect IRQ. > > > Once we have pen detect, we can use a periodic trigger to sample the > > > touch data, and poll the "touch" trigger. The consumer driver will wake > > > up and schedule a work , that will use the standard read raw interface > > > (inkern) that will read three virtual channels (position + pressure). > > > They are not actual hardware channels, as the touch information is being > > > received on channels 0,1,2,3, but reading these virtual channels will > > > read from different registers inside the ADC IP ( x position, y > > > position, pressure), do some computations on the data, and feed the > > > consumer with the values , hiding the behind the scenes hardware > > > specific calculations. > > > > I wouldn't worry about whether they are real channels or not. This > > is really similar to a differential ADC (some of those do the differential > > digitally). Light sensors often have a number of 'real' channels used > > to derive (via hideous non linear calculations) the illuminance as > > it's hard to build a light sensor with the same sensitivity as the human > > eye. We have worse 'non real' channels as well such as activity channels > > on some the accelerometers that report if it thinks you are walking / > > running etc. > > > > > After trigger is polled , the ADC will resume normal functionality, and > > > the consumer driver will continue to sleep. > > > > So this is where I'm unsure. Do you actually have a usecase where it > > makes the sense to read from the ADC only when there is no touch? Any > > system doing that has an obvious denial of service attack - touch the > > screen. > > > > You're right. We have an issue in this case due to the hardware. Using > touchscreen has side effects on other channels. We can use only one > trigger for all the channels. The situation would have been better with > a trigger dedicated to the touchscreen. > > At the moment, we have not really stated about the exclusive use or not > of the touchscreen. We suppose we can get some customers wanting to use > both touchscreen and ADC. Eugen tried to deal with this case but, as you > noticed, it can lead to DoS. It's a restriction people aren't going to expect unfortunately... > > > > We need to have a periodic trigger to sample the data because the actual > > > analog to digital conversion inside the IP block needs to be triggered. > > > The touchscreen data measurements cannot happen in hardware without > > > being triggered. If I try with a hrtimer to get a periodic IRQ to just > > > read the data, it will never be ready. The datasheet states that the > > > touchscreen measurements "will be attached to the conversion sequence". > > > So the periodic trigger is forcing a conversion sequence. This could be > > > done with a software trigger as well, but why the hassle to start it > > > every 2 milliseconds (or other time interval), if we can do it by > > > periodic trigger ? > > > > Ah, one reason here would be to allow separate consumers to use the > > device. In that case you'd run with a periodic trigger all the time > > and have two buffers attached, the buffer_cb that is feeding your > > touchscreen and another buffer to deal with the other channels > > (presumably the standard one an IIO device has when using buffered > > interfaces). > > The issue is that we are sharing the periodic trigger so we have to use > the same period for both usage. Whilst a somewhat irritating restriction, it's probably not disastrous for most ADC uses. Jonathan > > Regards > > Ludovic > > > > > The buffer demux would ensure the data from the right channels > > ends up in the right place. It makes it look to the buffer > > consumer like it is the only thing using / controlling the data > > flow. > > > > > Once we get the No pen IRQ, we stop the periodic trigger and it can be > > > used in another purpose (software or external as of now in the driver, > > > in the future we can add PWM trigger and Timer trigger) > > > > This case isn't really useful though as any other use is denied > > access when touch occurs. > > > > I'll summarise what I think would work for this below. > > > > > > > > In short, the ADC in Sama5D2 also supports touchscreen, and in > > > touchscreen mode , 4 of the channels are being used for this purpose. > > > This however, doesn't stop the ADC to use the other channels . The > > > hardware has 12 total single channels and they can be paired to have 6 > > > more differential channels. The only thing that is blocked is the > > > trigger, but only if the pen is touching (when we start the periodic > > > trigger to sample the touchscreen). If the pen is not touching, an > > > external trigger or software trigger can be used without any issues (so > > > why limit the functionality, if this is available from hardware ?). > > > Because of the reason I discussed above (touchscreen sequence must be > > > triggered), we cannot use another trigger in the same time. > > > > > > > > > I see your idea with the callback buffer and it's worth exploring. > > > Mainly this series was to actually show you what I had in mind about > > > supporting the resistive touchscreen, and to give you some actually > > > working code/patch, so we can discuss based on real implementation, not > > > just suppositions. > > > > That side of things is fine. > > > > > > > > You are right in many of the other comments that you said, and I will > > > come up with a v2 to this series. For now, I need to know if this is a > > > good or right direction in which I am going, or I should try to change > > > all the mechanism to callback buffer ? Or maybe I am totally in a bad > > > direction ? > > > The requirements are that the consumer driver needs to be somehow woke > > > up for every new touch data available, and report to the input > > > subsystem. As it was done before, the at91 old driver, just creates and > > > registers an input device by itself, and then reports the position and > > > touches. I was thinking that with this trigger consumer implementation, > > > things can be better in terms of subsystem separation and support. > > > > > > Thanks again and let me know of your thoughts, > > > > > > Eugen > > > > So a couple of things come to mind on how I'd structure this. > > So what we have (very briefly) > > > > No touch screen case: > > * Generic ADC using all sorts of different triggers > > > > Touch screen only case: > > * Interrupt to indicate pen on / off > > * A need to do a periodic trigger of the device but only > > useful when touch is in progress. > > > > Touch screen and other users: > > * Interrupt to indicate pen on / off > > * Periodic trigger needed for touchscreen when touch in progress. > > * Do not have denial of service on other channels. > > > > First two cases are easy enough by having a magic trigger, third > > case is harder. > > If we have the touchscreen then I would drop support for direct access to > > to ADC channels whilst it's in use (so no sysfs - or emulate it if you > > really want it by stashing results from scans done when touch is in > > progress). > > > > Have your touch screen channels just as normal additional channels, > > but only via the buffered interface (no _RAW attribute). > > If someone sets up to read them via buffered interface with > > a different trigger I think they'll get values - whether they > > are right is dependent (if I understand correctly) on whether > > there is a touch in progress. So no harm done and it'll make > > the logic simpler. > > > > The moment touch is opened and acquires the IIO channels > > fix the trigger (may need new interface) to the periodic one > > that you were enabling and disabling on touch. > > Things get dicey if there is an existing user so you may > > have to do it on driver probe rather than open of the input > > device if we effectively want touch to have the highest > > priority use of the ADC. > > > > If other channels are enabled for buffered mode then note > > this in the driver and have the periodic trigger on all the > > time (to ensure they keep getting read) This will pass > > garbage to your touch screen driver, but it'll remove it due > > to the pressure value being too low so no harm there. > > > > Normal path will work for non touch channels (and in theory > > the touch ones if they are turned on) via IIO buffer > > interface. It'll be restricted in form due to the needs of > > the touch driver, but better than nothing and should cover > > most usecases. > > > > Now the interrupt on / off on touch bit becomes an optimization > > in the case of only the buffer_cb being attached. > > > > I think that fits cleanly in the current IIO framework and > > looks more similar to our existing provider consumer approaches. > > > > Still needs the hooks to get hold of the trigger though so > > as to be able to tell the ADC which one to use. So rather > > than being a trigger consumer interface, it's more of a trigger > > configuration interface.. Exact term doesn't matter though. > > > > Jonathan > > > > > > > > > > > > > > [...] > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
