On Tue, 07 Mar 2023, ChiYuan Huang wrote:

Hi, Lee:

Thanks's for the reviewing.
To prevent the misunderstanding, reply as below.
No reply means will do.

On Sun, Mar 05, 2023 at 08:55:33AM +0000, Lee Jones wrote:

quoted

On Thu, 23 Feb 2023, ChiaEn Wu wrote:

quoted

From: ChiYuan Huang <redacted>

The MediaTek MT6370 is a highly-integrated smart power management IC,
which includes a single cell Li-Ion/Li-Polymer switching battery
charger, a USB Type-C & Power Delivery (PD) controller, dual
Flash LED current sources, a RGB LED driver, a backlight WLED driver,
a display bias driver and a general LDO for portable devices.

Add support for the MediaTek MT6370 Current Sink Type LED Indicator
driver. It can control four channels current-sink RGB LEDs with 3 modes:
constant current, PWM, and breath mode.

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Co-developed-by: Alice Chen <redacted>
Signed-off-by: Alice Chen <redacted>
Signed-off-by: ChiYuan Huang <redacted>
Signed-off-by: ChiaEn Wu <redacted>
---
v17
- Update the year of Copyright from 2022 to 2023

---
 drivers/leds/rgb/Kconfig           |   13 +
 drivers/leds/rgb/Makefile          |    1 +
 drivers/leds/rgb/leds-mt6370-rgb.c | 1009 ++++++++++++++++++++++++++++++++++++
 3 files changed, 1023 insertions(+)
 create mode 100644 drivers/leds/rgb/leds-mt6370-rgb.c

[...]

quoted

quoted

+static int mt6370_gen_breath_pattern(struct mt6370_priv *priv,
+				     struct led_pattern *pattern, u32 len,
+				     u8 *pattern_val, u32 val_len)
+{
+	enum mt6370_led_ranges sel_range;
+	struct led_pattern *curr;
+	unsigned int sel;
+	u32 val = 0;
+	int i;
+
+	if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
+		return -EINVAL;
+
+	/*
+	 * Pattern list
+	 * tr1:	 byte 0, b'[7: 4]

Perhaps this is standard formatting and I'm just not aware of it, but
the space is throwing me and making me think twice.  Does this mean bits
7 through 4, so b'11110000?

Yes. like as you said. Sorry for the redudant space make you confused.
I'm not sure whether it's a standard formating or not.
Sometimes, in datasheet, we'll use this format to represent the bitfield for functions.
Your format is also good for it.
And which one is preferable?

Since neither of us are sure, just take the space out for now please.

quoted

quoted

+	 * tr2:	 byte 0, b'[3: 0]
+	 * tf1:	 byte 1, b'[7: 4]
+	 * tf2:	 byte 1, b'[3: 0]
+	 * ton:	 byte 2, b'[7: 4]
+	 * toff: byte 2, b'[3: 0]
+	 */
+	for (i = 0; i < P_MAX_PATTERNS; i++) {
+		curr = pattern + i;
+
+		sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON;
+
+		linear_range_get_selector_within(priv->ranges + sel_range,
+						 curr->delta_t, &sel);
+
+		if (i % 2) {
+			val |= sel;
+		} else {
+			val <<= 8;
+			val |= sel << 4;
+		}
+	}

[...]

quoted

quoted

+static int mt6370_led_register(struct device *dev, struct mt6370_led *led,
+			       struct led_init_data *init_data)
+{
+	struct mt6370_priv *priv = led->priv;
+	int ret;
+
+	if (led->index == MT6370_VIRTUAL_MULTICOLOR) {

This too could be split into separate functions to tidy things up a
little.

Like as below?

if (led->index == MT6370_VIRTUAL_MULTICOLOR)
   return mt6370_multicolor_led_register(....)

if (led->index == MT6370_LED_ISNK4) {
    .....
}

ret = mt6370_init_isnk_default_state(...)

Yes, that kind of thing.

Since the multilor case directly return from the sub-function, else can be removed.

quoted

quoted

+		ret = mt6370_mc_brightness_set(&led->mc.led_cdev, 0);
+		if (ret)
+			return dev_err_probe(dev, ret, "Couldn't set multicolor brightness\n");
+
+		ret = devm_led_classdev_multicolor_register_ext(dev, &led->mc,
+								init_data);
+		if (ret)
+			return dev_err_probe(dev, ret, "Couldn't register multicolor\n");
+	} else {
+		if (led->index == MT6370_LED_ISNK4) {
+			ret = regmap_field_write(priv->fields[F_CHGIND_EN], 1);
+			if (ret)
+				return dev_err_probe(dev, ret, "Failed to set CHRIND to SW\n");
+		}
+
+		ret = mt6370_isnk_init_default_state(led);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to init %d isnk state\n",
+					     led->index);
+
+		ret = devm_led_classdev_register_ext(dev, &led->isink,
+						     init_data);
+		if (ret)
+			return dev_err_probe(dev, ret, "Couldn't register isink %d\n", led->index);
+	}
+
+	return 0;
+}
+
+static int mt6370_check_vendor_info(struct mt6370_priv *priv)
+{
+	unsigned int devinfo, vid;
+	int ret;
+
+	ret = regmap_read(priv->regmap, MT6370_REG_DEV_INFO, &devinfo);
+	if (ret)
+		return ret;
+
+	vid = FIELD_GET(MT6370_VENID_MASK, devinfo);
+	if (vid == 0x9 || vid == 0xb) {

Are there nice human readable associates of these (vendor?) IDS?

For clearly understanding, I'll define these two as 'MT6372_VENDOR_ID' and 'MT6372C_VENDOR_ID'.

Much better, thank you.

--
Lee Jones [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel