[PATCH v2] mtd: hyperbus: add Renesas RPC-IF driver

[Vignesh Raghavendra]

Hi Sergei,

On 5/14/20 2:13 AM, Sergei Shtylyov wrote:
> Add the HyperFLash driver for the Renesas RPC-IF.  It's the "front end"
> driver using the "back end" APIs in the main driver to talk to the real
> hardware.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov at cogentembedded.com>
> 

Mails to this ID is bouncing which meant I could not ask status of
dependencies.

> ---
> This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo.
> Requires  the RPC-IF main driver patch in order to build/work:
> 
> [1] https://patchwork.kernel.org/patch/11283127/
> 
[...]
> --- /dev/null
> +++ linux/drivers/mtd/hyperbus/rpc-if.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Linux driver for RPC-IF HyperFlash
> + *
> + * Copyright (C) 2019-2020 Cogent Embedded, Inc.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/hyperbus.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +
> +#include <memory/renesas-rpc-if.h>
> +
> +/* FIXME: How to drop this? */
> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP
> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)"
> +#endif

select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help?

> +
> + struct	rpcif_hyperbus {

WARNING: please, no spaces at the start of a line
#73: FILE: drivers/mtd/hyperbus/rpc-if.c:25:
+ struct^Irpcif_hyperbus {$



> +	struct rpcif rpc;
> +	struct hyperbus_ctlr ctlr;
> +	struct hyperbus_device hbdev;
> +};
> +
> +static const struct rpcif_op rpcif_op_tmpl = {
> +	.cmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.ocmd = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.addr = {
> +		.nbytes = 1,
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +	.data = {
> +		.buswidth = 8,
> +		.ddr = true,
> +	},
> +};
> +
> +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr)
> +{
> +	struct rpcif_hyperbus *hyperbus =
> +		container_of(hbdev, struct rpcif_hyperbus, hbdev);
> +	struct rpcif_op op = rpcif_op_tmpl;
> +	map_word data;
> +
> +	op.cmd.opcode = 0x80;

Here and elsewhere what does 0x80 represent? Maybe using a self
describing macro would be helpful when assigning values to op.cmd.opcode?

> +	op.addr.val = addr >> 1;
> +	op.dummy.buswidth = 1;
> +	op.dummy.ncycles = 15;
> +	op.data.dir = RPCIF_DATA_IN;
> +	op.data.nbytes = 2;
> +	op.data.buf.in = &data;

This code block seems like a good candidate to be converted to a macro/
template to avoid repetition. Template can initializes few values remain
common across the driver and override others based on parameters passed.
You could probably have one for write op and one for read op, if needed.

[...]

Regards
Vignesh