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

["Marek Behún" <kabel@kernel.org>]

Hello Andy,

On Thu, 21 Sep 2023 12:55:08 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> > checkpatch warns: 
> >   ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP  
> 
> So, fix checkpatch then. It has false positives, because it doesn't know
> if the actual error code will sneak outside the kernel or not. But in
> the subsystem we internally use ENOTSUPP.

I will change it, sorry, checkpatch warning confused me.

> > > > +	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...

Or am I wrong?

> > > > +void omnia_mcu_unregister_gpiochip(struct omnia_mcu *mcu)
> > > > +{
> > > > +	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!

Marek