On Fri, 2016-12-23 at 18:56 +1100, Cyril Bur wrote:
> On Fri, 2016-12-23 at 13:12 +1030, Andrew Jeffery wrote:
> > On Thu, 2016-12-22 at 17:06 +1100, Cyril Bur wrote:
> > > 
> > > +
> > > +static void mbox_outb(struct mbox *mbox, u8 data, int reg)
> > > +{
> > > +	regmap_write(mbox->regmap, mbox->base + reg, data);
> > 
> > We could use regmap_update_bits() here, but ultimately it's not going
> > to matter. Should we say something if regmap_write() fails (it
> > shouldn't, but if it does that's extra concerning)?
> 
> regmap_update_bits() to about touching the reserved section? Probs a
> good idea.

regmap_update_bits() will just write back what it read for the
untouched bits, which should be 0 in accordance with the datasheet.
It's not really a change in behaviour as we will still be writing to
reserved bits, but I think it makes the intent clear.

> Yeah I thought about putting a dev_err() in there. I'm not opposed, I
> sort of though, we'll you never see them or when you do something
> really bad has happened you'll probably notice everything broke...
> Still in the incredibly unlikely event that a print proves useful I'm
> happy to add it.

I feel an OCD itch to have something there, but I will leave it up to
you.

> > > > +	switch (cmd) {
> > > 
> > > +	case ASPEED_MBOX_IOCTL_ATN:
> > 
> > I think we should rename the IOCTL as what we do below doesn't
> > necessarily raise an interrupt.
> > 
> 
> Agreed, taking unput :). ASPEED_MBOX_IOCTL_WRITE_BYTE ?

That suggestion works for me.

Andrew