From: Sam Ravnborg <sam@ravnborg.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Venkateshwar Rao Gannavarapu
<venkateshwar.rao.gannavarapu@xilinx.com>,
airlied@linux.ie, vgannava@xilinx.com,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem
Date: Mon, 13 Jun 2022 07:53:25 +0200 [thread overview]
Message-ID: <YqbQ1fLUKR/iwbTD@ravnborg.org> (raw)
In-Reply-To: <YqZY4QMAkGiFOOWE@pendragon.ideasonboard.com>
Hi Laurent,
> [snip]
>
> > > +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)
> > > +{
> > > + writel(val, base + offset);
> > > +}
> > > +
> > > +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> > > +{
> > > + return readl(base + offset);
> > > +}
> >
> > When I see implementations like this I wonder if a regmap would be
> > beneficial?
>
> regmap often seems overkill to me when the driver only needs plain
> 32-bit mmio read/write, given the overhead it adds at runtime. Is it
> just me ?
There are several points that speaks for using regmap:
- The interface is well known
- It has nice helpers - like update_bits
- No need for own wrappers, that sometimes are made in creative ways
(not the case here)
- There is a possibility to add some run-time checks so one can catch
attempt to write outside the register window, write to read-only
registers etc.
On top of this - it is simple to configure:
static const struct regmap_config regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
};
From the probe function:
priv->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(priv->regs))
return dev_err_probe(dev, PTR_ERR(priv->regs), "Failed to get memory resource\n");
regmap_cfg = regmap_config;
regmap_cfg.max_register = res->end - res->start;
priv->regmap = devm_regmap_init_mmio(dev, priv->regs, ®map_cfg);
if (IS_ERR(priv->regmap))
return dev_err_probe(dev, PTR_ERR(priv->regmap), "Failed to init regmap\n");
The one point that brought me over was the well known interface.
But using wrappers works too.
Sam
WARNING: multiple messages have this Message-ID (diff)
From: Sam Ravnborg <sam@ravnborg.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: airlied@linux.ie, vgannava@xilinx.com,
Venkateshwar Rao Gannavarapu
<venkateshwar.rao.gannavarapu@xilinx.com>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem
Date: Mon, 13 Jun 2022 07:53:25 +0200 [thread overview]
Message-ID: <YqbQ1fLUKR/iwbTD@ravnborg.org> (raw)
In-Reply-To: <YqZY4QMAkGiFOOWE@pendragon.ideasonboard.com>
Hi Laurent,
> [snip]
>
> > > +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val)
> > > +{
> > > + writel(val, base + offset);
> > > +}
> > > +
> > > +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset)
> > > +{
> > > + return readl(base + offset);
> > > +}
> >
> > When I see implementations like this I wonder if a regmap would be
> > beneficial?
>
> regmap often seems overkill to me when the driver only needs plain
> 32-bit mmio read/write, given the overhead it adds at runtime. Is it
> just me ?
There are several points that speaks for using regmap:
- The interface is well known
- It has nice helpers - like update_bits
- No need for own wrappers, that sometimes are made in creative ways
(not the case here)
- There is a possibility to add some run-time checks so one can catch
attempt to write outside the register window, write to read-only
registers etc.
On top of this - it is simple to configure:
static const struct regmap_config regmap_config = {
.reg_bits = 32,
.val_bits = 32,
.reg_stride = 4,
};
From the probe function:
priv->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(priv->regs))
return dev_err_probe(dev, PTR_ERR(priv->regs), "Failed to get memory resource\n");
regmap_cfg = regmap_config;
regmap_cfg.max_register = res->end - res->start;
priv->regmap = devm_regmap_init_mmio(dev, priv->regs, ®map_cfg);
if (IS_ERR(priv->regmap))
return dev_err_probe(dev, PTR_ERR(priv->regmap), "Failed to init regmap\n");
The one point that brought me over was the well known interface.
But using wrappers works too.
Sam
next prev parent reply other threads:[~2022-06-13 5:53 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-12 13:53 [LINUX PATCH 0/2] Add Xilinx DSI-Tx DRM driver Venkateshwar Rao Gannavarapu
2022-05-12 13:53 ` Venkateshwar Rao Gannavarapu
2022-05-12 13:53 ` [LINUX PATCH 1/2] dt-bindings: display: xlnx: Add DSI 2.0 Tx subsystem documentation Venkateshwar Rao Gannavarapu
2022-05-12 13:53 ` Venkateshwar Rao Gannavarapu
2022-05-13 13:39 ` Laurent Pinchart
2022-05-13 13:39 ` Laurent Pinchart
2022-05-12 13:53 ` [LINUX PATCH 2/2] drm: xlnx: dsi: driver for Xilinx DSI Tx subsystem Venkateshwar Rao Gannavarapu
2022-05-12 13:53 ` Venkateshwar Rao Gannavarapu
2022-05-13 11:05 ` Sam Ravnborg
2022-05-13 11:05 ` Sam Ravnborg
2022-06-12 21:21 ` Laurent Pinchart
2022-06-12 21:21 ` Laurent Pinchart
2022-06-13 5:53 ` Sam Ravnborg [this message]
2022-06-13 5:53 ` Sam Ravnborg
2022-06-12 21:28 ` Laurent Pinchart
2022-06-12 21:28 ` Laurent Pinchart
2022-05-13 13:39 ` Laurent Pinchart
2022-05-13 13:39 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YqbQ1fLUKR/iwbTD@ravnborg.org \
--to=sam@ravnborg.org \
--cc=airlied@linux.ie \
--cc=dri-devel@lists.freedesktop.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=venkateshwar.rao.gannavarapu@xilinx.com \
--cc=vgannava@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.