Re: [PATCH v2 2/2] leds: bd2606mvv: Driver for the Rohm 6 Channel i2c LED driver

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 4/7/23 08:08, Andreas Kemnade wrote:
> The device provides 6 channels which can be individually
> turned off and on but groups of two channels share a common brightness
> register.
> 
> Limitation: The GPIO to enable the device is not used yet.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   drivers/leds/Kconfig          |  11 +++
>   drivers/leds/Makefile         |   1 +
>   drivers/leds/leds-bd2606mvv.c | 143 ++++++++++++++++++++++++++++++++++
>   3 files changed, 155 insertions(+)
>   create mode 100644 drivers/leds/leds-bd2606mvv.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 9dbce09eabacf..cc4eadbb2542e 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -551,6 +551,17 @@ config LEDS_REGULATOR
>   	help
>   	  This option enables support for regulator driven LEDs.
>   
> +config LEDS_BD2606MVV
> +	tristate "LED driver for BD2606MVV"
> +	depends on LEDS_CLASS
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for BD2606MVV LED driver chips
> +	  accessed via the I2C bus. It supports setting brightness, with
> +	  the limitiation that there are groups of two channels sharing
> +	  a brightness setting, but not the on/off setting.
> +
>   config LEDS_BD2802
>   	tristate "LED driver for BD2802 RGB LED"
>   	depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d30395d11fd84..c07d1512c745a 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_LEDS_ARIEL)		+= leds-ariel.o
>   obj-$(CONFIG_LEDS_AW2013)		+= leds-aw2013.o
>   obj-$(CONFIG_LEDS_BCM6328)		+= leds-bcm6328.o
>   obj-$(CONFIG_LEDS_BCM6358)		+= leds-bcm6358.o
> +obj-$(CONFIG_LEDS_BD2606MVV)		+= leds-bd2606mvv.o
>   obj-$(CONFIG_LEDS_BD2802)		+= leds-bd2802.o
>   obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
>   obj-$(CONFIG_LEDS_CLEVO_MAIL)		+= leds-clevo-mail.o
> diff --git a/drivers/leds/leds-bd2606mvv.c b/drivers/leds/leds-bd2606mvv.c
> new file mode 100644
> index 0000000000000..808100e50487c
> --- /dev/null
> +++ b/drivers/leds/leds-bd2606mvv.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Andreas Kemnade
> + *
> + * Datasheet:
> + * https://fscdn.rohm.com/en/products/databook/datasheet/ic/power/led_driver/bd2606mvv_1-e.pdf
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define BD2606_MAX_LEDS 6
> +#define BD2606_MAX_BRIGHTNESS 63
> +#define BD2606_REG_PWRCNT 3
> +#define ldev_to_led(c)	container_of(c, struct bd2606mvv_led, ldev)
> +
> +struct bd2606mvv_led {
> +	bool active;

I still think having this 'active' here for the whole lifetime of the 
driver is a bit misleading. This, however, is a minor thing. With or 
without the 'active' removed from the private data:

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~