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

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

On Fri, 22 Sep 2023 17:18:56 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> 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@kernel.org> 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@smile.fi.intel.com/
> That APIs you would need I am pretty sure.

Andy, thank you for patience in reviewing this.

Hmm. I like the names, scatter and gather. In the firmware, I used
interleave and deinterleave, see
  https://gitlab.nic.cz/turris/hw/omnia_hw_ctrl/-/blob/master/src/drivers/i2c_iface.c#L360

But those functions work bit-wise. I realize that the I2C transfers in
the driver are so slow that such bit-wise cycling over a bitmap won't
matter much, but I still find my original proposal more simple and
straight-forward. But I will cave if you insist. Please let me know
(and can I then send your local patch in the series?)

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

There are no devm_ calls for mutex and work initialization. Are you
suggesting that I should write a release function for the gpio
sub-driver? Something like

static void omnia_gpiochip_release(dev, res)
{
  cancel_work();
  mutex_destroy();
}

int omnia_mcu_register_gpiochip(mcu)
{
  ...
  x = devres_alloc(omnia_gpiochip_release);
  devres_add(dev, x);
  ...
}

Marek