On Mon, 07 Aug 2023, André wrote:
> Hi Lee Jones,
>
> thanks for your feedback and sorry for the late response.
>
> I'll try to address everything in the next version. But some things
> need clarification, see questions and comments below.
>
> Am Donnerstag, dem 22.06.2023 um 19:10 +0100 schrieb Lee Jones:
> > On Sun, 18 Jun 2023, André Apitzsch wrote:
> >
> > > This commit adds support for Kinetic KTD2026/7 RGB/White LED
> > > driver.
> > >
> > > Signed-off-by: André Apitzsch <git@xxxxxxxxxxx>
> > > ---
> > > drivers/leds/rgb/Kconfig | 12 +
> > > drivers/leds/rgb/Makefile | 1 +
> > > drivers/leds/rgb/leds-ktd202x.c | 610
> > > ++++++++++++++++++++++++++++++++++++++++
> > > 3 files changed, 623 insertions(+)
> > >
> > > diff --git a/drivers/leds/rgb/Kconfig b/drivers/leds/rgb/Kconfig
> > > index 360c8679c6e2..fa422e7a3f74 100644
> > > --- a/drivers/leds/rgb/Kconfig
> > > +++ b/drivers/leds/rgb/Kconfig
> > > @@ -2,6 +2,18 @@
> > >
> > > if LEDS_CLASS_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 LEDs connected to the
> > > KTD202x
> > > + chip.
> >
> > More info please.
> >
> > Who makes it? Where can it be found? What is it? What does it do?
> >
> > > + To compile this driver as a module, choose M here: the
> > > module
> > > + will be called leds-ktd202x.
> > > +
> > > config LEDS_PWM_MULTICOLOR
> > > tristate "PWM driven multi-color LED Support"
> > > depends on PWM
> > > diff --git a/drivers/leds/rgb/Makefile b/drivers/leds/rgb/Makefile
> > > index 8c01daf63f61..5b4f22e077c0 100644
> > > --- a/drivers/leds/rgb/Makefile
> > > +++ b/drivers/leds/rgb/Makefile
> > > @@ -1,5 +1,6 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > >
> > > +obj-$(CONFIG_LEDS_KTD202X) += leds-ktd202x.o
> > > obj-$(CONFIG_LEDS_PWM_MULTICOLOR) += leds-pwm-multicolor.o
> > > obj-$(CONFIG_LEDS_QCOM_LPG) += leds-qcom-lpg.o
> > > obj-$(CONFIG_LEDS_MT6370_RGB) += leds-mt6370-rgb.o
> > > diff --git a/drivers/leds/rgb/leds-ktd202x.c
> > > b/drivers/leds/rgb/leds-ktd202x.c
> > > new file mode 100644
> > > index 000000000000..4f0cc558c797
> > > --- /dev/null
> > > +++ b/drivers/leds/rgb/leds-ktd202x.c
> > > @@ -0,0 +1,610 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +// Driver for Kinetic KTD2026/7 RGB/White LED driver
> >
> > No C++ comments beyond the SPDX please.
> >
> > Copyright? Author? Date? Description.
> >
> > > +#include <linux/i2c.h>
> > > +#include <linux/led-class-multicolor.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/regulator/consumer.h>
> > > +
> > > +#define KTD202X_MAX_LEDS 4
> > > +
> > > +#define KTD202X_REG_RESET_CONTROL 0x00
> > > +#define KTD202X_REG_FLASH_PERIOD 0x01
> > > +#define KTD202X_REG_PWM1_TIMER 0x02
> > > +#define KTD202X_REG_PWM2_TIMER 0x03
> > > +#define KTD202X_REG_CHANNEL_CTRL 0x04
> > > +#define KTD202X_REG_TRISE_FALL 0x05
> > > +#define KTD202X_REG_LED_IOUT(x) (0x06 + (x))
> > > +
> > > +#define KTD202X_RSTR_RESET 0x07
> > > +
> > > +#define KTD202X_ENABLE_CTRL_WAKE 0x00 /* SCL & SDA High */
> > > +#define KTD202X_ENABLE_CTRL_SLEEP 0x08 /* SCL=High & SDA Toggling
> > > */
> >
> > The formatting between the 2 comments above is making my OCD twitch.
>
> Should I change anything here?
I guess just dropping the '=' would straighten things out.
> > > +#define KTD202X_CHANNEL_CTRL_MASK(x) (BIT(2 * (x)) | BIT(2 * (x) +
> > > 1))
> > > +#define KTD202X_CHANNEL_CTRL_OFF 0
> > > +#define KTD202X_CHANNEL_CTRL_ON(x) BIT(2 * (x))
> > > +#define KTD202X_CHANNEL_CTRL_PWM1(x) BIT(2 * (x) + 1)
> > > +#define KTD202X_CHANNEL_CTRL_PWM2(x) (BIT(2 * (x)) | BIT(2 * (x) +
> > > 1))
> > > +
> > > +#define KTD202X_TIME_MIN 256 /* ms */
> >
> > Put MS in the name, then omit the comment.
> >
> > > +#define KTD202X_TIME_STEP 128 /* ms */
> > > +#define KTD202X_ON_MAX 256
> > > +
> > > +static const struct reg_default ktd202x_reg_defaults[] = {
> > > + { KTD202X_REG_RESET_CONTROL, 0x00 },
> > > + { KTD202X_REG_FLASH_PERIOD, 0x00 },
> > > + { KTD202X_REG_PWM1_TIMER, 0x01 },
> > > + { KTD202X_REG_PWM2_TIMER, 0x01 },
> > > + { KTD202X_REG_CHANNEL_CTRL, 0x00 },
> > > + { KTD202X_REG_TRISE_FALL, 0x00 },
> > > + { KTD202X_REG_LED_IOUT(0), 0x4f },
> > > + { KTD202X_REG_LED_IOUT(1), 0x4f },
> > > + { KTD202X_REG_LED_IOUT(2), 0x4f },
> > > + { KTD202X_REG_LED_IOUT(3), 0x4f },
> >
> > What do these magic numbers mean?
>
> The default value (0x00) for KTD202X_REG_RESET_CONTROL seems difficult
> to describe in a variable name, as it changes multiple parts (Timer
> Slot Control, Enable Control and Rise/Fall Time Scaling;
> see https://www.kinet-ic.com/uploads/KTD2026-7-04h.pdf page 13)
We usually define each bit or selection of bits, then OR them.
[...]
--
Lee Jones [李琼斯]
