Hello

I have some minor comment below:

On Fri, Sep 16, 2016 at 12:39:25PM +0200, Cédric Le Goater wrote:
> From: Alistair Popple <alist...@popple.id.au>
> 
> This patch adds a simple device driver to expose the iBT interface on
> Aspeed SOCs (AST2400 and AST2500) as a character device. Such SOCs are
> commonly used as BMCs (BaseBoard Management Controllers) and this
> driver implements the BMC side of the BT interface.
> 
> The BT (Block Transfer) interface is used to perform in-band IPMI
> communication between a host and its BMC. Entire messages are buffered
> before sending a notification to the other end, host or BMC, that
> there is data to be read. Usually, the host emits requests and the BMC
> responses but the specification provides a mean for the BMC to send
> SMS Attention (BMC-to-Host attention or System Management Software
> attention) messages.
> 
> For this purpose, the driver introduces a specific ioctl on the
> device: 'BT_BMC_IOCTL_SMS_ATN' that can be used by the system running
> on the BMC to signal the host of such an event.
> 
> The device name defaults to '/dev/ipmi-bt-host'
> 
> Signed-off-by: Alistair Popple <alist...@popple.id.au>
> Signed-off-by: Jeremy Kerr <j...@ozlabs.org>
> Signed-off-by: Joel Stanley <j...@jms.id.au>
> [clg: - checkpatch fixes
>       - added a devicetree binding documentation
>       - replace 'bt_host' by 'bt_bmc' to reflect that the driver is
>         the BMC side of the IPMI BT interface
>       - renamed the device to 'ipmi-bt-host'
>       - introduced a temporary buffer to copy_{to,from}_user
>       - used platform_get_irq()
>       - moved the driver under drivers/char/ipmi/ but kept it as a misc
>         device
>       - changed the compatible cell to "aspeed,ast2400-bt-bmc"
> ]
> Signed-off-by: Cédric Le Goater <c...@kaod.org>
> ---
> 
>  Changes since v1:
> 
>  - replace 'bt_host' by 'bt_bmc' to reflect that the driver is
>    the BMC side of the IPMI BT interface
>  - renamed the device to 'ipmi-bt-host'
>  - introduced a temporary buffer to copy_{to,from}_user
>  - used platform_get_irq()
>  - moved the driver under drivers/char/ipmi/ but kept it as a misc
>    device
>  - changed the compatible cell to "aspeed,ast2400-bt-bmc"
> 
>  .../bindings/char/ipmi/aspeed,ast2400-bt-bmc.txt   |  23 +
>  drivers/Makefile                                   |   2 +-
>  drivers/char/ipmi/Kconfig                          |   7 +
>  drivers/char/ipmi/Makefile                         |   1 +
>  drivers/char/ipmi/bt-bmc.c                         | 486 
> +++++++++++++++++++++
>  include/uapi/linux/Kbuild                          |   1 +
>  include/uapi/linux/bt-bmc.h                        |  18 +
>  7 files changed, 537 insertions(+), 1 deletion(-)
>  create mode 100644 
> Documentation/devicetree/bindings/char/ipmi/aspeed,ast2400-bt-bmc.txt
>  create mode 100644 drivers/char/ipmi/bt-bmc.c
>  create mode 100644 include/uapi/linux/bt-bmc.h
> 
> diff --git 
> a/Documentation/devicetree/bindings/char/ipmi/aspeed,ast2400-bt-bmc.txt 
> b/Documentation/devicetree/bindings/char/ipmi/aspeed,ast2400-bt-bmc.txt
> new file mode 100644
> index 000000000000..fbbacd958240
> --- /dev/null

[..]

> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/errno.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/miscdevice.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> +#include <linux/bt-bmc.h>

Please sort them in alphabetical order, some of them seems not needed also 
(like spinlock.h)

> +
> +/*
> + * This is a BMC device used to communicate to the host
> + */
> +#define DEVICE_NAME  "ipmi-bt-host"
> +
> +#define BT_IO_BASE   0xe4
> +#define BT_IRQ               10
> +
> +#define BT_CR0               0x0
> +#define   BT_CR0_IO_BASE             16
> +#define   BT_CR0_IRQ                 12
> +#define   BT_CR0_EN_CLR_SLV_RDP              0x8
> +#define   BT_CR0_EN_CLR_SLV_WRP              0x4
> +#define   BT_CR0_ENABLE_IBT          0x1
> +#define BT_CR1               0x4
> +#define   BT_CR1_IRQ_H2B     0x01
> +#define   BT_CR1_IRQ_HBUSY   0x40
> +#define BT_CR2               0x8
> +#define   BT_CR2_IRQ_H2B     0x01
> +#define   BT_CR2_IRQ_HBUSY   0x40
> +#define BT_CR3               0xc
> +#define BT_CTRL              0x10
> +#define   BT_CTRL_B_BUSY             0x80
> +#define   BT_CTRL_H_BUSY             0x40
> +#define   BT_CTRL_OEM0                       0x20
> +#define   BT_CTRL_SMS_ATN            0x10
> +#define   BT_CTRL_B2H_ATN            0x08
> +#define   BT_CTRL_H2B_ATN            0x04
> +#define   BT_CTRL_CLR_RD_PTR         0x02
> +#define   BT_CTRL_CLR_WR_PTR         0x01
> +#define BT_BMC2HOST  0x14
> +#define BT_INTMASK   0x18
> +#define   BT_INTMASK_B2H_IRQEN               0x01
> +#define   BT_INTMASK_B2H_IRQ         0x02
> +#define   BT_INTMASK_BMC_HWRST               0x80

