Re: [PATCH v5 03/11] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún <kabel@xxxxxxxxxx>]

On Mon, 25 Mar 2024 11:10:04 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> Hi Marek,
> 
> I can't say I did a proper review but browsing through the code without 
> proper understanding of the platform raised one small question :)
> 
> On 3/23/24 18:43, Marek Behún wrote:
> > Add support for GPIOs connected to the MCU on the Turris Omnia board.
> > 
> > This includes:
> > - front button pin
> > - enable pins for USB regulators
> > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> > - on board revisions 32+ also various peripheral resets and another
> >    voltage regulator enable pin
> > 
> > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>  
> 
> ...
> 
> > +/**
> > + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
> > + *	@dst: the destination u8 array of interleaved bytes
> > + *	@rising: rising mask
> > + *	@falling: falling mask
> > + *
> > + * Interleaves the little-endian bytes from @rising and @falling words.  
> This means the 'rising' and 'falling' should always be little-endian? 
> Should the parameter types reflect this? Or should we see some 
> cpu_to_le() calls? (Or, is this code just guaranteed to be always 
> running on a le-machine?).
> 
> > + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
> > + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
> > + *
> > + * The MCU receives interrupt mask and reports pending interrupt bitmap int this
> > + * interleaved format. The rationale behind it is that the low-indexed bits are
> > + * more important - in many cases, the user will be interested only in
> > + * interrupts with indexes 0 to 7, and so the system can stop reading after
> > + * first 2 bytes (r0, f0), to save time on the slow I2C bus.
> > + *
> > + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> > + * and use an appropriate bitmap_* function once such a function exists.
> > + */
> > +static void omnia_mask_interleave(u8 *dst, u32 rising, u32 falling)
> > +{
> > +	for (int i = 0; i < sizeof(u32); ++i) {
> > +		dst[2 * i] = rising >> (8 * i);
> > +		dst[2 * i + 1] = falling >> (8 * i);
> > +	}
> > +}
> > +
> > +/**
> > + * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling
> > + *	@src: the source u8 array containing the interleaved bytes
> > + *	@rising: pointer where to store the rising mask gathered from @src
> > + *	@falling: pointer where to store the falling mask gathered from @src
> > + *
> > + * This is the inverse function to omnia_mask_interleave.
> > + */
> > +static void omnia_mask_deinterleave(const u8 *src, u32 *rising, u32 *falling)
> > +{
> > +	*rising = *falling = 0;
> > +
> > +	for (int i = 0; i < sizeof(u32); ++i) {
> > +		*rising |= src[2 * i] << (8 * i);
> > +		*falling |= src[2 * i + 1] << (8 * i);
> > +	}  
> 
> Also here I could expect seeing le_to_cpu() unless I am (again :]) 
> missing something.

I don't understand. The rising and falling variables have native
endiannes, as they should have.

And the src and dst are u8 arrays, which contain two LE32
unsigned integers, but these integers are interleaved. I am converting
then to two native unsigned integers byte by byte, so I am basically
doing what a theoretical le32_interleaved_le32_to_cpu() would have done
if it did exist. Putting another le*_to_cpu() would only break things.

Marek