Re: [PATCH V3] mtd: Add DiskOnChip G3 support

[Artem Bityutskiy]

I really feel unsure about merging this driver because no one reviewed
it. On the surface it does look neat, though. Could you please CC lkml
on next submission?

On Mon, 2011-09-19 at 19:43 +0200, Robert Jarzmik wrote:
> +static void doc_delay(struct docg3 *docg3, int nbNOPs)
> +{
> +	int i;
> +
> +	doc_dbg("NOP x %d\n", nbNOPs);
> +	for (i = 0; i < nbNOPs; i++)
> +		doc_writeb(0, DoC_NOP);
> +}

Why you implement dalaying this way, instead of using udelay/mdelay?

> +static int doc_wait_ready(struct docg3 *docg3)
> +{
> +	int maxWaitCycles = 100;
> +
> +	do {
> +		doc_delay(docg3, 4);
> +	} while (!doc_is_ready(docg3) && maxWaitCycles--);
> +	doc_delay(docg3, 2);
> +	if (maxWaitCycles > 0)
> +		return 0;
> +	else
> +		return -EIO;
> +}

There are things like cpu_relax() which are used in busy-loops - did you
look at those?


> +/*
> + * Debug sysfs entries
> + */
> +#ifdef CONFIG_DEBUG_FS

You do not need to use CONFIG_DEBUG_FS - debugfs makes all calls to be
noop if it is not present.

Either remove all macros or use DEBUG

> +#define DEBUGFS_RO_ATTR(name, show_fct) \
> +	static int name##_open(struct inode *inode, struct file *file) \
> +	{ return single_open(file, show_fct, inode->i_private); }      \
> +	static const struct file_operations name##_fops = { \
> +		.owner = THIS_MODULE, \
> +		.open = name##_open, \
> +		.llseek = seq_lseek, \
> +		.read = seq_read, \
> +		.release = single_release \
> +	};

Hmm, looks like something which should be generic, not DoC-specific.

-- 
Best Regards,
Artem Bityutskiy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/