From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752099AbdJKP2U (ORCPT ); Wed, 11 Oct 2017 11:28:20 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:33921 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750831AbdJKP2S (ORCPT ); Wed, 11 Oct 2017 11:28:18 -0400 Message-ID: <1507735693.22082.8.camel@pengutronix.de> Subject: Re: [PATCH v3 2/7] nvmem: imx-ocotp: Pass parameters via a struct From: Philipp Zabel To: "Bryan O'Donoghue" , richard.leitner@skidata.com, srinivas.kandagatla@linaro.org, axel.lin@ingics.com, ping.bai@nxp.com, d.schultz@phytec.de, peng.fan@nxp.com, van.freenix@gmail.com Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Date: Wed, 11 Oct 2017 17:28:13 +0200 In-Reply-To: <1507558312-20580-3-git-send-email-pure.logic@nexus-software.ie> References: <1507558312-20580-1-git-send-email-pure.logic@nexus-software.ie> <1507558312-20580-3-git-send-email-pure.logic@nexus-software.ie> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6-1+deb9u1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Bryan, On Mon, 2017-10-09 at 15:11 +0100, Bryan O'Donoghue wrote: > It will be useful in later patches to know the register access mode and > bit-shift to apply to a given input offset. > > Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") > > Signed-off-by: Bryan O'Donoghue > --- > drivers/nvmem/imx-ocotp.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c > index 17d160f..7798571 100644 > --- a/drivers/nvmem/imx-ocotp.c > +++ b/drivers/nvmem/imx-ocotp.c > @@ -53,11 +53,15 @@ > > static DEFINE_MUTEX(ocotp_mutex); > > +struct ocotp_params { > + unsigned int nregs; > +}; > + > struct ocotp_priv { > struct device *dev; > struct clk *clk; > void __iomem *base; > - unsigned int nregs; > + const struct ocotp_params *params; > struct nvmem_config *config; > }; > > @@ -121,8 +125,8 @@ static int imx_ocotp_read(void *context, unsigned int offset, > index = offset >> 2; > count = bytes >> 2; > > - if (count > (priv->nregs - index)) > - count = priv->nregs - index; > + if (count > (priv->params->nregs - index)) > + count = priv->params->nregs - index; > > mutex_lock(&ocotp_mutex); > > @@ -308,12 +312,20 @@ static struct nvmem_config imx_ocotp_nvmem_config = { > .reg_write = imx_ocotp_write, > }; > > +static const struct ocotp_params params[] = { > + { .nregs = 128 }, > + { .nregs = 64 }, > + { .nregs = 128 }, > + { .nregs = 128 }, > + { .nregs = 64 }, > +}; Could you either add an enum for the indices into this array or split the array into separate named structs? I find it already hard to see which parameter belongs to which compatible, and this will grow worse with increasing number of entries in the parameter set. For example, static const struct ocotp_params imx6q_params = { .nregs = 128, }; static const struct ocotp_params imx6sl_params = { .nregs = 64, }; ... > static const struct of_device_id imx_ocotp_dt_ids[] = { > - { .compatible = "fsl,imx6q-ocotp", (void *)128 }, > - { .compatible = "fsl,imx6sl-ocotp", (void *)64 }, > - { .compatible = "fsl,imx6sx-ocotp", (void *)128 }, > - { .compatible = "fsl,imx6ul-ocotp", (void *)128 }, > - { .compatible = "fsl,imx7d-ocotp", (void *)64 }, > + { .compatible = "fsl,imx6q-ocotp", .data = ¶ms[0] }, > + { .compatible = "fsl,imx6sl-ocotp", .data = ¶ms[1] }, > + { .compatible = "fsl,imx6sx-ocotp", .data = ¶ms[2] }, > + { .compatible = "fsl,imx6ul-ocotp", .data = ¶ms[3] }, > + { .compatible = "fsl,imx7d-ocotp", .data = ¶ms[4] }, { .compatible = "fsl,imx6q-ocotp", .data = &imx6q_params }, { .compatible = "fsl,imx6sl-ocotp", .data = &imx6sl_params }, ... Apart from that, Reviewed-by: Philipp Zabel > { }, > }; > MODULE_DEVICE_TABLE(of, imx_ocotp_dt_ids); > @@ -342,8 +354,8 @@ static int imx_ocotp_probe(struct platform_device *pdev) > return PTR_ERR(priv->clk); > > of_id = of_match_device(imx_ocotp_dt_ids, dev); > - priv->nregs = (unsigned long)of_id->data; > - imx_ocotp_nvmem_config.size = 4 * priv->nregs; > + priv->params = of_device_get_match_data(&pdev->dev); > + imx_ocotp_nvmem_config.size = 4 * priv->params->nregs; > imx_ocotp_nvmem_config.dev = dev; > imx_ocotp_nvmem_config.priv = priv; > priv->config = &imx_ocotp_nvmem_config; regards Philipp From mboxrd@z Thu Jan 1 00:00:00 1970 From: p.zabel@pengutronix.de (Philipp Zabel) Date: Wed, 11 Oct 2017 17:28:13 +0200 Subject: [PATCH v3 2/7] nvmem: imx-ocotp: Pass parameters via a struct In-Reply-To: <1507558312-20580-3-git-send-email-pure.logic@nexus-software.ie> References: <1507558312-20580-1-git-send-email-pure.logic@nexus-software.ie> <1507558312-20580-3-git-send-email-pure.logic@nexus-software.ie> Message-ID: <1507735693.22082.8.camel@pengutronix.de> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Bryan, On Mon, 2017-10-09 at 15:11 +0100, Bryan O'Donoghue wrote: > It will be useful in later patches to know the register access mode and > bit-shift to apply to a given input offset. > > Fixes: 0642bac7da42 ("nvmem: imx-ocotp: add write support") > > Signed-off-by: Bryan O'Donoghue > --- > drivers/nvmem/imx-ocotp.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c > index 17d160f..7798571 100644 > --- a/drivers/nvmem/imx-ocotp.c > +++ b/drivers/nvmem/imx-ocotp.c > @@ -53,11 +53,15 @@ > > static DEFINE_MUTEX(ocotp_mutex); > > +struct ocotp_params { > + unsigned int nregs; > +}; > + > struct ocotp_priv { > struct device *dev; > struct clk *clk; > void __iomem *base; > - unsigned int nregs; > + const struct ocotp_params *params; > struct nvmem_config *config; > }; > > @@ -121,8 +125,8 @@ static int imx_ocotp_read(void *context, unsigned int offset, > index = offset >> 2; > count = bytes >> 2; > > - if (count > (priv->nregs - index)) > - count = priv->nregs - index; > + if (count > (priv->params->nregs - index)) > + count = priv->params->nregs - index; > > mutex_lock(&ocotp_mutex); > > @@ -308,12 +312,20 @@ static struct nvmem_config imx_ocotp_nvmem_config = { > .reg_write = imx_ocotp_write, > }; > > +static const struct ocotp_params params[] = { > + { .nregs = 128 }, > + { .nregs = 64 }, > + { .nregs = 128 }, > + { .nregs = 128 }, > + { .nregs = 64 }, > +}; Could you either add an enum for the indices into this array or split the array into separate named structs? I find it already hard to see which parameter belongs to which compatible, and this will grow worse with increasing number of entries in the parameter set. For example, static const struct ocotp_params imx6q_params = { .nregs = 128, }; static const struct ocotp_params imx6sl_params = { .nregs = 64, }; ... > static const struct of_device_id imx_ocotp_dt_ids[] = { > - { .compatible = "fsl,imx6q-ocotp", (void *)128 }, > - { .compatible = "fsl,imx6sl-ocotp", (void *)64 }, > - { .compatible = "fsl,imx6sx-ocotp", (void *)128 }, > - { .compatible = "fsl,imx6ul-ocotp", (void *)128 }, > - { .compatible = "fsl,imx7d-ocotp", (void *)64 }, > + { .compatible = "fsl,imx6q-ocotp", .data = ¶ms[0] }, > + { .compatible = "fsl,imx6sl-ocotp", .data = ¶ms[1] }, > + { .compatible = "fsl,imx6sx-ocotp", .data = ¶ms[2] }, > + { .compatible = "fsl,imx6ul-ocotp", .data = ¶ms[3] }, > + { .compatible = "fsl,imx7d-ocotp", .data = ¶ms[4] }, { .compatible = "fsl,imx6q-ocotp", .data = &imx6q_params }, { .compatible = "fsl,imx6sl-ocotp", .data = &imx6sl_params }, ... Apart from that, Reviewed-by: Philipp Zabel > { }, > }; > MODULE_DEVICE_TABLE(of, imx_ocotp_dt_ids); > @@ -342,8 +354,8 @@ static int imx_ocotp_probe(struct platform_device *pdev) > return PTR_ERR(priv->clk); > > of_id = of_match_device(imx_ocotp_dt_ids, dev); > - priv->nregs = (unsigned long)of_id->data; > - imx_ocotp_nvmem_config.size = 4 * priv->nregs; > + priv->params = of_device_get_match_data(&pdev->dev); > + imx_ocotp_nvmem_config.size = 4 * priv->params->nregs; > imx_ocotp_nvmem_config.dev = dev; > imx_ocotp_nvmem_config.priv = priv; > priv->config = &imx_ocotp_nvmem_config; regards Philipp