Re: [PATCH v6 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors

[Jonathan Cameron]

On Tue, 9 Mar 2021 07:38:13 +0000
Cristian Marussi <cristian.marussi@xxxxxxx> wrote:

> Hi
> 
> On Tue, Mar 09, 2021 at 06:37:27AM +0000, Sudeep Holla wrote:
> > On Mon, Mar 08, 2021 at 07:48:41PM +0000, Jonathan Cameron wrote:  
> > > On Mon, 8 Mar 2021 04:28:42 +0000
> > > Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
> > >  
> > > > Hi Jonathan,
> > > >
> > > > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote:  
> > > > > Hi Jonathan,
> > > > >
> > > > > Thanks for the detailed and careful review of this patch. Good to hear
> > > > > that v7 is not required.   Please find below answers to your
> > > > > questions. Looking forward to seeing this patch merged in the next
> > > > > cycle. Thanks for your help in making this happen.
> > > > >  
> > > >
> > > > Any update on this ? Please share the branch with is patch so that I
> > > > can pull and ask Cristian to make his changes on top.  
> > > Running a bit behind at the moment.
> > >  
> > 
> > No worries.
> >   
> > > Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch
> > > on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git
> > >  
> > 
> > Thanks
> >   
> > > Includes making the various long long local variables explicitly
> > > s64 and u64 as relevant.
> > >
> > > Based on the rc1 that eats babies so handle with care :)
> > >  
> > 
> > 😁 
> >   
> > > I've also merge this into the togreg branch of iio.git.
> > > As explained above that wasn't entirely trivial so Jyoti
> > > please take a quick look and check that changes are fine.
> > > Pushed out as testing to let the autobuilders poke at it.
> > > Assuming they don't find anything, it should be fine
> > > for Sudeep to merge that ib and everything will unwind
> > > nicely in Linus' tree next cycle.
> > >  
> > 
> > Hope so.
> >   
> > > There is a bit of an ongoing discussion of an earlier patch
> > > in the IIO tree, so I might end up redoing this merge
> > > if I need to rebase to sort that out, but I'll make sure
> > > the diff is the same (git ID might change).
> > >  
> > 
> > I can wait for a week or 2 if you think things will settle down by then.
> > We can avoid 2 different git IDs if possible. The main intention was to
> > give some reference to Cristian to rebase/post his series. I am all
> > fine to wait for a week or 2 for final branch.
> >  
> 
> In the meantime, I've anyway started reworking just based on -rc2 and barely
> cherry-picked Jyoti v6 on top of it just to able to start porting early the
> iiodev to the new SCMI API and be able to fully test it out (no problems
> so far); I will finally rebase on whatever final base branch Sudeep will
> pick but as Sudeep said I can wait, since I'm not expecting so much work
> still to do in that final rebase to -rc1-smh-final (...last famous words o_O)

Side note, there was a build bug on 32 bit that needs fixing so we'll be
tweaking the original patch.

> 
> Side question for Jyoti/Jonathan, for basic testing of this IIO SCMI
> driver (given that I'm not really familiar with IIO and I have not a full
> Android CTS/VTS suite to use it for testing), I'm doing something like:
> 
> (debian-arm64)root@debarm64:~# cd /sys/bus/iio/devices/iio\:device0
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_x_en 
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_y_en 
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_accel_z_en 
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > scan_elements/in_timestamp_en 
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 1 > buffer/enable && cat /dev/iio\:device0 | xxd
> 00000000: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 00000010: 18da ffff ffff ffff 00b8 d3a0 c09b 6a16  ..............j.
> 00000020: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 00000030: 18da ffff ffff ffff 0082 6edc c09b 6a16  ..........n...j.
> 00000040: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 00000050: 18da ffff ffff ffff 004c 0918 c19b 6a16  .........L....j.
> 00000060: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 00000070: 18da ffff ffff ffff 0016 a453 c19b 6a16  ...........S..j.
> 00000080: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 00000090: 18da ffff ffff ffff 00aa d9ca c19b 6a16  ..............j.
> 000000a0: 08da ffff ffff ffff 10da ffff ffff ffff  ................
> 000000b0: 18da ffff ffff ffff 0074 7406 c29b 6a16  .........tt...j.
> ^C
> (debian-arm64)root@debarm64:/sys/bus/iio/devices/iio:device0# echo 0 > buffer/enable 
That looks superficially plausible. 

There is a test tool in 
tools/iio/iio_generic_buffer.c which is fairly simple to use and will pretty print values for
you.  That'll sanity check all the type descriptions etc are correct as well.

Jonathan

> 
> (plus a bunch of DEBUG in my series to track notifs flows...)
> 
> Is this any good ?
> 
> Thanks
> 
> Cristian
> 
> > --
> > Regards,
> > Sudeep