Re: [PATCH v3 2/2] leds: is31f132xx: Add support for is31fl3206

[Lee Jones]

--- checkpatch.pl (1 issue) ---
Line 172: WARNING: DT compatible string "issi,is31fl3206" appears un-documented -- check ./Documentation/devicetree/bindings/

This true, or is it documented in this set?

> This chip has a different layout for the LED Control Register when
> compared to the other variants. This register contains the enable bit
> (in the highest nibble) but also the current control.
>
> According to the datasheet, a value of 010000 (0x10) in the register
> enables an LED with the output current set to maximum.
>
> This commit adds support for this IC following the previous limitation
> of the other variants: all LEDs are enabled and set with maximum
> current, letting brightness be controlled by the PWM registers.
>
> Signed-off-by: Flavio Amieiro <flavio@xxxxxxxxxxx>
> ---
>  drivers/leds/leds-is31fl32xx.c | 95 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 79 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index 285085d27453..a22d814be4b0 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -49,6 +49,9 @@
>  #define IS31FL3293_RESET_MAGIC 0xc5
>  #define IS31FL3293_MAX_MICROAMP 20000
>
> +/* Custom LED Control enable value for IS31FL3206 */
> +#define IS31FL3206_CONTROL_ENABLE_WITH_IMAX BIT(4)
> +
>  struct is31fl32xx_priv;
>  struct is31fl32xx_led_data {
>  	struct led_classdev cdev;
> @@ -79,6 +82,7 @@ struct is31fl32xx_priv {
>   * @enable_bits_per_led_control_register: number of LEDs enable bits in each
>   * @brightness_steps    : number of brightness steps supported by the chip
>   * @reset_func          : pointer to reset function
> + * @enable_all_channels_func  : pointer to enable all channels function
>   * @sw_shutdown_func    : pointer to software shutdown function
>   *
>   * For all optional register addresses, the sentinel value %IS31FL32XX_REG_NONE
> @@ -88,6 +92,11 @@ struct is31fl32xx_priv {
>   * necessary registers to a known initialization state. This is needed
>   * for chips that do not have a @reset_reg.
>   *
> + * If non-NULL, @enable_all_channels_func will be used in place of the
> + * generic channel enable function. This is needed for chips that have
> + * a LED Control Register that control more than simply the
> + * enable/disable state of the channel.
> + *
>   * @enable_bits_per_led_control_register must be >=1 if
>   * @led_control_register_base != %IS31FL32XX_REG_NONE.
>   */
> @@ -105,6 +114,7 @@ struct is31fl32xx_chipdef {
>  	u8	enable_bits_per_led_control_register;
>  	u16	brightness_steps;
>  	int (*reset_func)(struct is31fl32xx_priv *priv);
> +	int (*enable_all_channels_func)(struct is31fl32xx_priv *priv);
>  	int (*sw_shutdown_func)(struct is31fl32xx_priv *priv, bool enable);
>  };
>
> @@ -228,6 +238,28 @@ static int is31fl3293_software_shutdown(struct is31fl32xx_priv *priv,
>  	return is31fl32xx_write(priv, IS31FL3293_SHUTDOWN_REG, value);
>  }
>
> +/*
> + * Custom enable_all_channels function for IS31FL3206 because this
> + * variant has only one bit in the higher nibble for enable, and the
> + * lower nibble controls output current.
> + */
> +static int is31fl3206_enable_all_channels(struct is31fl32xx_priv *priv)
> +{
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> +	u8 num_regs = cdef->channels / cdef->enable_bits_per_led_control_register;
> +	int i, ret;
> +
> +	for (i = 0; i < num_regs; i++) {

New for() loops should initiate the variable.

  for (int i = 0; ...

> +		ret = is31fl32xx_write(priv,
> +				       cdef->led_control_register_base+i,

Spaces around '+' please.

> +				       IS31FL3206_CONTROL_ENABLE_WITH_IMAX);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * NOTE: A mutex is not needed in this function because:
>   * - All referenced data is read-only after probe()
> @@ -315,6 +347,33 @@ static int is31fl32xx_reset_regs(struct is31fl32xx_priv *priv)
>  	return 0;
>  }
>
> +static int is31fl32xx_enable_all_channels(struct is31fl32xx_priv *priv)
> +{
> +	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> +	int ret;
> +
> +	if (cdef->enable_all_channels_func)
> +		return cdef->enable_all_channels_func(priv);

I detest pointers to functions at the device driver level.

Please find another way to do this.

> +	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
> +		u8 value =
> +			GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> +		u8 num_regs = cdef->channels /
> +			cdef->enable_bits_per_led_control_register;
> +		int i;
> +
> +		for (i = 0; i < num_regs; i++) {

As above.

> +			ret = is31fl32xx_write(priv,
> +					       cdef->led_control_register_base+i,

As above.

> +					       value);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int is31fl32xx_software_shutdown(struct is31fl32xx_priv *priv,
>  					bool enable)
>  {
> @@ -345,24 +404,12 @@ static int is31fl32xx_init_regs(struct is31fl32xx_priv *priv)
>  		return ret;
>
>  	/*
> -	 * Set enable bit for all channels.
> +	 * Enable all channels.
>  	 * We will control state with PWM registers alone.
>  	 */
> -	if (cdef->led_control_register_base != IS31FL32XX_REG_NONE) {
> -		u8 value =
> -		    GENMASK(cdef->enable_bits_per_led_control_register-1, 0);
> -		u8 num_regs = cdef->channels /
> -				cdef->enable_bits_per_led_control_register;
> -		int i;
> -
> -		for (i = 0; i < num_regs; i++) {
> -			ret = is31fl32xx_write(priv,
> -					       cdef->led_control_register_base+i,
> -					       value);
> -			if (ret)
> -				return ret;
> -		}
> -	}
> +	ret = is31fl32xx_enable_all_channels(priv);
> +	if (ret)
> +		return ret;
>
>  	ret = is31fl32xx_software_shutdown(priv, false);
>  	if (ret)
> @@ -552,7 +599,22 @@ static const struct is31fl32xx_chipdef is31fl3293_cdef = {
>  	.sw_shutdown_func			= is31fl3293_software_shutdown,
>  };
>
> +static const struct is31fl32xx_chipdef is31fl3206_cdef = {
> +	.channels				= 12,
> +	.shutdown_reg				= 0x00,
> +	.pwm_update_reg			= 0x13,
> +	.global_control_reg			= 0x26,
> +	.reset_reg				= 0x2f,
> +	.output_frequency_setting_reg		= 0x27,
> +	.pwm_register_base			= 0x04,
> +	.led_control_register_base		= 0x17,
> +	.enable_bits_per_led_control_register	= 1,
> +	.brightness_steps			= 256,
> +	.enable_all_channels_func		= is31fl3206_enable_all_channels,
> +};
> +
>  static const struct of_device_id of_is31fl32xx_match[] = {
> +	{ .compatible = "issi,is31fl3206",  .data = &is31fl3206_cdef, },
>  	{ .compatible = "issi,is31fl3293",  .data = &is31fl3293_cdef, },
>  	{ .compatible = "issi,is31fl3236",  .data = &is31fl3236_cdef, },
>  	{ .compatible = "issi,is31fl3236a", .data = &is31fl3236a_cdef, },
> @@ -616,6 +678,7 @@ static void is31fl32xx_remove(struct i2c_client *client)
>   * even though it is not used for DeviceTree based instantiation.
>   */
>  static const struct i2c_device_id is31fl32xx_id[] = {
> +	{ .name = "is31fl3206" },
>  	{ .name = "is31fl3293" },
>  	{ .name = "is31fl3236" },
>  	{ .name = "is31fl3236a" },
>
> -- 
> 2.53.0
>

-- 
Lee Jones