Re: [PATCH v2 2/2] mtd: spi-nor: expose internal parameters via debugfs

[Michael Walle <michael@walle.cc>]

Hi,

>> +#define SNOR_F_NAME(name) [ilog2(SNOR_F_##name)] = #name
>> +static const char *const snor_f_names[] = {
>> +	SNOR_F_NAME(HAS_SR_TB),
>> +	SNOR_F_NAME(NO_OP_CHIP_ERASE),
>> +	SNOR_F_NAME(BROKEN_RESET),
>> +	SNOR_F_NAME(4B_OPCODES),
>> +	SNOR_F_NAME(HAS_4BAIT),
>> +	SNOR_F_NAME(HAS_LOCK),
>> +	SNOR_F_NAME(HAS_16BIT_SR),
>> +	SNOR_F_NAME(NO_READ_CR),
>> +	SNOR_F_NAME(HAS_SR_TB_BIT6),
>> +	SNOR_F_NAME(HAS_4BIT_BP),
>> +	SNOR_F_NAME(HAS_SR_BP3_BIT6),
>> +	SNOR_F_NAME(IO_MODE_EN_VOLATILE),
>> +	SNOR_F_NAME(SOFT_RESET),
>> +	SNOR_F_NAME(SWP_IS_VOLATILE),
>> +};
>> +#undef SNOR_F_NAME
> 
> You said you would add a comment here. Changed your mind?

No, it just slipped through.

>> +void spi_nor_debugfs_register(struct spi_nor *nor)
>> +{
>> +	struct dentry *d;
>> +	int ret;
>> +
>> +	/* Create rootdir once. Will never be deleted again. */
>> +	if (!rootdir)
>> +		rootdir = debugfs_create_dir("spi-nor", NULL);
> 
> If I compile SPI NOR as module, I insmod it, rmmod it, and then insmod
> it again, I get:
> 
> 	[  360.623465] spi-nor spi0.0: mt35xu512aba (65536 Kbytes)
> 	[  360.623478] debugfs: Directory 'spi-nor' with parent '/' already 
> present!
> 	[  360.632237] spi-nor spi1.0: mt25qu512a (65536 Kbytes)
> 
> I guess when you remove the module, rootdir also gets destroyed, and
> then gets re-initialized on probing again. I am not familiar enough 
> with
> the debugfs APIs to suggest a fix though.

Thanks for testing. And yes, you are right. I've changed that code
quite a few times for this damn subdirectory. But it seems I didn't
get it right. Usually, it's created in the module_init() but since
we just have an implicit one (and I don't really want to change that),
that's not an option. Some subsystems don't create a subdirectory at
all, which doesn't appeal to me.

I'll first lookup if the directory is already there, if it is, use it,
if not, create it. That should work. FWIW, the mvpp2 network card driver
does it too.

-michael