Re: [PATCH] ipmi: bt-bmc: Use registers directly

[Corey Minyard <minyard@acm.org>]

On Wed, Oct 06, 2021 at 02:33:06AM +0000, Joel Stanley wrote:
> Hi Corey,
> 
> On Fri, 3 Sept 2021 at 05:10, Joel Stanley <joel@jms.id.au> wrote:
> >
> > This driver was originally written to use the regmap abstraction with no
> > clear benefit. As the registers are always mmio and there is no sharing
> > of the region with other devices, we can safely read and write without
> > the locking that regmap provides.
> >
> > This reduces the code size of the driver by about 25%.
> 
> Do you have any feedback on this one?

Ah, sorry, this looks ok to me.  I was unable to find much useful
information about the benefits of regmap, but it seems that if you had
registers layed out in various ways, with different caching, etc, that
regmap would be useful.  It might be useful for the host side driver
because it deals with various register layouts.  However, for this
application, it doesn't seem useful.  I didn't see any other comments on
it, but it looks clean.  Applied to my next tree.

-corey

> 
> Cheers,
> 
> Joel
> 
> 
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> >  drivers/char/ipmi/bt-bmc.c | 68 +++++++++-----------------------------
> >  1 file changed, 16 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/char/ipmi/bt-bmc.c b/drivers/char/ipmi/bt-bmc.c
> > index 6e3d247b55d1..fb771ce6f7bf 100644
> > --- a/drivers/char/ipmi/bt-bmc.c
> > +++ b/drivers/char/ipmi/bt-bmc.c
> > @@ -8,13 +8,11 @@
> >  #include <linux/errno.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> > -#include <linux/mfd/syscon.h>
> >  #include <linux/miscdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/poll.h>
> > -#include <linux/regmap.h>
> >  #include <linux/sched.h>
> >  #include <linux/timer.h>
> >
> > @@ -59,8 +57,7 @@
> >  struct bt_bmc {
> >         struct device           dev;
> >         struct miscdevice       miscdev;
> > -       struct regmap           *map;
> > -       int                     offset;
> > +       void __iomem            *base;
> >         int                     irq;
> >         wait_queue_head_t       queue;
> >         struct timer_list       poll_timer;
> > @@ -69,29 +66,14 @@ struct bt_bmc {
> >
> >  static atomic_t open_count = ATOMIC_INIT(0);
> >
> > -static const struct regmap_config bt_regmap_cfg = {
> > -       .reg_bits = 32,
> > -       .val_bits = 32,
> > -       .reg_stride = 4,
> > -};
> > -
> >  static u8 bt_inb(struct bt_bmc *bt_bmc, int reg)
> >  {
> > -       uint32_t val = 0;
> > -       int rc;
> > -
> > -       rc = regmap_read(bt_bmc->map, bt_bmc->offset + reg, &val);
> > -       WARN(rc != 0, "regmap_read() failed: %d\n", rc);
> > -
> > -       return rc == 0 ? (u8) val : 0;
> > +       return readb(bt_bmc->base + reg);
> >  }
> >
> >  static void bt_outb(struct bt_bmc *bt_bmc, u8 data, int reg)
> >  {
> > -       int rc;
> > -
> > -       rc = regmap_write(bt_bmc->map, bt_bmc->offset + reg, data);
> > -       WARN(rc != 0, "regmap_write() failed: %d\n", rc);
> > +       writeb(data, bt_bmc->base + reg);
> >  }
> >
> >  static void clr_rd_ptr(struct bt_bmc *bt_bmc)
> > @@ -376,18 +358,15 @@ static irqreturn_t bt_bmc_irq(int irq, void *arg)
> >  {
> >         struct bt_bmc *bt_bmc = arg;
> >         u32 reg;
> > -       int rc;
> >
> > -       rc = regmap_read(bt_bmc->map, bt_bmc->offset + BT_CR2, &reg);
> > -       if (rc)
> > -               return IRQ_NONE;
> > +       reg = readl(bt_bmc->base + BT_CR2);
> >
> >         reg &= BT_CR2_IRQ_H2B | BT_CR2_IRQ_HBUSY;
> >         if (!reg)
> >                 return IRQ_NONE;
> >
> >         /* ack pending IRQs */
> > -       regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR2, reg);
> > +       writel(reg, bt_bmc->base + BT_CR2);
> >
> >         wake_up(&bt_bmc->queue);
> >         return IRQ_HANDLED;
> > @@ -398,6 +377,7 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> >  {
> >         struct device *dev = &pdev->dev;
> >         int rc;
> > +       u32 reg;
> >
> >         bt_bmc->irq = platform_get_irq_optional(pdev, 0);
> >         if (bt_bmc->irq < 0)
> > @@ -417,11 +397,11 @@ static int bt_bmc_config_irq(struct bt_bmc *bt_bmc,
> >          * will be cleared (along with B2H) when we can write the next
> >          * message to the BT buffer
> >          */
> > -       rc = regmap_update_bits(bt_bmc->map, bt_bmc->offset + BT_CR1,
> > -                               (BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY),
> > -                               (BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY));
> > +       reg = readl(bt_bmc->base + BT_CR1);
> > +       reg |= BT_CR1_IRQ_H2B | BT_CR1_IRQ_HBUSY;
> > +       writel(reg, bt_bmc->base + BT_CR1);
> >
> > -       return rc;
> > +       return 0;
> >  }
> >
> >  static int bt_bmc_probe(struct platform_device *pdev)
> > @@ -439,25 +419,9 @@ static int bt_bmc_probe(struct platform_device *pdev)
> >
> >         dev_set_drvdata(&pdev->dev, bt_bmc);
> >
> > -       bt_bmc->map = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > -       if (IS_ERR(bt_bmc->map)) {
> > -               void __iomem *base;
> > -
> > -               /*
> > -                * Assume it's not the MFD-based devicetree description, in
> > -                * which case generate a regmap ourselves
> > -                */
> > -               base = devm_platform_ioremap_resource(pdev, 0);
> > -               if (IS_ERR(base))
> > -                       return PTR_ERR(base);
> > -
> > -               bt_bmc->map = devm_regmap_init_mmio(dev, base, &bt_regmap_cfg);
> > -               bt_bmc->offset = 0;
> > -       } else {
> > -               rc = of_property_read_u32(dev->of_node, "reg", &bt_bmc->offset);
> > -               if (rc)
> > -                       return rc;
> > -       }
> > +       bt_bmc->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(bt_bmc->base))
> > +               return PTR_ERR(bt_bmc->base);
> >
> >         mutex_init(&bt_bmc->mutex);
> >         init_waitqueue_head(&bt_bmc->queue);
> > @@ -483,12 +447,12 @@ static int bt_bmc_probe(struct platform_device *pdev)
> >                 add_timer(&bt_bmc->poll_timer);
> >         }
> >
> > -       regmap_write(bt_bmc->map, bt_bmc->offset + BT_CR0,
> > -                    (BT_IO_BASE << BT_CR0_IO_BASE) |
> > +       writel((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_CR0_ENABLE_IBT,
> > +               bt_bmc->base + BT_CR0);
> >
> >         clr_b_busy(bt_bmc);
> >
> > --
> > 2.33.0
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel