Re: [PATCH v7 4/4] leds: aw2013: Add AW2027 support

[Nikita Travkin <nikita@trvn.ru>]

Paul Sajna писал(а) 15.11.2026 03:55:
> The AW2027 is basically the same as the AW2013.
> 
> It differs in:
> - the max current (30mA rather than 15)
> - the number of current levels for colour-mixing (16 rather than 4)
> - the supply voltage max
> - the charge indicator (unsupported by our driver at this time)
> 
> The max current register (imax) also moves from LCFG to GCR2, and the
> selectable current levels for colour-mixing (LCFG_CUR) take their place.
> 
> The current driver implementation provided here sets the global imax to
> it's maximum, and scales each led to the closest value using the local
> imax. It's the best result achievable short of adding custom sysfs
> entries and mucking up the linux interfaces.
> 
> Comparison of all AW20XX chips https://www.awinic.com/en/productCompareList?id=47&ids=1022,74,96,75,72,71,928,927,73,76,578
> AW2027 datasheet: https://doc.awinic.com/doc/202507/77b87097-27bf-48cd-af9b-9b615064c1e1.pdf
> Vendor driver: https://www.awinic.com/en/driver/AW2027DNR/454
> 
> Signed-off-by: Paul Sajna <sajattack@postmarketos.org>

See some nits below, but regardless,

Reviewed-by: Nikita Travkin <nikita@trvn.ru>

