Re: [Outreachy kernel] iio meter patches - same!

[Gargi Sharma <gs051095@gmail.com>]

On Wed, Mar 22, 2017 at 2:01 AM, Alison Schofield <amsfield22@gmail.com> wrote:
>
> Simran, Gargi, Sayli, Arushi,
>
> With respect to these...
>
> meter/ade7753.c: simran singhal
> meter/ade7754.c: Gargi Sharma
> meter/ade7758_core.c: Sayli Karnik
> meter/ade7759.c: ArushiSinghal
>
> They are all near identical and therefore should have near identical
> solutions.  You could work together, or follow one another, but, in
> the end we don't want to see 4 different flavors of the solution.
> That makes it harder to maintain.
>
> Also - if 'we' can get one reviewed/ACK'd/applied, then we make the
> reviewers and maintainers life easier when we follow it with 3
> similar patches.
>
> Here's where I think you all dancing around...
> You have Lars' feedback from ade7753:
> > It might make sense to reuse the existing lock which currently
> > protects the
> > read/write functions. You can do this by introducing a variant of
> > ade7753_spi_{read,write}_reg_16() that does not take a lock and use
> > these to
> > implement the read-modify-write cycle in a protected section.
>
> I'll add to that:
> They were all using mlock to try to protect the spi read followed by
> write operation.
I have a question here: why do we want to protect the spi read?, I mean
usually we want to protect writes since there might be a race condition.
Or is it because if two threads are running the same function
concurrently we want to give access of the function to the thread
number 2 only once thread number 1 has written to the register?

>I don't think the lock was actually doing that.
> I don't see a guarantee that another spi write could not intervene,
> So perhaps there was no real protection.
Again, if we have two threads 1 and 2, won't the structure indio_dev
be same for the both of them? And only one thread can hold the lock at
a time.
>
> I'm thinking one function that locks the existing transaction lock, does
> the read, does the write, unlocks.
Simran wrote a function write_then_read in this patch
http://marc.info/?l=linux-driver-devel&m=149001635402891&w=2. Do we
want to do something similar?

>
> Can you all coordinate that please.  This way the next patch we see on
> this is agreed upon by all four.  Once that is applauded ;), the others
> can follow easily!
>
Yeah that makes sense and is the most productive use of all of our ability! :)

Thanks!
Gargi

> thanks,
> alison
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170321203110.GB12475%40d830.WORKGROUP.
> For more options, visit https://groups.google.com/d/optout.