[PATCH linux 2/3] drivers/misc: Add aspeed mbox driver

[Joel Stanley]

On Fri, Dec 9, 2016 at 4:43 PM, Cyril Bur <cyrilbur at gmail.com> wrote:
> Signed-off-by: Cyril Bur <cyrilbur at gmail.com>

Explain why we need this chardev, why it needs ioctls, etc.

> ---
>  drivers/misc/Kconfig           |   7 +
>  drivers/misc/Makefile          |   2 +
>  drivers/misc/mbox-host.c       | 326 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/mbox-host.h |  18 +++
>  4 files changed, 353 insertions(+)
>  create mode 100644 drivers/misc/mbox-host.c
>  create mode 100644 include/uapi/linux/mbox-host.h
>
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index a216b46..44f191d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -804,6 +804,13 @@ config PANEL_BOOT_MESSAGE
>           An empty message will only clear the display at driver init time. Any other
>           printf()-formatted message is valid with newline and escape codes.
>
> +config MBOX_HOST
> +       depends on ARCH_ASPEED

or COMPILE_TEST

> +       bool "Build MBOX driver for BMC to HOST communication"
> +       default "n"

If it's depending on ARCH_ASPEED I think default y is ok.

> +       ---help---
> +         Build the MBOX driver

More text here please.

> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  source "drivers/misc/cb710/Kconfig"
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index b2fb6dbf..7bcaf30 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile

"misc seems a bit too misc" - Andrew Jeffery.

How about drivers/soc? Or drivers/mfd?

> +#define DEVICE_NAME    "mbox-host"
> +
> +#define MBOX_NUM_REGS 16
> +#define MBOX_NUM_DATA_REGS 14
> +
> +#define MBOX_DATA_0 0x00
> +#define MBOX_STATUS_0 0x40
> +#define MBOX_STATUS_1 0x44
> +#define MBOX_BMC_CTRL 0x48
> +       #define MBOX_CTRL_RECV 0x80
> +       #define MBOX_CTRL_MASK 0x02
> +       #define MBOX_CTRL_SEND 0x01

If you want to do show these are bits in the CTRL register, do this. I
think it's more readable to represent the bits as shifts, or even
better, using the BIT() macro.

#define MBOX_BMC_CTRL 0x48
#define   MBOX_CTRL_RECV BIT(7)
#define   MBOX_CTRL_MASK BIT(1)

etc.

> +#define MBOX_HOST_CTRL 0x4c
> +#define MBOX_INTERRUPT_0 0x50
> +#define MBOX_INTERRUPT_1 0x54
> +

> +
> +static u8 mbox_inb(struct mbox_host *mbox_host, int reg)
> +{
> +       return ioread8(mbox_host->base + reg);
> +}
> +
> +static void mbox_outb(struct mbox_host *mbox_host, u8 data, int reg)
> +{
> +       iowrite8(data, mbox_host->base + reg);
> +}

As mentioned in the other email, all access to the LPC address space
is through regmap.

> +
> +static struct mbox_host *file_mbox_host(struct file *file)
> +{
> +       return container_of(file->private_data, struct mbox_host, miscdev);
> +}
> +
> +static int mbox_host_open(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}

This means userspace can open this device an unlimited number of
times. I think we only want one user at a time?


> +static int mbox_host_release(struct inode *inode, struct file *file)
> +{
> +       return 0;
> +}

Why not omit the callback?

> +
> +static unsigned int mbox_host_poll(struct file *file, poll_table *wait)
> +{
> +       struct mbox_host *mbox_host = file_mbox_host(file);
> +       unsigned int mask = 0;
> +
> +       poll_wait(file, &mbox_host->queue, wait);
> +
> +       if (mbox_inb(mbox_host, MBOX_BMC_CTRL) & MBOX_CTRL_RECV)
> +               mask |= POLLIN;
> +
> +       return mask;
> +}
> +
> +static const struct file_operations mbox_host_fops = {
> +       .owner          = THIS_MODULE,
> +       .open           = mbox_host_open,
> +       .read           = mbox_host_read,
> +       .write          = mbox_host_write,
> +       .release        = mbox_host_release,
> +       .poll           = mbox_host_poll,
> +       .unlocked_ioctl = mbox_host_ioctl,
> +};
> +
> +static void poll_timer(unsigned long data)
> +{
> +       struct mbox_host *mbox_host = (void *)data;
> +
> +       mbox_host->poll_timer.expires += msecs_to_jiffies(500);

Why 500?

> +       wake_up(&mbox_host->queue);
> +       add_timer(&mbox_host->poll_timer);
> +}
> +