Re: [PATCH v8 1/3] Runtime Interpreted Power Sequences

[Anton Vorontsov <cbouatmailru@gmail.com>]

On Fri, Nov 16, 2012 at 06:44:22PM +0900, Alex Courbot wrote:
> On 11/16/2012 04:26 PM, Anton Vorontsov wrote:
> >>+#include "power_seq_delay.c"
> >>+#include "power_seq_regulator.c"
> >>+#include "power_seq_pwm.c"
> >>+#include "power_seq_gpio.c"
> >
> >This is odd, although I remember you already explained why you have to
> >include the .c files, instead of linking them separately. But I forgot the
> >reason. :) I think this deserves a comment in the code.
> 
> This is because of the table right after these includes:
> 
> static const struct power_seq_res_ops power_seq_ops[POWER_SEQ_NUM_TYPES] = {
> 	[POWER_SEQ_DELAY] = POWER_SEQ_DELAY_TYPE,
> 	[POWER_SEQ_REGULATOR] = POWER_SEQ_REGULATOR_TYPE,
> 	[POWER_SEQ_PWM] = POWER_SEQ_PWM_TYPE,
> 	[POWER_SEQ_GPIO] = POWER_SEQ_GPIO_TYPE,
> };
> 
> The POWER_SEQ_*_TYPE macros are defined in the C files. It's the
> simplest way to initialize this table, and the code inside these C
> files is short and simple enough that I thought I would be forgiven.
> :)

I think in the header file you could just write

extern ..... power_seq_delay_type;
#ifndef ...
#define power_seq_delay_type NULL
#endif

And then, in the .c file:

static const struct power_seq_res_ops power_seq_ops[POWER_SEQ_NUM_TYPES] = {
	[POWER_SEQ_DELAY] = power_seq_delay_type,
	...
};

And then you can stop including the .c files directly, and link them
instead.

> At first everything was in power_seq.c and it was fine, then I
> thought it would be better to move resource support code into their
> own filesm and now everybody is asking. :P
> 
> But yeah, maybe it would be even better to not stop halfway and use
> dynamic linking.
> 
> Comment added for the time being. ;)
> 
> >>+static int of_power_seq_parse_step(struct device *dev,
> >>+				   struct device_node *node,
> >>+				   struct power_seq *seq,
> >>+				   unsigned int step_nbr,
> >>+				   struct list_head *resources)
> >>+{
> >>+	struct power_seq_step *step = &seq->steps[step_nbr];
> >>+	struct power_seq_resource res, *res2;
> >>+	const char *type;
> >>+	int i, err;
> >
> >nit: one variable declaration per line.
> 
> Fair enough - but is that a convention? checkpatch.pl was happy with these.

It's not a rule, although there is a small passage about it in CodingStyle
file when it describes commenting style. Though, some folks choose to
ignore this suggestion quite frequently, especially if variables have no
comments.

Often, the multiple declarations per line are used to hide ugly functions
w/ tons of variables, or just to confuse the reader for the fun of it.

There are exceptions, of course. E.g.

	int i, j, k; /* Our iterators. */

Is perfectly fine.

But
	int i, err;

At least to me seems weird, this stuff is logically disjunct.

Only human can truly differentiate when the stuff "looks better" together
or apart, so that's why checkpatch doesn't complain. Personally, I use
this rule of thumb: when in doubt, use just one declaration per line, it
is always OK. :)

Thanks,
Anton.