Why to use 3 space after some define ?

[..]

> +
> +#define BT_BMC_BUFFER_SIZE 256

Put this define with others

[..]

> +
> +static irqreturn_t bt_bmc_irq(int irq, void *arg)
> +{
> +     struct bt_bmc *bt_bmc = arg;
> +     uint32_t reg;

u32 is prefered other uint32_t, do you have run checkpatch --strict ?

> +
> +     reg = ioread32(bt_bmc->base + BT_CR2);
> +     reg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY;
> +     if (!reg)
> +             return IRQ_NONE;
> +
> +     /* ack pending IRQs */
> +     iowrite32(reg, bt_bmc->base + BT_CR2);
> +
> +     wake_up(&bt_bmc->queue);
> +     return IRQ_HANDLED;
> +}
> +
> +static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> +             struct platform_device *pdev)
> +{
> +     struct device *dev = &pdev->dev;
> +     uint32_t reg;
> +     int rc;
> +
> +     bt_bmc->irq = platform_get_irq(pdev, 0);
> +     if (!bt_bmc->irq)
> +             return -ENODEV;
> +
> +     rc = devm_request_irq(dev, bt_bmc->irq, bt_bmc_irq, IRQF_SHARED,
> +                     DEVICE_NAME, bt_bmc);
> +     if (rc < 0) {
> +             dev_warn(dev, "Unable to request IRQ %d\n", bt_bmc->irq);
> +             bt_bmc->irq = 0;
> +             return rc;
> +     }
> +
> +     /* Configure IRQs on the bmc clearing the H2B and HBUSY bits;
> +      * H2B will be asserted when the bmc has data for us; HBUSY
> +      * will be cleared (along with B2H) when we can write the next
> +      * message to the BT buffer
> +      */

This comment doesnt have the style recommended for new driver.

> +     reg = ioread32(bt_bmc->base + BT_CR1);
> +     reg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY;
> +     iowrite32(reg, bt_bmc->base + BT_CR1);
> +
> +     return 0;
> +}
> +
> +static int bt_bmc_probe(struct platform_device *pdev)
> +{
> +     struct bt_bmc *bt_bmc;
> +     struct device *dev;
> +     struct resource *res;
> +     int rc;
> +
> +     if (!pdev || !pdev->dev.of_node)
> +             return -ENODEV;
> +
> +     dev = &pdev->dev;
> +     dev_info(dev, "Found bt bmc device\n");
> +
> +     bt_bmc = devm_kzalloc(dev, sizeof(*bt_bmc), GFP_KERNEL);
> +     if (!bt_bmc)
> +             return -ENOMEM;
> +
> +     dev_set_drvdata(&pdev->dev, bt_bmc);
> +
> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +     if (!res) {
> +             dev_err(dev, "Unable to find resources\n");
> +             rc = -ENXIO;
> +             goto out_free;
> +     }
> +
> +     bt_bmc->base = devm_ioremap_resource(&pdev->dev, res);
> +     if (!bt_bmc->base) {
> +             rc = -ENOMEM;
> +             goto out_free;
> +     }
> +
> +     init_waitqueue_head(&bt_bmc->queue);
> +
> +     bt_bmc->miscdev.minor   = MISC_DYNAMIC_MINOR,
> +     bt_bmc->miscdev.name    = DEVICE_NAME,
> +     bt_bmc->miscdev.fops    = &bt_bmc_fops,
> +     bt_bmc->miscdev.parent = dev;
> +     rc = misc_register(&bt_bmc->miscdev);
> +     if (rc) {
> +             dev_err(dev, "Unable to register device\n");

Be more precise by saying misc device

> +             goto out_unmap;
> +     }
> +
> +     bt_bmc_config_irq(bt_bmc, pdev);
> +
> +     if (bt_bmc->irq) {
> +             dev_info(dev, "Using IRQ %d\n", bt_bmc->irq);
> +     } else {
> +             dev_info(dev, "No IRQ; using timer\n");
> +             setup_timer(&bt_bmc->poll_timer, poll_timer,
> +                             (unsigned long)bt_bmc);
> +             bt_bmc->poll_timer.expires = jiffies + msecs_to_jiffies(10);
> +             add_timer(&bt_bmc->poll_timer);
> +     }
> +
> +     iowrite32((BT_IO_BASE << BT_CR0_IO_BASE) |
> +               (BT_IRQ << BT_CR0_IRQ) |
> +               BT_CR0_EN_CLR_SLV_RDP |
> +               BT_CR0_EN_CLR_SLV_WRP |
> +               BT_CR0_ENABLE_IBT,
> +               bt_bmc->base + BT_CR0);
> +
> +     clr_b_busy(bt_bmc);
> +
> +     return 0;
> +
> +out_unmap:
> +     devm_iounmap(&pdev->dev, bt_bmc->base);

Why do you use devm_iounmap/devm_kfree since the interest with devm_ functions 
is that all cleanup is done when driver is removed.

> +
> +out_free:
> +     devm_kfree(dev, bt_bmc);
> +     return rc;

I think you should remove the space after this return

Regards

Corentin Labbe

------------------------------------------------------------------------------
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer

Reply via email to