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:
From: Rob Landley <[email protected]>
The LED driver changes that went into 4.14 to add device tree support broke
non-device-tree support.
Signed-off-by: Rob Landley <[email protected]>
leds-pca955x.c | 46 +++++++++++++++++++---------------------------
1 file changed, 19 insertions(+), 27 deletions(-)
diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 9057291..c1df4f1 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -134,11 +134,6 @@ struct pca955x_led {
const char *default_trigger;
};
-struct pca955x_platform_data {
- struct pca955x_led *leds;
- int num_leds;
-};
-
/* 8 bits per input register */
static inline int pca95xx_num_input_regs(int bits)
{
@@ -367,24 +362,25 @@ static int pca955x_gpio_direction_output(struct gpio_chip *gc,
#endif /* CONFIG_LEDS_PCA955X_GPIO */
#if IS_ENABLED(CONFIG_OF)
-static struct pca955x_platform_data *
+static struct led_platform_data *
pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
{
struct device_node *np = client->dev.of_node;
struct device_node *child;
- struct pca955x_platform_data *pdata;
+ struct led_platform_data *pdata;
int count;
count = of_get_child_count(np);
if (!count || count > chip->bits)
return ERR_PTR(-ENODEV);
+ /* Never freed, can be called multiple times with insmod/rmmod */
pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return ERR_PTR(-ENOMEM);
pdata->leds = devm_kzalloc(&client->dev,
- sizeof(struct pca955x_led) * chip->bits,
+ sizeof(struct led_platform_dat) * chip->bits,
GFP_KERNEL);
if (!pdata->leds)
return ERR_PTR(-ENOMEM);
@@ -401,11 +397,10 @@ pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
if (of_property_read_string(child, "label", &name))
name = child->name;
- snprintf(pdata->leds[reg].name, sizeof(pdata->leds[reg].name),
- "%s", name);
+ pdata->leds[reg].name = name;
- pdata->leds[reg].type = PCA955X_TYPE_LED;
- of_property_read_u32(child, "type", &pdata->leds[reg].type);
+ pdata->leds[reg].flags = PCA955X_TYPE_LED;
+ of_property_read_u32(child, "type", &pdata->leds[reg].flags);
of_property_read_string(child, "linux,default-trigger",
&pdata->leds[reg].default_trigger);
}
@@ -425,7 +420,7 @@ static const struct of_device_id of_pca955x_match[] = {
MODULE_DEVICE_TABLE(of, of_pca955x_match);
#else
-static struct pca955x_platform_data *
+static struct led_platform_data *
pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
{
return ERR_PTR(-ENODEV);
@@ -440,7 +435,7 @@ static int pca955x_probe(struct i2c_client *client,
struct pca955x_chipdef *chip;
struct i2c_adapter *adapter;
int i, err;
- struct pca955x_platform_data *pdata;
+ struct led_platform_data *pdata;
int ngpios = 0;
if (id) {
@@ -502,26 +497,23 @@ static int pca955x_probe(struct i2c_client *client,
pca955x_led = &pca955x->leds[i];
pca955x_led->led_num = i;
pca955x_led->pca955x = pca955x;
- pca955x_led->type = pdata->leds[i].type;
+ pca955x_led->type = pdata->leds[i].flags;
- switch (pca955x_led->type) {
- case PCA955X_TYPE_NONE:
- break;
- case PCA955X_TYPE_GPIO:
+ if (pca955x_led->type) {
ngpios++;
- break;
- case PCA955X_TYPE_LED:
+ } else {
/*
* Platform data can specify LED names and
* default triggers
*/
if (pdata->leds[i].name[0] == '\0')
- snprintf(pdata->leds[i].name,
- sizeof(pdata->leds[i].name), "%d", i);
-
- snprintf(pca955x_led->name,
- sizeof(pca955x_led->name), "pca955x:%s",
- pdata->leds[i].name);
+ snprintf(pca955x_led->name,
+ sizeof(pca955x_led->name), "pca955x:%d",
+ i);
+ else
+ snprintf(pca955x_led->name,
+ sizeof(pca955x_led->name), "pca955x:%s",
+ pdata->leds[i].name);
if (pdata->leds[i].default_trigger)
pca955x_led->led_cdev.default_trigger =
On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> 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
On Wed, Jul 4, 2018 at 8:00 PM, Andy Shevchenko
<[email protected]> wrote:
> On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> wrote:
> 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)
Taking into consideration that device is enumerated by i2c core, which
is being aware of device properties (1), better example might be
drivers/platform/x86/intel_cht_int33fe.c
(1)
commit d3e1b617ae20c459627f501b4bc55b1ea91f662b
Author: Dmitry Torokhov <[email protected]>
Date: Thu Feb 2 17:41:28 2017 -0800
i2c: allow specify device properties in i2c_board_info
--
With Best Regards,
Andy Shevchenko
On 07/04/2018 12:00 PM, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> 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.
Why?
Platform data is what this was using before the device tree migration broke it,
without regression testing any existing boards using the driver. I just put it
back to working with the existing structures defined in the existing board file,
which is as straightforward "undoing the regression" as I can.
I'm happy to migrate the whole thing to device tree, but that's bigger than
fixing an LED driver regression, and too big a change for this product release.
> 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).
I... don't see that necessity either?
The data structure the driver needed in 4.13 contained all the information
needed to run the device. The new data structure this driver created locally in
the C file had no obvious reason to exist, and didn't have visiblity outside the
driver file despite being the new input format the driver expected. How was that
thought through?
The new OF probe is allocating a temporary structure to copy data into from the
fdt, then feeding the intermediate structure to a probe function that allocates
_another_ set of structures to copy the data into from there, except the old
code copied _everything_ into the new structures and the new one is leaving
gratuitous pointers to the intermediate structures so they can't be freed. How
does that make sense?
I have no idea why they did any of this, but I don't see how my patch made it
worse. What's there is crazy, my fix was to back up to "not crazy". I'm all for
a better fix on top of that later, but "it works again" is the important bit.
(I admit I gave the device tree codepath exactly as much testing as the device
tree developers gave the platform data codepath, but it should work.)
> 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.
Ah, a static table of built-in device properties. You mean like:
+static struct led_info sh7760_led_info[] = {
+ { .name = "peer_com" },
+ { .name = "run" },
+ { .name = "gen_fault" },
+ { .name = "bat_fault" },
+ { .name = "eth_10" },
+ { .name = "bat_chg" }, /* Used as binary (low active) output */
+ { .name = "bat_load" }, /* Used as binary (low active) output */
+ { .name = "bo_on_n" }, /* Used as binary (low active) output */
+};
+
+static struct led_platform_data sh7760_led_platform_data = {
+ .num_leds = 8,
+ .leds = sh7760_led_info,
+};
+
+static struct i2c_board_info __initdata sh7760_i2c0_devices[] = {
+ {
+ I2C_BOARD_INFO("pca9551", 0x60), /* Use 7-bit addresses */
+ .platform_data = &sh7760_led_platform_data,
+ },
+};
I.E. the existing format that worked fine back in 3.x?
I've been trying to clean the board support patches up to get it all posted
upstream for a couple months now. Porting the product to a reasonably current
kernel was the first step. (Would have been 4.16 and then 4.17 but the flash
driver grew a weird data corruption bug in the dev cycle for 4.15 I haven't had
spare bandwidth to track down yet, it takes about fifteen minutes of jffs2
stressing to manifest, then you have to reformat the partition.)
I can post a 4.17 version of the patch if you like? (I have to port 3 _other_
patches to test that version on the actual hardware, and then the filesystem
will eat itself at some random point because of the flash issue unless that got
fixed since 4.16, but I can do that thursday if you think it would help?)
> (see include/linux/property.h, for example users of
> PROPERTY_ENTRY_U*() macros, like arch/arm/mach-pxa/raumfeld.c
Will that work with the driver as-is? Given that the structure the driver is
taking as external input is local to the driver .c file, I don't see how?
If an identical structure _is_ present in include/linux/property.h, presumably
not with the pca955x name in the generic header, then why is leds-pca955x.c
defining its own local copy of it? And who regression tests keeping the two in sync?
Or are you saying "let's create a third format and convert all the existing
users to something _other_ than device tree"? If so... why?
Rob
On 07/04/2018 12:04 PM, Andy Shevchenko wrote:
> On Wed, Jul 4, 2018 at 8:00 PM, Andy Shevchenko
> <[email protected]> wrote:
>> On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> wrote:
>
>> 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)
>
> Taking into consideration that device is enumerated by i2c core, which
> is being aware of device properties (1), better example might be
> drivers/platform/x86/intel_cht_int33fe.c
This file doesn't include the word "LED".
$ grep -i led drivers/platform/x86/intel_cht_int33fe.c
$
Examining it... this is an ACPI driver, Intel's Not-Invented-Here proprietary
device tree.
So I should convert an sh7760 board to ACPI? How would this fix the problem
where the driver's probe function expects a structure as input that is locally
defined, instead of the generic structure from linux/leds.h it used to accept?
If we feed the probe function NULL platform data _and_ don't have device tree
enabled, doesn't it error out?
Rob
On Wed, Jul 4, 2018 at 8:59 PM, Rob Landley <[email protected]> wrote:
> On 07/04/2018 12:00 PM, Andy Shevchenko wrote:
>> On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> wrote:
>> No platform data, please.
>
> Why?
Many reasons, starting with "no board files" approach.
Main reason for this particular topic is _unification_ of the access
to the device properties.
> Platform data is what this was using before the device tree migration broke it,
> without regression testing any existing boards using the driver. I just put it
> back to working with the existing structures defined in the existing board file,
> which is as straightforward "undoing the regression" as I can.
I agree that broken patch must be fixed, but I disagree on the approach how.
> I'm happy to migrate the whole thing to device tree, but that's bigger than
> fixing an LED driver regression, and too big a change for this product release.
Did you read my message carefully? Did I mention device tree there?
I'm proposing to move towards unified device properties API to forget
about DT/non-DT/ACPI/non-ACPI/etc property provider.
It's not a driver's business to categorize that crap.
>> 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).
>
> I... don't see that necessity either?
>
> The data structure the driver needed in 4.13 contained all the information
> needed to run the device. The new data structure this driver created locally in
> the C file had no obvious reason to exist, and didn't have visiblity outside the
> driver file despite being the new input format the driver expected. How was that
> thought through?
The author of the change didn't think about legacy platform data
variants for sure, no-one complained that time for already a year(?),
so, what do you expect.
Moreover, I agree with the author on the idea to move out from legacy
platform data.
> The new OF probe is allocating a temporary structure to copy data into from the
> fdt, then feeding the intermediate structure to a probe function that allocates
> _another_ set of structures to copy the data into from there, except the old
> code copied _everything_ into the new structures and the new one is leaving
> gratuitous pointers to the intermediate structures so they can't be freed. How
> does that make sense?
I didn't look at the details of the patch and I can admit that it
might have besides obvious regression (for you) some other non-best
design solutions.
> I have no idea why they did any of this, but I don't see how my patch made it
> worse. What's there is crazy, my fix was to back up to "not crazy". I'm all for
> a better fix on top of that later, but "it works again" is the important bit.
Again, I agree on the "regression must be fixed" and disagree on the
approach you chose.
> (I admit I gave the device tree codepath exactly as much testing as the device
> tree developers gave the platform data codepath, but it should work.)
>
>> 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.
>
> Ah, a static table of built-in device properties. You mean like:
>
> +static struct led_info sh7760_led_info[] = {
> + { .name = "peer_com" },
> + { .name = "run" },
> + { .name = "gen_fault" },
> + { .name = "bat_fault" },
> + { .name = "eth_10" },
> + { .name = "bat_chg" }, /* Used as binary (low active) output */
> + { .name = "bat_load" }, /* Used as binary (low active) output */
> + { .name = "bo_on_n" }, /* Used as binary (low active) output */
> +};
> +
> +static struct led_platform_data sh7760_led_platform_data = {
> + .num_leds = 8,
> + .leds = sh7760_led_info,
> +};
> +
> +static struct i2c_board_info __initdata sh7760_i2c0_devices[] = {
> + {
> + I2C_BOARD_INFO("pca9551", 0x60), /* Use 7-bit addresses */
> + .platform_data = &sh7760_led_platform_data,
> + },
> +};
>
> I.E. the existing format that worked fine back in 3.x?
Feels like similar, but no.
I already pointed out to the examples of provider
(intel_cht_int33fe.c) and if you look into it carefully you will find
the consumers, like fusb302 and max17047.
Moreover, I pointed out to the commit against I2C core to give you a
clue how it's connected between board info and actual consumer.
> I've been trying to clean the board support patches up to get it all posted
> upstream for a couple months now. Porting the product to a reasonably current
> kernel was the first step. (Would have been 4.16 and then 4.17 but the flash
> driver grew a weird data corruption bug in the dev cycle for 4.15 I haven't had
> spare bandwidth to track down yet, it takes about fifteen minutes of jffs2
> stressing to manifest, then you have to reformat the partition.)
>
> I can post a 4.17 version of the patch if you like? (I have to port 3 _other_
> patches to test that version on the actual hardware, and then the filesystem
> will eat itself at some random point because of the flash issue unless that got
> fixed since 4.16, but I can do that thursday if you think it would help?)
I'm sorry I don't follow here.
>> (see include/linux/property.h, for example users of
>> PROPERTY_ENTRY_U*() macros, like arch/arm/mach-pxa/raumfeld.c
>
> Will that work with the driver as-is? Given that the structure the driver is
> taking as external input is local to the driver .c file, I don't see how?
Yes, that's why I pointed to files and commit.
> If an identical structure _is_ present in include/linux/property.h, presumably
> not with the pca955x name in the generic header, then why is leds-pca955x.c
> defining its own local copy of it?
No, unified device properties are driver agnostic. It's a mechanism
how to provide and consume device properties independently on resource
provider (DT/ACPI/legacy platform code).
> And who regression tests keeping the two in sync?
This is not needed as you considered it. It's orthogonal to the
driver. What is needed is to follow device tree bindings correctly in
the driver and in the provider, but this part is being handled by DT
people like Rob Herring.
> Or are you saying "let's create a third format and convert all the existing
> users to something _other_ than device tree"? If so... why?
No.
--
With Best Regards,
Andy Shevchenko
On Wed, Jul 4, 2018 at 9:09 PM, Rob Landley <[email protected]> wrote:
> On 07/04/2018 12:04 PM, Andy Shevchenko wrote:
>> On Wed, Jul 4, 2018 at 8:00 PM, Andy Shevchenko
>> <[email protected]> wrote:
>>> On Wed, Jul 4, 2018 at 3:46 AM, Rob Landley <[email protected]> wrote:
>>
>>> 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)
>>
>> Taking into consideration that device is enumerated by i2c core, which
>> is being aware of device properties (1), better example might be
>> drivers/platform/x86/intel_cht_int33fe.c
>
> This file doesn't include the word "LED".
Should it?
You seems missed a point completely.
>
> $ grep -i led drivers/platform/x86/intel_cht_int33fe.c
> $
>
> Examining it... this is an ACPI driver, Intel's Not-Invented-Here proprietary
> device tree.
Huh?!
> So I should convert an sh7760 board to ACPI?
NO! (Of cource if you have ACPI ID and meaning of that device on ACPI
enabled platform, then it's your choice)
> How would this fix the problem
> where the driver's probe function expects a structure as input that is locally
> defined, instead of the generic structure from linux/leds.h it used to accept?
You missed a point.
> If we feed the probe function NULL platform data _and_ don't have device tree
> enabled, doesn't it error out?
No.
--
With Best Regards,
Andy Shevchenko
Hi Rob.
On 07/04/2018 02:46 AM, Rob Landley 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.
That is clear omission. Nonetheless, I like the i2c_board_info related
solution, proposed by Andy. Would you mind checking that approach?
Best regards,
Jacek Anaszewski
> 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:
>
>
> From: Rob Landley <[email protected]>
>
> The LED driver changes that went into 4.14 to add device tree support broke
> non-device-tree support.
>
> Signed-off-by: Rob Landley <[email protected]>
>
> leds-pca955x.c | 46 +++++++++++++++++++---------------------------
> 1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 9057291..c1df4f1 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -134,11 +134,6 @@ struct pca955x_led {
> const char *default_trigger;
> };
>
> -struct pca955x_platform_data {
> - struct pca955x_led *leds;
> - int num_leds;
> -};
> -
> /* 8 bits per input register */
> static inline int pca95xx_num_input_regs(int bits)
> {
> @@ -367,24 +362,25 @@ static int pca955x_gpio_direction_output(struct gpio_chip *gc,
> #endif /* CONFIG_LEDS_PCA955X_GPIO */
>
> #if IS_ENABLED(CONFIG_OF)
> -static struct pca955x_platform_data *
> +static struct led_platform_data *
> pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
> {
> struct device_node *np = client->dev.of_node;
> struct device_node *child;
> - struct pca955x_platform_data *pdata;
> + struct led_platform_data *pdata;
> int count;
>
> count = of_get_child_count(np);
> if (!count || count > chip->bits)
> return ERR_PTR(-ENODEV);
>
> + /* Never freed, can be called multiple times with insmod/rmmod */
> pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> return ERR_PTR(-ENOMEM);
>
> pdata->leds = devm_kzalloc(&client->dev,
> - sizeof(struct pca955x_led) * chip->bits,
> + sizeof(struct led_platform_dat) * chip->bits,
> GFP_KERNEL);
> if (!pdata->leds)
> return ERR_PTR(-ENOMEM);
> @@ -401,11 +397,10 @@ pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
> if (of_property_read_string(child, "label", &name))
> name = child->name;
>
> - snprintf(pdata->leds[reg].name, sizeof(pdata->leds[reg].name),
> - "%s", name);
> + pdata->leds[reg].name = name;
>
> - pdata->leds[reg].type = PCA955X_TYPE_LED;
> - of_property_read_u32(child, "type", &pdata->leds[reg].type);
> + pdata->leds[reg].flags = PCA955X_TYPE_LED;
> + of_property_read_u32(child, "type", &pdata->leds[reg].flags);
> of_property_read_string(child, "linux,default-trigger",
> &pdata->leds[reg].default_trigger);
> }
> @@ -425,7 +420,7 @@ static const struct of_device_id of_pca955x_match[] = {
>
> MODULE_DEVICE_TABLE(of, of_pca955x_match);
> #else
> -static struct pca955x_platform_data *
> +static struct led_platform_data *
> pca955x_pdata_of_init(struct i2c_client *client, struct pca955x_chipdef *chip)
> {
> return ERR_PTR(-ENODEV);
> @@ -440,7 +435,7 @@ static int pca955x_probe(struct i2c_client *client,
> struct pca955x_chipdef *chip;
> struct i2c_adapter *adapter;
> int i, err;
> - struct pca955x_platform_data *pdata;
> + struct led_platform_data *pdata;
> int ngpios = 0;
>
> if (id) {
> @@ -502,26 +497,23 @@ static int pca955x_probe(struct i2c_client *client,
> pca955x_led = &pca955x->leds[i];
> pca955x_led->led_num = i;
> pca955x_led->pca955x = pca955x;
> - pca955x_led->type = pdata->leds[i].type;
> + pca955x_led->type = pdata->leds[i].flags;
>
> - switch (pca955x_led->type) {
> - case PCA955X_TYPE_NONE:
> - break;
> - case PCA955X_TYPE_GPIO:
> + if (pca955x_led->type) {
> ngpios++;
> - break;
> - case PCA955X_TYPE_LED:
> + } else {
> /*
> * Platform data can specify LED names and
> * default triggers
> */
> if (pdata->leds[i].name[0] == '\0')
> - snprintf(pdata->leds[i].name,
> - sizeof(pdata->leds[i].name), "%d", i);
> -
> - snprintf(pca955x_led->name,
> - sizeof(pca955x_led->name), "pca955x:%s",
> - pdata->leds[i].name);
> + snprintf(pca955x_led->name,
> + sizeof(pca955x_led->name), "pca955x:%d",
> + i);
> + else
> + snprintf(pca955x_led->name,
> + sizeof(pca955x_led->name), "pca955x:%s",
> + pdata->leds[i].name);
>
> if (pdata->leds[i].default_trigger)
> pca955x_led->led_cdev.default_trigger =
>