> ---
>  drivers/leds/leds-aw2013.c | 206 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 175 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
> index 7fe7e9dae903..f62f977e2bfe 100644
> --- a/drivers/leds/leds-aw2013.c
> +++ b/drivers/leds/leds-aw2013.c
> @@ -1,5 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0+
> -// Driver for Awinic AW2013 3-channel LED driver
> +// Driver for Awinic AW20XX 3-channel LED drivers
>  
>  #include <linux/i2c.h>
>  #include <linux/leds.h>
> @@ -20,13 +20,15 @@
>  #define AW2013_GCR 0x01
>  #define AW2013_GCR_ENABLE BIT(0)
>  
> +#define AW20XX_GCR2 0x04
> +
>  /* LED channel enable register */
>  #define AW2013_LCTR 0x30
>  #define AW2013_LCTR_LE(x) BIT((x))
>  
>  /* LED channel control registers */
>  #define AW2013_LCFG(x) (0x31 + (x))
> -#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
> +#define AW2013_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
>  #define AW2013_LCFG_MD BIT(4)
>  #define AW2013_LCFG_FI BIT(5)
>  #define AW2013_LCFG_FO BIT(6)
> @@ -51,6 +53,16 @@
>  
>  #define AW2013_TIME_STEP 130 /* ms */
>  
> +/* AW2027-specific defines */
> +#define AW2016_23_27_33_CHIPID 0x09 /* vendor shared same chipid between several chips */
> +#define AW2027_REG_MAX 0x7f /* copied from vendor driver, but only up to 0x3F is documented */
> +#define AW2027_LOCAL_IMAX_MASK GENMASK(3, 0)
> +
> +#define AW2027_GLOBAL_IMAX_15MA 0
> +#define AW2027_GLOBAL_IMAX_30MA 1
> +#define AW2027_GLOBAL_IMAX_5MA  2
> +#define AW2027_GLOBAL_IMAX_10MA 3
> +
>  struct aw2013;
>  
>  struct aw2013_led {
> @@ -60,16 +72,88 @@ struct aw2013_led {
>  	unsigned int imax;
>  };
>  
> +struct aw2013_chipdef {
> +	u8 chip_id;
> +	const struct regmap_config *regmap_cfg;
> +	u32 default_imax;
> +	bool gcr2_imax;
> +	unsigned int current_levels;
> +	unsigned int current_max_ua;
> +};
> +
>  struct aw2013 {
>  	struct mutex mutex; /* held when writing to registers */
>  	struct regulator_bulk_data regulators[2];
>  	struct i2c_client *client;
>  	struct aw2013_led leds[AW2013_MAX_LEDS];
>  	struct regmap *regmap;
> +	const struct aw2013_chipdef *cdef;
>  	int num_leds;
>  	bool enabled;
>  };
>  
> +static const struct regmap_config aw2013_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AW2013_REG_MAX,
> +};
> +
> +static const struct regmap_config aw2027_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AW2027_REG_MAX,
> +};
> +
> +static const struct aw2013_chipdef aw2013_chipdef = {
> +	.chip_id = AW2013_RSTR_CHIP_ID,
> +	.regmap_cfg = &aw2013_regmap_config,
> +	.default_imax = 1, /* 5mA */
> +	.gcr2_imax = false,
> +	.current_levels = 4,
> +	.current_max_ua = 15000,
> +};
> +
> +static const struct aw2013_chipdef aw2027_chipdef = {
> +	.chip_id = AW2016_23_27_33_CHIPID,
> +	.regmap_cfg = &aw2027_regmap_config,
> +	.default_imax = 3, /* 6mA */
> +	.gcr2_imax = true,
> +	.current_levels = 16,
> +	.current_max_ua = 30000,
> +};
> +
> +/**
> + * aw2013_microamp_to_imax() - Finds the closest current step to the given microamp
> + * @microamp: Desired current in microamps
> + * @cdef: Chip definition
> + *
> + * Return: The closest current-step to the given microamp, rounded down
> + * and limited by the max current step possible for the chip.
> + */
> +static inline u32 aw2013_microamp_to_imax(u32 microamp, const struct aw2013_chipdef *cdef)
> +{
> +	u32 max_current_step = cdef->current_levels - 1;
> +	u32 closest_current_step_floor = (microamp * max_current_step) / cdef->current_max_ua;
> +
> +	return min_t(u32, closest_current_step_floor, max_current_step);
> +}
> +
> +static int aw2027_set_global_imax(struct aw2013 *chip)
> +{
> +	return regmap_update_bits(chip->regmap,
> +				 AW20XX_GCR2,
> +				 AW2013_IMAX_MASK,
> +				 AW2027_GLOBAL_IMAX_30MA);
> +}
> +
> +static int aw2027_set_local_imax(struct aw2013 *chip, struct aw2013_led *led, u32 imax)
> +{
> +	return regmap_update_bits(chip->regmap,
> +				 AW2013_LCFG(led->num),
> +				 AW2027_LOCAL_IMAX_MASK,
> +				 imax);
> +}
> +
>  static int aw2013_chip_init(struct aw2013 *chip)
>  {
>  	int ret;
> @@ -81,17 +165,47 @@ static int aw2013_chip_init(struct aw2013 *chip)
>  		return ret;
>  	}
>  
> -	for (int i = 0; i < chip->num_leds; i++) {
> -		ret = regmap_update_bits(chip->regmap,
> -					 AW2013_LCFG(chip->leds[i].num),
> -					 AW2013_LCFG_IMAX_MASK,
> -					 chip->leds[i].imax);
> +	if (chip->cdef->gcr2_imax) {
> +		/*
> +		 * AW2027 supports 4-step global imax, and also a 16-step control for limiting
> +		 * individual current per LED. This doesn't map to the single value the
> +		 * devicetree provides super well, so set global imax to maximum, and
> +		 * local imax to whatever gets us closest to the value in the devicetree.
> +		 */
> +		ret = aw2027_set_global_imax(chip);

Following Lee's point (that sashiko has reiterated on this revision too),
perhaps you could move if/else branch into aw20xx_set_imax()? and then
get rid of if/else here and down in probe_dt()?

>  		if (ret) {
>  			dev_err(&chip->client->dev,
> -				"Failed to set maximum current for led %d: %d\n",
> -				chip->leds[i].num, ret);
> +				"Failed to set maximum global current: %d\n",
> +				ret);
>  			return ret;
>  		}
> +		/*
> +		 * This still needs to be here despite also being in aw2013_probe_dt,
> +		 * for cases where chip is disabled and re-enabled.
> +		 */

nit: I'm not sure this comment adds value TBH

Thanks for taking your time to improve this driver!
Nikita

> +		for (int i = 0; i < chip->num_leds; i++) {
> +			ret = aw2027_set_local_imax(chip, &chip->leds[i], chip->leds[i].imax);
> +			if (ret) {
> +				dev_err(&chip->client->dev,
> +					"Failed to set maximum current for led %d: %d\n",
> +					chip->leds[i].num, ret);
> +				return ret;
> +			}
> +		}
> +	} else {
> +		/* AW2013 only supports 4-step individual current per LED */
> +		for (int i = 0; i < chip->num_leds; i++) {
> +			ret = regmap_update_bits(chip->regmap,
> +						 AW2013_LCFG(chip->leds[i].num),
> +						 AW2013_IMAX_MASK,
> +						 chip->leds[i].imax);
> +			if (ret) {
> +				dev_err(&chip->client->dev,
> +					"Failed to set maximum current for led %d: %d\n",
> +					chip->leds[i].num, ret);
> +				return ret;
> +			}
> +		}
>  	}
>  
>  	return ret;
> @@ -291,9 +405,9 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>  		init_data.fwnode = of_fwnode_handle(child);
>  
>  		if (!of_property_read_u32(child, "led-max-microamp", &imax)) {
> -			led->imax = min_t(u32, imax / 5000, 3);
> +			led->imax = aw2013_microamp_to_imax(imax, chip->cdef);
>  		} else {
> -			led->imax = 1; // 5mA
> +			led->imax = chip->cdef->default_imax;
>  			dev_info(&chip->client->dev,
>  				 "DT property led-max-microamp is missing\n");
>  		}
> @@ -303,14 +417,30 @@ static int aw2013_probe_dt(struct aw2013 *chip)
>  
>  		mutex_lock(&chip->mutex);
>  		chip->num_leds = ++i;
> -		if (chip->enabled) {
> +		/*
> +		 * needs to be here and in aw2013_chip_init because chip->num_leds increments
> +		 * just above here while we're parsing the devicetree, and chip->enabled
> +		 * ensures chip_init runs as little as possible.
> +		 * Requires this function to have regulators enabled.
> +		 */
> +		if (chip->cdef->gcr2_imax) {
> +			ret = aw2027_set_local_imax(chip, led, led->imax);
> +			if (ret) {
> +				mutex_unlock(&chip->mutex);
> +				return dev_err_probe(&chip->client->dev, ret,
> +						     "Failed to set maximum current for led %d\n",
> +						     led->num);
> +			}
> +		} else {
>  			ret = regmap_update_bits(chip->regmap,
>  						 AW2013_LCFG(led->num),
> -						 AW2013_LCFG_IMAX_MASK,
> +						 AW2013_IMAX_MASK,
>  						 led->imax);
>  			if (ret) {
>  				mutex_unlock(&chip->mutex);
> -				return ret;
> +				return dev_err_probe(&chip->client->dev, ret,
> +						     "Failed to set maximum current for led %d\n",
> +						     led->num);
>  			}
>  		}
>  		mutex_unlock(&chip->mutex);
> @@ -332,18 +462,18 @@ static void aw2013_chip_disable_action(void *data)
>  	aw2013_chip_disable(data);
>  }
>  
> -static const struct regmap_config aw2013_regmap_config = {
> -	.reg_bits = 8,
> -	.val_bits = 8,
> -	.max_register = AW2013_REG_MAX,
> -};
>  
>  static int aw2013_probe(struct i2c_client *client)
>  {
>  	struct aw2013 *chip;
> +	const struct aw2013_chipdef *cdef;
>  	int ret;
>  	unsigned int chipid;
>  
> +	cdef = device_get_match_data(&client->dev);
> +	if (!cdef)
> +		return -ENODEV;
> +
>  	chip = devm_kzalloc_obj(&client->dev, *chip);
>  	if (!chip)
>  		return -ENOMEM;
> @@ -355,9 +485,10 @@ static int aw2013_probe(struct i2c_client *client)
>  	mutex_lock(&chip->mutex);
>  
>  	chip->client = client;
> +	chip->cdef = cdef;
>  	i2c_set_clientdata(client, chip);
>  
> -	chip->regmap = devm_regmap_init_i2c(client, &aw2013_regmap_config);
> +	chip->regmap = devm_regmap_init_i2c(client, chip->cdef->regmap_cfg);
>  	if (IS_ERR(chip->regmap)) {
>  		ret = dev_err_probe(&client->dev, PTR_ERR(chip->regmap),
>  				    "Failed to allocate register map\n");
> @@ -390,7 +521,7 @@ static int aw2013_probe(struct i2c_client *client)
>  		goto error_reg;
>  	}
>  
> -	if (chipid != AW2013_RSTR_CHIP_ID) {
> +	if (chipid != chip->cdef->chip_id) {
>  		ret = dev_err_probe(&client->dev, -ENODEV,
>  				    "Chip reported wrong ID: %x\n",
>  				     chipid);
> @@ -403,21 +534,28 @@ static int aw2013_probe(struct i2c_client *client)
>  		goto error_reg;
>  	}
>  
> +	mutex_unlock(&chip->mutex);
> +
> +	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
> +	if (ret)
> +		goto error_reg_no_mutex;
> +
> +	ret = aw2013_probe_dt(chip);
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret,
> +			      "Failed to parse devicetree\n");
> +		goto error_reg_no_mutex;
> +	}
> +
>  	ret = regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
>  				     chip->regulators);
>  	if (ret) {
>  		dev_err_probe(&client->dev, ret,
>  			      "Failed to disable regulators\n");
> -		goto error;
> -	}
> -
> -	mutex_unlock(&chip->mutex);
> -
> -	ret = devm_add_action(&client->dev, aw2013_chip_disable_action, chip);
> -	if (ret)
>  		return ret;
> +	}
>  
> -	return aw2013_probe_dt(chip);
> +	return 0;
>  
>  error_reg:
>  	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
> @@ -426,10 +564,16 @@ static int aw2013_probe(struct i2c_client *client)
>  error:
>  	mutex_unlock(&chip->mutex);
>  	return ret;
> +
> +error_reg_no_mutex:
> +	regulator_bulk_disable(ARRAY_SIZE(chip->regulators),
> +			       chip->regulators);
> +	return ret;
>  }
>  
>  static const struct of_device_id aw2013_match_table[] = {
> -	{ .compatible = "awinic,aw2013", },
> +	{ .compatible = "awinic,aw2013", .data = &aw2013_chipdef },
> +	{ .compatible = "awinic,aw2027", .data = &aw2027_chipdef },
>  	{ /* sentinel */ },
>  };
>  
> @@ -446,5 +590,5 @@ static struct i2c_driver aw2013_driver = {
>  module_i2c_driver(aw2013_driver);
>  
>  MODULE_AUTHOR("Nikita Travkin <nikitos.tr@gmail.com>");
> -MODULE_DESCRIPTION("AW2013 LED driver");
> +MODULE_DESCRIPTION("AW20XX LED driver");
>  MODULE_LICENSE("GPL v2");