| Message ID | 39d8d4f0e0ff5a06be0303f7f4f2eac5fb45b9ca.1525684985.git.mchehab+samsung@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | Boris Brezillon |
| Headers | show |
| Series | Fix some build warnings/errors with Sphinx | expand |
Hi Mauro, On Mon, 7 May 2018 06:35:52 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote: > Gets rid of those warnings and better document the parameters. > > ./include/linux/mtd/rawnand.h:752: warning: Function parameter or member 'timings.sdr' not described in 'nand_data_interface' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.in' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.out' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.cmd' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.data' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.waitrdy' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.data' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.desc' not described in 'nand_chip' > ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.priv' not described in 'nand_chip' > > ./include/linux/mtd/rawnand.h:848: WARNING: Unexpected indentation. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> > --- > include/linux/mtd/rawnand.h | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > 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. Is there a simple way we can get rid of the warning we have when not describing timings but all of its fields? > */ > struct nand_data_interface { > enum nand_data_interface_type type; > @@ -798,8 +799,9 @@ struct nand_op_addr_instr { > /** > * struct nand_op_data_instr - Definition of a data instruction > * @len: number of data bytes to move > - * @in: buffer to fill when reading from the NAND chip > - * @out: buffer to read from when writing to the NAND chip > + * @buf: buffer to fill > + * @buf.in: buffer to fill when reading from the NAND chip > + * @buf.out: buffer to read from when writing to the NAND chip Same here. What we care about is @buf.in and @buf.out, the @buf description is useless... Regards, Boris
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();
On Mon, 7 May 2018 08:32:32 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote: > 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. Yep, don't like it either. I'll just take your initial patch. Thanks, Boris
Hi Boris, Em Mon, 7 May 2018 08:32:32 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu: > Hi Boris, > > Em Mon, 7 May 2018 11:46:50 +0200 > Boris Brezillon <boris.brezillon@bootlin.com> escreveu: > > 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. Didn't hear from you. What do you prefer for me to do with regards to this patch? Thanks, Mauro
On Wed, 9 May 2018 09:10:34 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote: > Hi Boris, > > Em Mon, 7 May 2018 08:32:32 -0300 > Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu: > > > Hi Boris, > > > > Em Mon, 7 May 2018 11:46:50 +0200 > > Boris Brezillon <boris.brezillon@bootlin.com> escreveu: > > > > 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. > > Didn't hear from you. I replied just a few minutes ago ;). > What do you prefer for me to do with regards to > this patch? Will queue the initial patch to nand/next (I can also queue it to the fixes branch if you prefer).
Em Wed, 9 May 2018 14:22:21 +0200 Boris Brezillon <boris.brezillon@bootlin.com> escreveu: > On Wed, 9 May 2018 09:10:34 -0300 > Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote: > > > Hi Boris, > > > > Em Mon, 7 May 2018 08:32:32 -0300 > > Mauro Carvalho Chehab <mchehab+samsung@kernel.org> escreveu: > > > > > Hi Boris, > > > > > > Em Mon, 7 May 2018 11:46:50 +0200 > > > Boris Brezillon <boris.brezillon@bootlin.com> escreveu: > > > > > > 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. > > > > Didn't hear from you. > > I replied just a few minutes ago ;). OK! > > What do you prefer for me to do with regards to > > this patch? > > Will queue the initial patch to nand/next (I can also queue it to the > fixes branch if you prefer). No need. it can follow the usual workflow. Thanks, Mauro
On Mon, 7 May 2018 06:35:52 -0300 Mauro Carvalho Chehab <mchehab+samsung@kernel.org> wrote: > Gets rid of those warnings and better document the parameters. > > ./include/linux/mtd/rawnand.h:752: warning: Function parameter or member 'timings.sdr' not described in 'nand_data_interface' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.in' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.out' not described in 'nand_op_data_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.cmd' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.data' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.waitrdy' not described in 'nand_op_instr' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.data' not described in 'nand_op_parser_pattern_elem' > ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.desc' not described in 'nand_chip' > ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.priv' not described in 'nand_chip' > > ./include/linux/mtd/rawnand.h:848: WARNING: Unexpected indentation. > > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> Applied. Thanks, Boris > --- > include/linux/mtd/rawnand.h | 26 ++++++++++++++++++-------- > 1 file changed, 18 insertions(+), 8 deletions(-) > > 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. > */ > struct nand_data_interface { > enum nand_data_interface_type type; > @@ -798,8 +799,9 @@ struct nand_op_addr_instr { > /** > * struct nand_op_data_instr - Definition of a data instruction > * @len: number of data bytes to move > - * @in: buffer to fill when reading from the NAND chip > - * @out: buffer to read from when writing to the NAND chip > + * @buf: buffer to fill > + * @buf.in: buffer to fill when reading from the NAND chip > + * @buf.out: buffer to read from when writing to the NAND chip > * @force_8bit: force 8-bit access > * > * Please note that "in" and "out" are inverted from the ONFI specification > @@ -842,9 +844,13 @@ enum nand_op_instr_type { > /** > * struct nand_op_instr - Instruction object > * @type: the instruction type > - * @cmd/@addr/@data/@waitrdy: extra data associated to the instruction. > - * You'll have to use the appropriate element > - * depending on @type > + * @ctx: extra data associated to the instruction. You'll have to use the > + * appropriate element depending on @type > + * @ctx.cmd: use it if @type is %NAND_OP_CMD_INSTR > + * @ctx.addr: use it if @type is %NAND_OP_ADDR_INSTR > + * @ctx.data: use it if @type is %NAND_OP_DATA_IN_INSTR > + * or %NAND_OP_DATA_OUT_INSTR > + * @ctx.waitrdy: use it if @type is %NAND_OP_WAITRDY_INSTR > * @delay_ns: delay the controller should apply after the instruction has been > * issued on the bus. Most modern controllers have internal timings > * control logic, and in this case, the controller driver can ignore > @@ -997,7 +1003,9 @@ struct nand_op_parser_data_constraints { > * struct nand_op_parser_pattern_elem - One element of a pattern > * @type: the instructuction type > * @optional: whether this element of the pattern is optional or mandatory > - * @addr/@data: address or data constraint (number of cycles or data length) > + * @ctx: address or data constraint > + * @ctx.addr: address constraint (number of cycles) > + * @ctx.data: data constraint (data length) > */ > struct nand_op_parser_pattern_elem { > enum nand_op_instr_type type; > @@ -1224,6 +1232,8 @@ int nand_op_parser_exec_op(struct nand_chip *chip, > * devices. > * @priv: [OPTIONAL] pointer to private chip data > * @manufacturer: [INTERN] Contains manufacturer information > + * @manufacturer.desc: [INTERN] Contains manufacturer's description > + * @manufacturer.priv: [INTERN] Contains manufacturer private information > */ > > struct nand_chip {
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. */ struct nand_data_interface { enum nand_data_interface_type type; @@ -798,8 +799,9 @@ struct nand_op_addr_instr { /** * struct nand_op_data_instr - Definition of a data instruction * @len: number of data bytes to move - * @in: buffer to fill when reading from the NAND chip - * @out: buffer to read from when writing to the NAND chip + * @buf: buffer to fill + * @buf.in: buffer to fill when reading from the NAND chip + * @buf.out: buffer to read from when writing to the NAND chip * @force_8bit: force 8-bit access * * Please note that "in" and "out" are inverted from the ONFI specification @@ -842,9 +844,13 @@ enum nand_op_instr_type { /** * struct nand_op_instr - Instruction object * @type: the instruction type - * @cmd/@addr/@data/@waitrdy: extra data associated to the instruction. - * You'll have to use the appropriate element - * depending on @type + * @ctx: extra data associated to the instruction. You'll have to use the + * appropriate element depending on @type + * @ctx.cmd: use it if @type is %NAND_OP_CMD_INSTR + * @ctx.addr: use it if @type is %NAND_OP_ADDR_INSTR + * @ctx.data: use it if @type is %NAND_OP_DATA_IN_INSTR + * or %NAND_OP_DATA_OUT_INSTR + * @ctx.waitrdy: use it if @type is %NAND_OP_WAITRDY_INSTR * @delay_ns: delay the controller should apply after the instruction has been * issued on the bus. Most modern controllers have internal timings * control logic, and in this case, the controller driver can ignore @@ -997,7 +1003,9 @@ struct nand_op_parser_data_constraints { * struct nand_op_parser_pattern_elem - One element of a pattern * @type: the instructuction type * @optional: whether this element of the pattern is optional or mandatory - * @addr/@data: address or data constraint (number of cycles or data length) + * @ctx: address or data constraint + * @ctx.addr: address constraint (number of cycles) + * @ctx.data: data constraint (data length) */ struct nand_op_parser_pattern_elem { enum nand_op_instr_type type; @@ -1224,6 +1232,8 @@ int nand_op_parser_exec_op(struct nand_chip *chip, * devices. * @priv: [OPTIONAL] pointer to private chip data * @manufacturer: [INTERN] Contains manufacturer information + * @manufacturer.desc: [INTERN] Contains manufacturer's description + * @manufacturer.priv: [INTERN] Contains manufacturer private information */ struct nand_chip {
Gets rid of those warnings and better document the parameters. ./include/linux/mtd/rawnand.h:752: warning: Function parameter or member 'timings.sdr' not described in 'nand_data_interface' ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf' not described in 'nand_op_data_instr' ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.in' not described in 'nand_op_data_instr' ./include/linux/mtd/rawnand.h:817: warning: Function parameter or member 'buf.out' not described in 'nand_op_data_instr' ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx' not described in 'nand_op_instr' ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.cmd' not described in 'nand_op_instr' ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_instr' ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.data' not described in 'nand_op_instr' ./include/linux/mtd/rawnand.h:863: warning: Function parameter or member 'ctx.waitrdy' not described in 'nand_op_instr' ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx' not described in 'nand_op_parser_pattern_elem' ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_parser_pattern_elem' ./include/linux/mtd/rawnand.h:1010: warning: Function parameter or member 'ctx.data' not described in 'nand_op_parser_pattern_elem' ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.desc' not described in 'nand_chip' ./include/linux/mtd/rawnand.h:1313: warning: Function parameter or member 'manufacturer.priv' not described in 'nand_chip' ./include/linux/mtd/rawnand.h:848: WARNING: Unexpected indentation. Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org> --- include/linux/mtd/rawnand.h | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)