[PATCH v2] drivers/misc: Add ASpeed LPC control driver

[Cyril Bur]

On Mon, 2017-01-16 at 07:34 -0600, Benjamin Herrenschmidt wrote:
> On Mon, 2017-01-16 at 11:45 +0100, Greg KH wrote:
> > For an ioctl structure like this, can you guarantee that the "padding"
> > will always be the same?  Last time I looked, the C standard didn't say
> > anything about that, so I would strongly recommend making it so that no
> > padding is needed at all...
> 
> The implicit padding is a matter of ABI more than C standard, we
> already rely on this in a bazillion of places but it *has* bitten us in
> a few corner cases (mostly when u64 is involved due to ABI differences
> between 32-bit and 64-bit), so explicit padding is indeed preferred.
> 
> Cyril can you respin with:
> 
> 	struct aspeed_lpc_ctrl_mapping {
> 		__u8    window_type;
> 		__u8    window_id;
> 		__u16   pad;
> 		__u32   addr;
> 		__u32   offset;
> 		__u32   size;
> 
> I prefer that, it makes more sense to keep the window type/id at the
> top. Alternatively call "pad", "flags", and describe that clients must
> zero it, that way we can use it for future extensions of we ever have
> to.

I've gone with calling it 'flags'. I'll resend.

> 
> Also for mmap, you have:
> 
>        /* AHB accesses are not cache coherent */
>        if (file->f_flags & O_DSYNC)
>                prot = pgprot_noncached(prot);
> 
> Why the test for O_DSYNC ? I'd rather not make this a userspace
> responsibility to get right... I'd have made it unconditional.
> 

Sure 

> I notice ARM has pgprot_dmacoherent() that might just do what we want
> here, ie, non-cachable if needed. Otherwise, I'd just unconditionally
> set it to noncached, we can revisit that if Aspeed ever comes up with a
> coherent SoC which I very much doubt but ...
> 

If we're happy with adding the check if Aspeed ever come with with a
coherent SoC then I'm cool with removing it. I'll do in v3.

Thanks Ben,

Cyril


> Cheers,
> Ben.
> 
>