linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core
@ 2020-12-11 16:33 cy_huang
  2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: cy_huang @ 2020-12-11 16:33 UTC (permalink / raw)
  To: lee.jones, robh+dt; +Cc: cy_huang, linux-kernel, devicetree

From: ChiYuan Huang <cy_huang@richtek.com>

This adds support Richtek RT4831 MFD core. It includes four channel WLED driver
and Display Bias Voltage outputs.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
since v2
- Refine Kconfig descriptions.
- Add copyright.
- Refine error logs in probe.
- Refine comment lines in remove and shutdown.
---
 drivers/mfd/Kconfig       |  10 ++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b99a13..dfb2640 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1088,6 +1088,16 @@ config MFD_RDC321X
 	  southbridge which provides access to GPIOs and Watchdog using the
 	  southbridge PCI device configuration space.
 
+config MFD_RT4831
+	tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This enables support for the Richtek RT4831 that includes 4 channel
+	  WLED driving and Display Bias Voltage. It's commonly used to provide
+	  power to the LCD display and LCD backlight.
+
 config MFD_RT5033
 	tristate "Richtek RT5033 Power Management IC"
 	depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 1780019..4108141 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
 obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
 obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
 obj-$(CONFIG_MFD_DLN2)		+= dln2.o
+obj-$(CONFIG_MFD_RT4831)	+= rt4831-core.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
new file mode 100644
index 00000000..f837c06
--- /dev/null
+++ b/drivers/mfd/rt4831-core.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020 Richtek Technology Corp.
+ *
+ * Author: ChiYuan Huang <cy_huang@richtek.com>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define RT4831_REG_REVISION	0x01
+#define RT4831_REG_ENABLE	0x08
+#define RT4831_REG_I2CPROT	0x15
+
+#define RICHTEK_VID		0x03
+#define RT4831_VID_MASK		GENMASK(1, 0)
+#define RT4831_RESET_MASK	BIT(7)
+#define RT4831_I2CSAFETMR_MASK	BIT(0)
+
+static const struct mfd_cell rt4831_subdevs[] = {
+	OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0, "richtek,rt4831-backlight"),
+	MFD_CELL_NAME("rt4831-regulator")
+};
+
+static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
+		return true;
+	return false;
+}
+
+static const struct regmap_config rt4831_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = RT4831_REG_I2CPROT,
+
+	.readable_reg = rt4831_is_accessible_reg,
+	.writeable_reg = rt4831_is_accessible_reg,
+};
+
+static int rt4831_probe(struct i2c_client *client)
+{
+	struct gpio_desc *enable;
+	struct regmap *regmap;
+	unsigned int val;
+	int ret;
+
+	enable = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(enable)) {
+		dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
+		return PTR_ERR(enable);
+	}
+
+	regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
+	if (ret) {
+		dev_err(&client->dev, "Failed to get H/W revision\n");
+		return ret;
+	}
+
+	if ((val & RT4831_VID_MASK) != RICHTEK_VID) {
+		dev_err(&client->dev, "VID not matched, val = 0x%02x\n", val);
+		return -ENODEV;
+	}
+
+	/*
+	 * Used to prevent the abnormal shutdown.
+	 * If SCL/SDA both keep low for one second to reset HW.
+	 */
+	ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
+				 RT4831_I2CSAFETMR_MASK);
+	if (ret) {
+		dev_err(&client->dev, "Failed to enable I2C safety timer\n");
+		return ret;
+	}
+
+	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
+				    ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
+}
+
+static int rt4831_remove(struct i2c_client *client)
+{
+	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+	/* Disable WLED and DSV outputs */
+	return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
+}
+
+static void rt4831_shutdown(struct i2c_client *client)
+{
+	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+	/* Disable WLED and DSV outputs */
+	regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
+}
+
+static const struct of_device_id __maybe_unused rt4831_of_match[] = {
+	{ .compatible = "richtek,rt4831", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rt4831_of_match);
+
+static struct i2c_driver rt4831_driver = {
+	.driver = {
+		.name = "rt4831",
+		.of_match_table = of_match_ptr(rt4831_of_match),
+	},
+	.probe_new = rt4831_probe,
+	.remove = rt4831_remove,
+	.shutdown = rt4831_shutdown,
+};
+module_i2c_driver(rt4831_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-11 16:33 [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
@ 2020-12-11 16:33 ` cy_huang
  2020-12-14  9:59   ` Daniel Thompson
  2020-12-14 23:18   ` Rob Herring
  2020-12-11 16:33 ` [PATCH v4 3/3] mfd: rt4831: Adds DT binding document for Richtek RT4831 MFD core cy_huang
  2020-12-16 14:12 ` [PATCH v4 1/3] mfd: rt4831: Adds support " Lee Jones
  2 siblings, 2 replies; 17+ messages in thread
From: cy_huang @ 2020-12-11 16:33 UTC (permalink / raw)
  To: lee.jones, robh+dt; +Cc: cy_huang, linux-kernel, devicetree

From: ChiYuan Huang <cy_huang@richtek.com>

Adds DT binding document for Richtek RT4831 backlight.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
since v3
- Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch to here.
- Add dual license tag in header and backlight binding document.
- Left backlight dt-binding example only.
---
 .../leds/backlight/richtek,rt4831-backlight.yaml   | 76 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
 2 files changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h

diff --git a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
new file mode 100644
index 00000000..f24c8d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT4831 Backlight
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  RT4831 is a mutifunctional device that can provide power to the LCD display
+  and LCD backlight.
+
+  For the LCD backlight, it can provide four channel WLED driving capability.
+  Each channel driving current is up to 30mA
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
+
+properties:
+  compatible:
+    const: richtek,rt4831-backlight
+
+  default-brightness:
+    description: |
+      The default brightness that applied to the system on start-up.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 2048
+
+  max-brightness:
+    description: |
+      The max brightness for the H/W limit
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 2048
+
+  richtek,pwm-enable:
+    description: |
+      Specify the backlight dimming following by PWM duty or by SW control.
+    type: boolean
+
+  richtek,bled-ovp-sel:
+    description: |
+      Backlight OVP level selection, currently support 17V/21V/25V/29V.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    default: 1
+    minimum: 0
+    maximum: 3
+
+  richtek,channel-use:
+    description: |
+      Backlight LED channel to be used.
+      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
+    $ref: /schemas/types.yaml#/definitions/uint8
+    minimum: 1
+    maximum: 15
+
+required:
+  - compatible
+  - richtek,channel-use
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/rt4831-backlight.h>
+    backlight {
+      compatible = "richtek,rt4831-backlight";
+      default-brightness = <1024>;
+      max-brightness = <2048>;
+      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
+      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
+    };
diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
new file mode 100644
index 00000000..125c635
--- /dev/null
+++ b/include/dt-bindings/leds/rt4831-backlight.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * This header provides constants for rt4831 backlight bindings.
+ *
+ * Copyright (C) 2020, Richtek Technology Corp.
+ * Author: ChiYuan Huang <cy_huang@richtek.com>
+ */
+
+#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
+#define _DT_BINDINGS_RT4831_BACKLIGHT_H
+
+#define RT4831_BLOVPLVL_17V	0
+#define RT4831_BLOVPLVL_21V	1
+#define RT4831_BLOVPLVL_25V	2
+#define RT4831_BLOVPLVL_29V	3
+
+#define RT4831_BLED_CH1EN	(1 << 0)
+#define RT4831_BLED_CH2EN	(1 << 1)
+#define RT4831_BLED_CH3EN	(1 << 2)
+#define RT4831_BLED_CH4EN	(1 << 3)
+#define RT4831_BLED_ALLCHEN	((1 << 4) - 1)
+
+#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v4 3/3] mfd: rt4831: Adds DT binding document for Richtek RT4831 MFD core
  2020-12-11 16:33 [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
  2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
@ 2020-12-11 16:33 ` cy_huang
  2020-12-16 14:12 ` [PATCH v4 1/3] mfd: rt4831: Adds support " Lee Jones
  2 siblings, 0 replies; 17+ messages in thread
From: cy_huang @ 2020-12-11 16:33 UTC (permalink / raw)
  To: lee.jones, robh+dt; +Cc: cy_huang, linux-kernel, devicetree

From: ChiYuan Huang <cy_huang@richtek.com>

Adds DT binding document for Richtek RT4831 MFD core.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
This patch depends on
 "backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight".

since v4
- remove v3 regulator binding patch, directly merge it into mfd binding.

since v3
- Move include/dt-bindings/leds/rt4831-backlight.h to patch 0002.
- Add dual license tag in mfd binding document.

since v2
- Add regulator-allow-bypass flag in DSVLCM.
---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 108 +++++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml

diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
new file mode 100644
index 00000000..9de7c4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/richtek,rt4831.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT4831 DSV and Backlight Integrated IC
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  RT4831 is a multifunctional device that can provide power to the LCD display
+  and LCD backlight.
+
+  For Display Bias Voltage DSVP and DSVN, the output range is about 4V to 6.5V.
+  It's sufficient to meet the current LCD power requirement.
+
+  DSVLCM is a boost regulator in IC internal as DSVP and DSVN input power.
+  Its voltage should be configured above 0.15V to 0.2V gap larger than the
+  voltage needed for DSVP and DSVN. Too much voltage gap could improve the
+  voltage drop from the heavy loading scenario. But it also make the power
+  efficiency worse. It's a trade-off.
+
+  For the LCD backlight, it can provide four channel WLED driving capability.
+  Each channel driving current is up to 30mA
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
+
+properties:
+  compatible:
+    const: richtek,rt4831
+
+  reg:
+    description: I2C device address.
+    maxItems: 1
+
+  enable-gpios:
+    description: |
+      GPIO to enable/disable the chip. It is optional.
+      Some usage directly tied this pin to follow VIO 1.8V power on sequence.
+    maxItems: 1
+
+  regulators:
+    type: object
+
+    patternProperties:
+      "^DSV(LCM|P|N)$":
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+
+    required:
+      - DSVLCM
+      - DSVP
+      - DSVN
+
+    additionalProperties: false
+
+  backlight:
+    $ref: ../leds/backlight/richtek,rt4831-backlight.yaml
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/rt4831-backlight.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      rt4831@11 {
+        compatible = "richtek,rt4831";
+        reg = <0x11>;
+
+        regulators {
+          DSVLCM {
+            regulator-min-microvolt = <4000000>;
+            regulator-max-microvolt = <7150000>;
+            regulator-allow-bypass;
+          };
+          DSVP {
+            regulator-name = "rt4831-dsvp";
+            regulator-min-microvolt = <4000000>;
+            regulator-max-microvolt = <6500000>;
+            regulator-boot-on;
+          };
+          DSVN {
+            regulator-name = "rt4831-dsvn";
+            regulator-min-microvolt = <4000000>;
+            regulator-max-microvolt = <6500000>;
+            regulator-boot-on;
+          };
+        };
+
+        backlight {
+          compatible = "richtek,rt4831-backlight";
+          default-brightness = <1024>;
+          max-brightness = <2048>;
+          richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
+          richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
+        };
+      };
+    };
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
@ 2020-12-14  9:59   ` Daniel Thompson
  2020-12-14 14:40     ` ChiYuan Huang
  2020-12-14 23:18   ` Rob Herring
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Thompson @ 2020-12-14  9:59 UTC (permalink / raw)
  To: cy_huang; +Cc: lee.jones, robh+dt, cy_huang, linux-kernel, devicetree

Hi CY

On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Adds DT binding document for Richtek RT4831 backlight.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>

This patch got keyword filtered and brought to my attention
but the rest of the series did not.

If it was a backlight patch series you need to send it To: the
all the backlight maintainers.


Daniel.


> ---
> since v3
> - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch to here.
> - Add dual license tag in header and backlight binding document.
> - Left backlight dt-binding example only.
> ---
>  .../leds/backlight/richtek,rt4831-backlight.yaml   | 76 ++++++++++++++++++++++
>  include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
>  2 files changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
>  create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> new file mode 100644
> index 00000000..f24c8d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT4831 Backlight
> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  RT4831 is a mutifunctional device that can provide power to the LCD display
> +  and LCD backlight.
> +
> +  For the LCD backlight, it can provide four channel WLED driving capability.
> +  Each channel driving current is up to 30mA
> +
> +  Datasheet is available at
> +  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> +
> +properties:
> +  compatible:
> +    const: richtek,rt4831-backlight
> +
> +  default-brightness:
> +    description: |
> +      The default brightness that applied to the system on start-up.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2048
> +
> +  max-brightness:
> +    description: |
> +      The max brightness for the H/W limit
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2048
> +
> +  richtek,pwm-enable:
> +    description: |
> +      Specify the backlight dimming following by PWM duty or by SW control.
> +    type: boolean
> +
> +  richtek,bled-ovp-sel:
> +    description: |
> +      Backlight OVP level selection, currently support 17V/21V/25V/29V.
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    default: 1
> +    minimum: 0
> +    maximum: 3
> +
> +  richtek,channel-use:
> +    description: |
> +      Backlight LED channel to be used.
> +      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    minimum: 1
> +    maximum: 15
> +
> +required:
> +  - compatible
> +  - richtek,channel-use
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/rt4831-backlight.h>
> +    backlight {
> +      compatible = "richtek,rt4831-backlight";
> +      default-brightness = <1024>;
> +      max-brightness = <2048>;
> +      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
> +      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
> +    };
> diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
> new file mode 100644
> index 00000000..125c635
> --- /dev/null
> +++ b/include/dt-bindings/leds/rt4831-backlight.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * This header provides constants for rt4831 backlight bindings.
> + *
> + * Copyright (C) 2020, Richtek Technology Corp.
> + * Author: ChiYuan Huang <cy_huang@richtek.com>
> + */
> +
> +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> +
> +#define RT4831_BLOVPLVL_17V	0
> +#define RT4831_BLOVPLVL_21V	1
> +#define RT4831_BLOVPLVL_25V	2
> +#define RT4831_BLOVPLVL_29V	3
> +
> +#define RT4831_BLED_CH1EN	(1 << 0)
> +#define RT4831_BLED_CH2EN	(1 << 1)
> +#define RT4831_BLED_CH3EN	(1 << 2)
> +#define RT4831_BLED_CH4EN	(1 << 3)
> +#define RT4831_BLED_ALLCHEN	((1 << 4) - 1)
> +
> +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14  9:59   ` Daniel Thompson
@ 2020-12-14 14:40     ` ChiYuan Huang
  2020-12-14 15:14       ` Daniel Thompson
  2020-12-14 23:17       ` Rob Herring
  0 siblings, 2 replies; 17+ messages in thread
From: ChiYuan Huang @ 2020-12-14 14:40 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: Lee Jones, Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi,

Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
>
> Hi CY
>
> On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Adds DT binding document for Richtek RT4831 backlight.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
>
> This patch got keyword filtered and brought to my attention
> but the rest of the series did not.
>
> If it was a backlight patch series you need to send it To: the
> all the backlight maintainers.
>
Yes, I'm waiting for mfd reviewing.
Due to mfd patch, I need to add backlight dt-binding patch prior to
backlight source code.
Or autobuild robot will said mfd dt-binding build fail from Rob.
That's why I send the backlight dt-binding prior to the source code.

I still have backlight/regulator source code patch after mfd reviewing.
Do you want me to send all the patches without waiting for mfd reviewing?
>
> Daniel.
>
>
> > ---
> > since v3
> > - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch to here.
> > - Add dual license tag in header and backlight binding document.
> > - Left backlight dt-binding example only.
> > ---
> >  .../leds/backlight/richtek,rt4831-backlight.yaml   | 76 ++++++++++++++++++++++
> >  include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
> >  2 files changed, 99 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> >  create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> >
> > diff --git a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > new file mode 100644
> > index 00000000..f24c8d1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 Backlight
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  RT4831 is a mutifunctional device that can provide power to the LCD display
> > +  and LCD backlight.
> > +
> > +  For the LCD backlight, it can provide four channel WLED driving capability.
> > +  Each channel driving current is up to 30mA
> > +
> > +  Datasheet is available at
> > +  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: richtek,rt4831-backlight
> > +
> > +  default-brightness:
> > +    description: |
> > +      The default brightness that applied to the system on start-up.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 2048
> > +
> > +  max-brightness:
> > +    description: |
> > +      The max brightness for the H/W limit
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 2048
> > +
> > +  richtek,pwm-enable:
> > +    description: |
> > +      Specify the backlight dimming following by PWM duty or by SW control.
> > +    type: boolean
> > +
> > +  richtek,bled-ovp-sel:
> > +    description: |
> > +      Backlight OVP level selection, currently support 17V/21V/25V/29V.
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    default: 1
> > +    minimum: 0
> > +    maximum: 3
> > +
> > +  richtek,channel-use:
> > +    description: |
> > +      Backlight LED channel to be used.
> > +      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    minimum: 1
> > +    maximum: 15
> > +
> > +required:
> > +  - compatible
> > +  - richtek,channel-use
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/rt4831-backlight.h>
> > +    backlight {
> > +      compatible = "richtek,rt4831-backlight";
> > +      default-brightness = <1024>;
> > +      max-brightness = <2048>;
> > +      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
> > +      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
> > +    };
> > diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
> > new file mode 100644
> > index 00000000..125c635
> > --- /dev/null
> > +++ b/include/dt-bindings/leds/rt4831-backlight.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +/*
> > + * This header provides constants for rt4831 backlight bindings.
> > + *
> > + * Copyright (C) 2020, Richtek Technology Corp.
> > + * Author: ChiYuan Huang <cy_huang@richtek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> > +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> > +
> > +#define RT4831_BLOVPLVL_17V  0
> > +#define RT4831_BLOVPLVL_21V  1
> > +#define RT4831_BLOVPLVL_25V  2
> > +#define RT4831_BLOVPLVL_29V  3
> > +
> > +#define RT4831_BLED_CH1EN    (1 << 0)
> > +#define RT4831_BLED_CH2EN    (1 << 1)
> > +#define RT4831_BLED_CH3EN    (1 << 2)
> > +#define RT4831_BLED_CH4EN    (1 << 3)
> > +#define RT4831_BLED_ALLCHEN  ((1 << 4) - 1)
> > +
> > +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> > --
> > 2.7.4
> >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14 14:40     ` ChiYuan Huang
@ 2020-12-14 15:14       ` Daniel Thompson
  2020-12-15  7:53         ` Lee Jones
  2020-12-14 23:17       ` Rob Herring
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Thompson @ 2020-12-14 15:14 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: Lee Jones, Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> Hi,
> 
> Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
> >
> > Hi CY
> >
> > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > Adds DT binding document for Richtek RT4831 backlight.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >
> > This patch got keyword filtered and brought to my attention
> > but the rest of the series did not.
> >
> > If it was a backlight patch series you need to send it To: the
> > all the backlight maintainers.
> >
> Yes, I'm waiting for mfd reviewing.
> Due to mfd patch, I need to add backlight dt-binding patch prior to
> backlight source code.
> Or autobuild robot will said mfd dt-binding build fail from Rob.
> That's why I send the backlight dt-binding prior to the source code.
> 
> I still have backlight/regulator source code patch after mfd reviewing.
> Do you want me to send all the patches without waiting for mfd reviewing?

To some extent it's up to you.

I think I would have shared all the pieces at once (although not it Lee,
as mfd maintainer, had suggested otherwise).


Daniel.


> >
> > Daniel.
> >
> >
> > > ---
> > > since v3
> > > - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch to here.
> > > - Add dual license tag in header and backlight binding document.
> > > - Left backlight dt-binding example only.
> > > ---
> > >  .../leds/backlight/richtek,rt4831-backlight.yaml   | 76 ++++++++++++++++++++++
> > >  include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
> > >  2 files changed, 99 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > >  create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > > new file mode 100644
> > > index 00000000..f24c8d1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> > > @@ -0,0 +1,76 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Richtek RT4831 Backlight
> > > +
> > > +maintainers:
> > > +  - ChiYuan Huang <cy_huang@richtek.com>
> > > +
> > > +description: |
> > > +  RT4831 is a mutifunctional device that can provide power to the LCD display
> > > +  and LCD backlight.
> > > +
> > > +  For the LCD backlight, it can provide four channel WLED driving capability.
> > > +  Each channel driving current is up to 30mA
> > > +
> > > +  Datasheet is available at
> > > +  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: richtek,rt4831-backlight
> > > +
> > > +  default-brightness:
> > > +    description: |
> > > +      The default brightness that applied to the system on start-up.
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 0
> > > +    maximum: 2048
> > > +
> > > +  max-brightness:
> > > +    description: |
> > > +      The max brightness for the H/W limit
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 0
> > > +    maximum: 2048
> > > +
> > > +  richtek,pwm-enable:
> > > +    description: |
> > > +      Specify the backlight dimming following by PWM duty or by SW control.
> > > +    type: boolean
> > > +
> > > +  richtek,bled-ovp-sel:
> > > +    description: |
> > > +      Backlight OVP level selection, currently support 17V/21V/25V/29V.
> > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > +    default: 1
> > > +    minimum: 0
> > > +    maximum: 3
> > > +
> > > +  richtek,channel-use:
> > > +    description: |
> > > +      Backlight LED channel to be used.
> > > +      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
> > > +    $ref: /schemas/types.yaml#/definitions/uint8
> > > +    minimum: 1
> > > +    maximum: 15
> > > +
> > > +required:
> > > +  - compatible
> > > +  - richtek,channel-use
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/leds/rt4831-backlight.h>
> > > +    backlight {
> > > +      compatible = "richtek,rt4831-backlight";
> > > +      default-brightness = <1024>;
> > > +      max-brightness = <2048>;
> > > +      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
> > > +      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
> > > +    };
> > > diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
> > > new file mode 100644
> > > index 00000000..125c635
> > > --- /dev/null
> > > +++ b/include/dt-bindings/leds/rt4831-backlight.h
> > > @@ -0,0 +1,23 @@
> > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > > +/*
> > > + * This header provides constants for rt4831 backlight bindings.
> > > + *
> > > + * Copyright (C) 2020, Richtek Technology Corp.
> > > + * Author: ChiYuan Huang <cy_huang@richtek.com>
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> > > +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> > > +
> > > +#define RT4831_BLOVPLVL_17V  0
> > > +#define RT4831_BLOVPLVL_21V  1
> > > +#define RT4831_BLOVPLVL_25V  2
> > > +#define RT4831_BLOVPLVL_29V  3
> > > +
> > > +#define RT4831_BLED_CH1EN    (1 << 0)
> > > +#define RT4831_BLED_CH2EN    (1 << 1)
> > > +#define RT4831_BLED_CH3EN    (1 << 2)
> > > +#define RT4831_BLED_CH4EN    (1 << 3)
> > > +#define RT4831_BLED_ALLCHEN  ((1 << 4) - 1)
> > > +
> > > +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> > > --
> > > 2.7.4
> > >

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14 14:40     ` ChiYuan Huang
  2020-12-14 15:14       ` Daniel Thompson
@ 2020-12-14 23:17       ` Rob Herring
  2020-12-15  5:08         ` cy_huang(黃啟原)
  1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2020-12-14 23:17 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: Daniel Thompson, Lee Jones, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> Hi,
> 
> Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
> >
> > Hi CY
> >
> > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > Adds DT binding document for Richtek RT4831 backlight.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> >
> > This patch got keyword filtered and brought to my attention
> > but the rest of the series did not.
> >
> > If it was a backlight patch series you need to send it To: the
> > all the backlight maintainers.
> >
> Yes, I'm waiting for mfd reviewing.
> Due to mfd patch, I need to add backlight dt-binding patch prior to
> backlight source code.
> Or autobuild robot will said mfd dt-binding build fail from Rob.
> That's why I send the backlight dt-binding prior to the source code.
> 
> I still have backlight/regulator source code patch after mfd reviewing.
> Do you want me to send all the patches without waiting for mfd reviewing?

What happened to the regulator part of the binding? I said you could 
merge it into the mfd schema, not drop it. Bindings should be complete 
so we get a full picture of a device.

Rob

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
  2020-12-14  9:59   ` Daniel Thompson
@ 2020-12-14 23:18   ` Rob Herring
  2020-12-15  5:13     ` cy_huang(黃啟原)
  1 sibling, 1 reply; 17+ messages in thread
From: Rob Herring @ 2020-12-14 23:18 UTC (permalink / raw)
  To: cy_huang; +Cc: lee.jones, cy_huang, linux-kernel, devicetree

On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Adds DT binding document for Richtek RT4831 backlight.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> since v3
> - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd binding patch to here.
> - Add dual license tag in header and backlight binding document.
> - Left backlight dt-binding example only.
> ---
>  .../leds/backlight/richtek,rt4831-backlight.yaml   | 76 ++++++++++++++++++++++
>  include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
>  2 files changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
>  create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> new file mode 100644
> index 00000000..f24c8d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-backlight.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Richtek RT4831 Backlight
> +
> +maintainers:
> +  - ChiYuan Huang <cy_huang@richtek.com>
> +
> +description: |
> +  RT4831 is a mutifunctional device that can provide power to the LCD display
> +  and LCD backlight.
> +
> +  For the LCD backlight, it can provide four channel WLED driving capability.
> +  Each channel driving current is up to 30mA
> +
> +  Datasheet is available at
> +  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
> +
> +properties:
> +  compatible:
> +    const: richtek,rt4831-backlight
> +
> +  default-brightness:
> +    description: |
> +      The default brightness that applied to the system on start-up.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2048
> +
> +  max-brightness:
> +    description: |
> +      The max brightness for the H/W limit
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 2048
> +
> +  richtek,pwm-enable:
> +    description: |
> +      Specify the backlight dimming following by PWM duty or by SW control.
> +    type: boolean
> +
> +  richtek,bled-ovp-sel:
> +    description: |
> +      Backlight OVP level selection, currently support 17V/21V/25V/29V.
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    default: 1
> +    minimum: 0
> +    maximum: 3
> +
> +  richtek,channel-use:
> +    description: |
> +      Backlight LED channel to be used.
> +      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable or disable.
> +    $ref: /schemas/types.yaml#/definitions/uint8
> +    minimum: 1
> +    maximum: 15
> +
> +required:
> +  - compatible
> +  - richtek,channel-use
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/rt4831-backlight.h>
> +    backlight {
> +      compatible = "richtek,rt4831-backlight";
> +      default-brightness = <1024>;
> +      max-brightness = <2048>;
> +      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
> +      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;

This is in the MFD schema already, so drop the example.

> +    };
> diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
> new file mode 100644
> index 00000000..125c635
> --- /dev/null
> +++ b/include/dt-bindings/leds/rt4831-backlight.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * This header provides constants for rt4831 backlight bindings.
> + *
> + * Copyright (C) 2020, Richtek Technology Corp.
> + * Author: ChiYuan Huang <cy_huang@richtek.com>
> + */
> +
> +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> +
> +#define RT4831_BLOVPLVL_17V	0
> +#define RT4831_BLOVPLVL_21V	1
> +#define RT4831_BLOVPLVL_25V	2
> +#define RT4831_BLOVPLVL_29V	3
> +
> +#define RT4831_BLED_CH1EN	(1 << 0)
> +#define RT4831_BLED_CH2EN	(1 << 1)
> +#define RT4831_BLED_CH3EN	(1 << 2)
> +#define RT4831_BLED_CH4EN	(1 << 3)
> +#define RT4831_BLED_ALLCHEN	((1 << 4) - 1)
> +
> +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14 23:17       ` Rob Herring
@ 2020-12-15  5:08         ` cy_huang(黃啟原)
  2020-12-15  5:17           ` ChiYuan Huang
  0 siblings, 1 reply; 17+ messages in thread
From: cy_huang(黃啟原) @ 2020-12-15  5:08 UTC (permalink / raw)
  To: robh, u0084500; +Cc: daniel.thompson, linux-kernel, lee.jones, devicetree

On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
>
> Hi,
>
> Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一
> 下午5:59寫道:
> >
> >
> > Hi CY
> >
> > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > Adds DT binding document for Richtek RT4831 backlight.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > This patch got keyword filtered and brought to my attention
> > but the rest of the series did not.
> >
> > If it was a backlight patch series you need to send it To: the
> > all the backlight maintainers.
> >
> Yes, I'm waiting for mfd reviewing.
> Due to mfd patch, I need to add backlight dt-binding patch prior to
> backlight source code.
> Or autobuild robot will said mfd dt-binding build fail from Rob.
> That's why I send the backlight dt-binding prior to the source code.
>
> I still have backlight/regulator source code patch after mfd
> reviewing.
> Do you want me to send all the patches without waiting for mfd
> reviewing?
What happened to the regulator part of the binding? I said you could
merge it into the mfd schema, not drop it. Bindings should be complete
so we get a full picture of a device.

Yes, I remove the regulator dt-binding and directly merge into mfd
schema. Could you check the v4 3/3 patch?
Or you just want me to remove regulator dt-binding example in regulator
dt-binding patch?

Sorry, please also loop my gmail account.
Due to my company mailbox, I'm not sure whether the reply format is
correct or not.

Rob
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14 23:18   ` Rob Herring
@ 2020-12-15  5:13     ` cy_huang(黃啟原)
  0 siblings, 0 replies; 17+ messages in thread
From: cy_huang(黃啟原) @ 2020-12-15  5:13 UTC (permalink / raw)
  To: robh, u0084500; +Cc: linux-kernel, lee.jones, devicetree


> On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> >
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Adds DT binding document for Richtek RT4831 backlight.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > since v3
> > - Move inlcude/dt-bindings/leds/rt4831-backlight.h from v3 mfd
> > binding patch to here.
> > - Add dual license tag in header and backlight binding document.
> > - Left backlight dt-binding example only.
> > ---
> >  .../leds/backlight/richtek,rt4831-backlight.yaml   | 76
> > ++++++++++++++++++++++
> >  include/dt-bindings/leds/rt4831-backlight.h        | 23 +++++++
> >  2 files changed, 99 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-
> > backlight.yaml
> >  create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-
> > backlight.yaml
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-
> > backlight.yaml
> > new file mode 100644
> > index 00000000..f24c8d1
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-
> > backlight.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/backlight/richtek,rt4831-b
> > acklight.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Richtek RT4831 Backlight
> > +
> > +maintainers:
> > +  - ChiYuan Huang <cy_huang@richtek.com>
> > +
> > +description: |
> > +  RT4831 is a mutifunctional device that can provide power to the
> > LCD display
> > +  and LCD backlight.
> > +
> > +  For the LCD backlight, it can provide four channel WLED driving
> > capability.
> > +  Each channel driving current is up to 30mA
> > +
> > +  Datasheet is available at
> > +  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.p
> > df
> > +
> > +properties:
> > +  compatible:
> > +    const: richtek,rt4831-backlight
> > +
> > +  default-brightness:
> > +    description: |
> > +      The default brightness that applied to the system on start-
> > up.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 2048
> > +
> > +  max-brightness:
> > +    description: |
> > +      The max brightness for the H/W limit
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0
> > +    maximum: 2048
> > +
> > +  richtek,pwm-enable:
> > +    description: |
> > +      Specify the backlight dimming following by PWM duty or by SW
> > control.
> > +    type: boolean
> > +
> > +  richtek,bled-ovp-sel:
> > +    description: |
> > +      Backlight OVP level selection, currently support
> > 17V/21V/25V/29V.
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    default: 1
> > +    minimum: 0
> > +    maximum: 3
> > +
> > +  richtek,channel-use:
> > +    description: |
> > +      Backlight LED channel to be used.
> > +      BIT 0/1/2/3 is used to indicate led channel 1/2/3/4 enable
> > or disable.
> > +    $ref: /schemas/types.yaml#/definitions/uint8
> > +    minimum: 1
> > +    maximum: 15
> > +
> > +required:
> > +  - compatible
> > +  - richtek,channel-use
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/leds/rt4831-backlight.h>
> > +    backlight {
> > +      compatible = "richtek,rt4831-backlight";
> > +      default-brightness = <1024>;
> > +      max-brightness = <2048>;
> > +      richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
> > +      richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
> This is in the MFD schema already, so drop the example.
>
Will ack in next series patch.

> >
> > +    };
> > diff --git a/include/dt-bindings/leds/rt4831-backlight.h
> > b/include/dt-bindings/leds/rt4831-backlight.h
> > new file mode 100644
> > index 00000000..125c635
> > --- /dev/null
> > +++ b/include/dt-bindings/leds/rt4831-backlight.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +/*
> > + * This header provides constants for rt4831 backlight bindings.
> > + *
> > + * Copyright (C) 2020, Richtek Technology Corp.
> > + * Author: ChiYuan Huang <cy_huang@richtek.com>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_RT4831_BACKLIGHT_H
> > +#define _DT_BINDINGS_RT4831_BACKLIGHT_H
> > +
> > +#define RT4831_BLOVPLVL_17V0
> > +#define RT4831_BLOVPLVL_21V1
> > +#define RT4831_BLOVPLVL_25V2
> > +#define RT4831_BLOVPLVL_29V3
> > +
> > +#define RT4831_BLED_CH1EN(1 << 0)
> > +#define RT4831_BLED_CH2EN(1 << 1)
> > +#define RT4831_BLED_CH3EN(1 << 2)
> > +#define RT4831_BLED_CH4EN(1 << 3)
> > +#define RT4831_BLED_ALLCHEN((1 << 4) - 1)
> > +
> > +#endif /* _DT_BINDINGS_RT4831_BACKLIGHT_H */
************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-15  5:08         ` cy_huang(黃啟原)
@ 2020-12-15  5:17           ` ChiYuan Huang
  0 siblings, 0 replies; 17+ messages in thread
From: ChiYuan Huang @ 2020-12-15  5:17 UTC (permalink / raw)
  To: cy_huang(黃啟原)
  Cc: robh, daniel.thompson, linux-kernel, lee.jones, devicetree

cy_huang(黃啟原) <cy_huang@richtek.com> 於 2020年12月15日 週二 下午1:08寫道:
>
> On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> >
> > Hi,
> >
> > Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一
> > 下午5:59寫道:
> > >
> > >
> > > Hi CY
> > >
> > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > >
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > Adds DT binding document for Richtek RT4831 backlight.
> > > >
> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > This patch got keyword filtered and brought to my attention
> > > but the rest of the series did not.
> > >
> > > If it was a backlight patch series you need to send it To: the
> > > all the backlight maintainers.
> > >
> > Yes, I'm waiting for mfd reviewing.
> > Due to mfd patch, I need to add backlight dt-binding patch prior to
> > backlight source code.
> > Or autobuild robot will said mfd dt-binding build fail from Rob.
> > That's why I send the backlight dt-binding prior to the source code.
> >
> > I still have backlight/regulator source code patch after mfd
> > reviewing.
> > Do you want me to send all the patches without waiting for mfd
> > reviewing?
> What happened to the regulator part of the binding? I said you could
> merge it into the mfd schema, not drop it. Bindings should be complete
> so we get a full picture of a device.
>
Sorry I found the gmail account already loop in. Just my gmail problem.
I cannot see the email in it.....
The reply is below.
Yes, I remove the regulator dt-binding and directly merge into mfd
schema. Could you check the v4 3/3 patch?
Or you just want me to remove regulator dt-binding example, not whole
dt-binding file?
>
> Rob
> ************* Email Confidentiality Notice ********************
>
> The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-14 15:14       ` Daniel Thompson
@ 2020-12-15  7:53         ` Lee Jones
  2020-12-15  9:03           ` ChiYuan Huang
  0 siblings, 1 reply; 17+ messages in thread
From: Lee Jones @ 2020-12-15  7:53 UTC (permalink / raw)
  To: Daniel Thompson
  Cc: ChiYuan Huang, Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Mon, 14 Dec 2020, Daniel Thompson wrote:

> On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> > Hi,
> > 
> > Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
> > >
> > > Hi CY
> > >
> > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > Adds DT binding document for Richtek RT4831 backlight.
> > > >
> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > This patch got keyword filtered and brought to my attention
> > > but the rest of the series did not.
> > >
> > > If it was a backlight patch series you need to send it To: the
> > > all the backlight maintainers.
> > >
> > Yes, I'm waiting for mfd reviewing.
> > Due to mfd patch, I need to add backlight dt-binding patch prior to
> > backlight source code.
> > Or autobuild robot will said mfd dt-binding build fail from Rob.
> > That's why I send the backlight dt-binding prior to the source code.
> > 
> > I still have backlight/regulator source code patch after mfd reviewing.
> > Do you want me to send all the patches without waiting for mfd reviewing?
> 
> To some extent it's up to you.
> 
> I think I would have shared all the pieces at once (although not it Lee,
> as mfd maintainer, had suggested otherwise).

You should not need to concern yourself with patch ordering outside
of the realms of the set i.e. [PATCH 1/x], [PATCH 2/x], etc.

If you just send the whole patch set and you do not specify otherwise,
it will be applied, in order, as a set.

Sending subsystem patches without the correct maintainers as recipients
is bad form.  Many of us have filters on, so this tactic will seldom
work in any case.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-15  7:53         ` Lee Jones
@ 2020-12-15  9:03           ` ChiYuan Huang
  2020-12-15  9:23             ` Lee Jones
  0 siblings, 1 reply; 17+ messages in thread
From: ChiYuan Huang @ 2020-12-15  9:03 UTC (permalink / raw)
  To: Lee Jones
  Cc: Daniel Thompson, Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Hi, Lee:

Lee Jones <lee.jones@linaro.org> 於 2020年12月15日 週二 下午3:53寫道:
>
> On Mon, 14 Dec 2020, Daniel Thompson wrote:
>
> > On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> > > Hi,
> > >
> > > Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
> > > >
> > > > Hi CY
> > > >
> > > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > > >
> > > > > Adds DT binding document for Richtek RT4831 backlight.
> > > > >
> > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > This patch got keyword filtered and brought to my attention
> > > > but the rest of the series did not.
> > > >
> > > > If it was a backlight patch series you need to send it To: the
> > > > all the backlight maintainers.
> > > >
> > > Yes, I'm waiting for mfd reviewing.
> > > Due to mfd patch, I need to add backlight dt-binding patch prior to
> > > backlight source code.
> > > Or autobuild robot will said mfd dt-binding build fail from Rob.
> > > That's why I send the backlight dt-binding prior to the source code.
> > >
> > > I still have backlight/regulator source code patch after mfd reviewing.
> > > Do you want me to send all the patches without waiting for mfd reviewing?
> >
> > To some extent it's up to you.
> >
> > I think I would have shared all the pieces at once (although not it Lee,
> > as mfd maintainer, had suggested otherwise).
>
> You should not need to concern yourself with patch ordering outside
> of the realms of the set i.e. [PATCH 1/x], [PATCH 2/x], etc.
>
> If you just send the whole patch set and you do not specify otherwise,
> it will be applied, in order, as a set.
>
> Sending subsystem patches without the correct maintainers as recipients
> is bad form.  Many of us have filters on, so this tactic will seldom
> work in any case.
>

In my case, there're mfd/backlight/regulator for RT4831.
You mean I can just send the whole patch set directly to whole
mfd/backlight/regulator maintainers.
And you can filter like as the keyword to review the related contents, right?

From my original thought, the order is mfd -> backlight-> regulator,
one by one due to different maintainers.
Maybe I think too much about the patch ordering

If so, after getting the comment from Rob, I'll send the whole patch to you.
Thanks for the notice.
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-15  9:03           ` ChiYuan Huang
@ 2020-12-15  9:23             ` Lee Jones
  0 siblings, 0 replies; 17+ messages in thread
From: Lee Jones @ 2020-12-15  9:23 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: Daniel Thompson, Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, 15 Dec 2020, ChiYuan Huang wrote:

> Hi, Lee:
> 
> Lee Jones <lee.jones@linaro.org> 於 2020年12月15日 週二 下午3:53寫道:
> >
> > On Mon, 14 Dec 2020, Daniel Thompson wrote:
> >
> > > On Mon, Dec 14, 2020 at 10:40:55PM +0800, ChiYuan Huang wrote:
> > > > Hi,
> > > >
> > > > Daniel Thompson <daniel.thompson@linaro.org> 於 2020年12月14日 週一 下午5:59寫道:
> > > > >
> > > > > Hi CY
> > > > >
> > > > > On Sat, Dec 12, 2020 at 12:33:43AM +0800, cy_huang wrote:
> > > > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > > > >
> > > > > > Adds DT binding document for Richtek RT4831 backlight.
> > > > > >
> > > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > >
> > > > > This patch got keyword filtered and brought to my attention
> > > > > but the rest of the series did not.
> > > > >
> > > > > If it was a backlight patch series you need to send it To: the
> > > > > all the backlight maintainers.
> > > > >
> > > > Yes, I'm waiting for mfd reviewing.
> > > > Due to mfd patch, I need to add backlight dt-binding patch prior to
> > > > backlight source code.
> > > > Or autobuild robot will said mfd dt-binding build fail from Rob.
> > > > That's why I send the backlight dt-binding prior to the source code.
> > > >
> > > > I still have backlight/regulator source code patch after mfd reviewing.
> > > > Do you want me to send all the patches without waiting for mfd reviewing?
> > >
> > > To some extent it's up to you.
> > >
> > > I think I would have shared all the pieces at once (although not it Lee,
> > > as mfd maintainer, had suggested otherwise).
> >
> > You should not need to concern yourself with patch ordering outside
> > of the realms of the set i.e. [PATCH 1/x], [PATCH 2/x], etc.
> >
> > If you just send the whole patch set and you do not specify otherwise,
> > it will be applied, in order, as a set.
> >
> > Sending subsystem patches without the correct maintainers as recipients
> > is bad form.  Many of us have filters on, so this tactic will seldom
> > work in any case.
> >
> 
> In my case, there're mfd/backlight/regulator for RT4831.
> You mean I can just send the whole patch set directly to whole
> mfd/backlight/regulator maintainers.
> And you can filter like as the keyword to review the related contents, right?
> 
> From my original thought, the order is mfd -> backlight-> regulator,
> one by one due to different maintainers.
> Maybe I think too much about the patch ordering
> 
> If so, after getting the comment from Rob, I'll send the whole patch to you.
> Thanks for the notice.

Simply send them all as a single patch-set.  It's a good idea to add
all maintainers to all patches.  We will then coordinate amongst
ourselves and come up with the best merge strategy.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-12-11 16:33 [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
  2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
  2020-12-11 16:33 ` [PATCH v4 3/3] mfd: rt4831: Adds DT binding document for Richtek RT4831 MFD core cy_huang
@ 2020-12-16 14:12 ` Lee Jones
  2020-12-16 14:49   ` ChiYuan Huang
  2 siblings, 1 reply; 17+ messages in thread
From: Lee Jones @ 2020-12-16 14:12 UTC (permalink / raw)
  To: cy_huang; +Cc: robh+dt, cy_huang, linux-kernel, devicetree

On Sat, 12 Dec 2020, cy_huang wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> This adds support Richtek RT4831 MFD core. It includes four channel WLED driver

Drop mentions of MFD.  Just core driver will do.

> and Display Bias Voltage outputs.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> since v2
> - Refine Kconfig descriptions.
> - Add copyright.
> - Refine error logs in probe.
> - Refine comment lines in remove and shutdown.
> ---
>  drivers/mfd/Kconfig       |  10 ++++
>  drivers/mfd/Makefile      |   1 +
>  drivers/mfd/rt4831-core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 135 insertions(+)
>  create mode 100644 drivers/mfd/rt4831-core.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13..dfb2640 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1088,6 +1088,16 @@ config MFD_RDC321X
>  	  southbridge which provides access to GPIOs and Watchdog using the
>  	  southbridge PCI device configuration space.
>  
> +config MFD_RT4831
> +	tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  This enables support for the Richtek RT4831 that includes 4 channel
> +	  WLED driving and Display Bias Voltage. It's commonly used to provide
> +	  power to the LCD display and LCD backlight.
> +
>  config MFD_RT5033
>  	tristate "Richtek RT5033 Power Management IC"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 1780019..4108141 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
>  obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
> +obj-$(CONFIG_MFD_RT4831)	+= rt4831-core.o

Why is this called -core ...

>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o

... and this isn't?

>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> new file mode 100644
> index 00000000..f837c06
> --- /dev/null
> +++ b/drivers/mfd/rt4831-core.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang <cy_huang@richtek.com>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define RT4831_REG_REVISION	0x01
> +#define RT4831_REG_ENABLE	0x08
> +#define RT4831_REG_I2CPROT	0x15
> +
> +#define RICHTEK_VID		0x03
> +#define RT4831_VID_MASK		GENMASK(1, 0)
> +#define RT4831_RESET_MASK	BIT(7)
> +#define RT4831_I2CSAFETMR_MASK	BIT(0)
> +
> +static const struct mfd_cell rt4831_subdevs[] = {
> +	OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0, "richtek,rt4831-backlight"),
> +	MFD_CELL_NAME("rt4831-regulator")
> +};

Just a little note about these helpers.  I'm planning on unifying the
names pretty soon.  So if you have to rebase, please watch out for the
rename.

Essentially OF_MFD_CELL() will soon become MFD_CELL_OF().

> +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> +		return true;
> +	return false;
> +}
> +
> +static const struct regmap_config rt4831_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = RT4831_REG_I2CPROT,
> +
> +	.readable_reg = rt4831_is_accessible_reg,
> +	.writeable_reg = rt4831_is_accessible_reg,
> +};
> +
> +static int rt4831_probe(struct i2c_client *client)
> +{
> +	struct gpio_desc *enable;

My preference would be "enable_gpio" to ensure it's easily
identifiable further on.

> +	struct regmap *regmap;
> +	unsigned int val;

'val' is not a great name for a variable that is used for a specific
purpose.  How about 'revision'?

> +	int ret;
> +
> +	enable = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(enable)) {
> +		dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> +		return PTR_ERR(enable);
> +	}
> +
> +	regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> + 
> +	ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to get H/W revision\n");
> +		return ret;
> +	}
> +
> +	if ((val & RT4831_VID_MASK) != RICHTEK_VID) {

What does VID stand for here?

The fact that I have to ask probably means it should be changed.

Variant ID perhaps?  If so, call the define 'VARIANT_ID'.

> +		dev_err(&client->dev, "VID not matched, val = 0x%02x\n", val);

Using variable names means nothing to the user.

"revision:  0x%02x" ?

> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Used to prevent the abnormal shutdown.
> +	 * If SCL/SDA both keep low for one second to reset HW.
> +	 */
> +	ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> +				 RT4831_I2CSAFETMR_MASK);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> +		return ret;
> +	}
> +
> +	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> +				    ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> +}
> +
> +static int rt4831_remove(struct i2c_client *client)
> +{
> +	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> +
> +	/* Disable WLED and DSV outputs */
> +	return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> +}
> +
> +static void rt4831_shutdown(struct i2c_client *client)
> +{
> +	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> +
> +	/* Disable WLED and DSV outputs */
> +	regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> +}
> +
> +static const struct of_device_id __maybe_unused rt4831_of_match[] = {
> +	{ .compatible = "richtek,rt4831", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rt4831_of_match);
> +
> +static struct i2c_driver rt4831_driver = {
> +	.driver = {
> +		.name = "rt4831",
> +		.of_match_table = of_match_ptr(rt4831_of_match),

The trend is to not use of_match_ptr() anymore, as these devices can
not be instantiated using ACPI.

> +	},
> +	.probe_new = rt4831_probe,
> +	.remove = rt4831_remove,
> +	.shutdown = rt4831_shutdown,
> +};
> +module_i2c_driver(rt4831_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_LICENSE("GPL v2");

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-12-16 14:12 ` [PATCH v4 1/3] mfd: rt4831: Adds support " Lee Jones
@ 2020-12-16 14:49   ` ChiYuan Huang
  2020-12-16 14:58     ` Lee Jones
  0 siblings, 1 reply; 17+ messages in thread
From: ChiYuan Huang @ 2020-12-16 14:49 UTC (permalink / raw)
  To: Lee Jones
  Cc: Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Lee Jones <lee.jones@linaro.org> 於 2020年12月16日 週三 下午10:12寫道:
>
> On Sat, 12 Dec 2020, cy_huang wrote:
>
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > This adds support Richtek RT4831 MFD core. It includes four channel WLED driver
>
> Drop mentions of MFD.  Just core driver will do.
>
> > and Display Bias Voltage outputs.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > since v2
> > - Refine Kconfig descriptions.
> > - Add copyright.
> > - Refine error logs in probe.
> > - Refine comment lines in remove and shutdown.
> > ---
> >  drivers/mfd/Kconfig       |  10 ++++
> >  drivers/mfd/Makefile      |   1 +
> >  drivers/mfd/rt4831-core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 135 insertions(+)
> >  create mode 100644 drivers/mfd/rt4831-core.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..dfb2640 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> >         southbridge which provides access to GPIOs and Watchdog using the
> >         southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > +     tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > +     depends on I2C
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     help
> > +       This enables support for the Richtek RT4831 that includes 4 channel
> > +       WLED driving and Display Bias Voltage. It's commonly used to provide
> > +       power to the LCD display and LCD backlight.
> > +
> >  config MFD_RT5033
> >       tristate "Richtek RT5033 Power Management IC"
> >       depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..4108141 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC)       += menf21bmc.o
> >  obj-$(CONFIG_MFD_HI6421_PMIC)        += hi6421-pmic-core.o
> >  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
> >  obj-$(CONFIG_MFD_DLN2)               += dln2.o
> > +obj-$(CONFIG_MFD_RT4831)     += rt4831-core.o
>
> Why is this called -core ...
>
> >  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
>
> ... and this isn't?
>

Ok, I'm rename the mfd file to rt4831 only.
Due to this mfd is the parent of all sub device, to use 'core' is
trying to distinguish from rt4831-regulator  or rt4831-backlight.
My original thought is not to let the user confused.
If to add the postfix '-core' in the file name is bad, I think it can
be removed.

> >  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> > new file mode 100644
> > index 00000000..f837c06
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831-core.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020 Richtek Technology Corp.
> > + *
> > + * Author: ChiYuan Huang <cy_huang@richtek.com>
> > + */
> > +
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#define RT4831_REG_REVISION  0x01
> > +#define RT4831_REG_ENABLE    0x08
> > +#define RT4831_REG_I2CPROT   0x15
> > +
> > +#define RICHTEK_VID          0x03
> > +#define RT4831_VID_MASK              GENMASK(1, 0)
> > +#define RT4831_RESET_MASK    BIT(7)
> > +#define RT4831_I2CSAFETMR_MASK       BIT(0)
> > +
> > +static const struct mfd_cell rt4831_subdevs[] = {
> > +     OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0, "richtek,rt4831-backlight"),
> > +     MFD_CELL_NAME("rt4831-regulator")
> > +};
>
> Just a little note about these helpers.  I'm planning on unifying the
> names pretty soon.  So if you have to rebase, please watch out for the
> rename.
>
> Essentially OF_MFD_CELL() will soon become MFD_CELL_OF().
>
Yes, I'll check this change. BTW, is it possible to get this patch
that I can integrate it into my codebase in advance.
> > +static bool rt4831_is_accessible_reg(struct device *dev, unsigned int reg)
> > +{
> > +     if (reg >= RT4831_REG_REVISION && reg <= RT4831_REG_I2CPROT)
> > +             return true;
> > +     return false;
> > +}
> > +
> > +static const struct regmap_config rt4831_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .max_register = RT4831_REG_I2CPROT,
> > +
> > +     .readable_reg = rt4831_is_accessible_reg,
> > +     .writeable_reg = rt4831_is_accessible_reg,
> > +};
> > +
> > +static int rt4831_probe(struct i2c_client *client)
> > +{
> > +     struct gpio_desc *enable;
>
> My preference would be "enable_gpio" to ensure it's easily
> identifiable further on.
>
Yes, it make sense.
> > +     struct regmap *regmap;
> > +     unsigned int val;
>
> 'val' is not a great name for a variable that is used for a specific
> purpose.  How about 'revision'?
>
Actually, this is just for the vendor id check.
Maybe to named 'vendor_id' is more suitable.
This register can be divided into two parts, bit[7:4] is the fixed
vendor identifier, only bit[3:0] is for the chip revision.
> > +     int ret;
> > +
> > +     enable = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(enable)) {
> > +             dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> > +             return PTR_ERR(enable);
> > +     }
> > +
> > +     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> > +     if (IS_ERR(regmap)) {
> > +             dev_err(&client->dev, "Failed to initialize regmap\n");
> > +             return PTR_ERR(regmap);
> > +     }
> > +
> > +     ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
> > +     if (ret) {
> > +             dev_err(&client->dev, "Failed to get H/W revision\n");
> > +             return ret;
> > +     }
> > +
> > +     if ((val & RT4831_VID_MASK) != RICHTEK_VID) {
>
> What does VID stand for here?
>
> The fact that I have to ask probably means it should be changed.
>
> Variant ID perhaps?  If so, call the define 'VARIANT_ID'.
>
Like as the the above explaination.
'RICHTEK_VID' may not be good. So it make you confused.
I'll rename it to 'RICHTEK_VENDOR_ID'. How about this naming?

> > +             dev_err(&client->dev, "VID not matched, val = 0x%02x\n", val);
>
> Using variable names means nothing to the user.
>
> "revision:  0x%02x" ?
>
How about this one "CHIP ID: 0x%02x"?
May I use 'chip_id to replace this variable 'val' naming?
> > +             return -ENODEV;
> > +     }
> > +
> > +     /*
> > +      * Used to prevent the abnormal shutdown.
> > +      * If SCL/SDA both keep low for one second to reset HW.
> > +      */
> > +     ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> > +                              RT4831_I2CSAFETMR_MASK);
> > +     if (ret) {
> > +             dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> > +             return ret;
> > +     }
> > +
> > +     return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> > +                                 ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> > +}
> > +
> > +static int rt4831_remove(struct i2c_client *client)
> > +{
> > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > +
> > +     /* Disable WLED and DSV outputs */
> > +     return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > +}
> > +
> > +static void rt4831_shutdown(struct i2c_client *client)
> > +{
> > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > +
> > +     /* Disable WLED and DSV outputs */
> > +     regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > +}
> > +
> > +static const struct of_device_id __maybe_unused rt4831_of_match[] = {
> > +     { .compatible = "richtek,rt4831", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, rt4831_of_match);
> > +
> > +static struct i2c_driver rt4831_driver = {
> > +     .driver = {
> > +             .name = "rt4831",
> > +             .of_match_table = of_match_ptr(rt4831_of_match),
>
> The trend is to not use of_match_ptr() anymore, as these devices can
> not be instantiated using ACPI.
>
OK, I'll remove 'of_match_ptr'.
> > +     },
> > +     .probe_new = rt4831_probe,
> > +     .remove = rt4831_remove,
> > +     .shutdown = rt4831_shutdown,
> > +};
> > +module_i2c_driver(rt4831_driver);
> > +
> > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> > +MODULE_LICENSE("GPL v2");
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-12-16 14:49   ` ChiYuan Huang
@ 2020-12-16 14:58     ` Lee Jones
  0 siblings, 0 replies; 17+ messages in thread
From: Lee Jones @ 2020-12-16 14:58 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: Rob Herring, cy_huang, lkml,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, 16 Dec 2020, ChiYuan Huang wrote:

> Lee Jones <lee.jones@linaro.org> 於 2020年12月16日 週三 下午10:12寫道:
> >
> > On Sat, 12 Dec 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > This adds support Richtek RT4831 MFD core. It includes four channel WLED driver
> >
> > Drop mentions of MFD.  Just core driver will do.
> >
> > > and Display Bias Voltage outputs.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > > since v2
> > > - Refine Kconfig descriptions.
> > > - Add copyright.
> > > - Refine error logs in probe.
> > > - Refine comment lines in remove and shutdown.
> > > ---
> > >  drivers/mfd/Kconfig       |  10 ++++
> > >  drivers/mfd/Makefile      |   1 +
> > >  drivers/mfd/rt4831-core.c | 124 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 135 insertions(+)
> > >  create mode 100644 drivers/mfd/rt4831-core.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..dfb2640 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > >         southbridge which provides access to GPIOs and Watchdog using the
> > >         southbridge PCI device configuration space.
> > >
> > > +config MFD_RT4831
> > > +     tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > > +     depends on I2C
> > > +     select MFD_CORE
> > > +     select REGMAP_I2C
> > > +     help
> > > +       This enables support for the Richtek RT4831 that includes 4 channel
> > > +       WLED driving and Display Bias Voltage. It's commonly used to provide
> > > +       power to the LCD display and LCD backlight.
> > > +
> > >  config MFD_RT5033
> > >       tristate "Richtek RT5033 Power Management IC"
> > >       depends on I2C
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 1780019..4108141 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -235,6 +235,7 @@ obj-$(CONFIG_MFD_MENF21BMC)       += menf21bmc.o
> > >  obj-$(CONFIG_MFD_HI6421_PMIC)        += hi6421-pmic-core.o
> > >  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
> > >  obj-$(CONFIG_MFD_DLN2)               += dln2.o
> > > +obj-$(CONFIG_MFD_RT4831)     += rt4831-core.o
> >
> > Why is this called -core ...
> >
> > >  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
> >
> > ... and this isn't?
> >
> 
> Ok, I'm rename the mfd file to rt4831 only.
> Due to this mfd is the parent of all sub device, to use 'core' is
> trying to distinguish from rt4831-regulator  or rt4831-backlight.
> My original thought is not to let the user confused.
> If to add the postfix '-core' in the file name is bad, I think it can
> be removed.
> 
> > >  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
> > >
> > > diff --git a/drivers/mfd/rt4831-core.c b/drivers/mfd/rt4831-core.c
> > > new file mode 100644
> > > index 00000000..f837c06
> > > --- /dev/null
> > > +++ b/drivers/mfd/rt4831-core.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2020 Richtek Technology Corp.
> > > + *
> > > + * Author: ChiYuan Huang <cy_huang@richtek.com>
> > > + */
> > > +
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#define RT4831_REG_REVISION  0x01
> > > +#define RT4831_REG_ENABLE    0x08
> > > +#define RT4831_REG_I2CPROT   0x15
> > > +
> > > +#define RICHTEK_VID          0x03
> > > +#define RT4831_VID_MASK              GENMASK(1, 0)
> > > +#define RT4831_RESET_MASK    BIT(7)
> > > +#define RT4831_I2CSAFETMR_MASK       BIT(0)
> > > +
> > > +static const struct mfd_cell rt4831_subdevs[] = {
> > > +     OF_MFD_CELL("rt4831-backlight", NULL, NULL, 0, 0, "richtek,rt4831-backlight"),
> > > +     MFD_CELL_NAME("rt4831-regulator")
> > > +};
> >
> > Just a little note about these helpers.  I'm planning on unifying the
> > names pretty soon.  So if you have to rebase, please watch out for the
> > rename.
> >
> > Essentially OF_MFD_CELL() will soon become MFD_CELL_OF().
> >
> Yes, I'll check this change. BTW, is it possible to get this patch
> that I can integrate it into my codebase in advance.

Once it's applied, it will be in Linux -next.

Until then, don't worry about it.

> > > +static int rt4831_probe(struct i2c_client *client)
> > > +{
> > > +     struct gpio_desc *enable;
> >
> > My preference would be "enable_gpio" to ensure it's easily
> > identifiable further on.
> >
> Yes, it make sense.
> > > +     struct regmap *regmap;
> > > +     unsigned int val;
> >
> > 'val' is not a great name for a variable that is used for a specific
> > purpose.  How about 'revision'?
> >
> Actually, this is just for the vendor id check.
> Maybe to named 'vendor_id' is more suitable.
> This register can be divided into two parts, bit[7:4] is the fixed
> vendor identifier, only bit[3:0] is for the chip revision.
> > > +     int ret;
> > > +
> > > +     enable = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > > +     if (IS_ERR(enable)) {
> > > +             dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> > > +             return PTR_ERR(enable);
> > > +     }
> > > +
> > > +     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> > > +     if (IS_ERR(regmap)) {
> > > +             dev_err(&client->dev, "Failed to initialize regmap\n");
> > > +             return PTR_ERR(regmap);
> > > +     }
> > > +
> > > +     ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "Failed to get H/W revision\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     if ((val & RT4831_VID_MASK) != RICHTEK_VID) {
> >
> > What does VID stand for here?
> >
> > The fact that I have to ask probably means it should be changed.
> >
> > Variant ID perhaps?  If so, call the define 'VARIANT_ID'.
> >
> Like as the the above explaination.
> 'RICHTEK_VID' may not be good. So it make you confused.
> I'll rename it to 'RICHTEK_VENDOR_ID'. How about this naming?
> 
> > > +             dev_err(&client->dev, "VID not matched, val = 0x%02x\n", val);
> >
> > Using variable names means nothing to the user.
> >
> > "revision:  0x%02x" ?
> >
> How about this one "CHIP ID: 0x%02x"?
> May I use 'chip_id to replace this variable 'val' naming?
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     /*
> > > +      * Used to prevent the abnormal shutdown.
> > > +      * If SCL/SDA both keep low for one second to reset HW.
> > > +      */
> > > +     ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> > > +                              RT4831_I2CSAFETMR_MASK);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> > > +                                 ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> > > +}
> > > +
> > > +static int rt4831_remove(struct i2c_client *client)
> > > +{
> > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > +     /* Disable WLED and DSV outputs */
> > > +     return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > +}
> > > +
> > > +static void rt4831_shutdown(struct i2c_client *client)
> > > +{
> > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > +     /* Disable WLED and DSV outputs */
> > > +     regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > +}
> > > +
> > > +static const struct of_device_id __maybe_unused rt4831_of_match[] = {
> > > +     { .compatible = "richtek,rt4831", },
> > > +     {}
> > > +};
> > > +MODULE_DEVICE_TABLE(of, rt4831_of_match);
> > > +
> > > +static struct i2c_driver rt4831_driver = {
> > > +     .driver = {
> > > +             .name = "rt4831",
> > > +             .of_match_table = of_match_ptr(rt4831_of_match),
> >
> > The trend is to not use of_match_ptr() anymore, as these devices can
> > not be instantiated using ACPI.
> >
> OK, I'll remove 'of_match_ptr'.
> > > +     },
> > > +     .probe_new = rt4831_probe,
> > > +     .remove = rt4831_remove,
> > > +     .shutdown = rt4831_shutdown,
> > > +};
> > > +module_i2c_driver(rt4831_driver);
> > > +
> > > +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> > > +MODULE_LICENSE("GPL v2");

Yes, all fine.  Thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-12-16 14:59 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-11 16:33 [PATCH v4 1/3] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
2020-12-11 16:33 ` [PATCH v4 2/3] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
2020-12-14  9:59   ` Daniel Thompson
2020-12-14 14:40     ` ChiYuan Huang
2020-12-14 15:14       ` Daniel Thompson
2020-12-15  7:53         ` Lee Jones
2020-12-15  9:03           ` ChiYuan Huang
2020-12-15  9:23             ` Lee Jones
2020-12-14 23:17       ` Rob Herring
2020-12-15  5:08         ` cy_huang(黃啟原)
2020-12-15  5:17           ` ChiYuan Huang
2020-12-14 23:18   ` Rob Herring
2020-12-15  5:13     ` cy_huang(黃啟原)
2020-12-11 16:33 ` [PATCH v4 3/3] mfd: rt4831: Adds DT binding document for Richtek RT4831 MFD core cy_huang
2020-12-16 14:12 ` [PATCH v4 1/3] mfd: rt4831: Adds support " Lee Jones
2020-12-16 14:49   ` ChiYuan Huang
2020-12-16 14:58     ` Lee Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).