Re: [PATCH 16/18] mtd: rawnand.h: use nested union kernel-doc markups

Mauro Carvalho Chehab
2018-05-07T11:32:41Z

Hi Boris,

Em Mon, 7 May 2018 11:46:50 +0200
Boris Brezillon <boris.brezillon@bootlin.com> escreveu:

> Hi Mauro,

> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index 5dad59b31244..b986f94906df 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -740,8 +740,9 @@ enum nand_data_interface_type {
> >
> > /**
> > * struct nand_data_interface - NAND interface timing
> > - * @type: type of the timing
> > - * @timings: The timing, type according to @type
> > + * @type: type of the timing
> > + * @timings: The timing, type according to @type
> > + * @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
>
> Hm, really feels weird to do that. I mean, either we describe
> timings.sdr or timings, but not both. I this case, I agree that
> describing timings.sdr would make more sense than generically
> describing any possible entries in the timings union.

This struct is funny, as the union has just one item. I assume
that the original intend is to be ready to have more timing
types (or you had it in the past). By the time you have a
second value there, describing the union would make more
sense.

> Is there a simple
> way we can get rid of the warning we have when not describing timings
> but all of its fields?

There's no direct way. It won't be hard to add a way to do it,
like applying the enclosed patch with ends by declaring timings as:

* @timings: -- undescribed --

IMHO, this is uglier.

The way I see is that, if the embed struct/union is not interesting
enough to be described, the best would be to use an anonymous one like:

struct nand_data_interface {
enum nand_data_interface_type type;
union {
struct nand_sdr_timings sdr;
};
};

With the above, kernel-doc will expect a description just for @sdr.

Btw, if you want to see how nested struct/union is parsed, the
scripts/kernel_doc logic is at dump_struct() function.

When it finds an struct, it first handles private/public by simply
removing everything that it is not public from the struct, by a
very simple parser. Then, it converts nested struct/union into
un-nested ones. E. g. it converts:

struct {
union {
int foo1;
};
union {
int foo2;
} bar;
} foobar;

into this internal representation:

struct {
int foo1;
union;
int bar.foo2;
union bar;
};

Then it runs the normal un-nested struct parser.

Thanks,
Mauro

[PATCH] don't describe nested struct timings

IMHO, this is an ugly hack, but it allows having nested
structs (or fields) undescribed by purpose.

Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index b986f94906df..b724c7edf532 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -741,7 +741,7 @@ enum nand_data_interface_type {
/**
* struct nand_data_interface - NAND interface timing
* @type: type of the timing
- * @timings: The timing, type according to @type
+ * @timings: -- undescribed --
* @timings.sdr: Use it when @type is %NAND_SDR_IFACE.
*/
struct nand_data_interface {
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 0057d8eafcc1..196a2107c8c1 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -390,7 +390,7 @@ my $section = $section_default;
my $section_context = "Context";
my $section_return = "Return";

-my $undescribed = "-- undescribed --";
+my $undescribed = "-- undescribed --\n";

reset_state();