> -----Original Message----- > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Sent: Monday, December 6, 2021 2:10 PM > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx > Subject: Re: [RFC PATCH 0/1] LTC2688 support > > [External] > > On Mon, 6 Dec 2021 11:07:55 +0000 > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > From: Jonathan Cameron <jic23@xxxxxxxxxx> > > > Sent: Sunday, December 5, 2021 7:04 PM > > > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > Cc: linux-iio@xxxxxxxxxxxxxxx > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support > > > > > > [External] > > > > > > On Thu, 2 Dec 2021 15:37:40 +0000 > > > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > > > > > -----Original Message----- > > > > > From: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > > > Sent: Tuesday, November 30, 2021 3:43 PM > > > > > To: Jonathan Cameron <jic23@xxxxxxxxxx> > > > > > Cc: linux-iio@xxxxxxxxxxxxxxx > > > > > Subject: RE: [RFC PATCH 0/1] LTC2688 support > > > > > > > > > > [External] > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Jonathan Cameron <jic23@xxxxxxxxxx> > > > > > > Sent: Sunday, November 21, 2021 1:18 PM > > > > > > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > > > > Cc: linux-iio@xxxxxxxxxxxxxxx > > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support > > > > > > > > > > > > [External] > > > > > > > > > > > > On Mon, 15 Nov 2021 10:28:51 +0000 > > > > > > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > > > Thanks for your inputs... > > > > > > > > > > > > > > > From: Jonathan Cameron <jic23@xxxxxxxxxx> > > > > > > > > Sent: Friday, November 12, 2021 5:15 PM > > > > > > > > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > > > > > > Cc: linux-iio@xxxxxxxxxxxxxxx > > > > > > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support > > > > > > > > > > > > > > > > [External] > > > > > > > > > > > > > > > > On Thu, 11 Nov 2021 12:00:42 +0100 > > > > > > > > Nuno Sá <nuno.sa@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > Hi Nuno, > > > > > > > > > > > > > > > > > The reason why this is a RFC is because I'm still waiting > for > > > > > proper > > > > > > HW > > > > > > > > > to test this thing. The reason why I'm sending this > already is > > > > > > because > > > > > > > > > there's some non usual features which might require > extra > > > ABI. > > > > > > > > Hence, > > > > > > > > > while waiting I thought we could already speed up the > > > process > > > > > in > > > > > > > > regards > > > > > > > > > with the ABI. > > > > > > > > > > > > > > > > Wise move as this is an unusual beast :) > > > > > > > > > > > > > > > > > > > > > > > > > > I still pushed the driver but the intent here is not really > to > > > > > review > > > > > > it. > > > > > > > > > Naturally, if someone already wants to give some > feedback, > > > > > > that's > > > > > > > > very > > > > > > > > > much appreciated :) > > > > > > > > > > > > > > > > > > > > > > > > > > Now, there are three main interfaces depending on the > > > > > channel > > > > > > > > mode: > > > > > > > > > 1) default (no new ABI) > > > > > > > > > 2) toggle mode > > > > > > > > > 3) dither mode > > > > > > > > > > > > > > > > > > The channel mode will be a devicetree property as it > does > > > not > > > > > > really > > > > > > > > > make much sense to change between modes at runtime > > > even > > > > > > more > > > > > > > > because the > > > > > > > > > piece of HW we might want to control with a channel > might > > > be > > > > > > > > different > > > > > > > > > depending on the selected mode (I'm fairly sure on this > > > > > between > > > > > > > > toggle > > > > > > > > > and other modes but not so sure between dither and > > > default > > > > > > mode). > > > > > > > > > > > > > > > > I agree on toggle vs dither definitely being different, but > > > normal > > > > > > you > > > > > > > > could implement either as software toggle, or dither with > a 0 > > > > > > > > magnitude > > > > > > > > sine wave. So might not be worth implementing default > > > mode at > > > > > > all. > > > > > > > > No harm in doing so though if there are advantages to > having > > > it. > > > > > > > > > > > > > > My feeling is that we could probably have dither as the > "default > > > > > > mode". > > > > > > > More on this below... > > > > > > > > > > > > > > > > > > > > > > > > > toggle mode special ABI: > > > > > > > > > > > > > > > > > > * Toggle operation enables fast switching of a DAC > output > > > > > > between > > > > > > > > two > > > > > > > > > different DAC codes without any SPI transaction. Two > codes > > > are > > > > > > set > > > > > > > > in > > > > > > > > > input_a and input_b and then the output switches > > > according to > > > > > > an > > > > > > > > input > > > > > > > > > signal (input_a -> clk high; input_b -> clk low). > > > > > > > > > > > > > > > > > > out_voltageY_input_register > > > > > > > > > - selects between input_a and input_b. After selecting > one > > > > > > register, > > > > > > > > we > > > > > > > > > can write the dac code in out_voltageY_raw. > > > > > > > > > out_voltageY_toggle_en > > > > > > > > > - Disable/Enable toggle mode. The reason why I think > this > > > one > > > > > is > > > > > > > > > important is because if we wanna change the 2 codes, > we > > > > > > should > > > > > > > > first > > > > > > > > > disable this, set the codes and only then enable the > mode > > > > > > back... > > > > > > > > > As I'm writing this, It kind of came to me that we can > > > probably > > > > > > > > > achieve this with out_voltageY_powerdown attr > (maybe > > > > > takes a > > > > > > bit > > > > > > > > more > > > > > > > > > time to see the outputs but...) > > > > > > > > > > > > > > > > Hmm. These corner cases always take a bit of figuring out. > > > What > > > > > > you > > > > > > > > have > > > > > > > > here is a bit 'device specific' for my liking. > > > > > > > > > > > > > > > > So there is precedence for ABI in this area, on the > frequency > > > > > > devices > > > > > > > > but only > > > > > > > > for devices we still haven't moved out of staging. For > those > > > we > > > > > > > > needed a means > > > > > > > > to define selectable phases for PSK - where the selection > was > > > > > > either > > > > > > > > software or, > > > > > > > > much like here, a selection pin. > > > > > > > > > > > > > > > > out_altvotage0_phase0 etc > > > > > > > > > > > > > > > > so I guess the equivalent here would be > > > > > > > > out_voltageY_raw0 > > > > > > > > out_voltageY_raw1 > > > > > > > > and the selection would be via something like > > > > > > > > out_voltageY_symbol (0 or 1 in this case). - note this is > only > > > > > > > > relevant if you have the software toggle. Any enable > needed > > > for > > > > > > > > setting > > > > > > > > can be done as part of the write sequence for the raw0 / > > > raw1 > > > > > and > > > > > > > > should > > > > > > > > ideally not be exposed to userspace (controls that require > > > > > manual > > > > > > > > sequencing > > > > > > > > tend to be hard to use / document). > > > > > > > > > > > > > > Yeah, I thought about that. I was even thinking in having > > > something > > > > > > like > > > > > > > *_amplitude for dither mode. In some cases, where we > might > > > be > > > > > left > > > > > > > in some non obvious state (eg: moved the select register to > > > input b > > > > > > and > > > > > > > then we failed to set the code;), I prefer to leave things as > > > flexible > > > > > as > > > > > > > possible for userspace. But I agree it adds up more > complexity > > > and > > > > > in > > > > > > > this case, I can just always go to 'input_a' when writing > 'raw0'... > > > > > > > > > > > > > > > However, I'm not 100% sure it really maps to this case. > What > > > do > > > > > > you > > > > > > > > think? > > > > > > > > > > > > > > I think it can work. Though out_voltageY_symbol probably > > > needs to > > > > > > be > > > > > > > shared by type to be coherent with what we might have > with > > > > > TGPx. > > > > > > > > > > > > That's fine. > > > > > > > > > > > > > Note the sw_toggle register has a bit mask of the channels > we > > > > > want > > > > > > to > > > > > > > toggle which means we can toggle multiple channels at the > > > same > > > > > > time. > > > > > > > > > > > > Using that wired up to buffer mode might make sense. You'd > > > > > provide > > > > > > multiple > > > > > > buffers and allow channels to be selected into one of them at > a > > > time. > > > > > > Each > > > > > > buffer is then tied to a different toggle (TGP0, TGP1, etc) > > > > > > > > > > > > The same could be true for the software toggle. It'll get a bit > > > fiddly > > > > > > though. > > > > > > > > > > > > Perhaps this is an advanced feature to think about later... > > > > > > > > > > > > > It works the same with TGPx if you map multiple channels > to > > > the > > > > > > same > > > > > > > pin. > > > > > > > > > > > > > > There's also the question on how to handle this if a TGPx is > > > > > provided? > > > > > > > I guess it will just override the pin... But most likely having > them > > > > > both > > > > > > > at the same time will lead to non desired results (unless we > > > have a > > > > > > > way to gate/ungate the clocks)... > > > > > > I don't follow this bit. You mean TGPx and software toggle. As > far > > > as I > > > > > > can > > > > > > tell it's an either/or thing per channel. > > > > > > > > > > > > > > > > Here I meant that if we have a TGPx pin bundled to some > > > channel(s) > > > > > we > > > > > do not want to also dance with the sw_toggle bit of that > channel. > > > > > > > > Just a note on this. After starting my tests with the device, I can > > > actually > > > > say that if we have a TGPx set in the channel settings register, the > > > device > > > > will pretty much ignore the sw_toggle settings for that channel. I > > > could > > > > see that the output voltage was not toggling at all. As soon as I > > > removed > > > > the TGPx setting, then dancing with the sw_toggle started to > work. > > > So, for > > > > the HW this is not really an issue as it just ignores the sw_toggle. > On a > > > SW > > > > perspective, I'm still not sure if I just ignore this and write > whatever > > > the > > > > user sets or if I return some error code in the case a user tries to > > > toggle > > > > a channel with a binded TGPx. The first one is appealing as it > makes > > > the > > > > code much simpler while OTHO it might make sense to be > verbose > > > here > > > > otherwise the user might think something is happening when it > > > isn't... > > > > > > If we are in a static configuration (see below) then just don't > expose > > > the > > > software toggle control. Not having a big red button to press is the > > > best way to > > > tell userspace to not press the big red button... > > > > > > Hmm, I get your point and that's valid if I have the sw_toggle as a per > > channel attribute. Right now, I'm doing it as shared_by_type. The > reason is > > the sw_toggling is done by writing 1/0 in the toggle register and that > register > > is a bitmask being the mask 16bits wide. This allows you to toggle > channels > > at the same time in the same way you can do it if, say, you map 2,3 or > more > > channels to the same TGPx pin. > > Hmm. That will be tricky to support in a remotely 'general' way. > > > > > However, I'm also not happy for having this as shared_by_type attr. > One of > > my complains is that it makes it look like a dither capable channel will > also > > support this (and we already agreed that sw_toggle does not make > sense > > for dither mode; so do not expose it). For instance the output of > > 'iio_attr' on a dither enabled channel is: > > > > ``` > > root@analog:~# iio_attr -c ltc2688 voltage0 > > dev 'ltc2688', channel 'voltage0' (output), attr 'calibbias', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'calibscale', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_en', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_frequency', > value '32768' > > dev 'ltc2688', channel 'voltage0' (output), attr > 'dither_frequency_available', value '32768 16384 8192 4096 2048' > > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_phase', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr > 'dither_phase_available', value '0 90 180 270' > > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'dither_raw_available', > value '[0 1 65535]' > > dev 'ltc2688', channel 'voltage0' (output), attr 'offset', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'powerdown', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'raw', value '0' > > dev 'ltc2688', channel 'voltage0' (output), attr 'raw_available', value > '[0 1 65535]' > > dev 'ltc2688', channel 'voltage0' (output), attr 'scale', value > '0.076293945' > > dev 'ltc2688', channel 'voltage0' (output), attr 'symbol', value '0' > > ``` > > > > So you can see that symbol attr which does not make sense to be > there. And it's > > definitely not something wrong in the iio_attr app as the attr is > shared by type. > > > > Also, as you suggested, not having the symbol attr when it does not > make sense > > to have it also makes a lot of sense to me and that is one more plus > point to have > > this as a per channel thing. > > > > Anyways, I will probably send the patch with things as I have now so > you can > > have a felling of how it looks like. Unless you already tell me to just > not have it > > as a shared_by_type attr (which I'm getting more and more > convinced on my own; > > I guess I just need an extra push :D). > > Shared by type indeed doesn't work as it's a subset - however we > would need > a means to indicate what subset is used if we want to allow single write > to toggle multiple. Mind you - the moment we get to multiple > channels > this should probably be using the chrdev route rather than sysfs and > I'm > not sure how that would map to this at all. If someone really wants to toggle multiple channels through SW, one route would also be to use TGPx (mapping it to multiple channels) and use some of the host GPIOs to control the pin. Not nice as we would still need to provide a dummy clock to make the driver "happy" but it would be a possible workaround. This would only fail if all TGPx are already allocated for dither channels but that's very unlikely. OTHO, if we are doing the toggle through SW maybe our timings are not that strict so toggling all the channels in a loop is not that bad... Well, this is just wondering and playing "if" games so better to keep it simple and worry about this if such a usecase ever pops up. > So for now maybe take the view that software control of this is a weird > feature > anyway so make it per channel? Agreed, will make it per channel and only expose it if a TGPx mapping is not present. I was leaning in that direction anyways :). - Nuno Sá
