On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <rob@xxxxxxxxxxx> wrote: > I have some questions about recent changes to leds-pca955x.c since 4.13: > > How is non-of platform data supposed to work now? Commit ed1f4b9676a8 switched > struct led_platform_data *pdata in the _probe() function to a locally defined > structure that platform data can't provide because it's not in any header it > can #include. > > This is disguised by dev_get_platdata() returning a void * so changing the type > of pdata the returned pointer is assigned to didn't require a new typecast, > instead existing board definitions still compile but quietly break at runtime. > (Specifically the SH7760 board I use at work broke in the pdata->num_leds != > chip->bits sanity check, and then userpace sees an empty /sys/class/leds and I > started start digging because "huh"?) > > Why did the type change, anyway? The generic led_platform_data it was > using before has all the fields the device tree's actually initializing, at > least if you use flags for the new gpio stuff. > > Commit 561099a1a2e9 added CONFIG_LEDS_PCA955X_GPIO, but the initialization > code adds gpio logic outside this config symbol: probe only calls > devm_led_classdev_register() within a case statement that depends on setting the > right "this is not GPIO" value. > > The "GPIO" indicator could have been a flag in the existing LED structure's > flags field, but instead of a bit it's #defining three symbols. The > PCA955X_TYPE_* macros with the new type constants only exist in the device tree > header. Strangely, the old default "this is an LED" value isn't zero, zero is > PCA955X_TYPE_NONE which is unused (never set anywhere in the tree), and would > cause the LED to be skipped: you have to set a field platform data can't > access, using a macro platform data probably doesn't have, in order for > devm_led_classdev_register() to get called on that LED at all. Why? > > This is especially odd since if you did want to skip an LED, there was already a > way to indicate that: by giving it an empty string as a name. (It doesn't seem > to have come up, but it's the obvious way to do it.) Except commit 390c97dc6e34 > deals with that by writing the index number as a string to the platform data > struct. Leaving aside "why did you do that?", isn't the platform data supposed to > be in a read only section when it's actual platform data? And since the probe > function then immediately copies the data into the another structure, can't we > just fill out the other one directly without overwriting our arguments? > > As for the lifetime rules, the local pca955x_led (writeable copy initialized from > the read-only platform data) had the name[] array locally living in the > struct, but the device tree commits added char *default_trigger pointing to > external memory. Is there a reason this is now inconsistent? > > Here's the patch I whipped up at work today (applied to v4.14) that undid enough > of this to make the driver work again with platform data on the board we ship: No platform data, please. So, we have two options here: - use Unified Device Properties API - introduce something like LED_LOOKUP_TABLE (see analogue with GPIO / regulator / PWM) Looking at the platform data LED framework provides I don't see for now a necessity of creating lookup tables (though it might be better choice in long term). For now, you can switch to unified device properties API (basically un-ifdef pca955x_pdata_of_init() and replacing of_* by device_* or fwnode_* compatible calls) and providing a static table of built-in device properties in the platform code in question. (see include/linux/property.h, for example users of PROPERTY_ENTRY_U*() macros, like arch/arm/mach-pxa/raumfeld.c) -- With Best Regards, Andy Shevchenko
