Date:   Thu, 5 Jul 2018 22:53:12 +0200
From:   Jacek Anaszewski <jacek.anaszewski@...il.com>
To:     Rob Landley <rob@...dley.net>, clg@...d.org, pavel@....cz,
        andrew@...id.au, linux-leds@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix platform data in leds-pca955x.c

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 <rob@...dley.net>
> 
> The LED driver changes that went into 4.14 to add device tree support broke
> non-device-tree support.
> 
> Signed-off-by: Rob Landley <rob@...dley.net>
> 
>   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 =
> 

Powered by blists - more mailing lists