Re: [RFC PATCH RESEND 2/3] leds: upboard: Add LED support

[Javier Arteaga]

Hi Lee,

On Thu, Apr 26, 2018 at 08:34:05AM +0100, Lee Jones wrote:
> >  drivers/mfd/upboard.c       | 19 ++++++++
> 
> This change needs to be placed into a separate patch.

OK. I'll do ("add driver" -> "add cell") pairs of patches then.

> > +#define UPBOARD_LED_CELL(led_data, n)                   \
> > +	{                                               \
> > +		.name = "upboard-led",                  \
> > +		.id = (n),                              \
> > +		.platform_data = &led_data[(n)],        \
> > +		.pdata_size = sizeof(*(led_data)),      \
> > +	}
> > +
> 
> There is a subsystem-level MACRO currently being reviewed on the list.
> 
> Just use the full format in your structs for now.

Will do.

> >  /* UP Squared */
> >  
> >  static const struct regmap_range upboard_up2_readable_ranges[] = {
> > @@ -116,7 +124,18 @@ static const struct regmap_config upboard_up2_regmap_config = {
> >  	.wr_table = &upboard_up2_writable_table,
> >  };
> >  
> > +static struct upboard_led_data upboard_up2_led_data[] = {
> > +	{ .id = 0, .color = "blue" },
> > +	{ .id = 1, .color = "yellow" },
> > +	{ .id = 2, .color = "green" },
> > +	{ .id = 3, .color = "red" },
> > +};
> 
> How is this data used?
> 
> Does it ever change, from board to board?

This provides indexes into the LED control register, so the leds driver
knows which regmap bits to flip, and maps them to color names, so it can
name the led devices accordingly. The mapping does change for each board
(UP1 has 3 LEDs, UP Core depends on the carrier board).

> > +struct upboard_led_data {
> > +	unsigned int id;
> > +	const char *color;
> > +};
> 
> If this is going to stick around, which I'm not sure it should, you
> need to document it (using kerneldoc format), since 'id' is quite
> ambiguous.

True, it's not very clear. Would it help here to also pass the regmap
explicitly as platform_data (instead of leds-upboard getting to the
regmap through parent driver drvdata)?

Thanks for your review!