Re: [PATCH] [MTD] MTD driver for LPDDR2-NVM PCM memories

[Artem Bityutskiy <dedekind1@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

On Tue, 2012-10-16 at 23:24 +0200, Vincenzo Aliberti wrote:
> +/*
> + * Internal Type Definitions
> + * pcm_int_data contains memory controller details:
> + * @reg_data : LPDDR2_MODE_REG_DATA register address after remapping
> + * @reg_cfg  : LPDDR2_MODE_REG_CFG register address after remapping
> + * &bus_width: memory bus-width (eg: x16 2 Bytes, x32 4 Bytes)
> + */
> +struct pcm_int_data {
> +	void __iomem *reg_data;
> +	void __iomem *reg_cfg;
> +	int bus_width;
> +};
> +
> +static DEFINE_MUTEX(lpdd2_nvm_mutex);

A global mutes? So if you have 2 independent PCM chips handled by this
driver, the access to them will be mutually excluded?

> +
> +static inline map_word build_map_word(u_long myword)
> +{
> +	map_word val = { {0} };
> +	val.x[0] = myword;
> +	return val;
> +}
> +
> +static inline u_int build_mr_cfgmaks(u_int bus_width)
> +{
> +	u_int val = MR_CFGMASK;
> +
> +	if (bus_width == 0x0004)		/* x32 device */
> +		val = val<<16;
> +
> +	return val;
> +}
> +
> +static inline u_int build_sr_ok_datamask(u_int bus_width)
> +{
> +	u_int val = SR_OK_DATAMASK;
> +
> +	if (bus_width == 0x0004)		/* x32 device */
> +		val = (val<<16)+val;
> +
> +	return val;
> +}

How about adding some comments about what the functions above do?

> +
> +/*
> + * Enable lpddr2-nvm Overlay Window
> + */

How about a short comment explaining what is the Overlay Window?

> +static int lpddr2_nvm_do_op(struct map_info *map, u_long cmd_code,
> +	u_long cmd_data, u_long cmd_add, u_long cmd_mpr, u_char *buf)
> +{
> +	map_word add_l = { {0} }, add_h = { {0} }, mpr_l = { {0} },
> +		mpr_h = { {0} }, data_l = { {0} }, cmd = { {0} },
> +		exec_cmd = { {0} }, sr;
> +	map_word data_h = { {0} };	/* only for 2x x16 devices stacked */
> +	u_long i, status_reg, prg_buff_ofs;
> +	struct pcm_int_data *pcm_data = map->fldrv_priv;
> +	u_int sr_ok_datamask = build_sr_ok_datamask(pcm_data->bus_width);
> +
> +	/* Builds low and high words for OW Control Registers */
> +	add_l.x[0]	= cmd_add&0x0000FFFF;
> +	add_h.x[0]	= (cmd_add>>16)&0x0000FFFF;
> +	mpr_l.x[0]	= cmd_mpr&0x0000FFFF;
> +	mpr_h.x[0]	= (cmd_mpr>>16)&0x0000FFFF;
> +	cmd.x[0]	= cmd_code&0x0000FFFF;
> +	exec_cmd.x[0]	= 0x0001;
> +	data_l.x[0]	= cmd_data&0x0000FFFF;
> +	data_h.x[0]	= (cmd_data>>16)&0x0000FFFF; /* only for 2x x16 */
> +
> +	/* Set Overlay Window Control Registers */
> +	map_write(map, cmd, map->pfow_base+CMD_CODE_OFS*pcm_data->bus_width);
> +	map_write(map, data_l, map->pfow_base+CMD_DATA_OFS*pcm_data->bus_width);
> +	map_write(map, add_l, map->pfow_base+CMD_ADD_L_OFS*pcm_data->bus_width);
> +	map_write(map, add_h, map->pfow_base+CMD_ADD_H_OFS*pcm_data->bus_width);
> +	map_write(map, mpr_l, map->pfow_base+MPR_L_OFS*pcm_data->bus_width);
> +	map_write(map, mpr_h, map->pfow_base+MPR_H_OFS*pcm_data->bus_width);

How about introducing a small helper inline unction for

map->pfow_base+ X * pcm_data->bus_width

It is repeated everywhere and makes driver difficult to follow, no?

Also, see the coding style about how to place spaces correctly. Make
sure checkpatch.pl is happy or almost happy (sometimes you should ignore
its complaints, they and not always correct).


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]