Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Aug 2023 13:07:18 +0100
From:   Lee Jones <lee@kernel.org>
To:     =?iso-8859-1?Q?Andr=E9?= <git@apitzsch.eu>
Cc:     Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, linux-leds@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        ~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH 2/2] leds: add ktd202x driver
Message-ID: <20230817120718.GE986605@google.com>
References: <20230618-ktd202x-v1-0-fc182fefadd7@apitzsch.eu>
 <20230618-ktd202x-v1-2-fc182fefadd7@apitzsch.eu>
 <20230622181050.GY10378@google.com>
 <ba6aa842190956cb3e513d7fc0d90e6b97a07b99.camel@apitzsch.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ba6aa842190956cb3e513d7fc0d90e6b97a07b99.camel@apitzsch.eu>
Precedence: bulk

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@apitzsch.eu>
> > > ---
> > >  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 [李琼斯]