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

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

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

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> 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?
OK, when we have covered your comments, I'll report a V4 to lkml as well.

> 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?
That's from observation, as I have no specification available.

>From my understanding, the clock applied to the chip can be variable, but the
memory bus writes ensure the necessary time, as the NOP write takes as much time
as the DOCG3 decides it to last.

Unless you have a timing to provide (or even better, a specification), I'll
leave the NOP writes.

>> +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?
No, but I'll amend that loop with cpu_relax, that makes perfect sense.

>
>> +/*
>> + * 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
OK, will remove the ifdef.

>> +#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.
True, but it's not available yet.
And as it's not available, and I don't think debugfs will accept such a patch, I
think I'll leave it here. And if you wish, I'll fill in a separate patch to
debugfs, and *if* it's merged, I'll remove that part then.

Cheers.

-- 
Robert

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

[Artem Bityutskiy]

On Tue, 2011-09-20 at 17:44 +0200, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
> 
> > 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?
> OK, when we have covered your comments, I'll report a V4 to lkml as well.
> 
> > 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?
> That's from observation, as I have no specification available.
> 
> From my understanding, the clock applied to the chip can be variable, but the
> memory bus writes ensure the necessary time, as the NOP write takes as much time
> as the DOCG3 decides it to last.

OK, would be nice to have this kind of comment in that function.

> >> +	};
> >
> > Hmm, looks like something which should be generic, not DoC-specific.
> True, but it's not available yet.
> And as it's not available, and I don't think debugfs will accept such a patch, I
> think I'll leave it here. And if you wish, I'll fill in a separate patch to
> debugfs, and *if* it's merged, I'll remove that part then.

Fine with me.

-- 
Best Regards,
Artem Bityutskiy