linux-kernel - Re: [PATCH v3 2/7] nvmem: imx-ocotp: Pass parameters via a struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Oct 2017 17:28:13 +0200
From:   Philipp Zabel <p.zabel@...gutronix.de>
To:     Bryan O'Donoghue <pure.logic@...us-software.ie>,
        richard.leitner@...data.com, srinivas.kandagatla@...aro.org,
        axel.lin@...ics.com, ping.bai@....com, d.schultz@...tec.de,
        peng.fan@....com, van.freenix@...il.com
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/7] nvmem: imx-ocotp: Pass parameters via a struct

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 <pure.logic@...us-software.ie>
> ---
>  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 = &params[0] },
> +	{ .compatible = "fsl,imx6sl-ocotp", .data = &params[1] },
> +	{ .compatible = "fsl,imx6sx-ocotp", .data = &params[2] },
> +	{ .compatible = "fsl,imx6ul-ocotp", .data = &params[3] },
> +	{ .compatible = "fsl,imx7d-ocotp",  .data = &params[4] },

	{ .compatible = "fsl,imx6q-ocotp",  .data = &imx6q_params },
	{ .compatible = "fsl,imx6sl-ocotp",  .data = &imx6sl_params },
...

Apart from that,

Reviewed-by: Philipp Zabel <p.zabel@...gutronix.de>

>  	{ },
>  };
>  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