> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > Sent: Monday, December 6, 2021 2:16 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 10:49:17 +0000 > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > -----Original Message----- > > > From: Jonathan Cameron <jic23@xxxxxxxxxx> > > > Sent: Sunday, December 5, 2021 7:01 PM > > > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > > > Cc: linux-iio@xxxxxxxxxxxxxxx > > > Subject: Re: [RFC PATCH 0/1] LTC2688 support > > > > > > [External] > > > > > > On Tue, 30 Nov 2021 14:43:25 +0000 > > > "Sa, Nuno" <Nuno.Sa@xxxxxxxxxx> wrote: > > > > > > Hi Nuno > > > > > > Hopefully I've not lost the plot on this! > > > > Not really. I had some days off so this was also set on hold from > > my side. > > > > > > > -----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. > > > Ultimately, > > > > that's on the user responsibility but we could also add some > guards I > > > guess. > > > > I'm not sure if it's an either/or thing per channel... IIUC, we spoke > > > about > > > > making dither and default mode the same. That might complicate > > > things a bit > > > > as: > > > > > > > > 1) We should not force a user to specify a TGPx pin for those > > > channels (since > > > > it can also work with dithering disabled). > > > > 2) Because of 1), we should also support sw_toggle for these > > > channels since > > > > someone might want to enable dither mode (at runtime) and the > > > TGPx pin was > > > > not given. Hence, we need to have a way to update the DAC > using > > > the sw_toggle. > > > > > > > > Did I understood things wrong? One thing that comes to my > mind, is > > > to return > > > > error (eg: EPERM or ENOTSUPP) if someone tries to enable dither > > > mode and > > > > no TGPx pin was selected for that channel? Hence, we do not > need > > > to add > > > > the sw_toggle ABI (out_voltage_symbol) for the default/dither > > > mode. Or > > > > maybe even better, we just expose the dither ABI if a TGPx pin is > > > given over > > > > dt (I try to explain the toggle/dither modes below; that might > help > > > you in > > > > understanding my reasoning here)? > > > > > > > > Alternatively, we just keep the approach I have in this RFC and > we > > > keep the > > > > 3 different modes (being mode a dt property; in the current state > I'm > > > using > > > > a boolean to say a channel Is in toggle mode)... Maybe with the > > > difference > > > > that we allow sw_toggle for toggle enabled channels. > > > > > > The corner I'm not clear on is what we do if for example all TGPx > pins > > > are > > > specified in DT. Is the mapping from channel to TGPx things in > toggle > > > mode > > > always a function of the external circuit or do we want to make it > > > runtime > > > controllable? > > > > > > I'm absolutely fine if we just make it a dt property - particularly > > > as those TGPx pins may well not be visible to the host processor. > > > > > > We probably do want to provide some options in dt for what they > > > might be > > > connnected to on the host. I'm guessing potentially a gpio, or a clk? > > > > For each TGPx pin (from the point you bind it to some channel), I'm > > actually making it mandatory to have a clock (the reasoning being, if > you > > say some channel is controlled over TGPx [being for toggle or dither > mode], > > you need to have some input at the pin). > > I'm not really sure what the usecases behind toggle are... Using a clock > with dither makes sense, but toggle might not be fixed frequency but > driven by some other type of event. Taken from the datasheet (usecases for toggle mode): "Examples include injection of a small ac bias, or independently switching between on and off states". A clock seems to fit even though I get your point that it might not fit all usecases for this. In that case, maybe sw_toggle can be enough :). Anyways, agreed that we do not have to support all the stuff at the very beginning and it's probably a bad idea specially in cases like this where we are not sure about all possible usecases. - Nuno Sá
