Re: [PATCH v2 3/7] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Thu, Sep 21, 2023 at 10:25:01PM +0200, Marek Behún wrote:
> On Thu, 21 Sep 2023 12:55:08 +0300
> Andy Shevchenko <andy@xxxxxxxxxx> wrote:

...

> > > > > +	rising = reply[0] | (reply[2] << 8) | (reply[4] << 16) |
> > > > > +		 (reply[6] << 24);
> > > > > +	falling = reply[1] | (reply[3] << 8) | (reply[5] << 16) |
> > > > > +		  (reply[7] << 24);    
> > > > 
> > > > With a help of two masks, you can access to the both edges as to
> > > >   64-bit value and simplify the code.  
> > > 
> > > Huh? As in
> > >   rising = reply & 0x00ff00ff00ff00ff;
> > >   falling = reply & 0xff00ff00ff00ff00;
> > > ?
> > > But then I can't or the rising bit with the corresponding falling bit
> > > to get pending...
> > > Or I guess i can with:
> > >   pending = rising & (pending >> 8);
> > > 
> > > Am I understanding you correctly?
> > > 
> > > But then I would need to store the mask in driver data as a 64-bit
> > > value with half the data not used. Also the CPU is 32-bit.  
> > 
> > If you use proper bitmaps, perhaps this will be easier. You can use one for
> > each and merge them whenever you want (with bitmap_or() call) or split (with
> > bitmap_and() respectively):
> > 
> > 	bitmap_or(full, raising, failing); // merge
> > 	bitmap_and(raising, full, rasing_mask); // split
> 
> Hmm. But then what? I or the result and use it as pending interrupt
> bitmap, to be iterated over. The indexes of the bits correspond to the
> constants in the MCU API.
> 
> So after your suggestion I have rising and falling containgin
>   rising = 00rr00rr00rr00rr; /* r means rising bits */
>   falling = 00ff00ff00ff00ff; /* f means falling bits */
>   pending = rising | falling;
> which means:
>   pending = pp00pp00pp00pp; /* p means pending bits */
> But these bit positions do not correspond to the interrupt number
> anymore.
> 
> I still think the de-interleaving of the buffer from
>   rr ff rr ff rr ff rr ff
> into two words:
>   rising = rrrrrrrr;
>   falling = ffffffff;
> is simpler...

There are two sides of this: OS and hardware. See Xilinx GPIO driver how it's
made there. But before going that way, check on
https://lore.kernel.org/all/ZOMmuZuhdjA6mdIG@xxxxxxxxxxxxxxxxxx/
That APIs you would need I am pretty sure.

...

> > > > > +	if (!(mcu->features & FEAT_NEW_INT_API))
> > > > > +
> > > > >   cancel_delayed_work_sync(&mcu->button_release_emul_work); +
> > > > > +	mutex_destroy(&mcu->lock);    
> > > > 
> > > > Wrong order?  
> > > 
> > > No, the mutex may be used in the work. Can't destroy it first. Or am I
> > > misunderstanding something?  
> > 
> > I mean you are using a lot of devm(), can mutex be used in IRQ or whatever
> > that can be triggered after this call?
> 
> OK, I think I need to free the irq before canceling the work. Thank you!

Can you rather switch everything to be devm managed?

-- 
With Best Regards,
Andy Shevchenko