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

[Andy Shevchenko <andy@xxxxxxxxxx>]

On Mon, Sep 25, 2023 at 12:03:56PM +0200, Marek Behún wrote:
> On Fri, 22 Sep 2023 17:18:56 +0300
> Andy Shevchenko <andy@xxxxxxxxxx> 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@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.
> 
> 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?)

You can. but I need to add test cases there.

Yes, I think the best is to have hardware values and Linux cached ones
to be separated. Let me try my best and send it out this week.

...

> > > > > > > +	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();
> }

Not together, but
- for mutex use devm_add_action_or_reset() as done in many other drivers
  for the same reason;
- for the work we have devm_work_autocancel()
  (you need to include devm-helpers.h)

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

-- 
With Best Regards,
Andy Shevchenko