Received-SPF: pass (google.com: domain of linux-kernel+bounces-162108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <fe0bb552-52d8-4ae1-a20b-c7236cf1f255@redhat.com>
Date: Mon, 29 Apr 2024 13:05:23 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v7 1/6] leds: rgb: leds-ktd202x: Get device properties
 through fwnode to support ACPI
To: Kate Hsuan <hpa@redhat.com>, Pavel Machek <pavel@ucw.cz>,
 Lee Jones <lee@kernel.org>, linux-leds@vger.kernel.org,
 platform-driver-x86@vger.kernel.org,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
 =?UTF-8?Q?Andr=C3=A9_Apitzsch?= <git@apitzsch.eu>,
 linux-kernel@vger.kernel.org, Andy Shevchenko <andy.shevchenko@gmail.com>,
 Sebastian Reichel <sre@kernel.org>, linux-pm@vger.kernel.org
References: <20240424065212.263784-1-hpa@redhat.com>
 <20240424065212.263784-2-hpa@redhat.com>
Content-Language: en-US
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20240424065212.263784-2-hpa@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi,

On 4/24/24 8:52 AM, Kate Hsuan wrote:
> This LED controller is installed on a Xiaomi pad2 and it is an x86
> platform. The original driver is based on the device tree and can't be
> used for this ACPI based system. This patch migrated the driver to use
> fwnode to access the properties. Moreover, the fwnode API supports the
> device tree so this work won't affect the original implementations.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> Tested-by: André Apitzsch <git@apitzsch.eu> # on BQ Aquaris M5

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/leds/rgb/Kconfig        |  1 -
>  drivers/leds/rgb/leds-ktd202x.c | 64 +++++++++++++++++----------------
>  2 files changed, 34 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> index e66bd21b9852..14d6b294a786 100644
> --- a/drivers/leds/rgb/Kconfig
> +++ b/drivers/leds/rgb/Kconfig
> @@ -17,7 +17,6 @@ config LEDS_GROUP_MULTICOLOR
>  config LEDS_KTD202X
>  	tristate "LED support for KTD202x Chips"
>  	depends on I2C
> -	depends on OF
>  	select REGMAP_I2C
>  	help
>  	  This option enables support for the Kinetic KTD2026/KTD2027
> diff --git a/drivers/leds/rgb/leds-ktd202x.c b/drivers/leds/rgb/leds-ktd202x.c
> index 514965795a10..f1c810c415a4 100644
> --- a/drivers/leds/rgb/leds-ktd202x.c
> +++ b/drivers/leds/rgb/leds-ktd202x.c
> @@ -99,7 +99,7 @@ struct ktd202x {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	bool enabled;
> -	int num_leds;
> +	unsigned long num_leds;
>  	struct ktd202x_led leds[] __counted_by(num_leds);
>  };
>  
> @@ -381,16 +381,19 @@ static int ktd202x_blink_mc_set(struct led_classdev *cdev,
>  				 mc->num_colors);
>  }
>  
> -static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
> +static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct fwnode_handle *fwnode,
>  				 struct ktd202x_led *led, struct led_init_data *init_data)
>  {
> +	struct fwnode_handle *child;
>  	struct led_classdev *cdev;
> -	struct device_node *child;
>  	struct mc_subled *info;
>  	int num_channels;
>  	int i = 0;
>  
> -	num_channels = of_get_available_child_count(np);
> +	num_channels = 0;
> +	fwnode_for_each_available_child_node(fwnode, child)
> +		num_channels++;
> +
>  	if (!num_channels || num_channels > chip->num_leds)
>  		return -EINVAL;
>  
> @@ -398,22 +401,22 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
>  	if (!info)
>  		return -ENOMEM;
>  
> -	for_each_available_child_of_node(np, child) {
> +	fwnode_for_each_available_child_node(fwnode, child) {
>  		u32 mono_color;
>  		u32 reg;
>  		int ret;
>  
> -		ret = of_property_read_u32(child, "reg", &reg);
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
>  		if (ret != 0 || reg >= chip->num_leds) {
> -			dev_err(chip->dev, "invalid 'reg' of %pOFn\n", child);
> -			of_node_put(child);
> -			return -EINVAL;
> +			dev_err(chip->dev, "invalid 'reg' of %pfw\n", child);
> +			fwnode_handle_put(child);
> +			return ret;
>  		}
>  
> -		ret = of_property_read_u32(child, "color", &mono_color);
> +		ret = fwnode_property_read_u32(child, "color", &mono_color);
>  		if (ret < 0 && ret != -EINVAL) {
> -			dev_err(chip->dev, "failed to parse 'color' of %pOF\n", child);
> -			of_node_put(child);
> +			dev_err(chip->dev, "failed to parse 'color' of %pfw\n", child);
> +			fwnode_handle_put(child);
>  			return ret;
>  		}
>  
> @@ -433,16 +436,16 @@ static int ktd202x_setup_led_rgb(struct ktd202x *chip, struct device_node *np,
>  	return devm_led_classdev_multicolor_register_ext(chip->dev, &led->mcdev, init_data);
>  }
>  
> -static int ktd202x_setup_led_single(struct ktd202x *chip, struct device_node *np,
> +static int ktd202x_setup_led_single(struct ktd202x *chip, struct fwnode_handle *fwnode,
>  				    struct ktd202x_led *led, struct led_init_data *init_data)
>  {
>  	struct led_classdev *cdev;
>  	u32 reg;
>  	int ret;
>  
> -	ret = of_property_read_u32(np, "reg", &reg);
> +	ret = fwnode_property_read_u32(fwnode, "reg", &reg);
>  	if (ret != 0 || reg >= chip->num_leds) {
> -		dev_err(chip->dev, "invalid 'reg' of %pOFn\n", np);
> +		dev_err(chip->dev, "invalid 'reg' of %pfw\n", fwnode);
>  		return -EINVAL;
>  	}
>  	led->index = reg;
> @@ -454,7 +457,7 @@ static int ktd202x_setup_led_single(struct ktd202x *chip, struct device_node *np
>  	return devm_led_classdev_register_ext(chip->dev, &led->cdev, init_data);
>  }
>  
> -static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigned int index)
> +static int ktd202x_add_led(struct ktd202x *chip, struct fwnode_handle *fwnode, unsigned int index)
>  {
>  	struct ktd202x_led *led = &chip->leds[index];
>  	struct led_init_data init_data = {};
> @@ -463,21 +466,21 @@ static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigne
>  	int ret;
>  
>  	/* Color property is optional in single color case */
> -	ret = of_property_read_u32(np, "color", &color);
> +	ret = fwnode_property_read_u32(fwnode, "color", &color);
>  	if (ret < 0 && ret != -EINVAL) {
> -		dev_err(chip->dev, "failed to parse 'color' of %pOF\n", np);
> +		dev_err(chip->dev, "failed to parse 'color' of %pfw\n", fwnode);
>  		return ret;
>  	}
>  
>  	led->chip = chip;
> -	init_data.fwnode = of_fwnode_handle(np);
> +	init_data.fwnode = fwnode;
>  
>  	if (color == LED_COLOR_ID_RGB) {
>  		cdev = &led->mcdev.led_cdev;
> -		ret = ktd202x_setup_led_rgb(chip, np, led, &init_data);
> +		ret = ktd202x_setup_led_rgb(chip, fwnode, led, &init_data);
>  	} else {
>  		cdev = &led->cdev;
> -		ret = ktd202x_setup_led_single(chip, np, led, &init_data);
> +		ret = ktd202x_setup_led_single(chip, fwnode, led, &init_data);
>  	}
>  
>  	if (ret) {
> @@ -490,15 +493,14 @@ static int ktd202x_add_led(struct ktd202x *chip, struct device_node *np, unsigne
>  	return 0;
>  }
>  
> -static int ktd202x_probe_dt(struct ktd202x *chip)
> +static int ktd202x_probe_fw(struct ktd202x *chip)
>  {
> -	struct device_node *np = dev_of_node(chip->dev), *child;
> +	struct fwnode_handle *child;
> +	struct device *dev = chip->dev;
>  	int count;
>  	int i = 0;
>  
> -	chip->num_leds = (int)(unsigned long)of_device_get_match_data(chip->dev);
> -
> -	count = of_get_available_child_count(np);
> +	count = device_get_child_node_count(dev);
>  	if (!count || count > chip->num_leds)
>  		return -EINVAL;
>  
> @@ -507,11 +509,11 @@ static int ktd202x_probe_dt(struct ktd202x *chip)
>  	/* Allow the device to execute the complete reset */
>  	usleep_range(200, 300);
>  
> -	for_each_available_child_of_node(np, child) {
> +	device_for_each_child_node(dev, child) {
>  		int ret = ktd202x_add_led(chip, child, i);
>  
>  		if (ret) {
> -			of_node_put(child);
> +			fwnode_handle_put(child);
>  			return ret;
>  		}
>  		i++;
> @@ -554,6 +556,8 @@ static int ktd202x_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> +	chip->num_leds = (unsigned long)i2c_get_match_data(client);
> +
>  	chip->regulators[0].supply = "vin";
>  	chip->regulators[1].supply = "vio";
>  	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(chip->regulators), chip->regulators);
> @@ -568,7 +572,7 @@ static int ktd202x_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ret = ktd202x_probe_dt(chip);
> +	ret = ktd202x_probe_fw(chip);
>  	if (ret < 0) {
>  		regulator_bulk_disable(ARRAY_SIZE(chip->regulators), chip->regulators);
>  		return ret;
> @@ -605,7 +609,7 @@ static void ktd202x_shutdown(struct i2c_client *client)
>  static const struct of_device_id ktd202x_match_table[] = {
>  	{ .compatible = "kinetic,ktd2026", .data = (void *)KTD2026_NUM_LEDS },
>  	{ .compatible = "kinetic,ktd2027", .data = (void *)KTD2027_NUM_LEDS },
> -	{},
> +	{}
>  };
>  MODULE_DEVICE_TABLE(of, ktd202x_match_table);
>