mfd: rt4831: Adds support for Richtek RT4831 MFD core

PATCH Superseded cy_huang 3 participants 12 messages Nov 2, 2020 – Dec 2, 2020

Subsystems (3)

Device tree bindings Multifunction devices (MFD) LED

lore

Share & tools

lore

cy_huang

author

2020-11-02 03:13 UTC #0 lore raw

PATCH 1/2 mfd: rt4831: Adds support for Richtek RT4831 MFD core

From: ChiYuan Huang <[email protected]>

Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.

Signed-off-by ChiYuan Huang <[email protected]>

---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c

drivers/mfd/Kconfig +11 -0

index 8b99a13..a22f002 100644
@@ -1088,6 +1088,17 @@ config MFD_RDC321X
 	  southbridge which provides access to GPIOs and Watchdog using the
 	  southbridge PCI device configuration space.
 
+config MFD_RT4831
+	tristate "Richtek RT4831 WLED and DSV IC"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This enables support for the Richtek RT4831.
+	  RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+	  It's common used to provide the display power and to drive the
+	  display backlight WLED.
+
 config MFD_RT5033
 	tristate "Richtek RT5033 Power Management IC"
 	depends on I2C

drivers/mfd/Makefile +1 -0

index 1780019..4108141 100644
@@ -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

drivers/mfd/rt4831-core.c new file +119 -0

119 lines not on this page · raw message

Signature · 1 line

2.7.4

cy_huang

author

2020-11-02 03:13 UTC #1 lore raw

PATCH 2/2 mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core

From: ChiYuan Huang <[email protected]>

Adds DT binding document for Richtek RT4831 MFD core.

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 89 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h

Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml new file +89 -0

new file mode 100644
index 00000000..c602d50
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: GPL-2.0
+%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 <[email protected]>
+
+description: |
+  RT4831 is a mutifunctional device that can provide display panel power for
+  positive/negative voltage and also display panel wled driving.
+
+  For the display voltage output, the range is about 4V to 6.5V. It is sufficient
+  to meet the current display panel design.
+
+  For the panel backlight, it can provide four channels driving capability
+  Each 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:
+    $ref: ../regulator/richtek,rt4831-regulator.yaml
+
+  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>;
+          };
+          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>;
+        };
+      };
+    };

include/dt-bindings/leds/rt4831-backlight.h new file +23 -0

new file mode 100644
index 00000000..7084906
@@ -0,0 +1,23 @@
+/*
+ * This header provides constants for rt4831 backlight bindings.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#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 */

Signature · 1 line

2.7.4

Rob Herring

maintainer

2020-11-02 17:21 UTC #3 lore raw

Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core

In reply to cy_huang

Quoted 4 lines · cy_huang

From: ChiYuan Huang <[email protected]>
Adds DT binding document for Richtek RT4831 MFD core.

Quoted 8 lines

Signed-off-by ChiYuan Huang <[email protected]>

---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 89 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml'
xargs: dt-doc-validate: exited with status 255; aborting
make[1]: *** [Documentation/devicetree/bindings/Makefile:59: Documentation/devicetree/bindings/processed-schema-examples.json] Error 124
make: *** [Makefile:1364: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1391911

The base for the patch is generally the last rc1. Any dependencies should be noted.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:

pip3 install dtschema --upgrade

Please check and re-submit.

ChiYuan Huang

author

2020-11-03 01:14 UTC #4 lore raw

Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core

Rob Herring <[email protected]> 於 2020年11月3日週二上午1:21寫道：

Quoted 32 lines · cy_huang

Quoted 4 lines
From: ChiYuan Huang <[email protected]>
Adds DT binding document for Richtek RT4831 MFD core.
Quoted 8 lines
Signed-off-by ChiYuan Huang <[email protected]>
---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 89 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml'
xargs: dt-doc-validate: exited with status 255; aborting
make[1]: *** [Documentation/devicetree/bindings/Makefile:59: Documentation/devicetree/bindings/processed-schema-examples.json] Error 124
make: *** [Makefile:1364: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1391911
The base for the patch is generally the last rc1. Any dependencies should be noted.

Quoted 8 lines

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.

Sorry, I have one question.
If the richtek,rt4831.yaml is depend upon the other yaml, do I need to merge it all into one patch?
Currently, my submitting order is mfd, backlight, and regulator. Each part divided into two patches (one for source code, another for dt_binding_document)
I have tried dt_binding_check. It cause the error like your bot said. If dt_binding_check want to be pass, it means I should merge all dt binding document into one patch due to the dependency.

If yes, I'll do it in next series patch.

Rob Herring

maintainer

2020-11-03 01:58 UTC #5 lore raw

Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core

Quoted 40 lines · ChiYuan Huang

Rob Herring <[email protected]> 於 2020年11月3日週二上午1:21寫道：
Quoted 38 lines · cy_huang
Quoted 4 lines
From: ChiYuan Huang <[email protected]>
Adds DT binding document for Richtek RT4831 MFD core.
Quoted 8 lines
Signed-off-by ChiYuan Huang <[email protected]>
---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 89 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml'
xargs: dt-doc-validate: exited with status 255; aborting
make[1]: *** [Documentation/devicetree/bindings/Makefile:59: Documentation/devicetree/bindings/processed-schema-examples.json] Error 124
make: *** [Makefile:1364: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1391911
The base for the patch is generally the last rc1. Any dependencies should be noted.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade

Quoted 8 lines

Quoted 2 lines
Please check and re-submit.
Sorry, I have one question.
If the richtek,rt4831.yaml is depend upon the other yaml, do I need to merge it all into one patch?
Currently, my submitting order is mfd, backlight, and regulator. Each part divided into two patches (one for source code, another for dt_binding_document)

Doesn't have to be 1 patch, but should be one series with MFD coming last as it references the others. Example goes in the MFD binding. I need to see a complete picture for what the device is to effectively review the binding.

Rob

ChiYuan Huang

author

2020-11-03 05:48 UTC #6 lore raw

Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core

Rob Herring <[email protected]> 於 2020年11月3日週二上午9:58寫道：

Quoted 51 lines · ChiYuan Huang

Quoted 40 lines
Rob Herring <[email protected]> 於 2020年11月3日週二上午1:21寫道：
Quoted 38 lines · cy_huang
Quoted 4 lines
From: ChiYuan Huang <[email protected]>
Adds DT binding document for Richtek RT4831 MFD core.
Quoted 8 lines
Signed-off-by ChiYuan Huang <[email protected]>
---
 .../devicetree/bindings/mfd/richtek,rt4831.yaml    | 89 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++
 2 files changed, 112 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Unknown file referenced: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/dist-packages/dtschema/schemas/regulator/richtek,rt4831-regulator.yaml'
xargs: dt-doc-validate: exited with status 255; aborting
make[1]: *** [Documentation/devicetree/bindings/Makefile:59: Documentation/devicetree/bindings/processed-schema-examples.json] Error 124
make: *** [Makefile:1364: dt_binding_check] Error 2
See https://patchwork.ozlabs.org/patch/1391911
The base for the patch is generally the last rc1. Any dependencies should be noted.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Quoted 8 lines
Quoted 2 lines
Please check and re-submit.
Sorry, I have one question.
If the richtek,rt4831.yaml is depend upon the other yaml, do I need to merge it all into one patch?
Currently, my submitting order is mfd, backlight, and regulator. Each part divided into two patches (one for source code, another for dt_binding_document)

Quoted 5 lines

Doesn't have to be 1 patch, but should be one series with MFD coming last as it references the others. Example goes in the MFD binding. I need to see a complete picture for what the device is to effectively review the binding.

Got it. Next, I'll add the regulator and backlight dt-binding into the series patch.
And add the description into mfd core dt-binding like as below.

This patch depends on
"backlight: rt4831: Adds DT binding document for Richtek RT4831
backlight module".
"regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV module".

Quoted 1 line

Rob

ChiYuan Huang

author

2020-11-06 16:29 UTC #7 lore raw

In reply to cy_huang

HI, Lee:
Any advice about this rt4831 mfd patch?

cy_huang <[email protected]> 於 2020年11月2日週一上午11:13寫道：

Quoted 177 lines

From: ChiYuan Huang <[email protected]>

Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.

Signed-off-by ChiYuan Huang <[email protected]>

---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c

drivers/mfd/Kconfig +11 -0

index 8b99a13..a22f002 100644
@@ -1088,6 +1088,17 @@ config MFD_RDC321X
          southbridge which provides access to GPIOs and Watchdog using the
          southbridge PCI device configuration space.
 
+config MFD_RT4831
+       tristate "Richtek RT4831 WLED and DSV IC"
+       depends on I2C
+       select MFD_CORE
+       select REGMAP_I2C
+       help
+         This enables support for the Richtek RT4831.
+         RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+         It's common used to provide the display power and to drive the
+         display backlight WLED.
+
 config MFD_RT5033
        tristate "Richtek RT5033 Power Management IC"
        depends on I2C

drivers/mfd/Makefile +1 -0

index 1780019..4108141 100644
@@ -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

drivers/mfd/rt4831-core.c new file +119 -1

new file mode 100644
index 00000000..5342959
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#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 chip 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 init regmap\n");
+               return PTR_ERR(regmap);
+       }
+
+       ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
+       if (ret) {
+               dev_err(&client->dev, "Fail to get version id\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);
+
+       /* Make sure all off before module removal */
+       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);
+
+       /* Make sure all off before machine shutdown */
+       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 <[email protected]>");
+MODULE_LICENSE("GPL v2");
--

2.7.4

Lee Jones

2020-11-25 16:42 UTC #8 lore raw

In reply to cy_huang

Quoted 19 lines · cy_huang

From: ChiYuan Huang <[email protected]>
Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.
Signed-off-by ChiYuan Huang <[email protected]>
---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c
drivers/mfd/Kconfig +0 -0
index 8b99a13..a22f002 100644

drivers/mfd/Kconfig config MFD_RDC321X · line 1092

Quoted 5 lines

drivers/mfd/Kconfig +2 -0

@@ -1088,6 +1088,17 @@ config MFD_RDC321X
 	  southbridge which provides access to GPIOs and Watchdog using the
 	  southbridge PCI device configuration space.
 
+config MFD_RT4831
+	tristate "Richtek RT4831 WLED and DSV IC"

Please expand on WLED and DSV.

This is documentation and should leave nothing to the imagination.

drivers/mfd/Kconfig config MFD_RDC321X · line 1100

Quoted 8 lines

drivers/mfd/Kconfig +8 -0

+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This enables support for the Richtek RT4831.
+	  RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+	  It's common used to provide the display power and to drive the
+	  display backlight WLED.

Please don't line-wrap unnecessarily.

Please re-work the last sentence, as it doesn't quite make sense.

Quoted 21 lines

drivers/mfd/Kconfig +0 -0

 config MFD_RT5033
 	tristate "Richtek RT5033 Power Management IC"
 	depends on I2C

drivers/mfd/Makefile +1 -0

index 1780019..4108141 100644
@@ -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

drivers/mfd/rt4831-core.c new file +0 -0

new file mode 100644
index 00000000..5342959

drivers/mfd/rt4831-core.c line 1

Quoted 1 line

drivers/mfd/rt4831-core.c new file +1 -0
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+

No copyright?

Quoted 37 lines

drivers/mfd/rt4831-core.c new file +37 -0

+#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,
+};
+

drivers/mfd/rt4831-core.c line 49

Quoted 10 lines

drivers/mfd/rt4831-core.c new file +10 -0

+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 chip enable gpio\n");

"Failed to get 'enable' GPIO chip"

drivers/mfd/rt4831-core.c line 55

Quoted 6 lines

drivers/mfd/rt4831-core.c new file +6 -0

+		return PTR_ERR(enable);
+	}
+
+	regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "Failed to init regmap\n");

"initialise"

drivers/mfd/rt4831-core.c line 61

Quoted 6 lines

drivers/mfd/rt4831-core.c new file +6 -0

+		return PTR_ERR(regmap);
+	}
+
+	ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
+	if (ret) {
+		dev_err(&client->dev, "Fail to get version id\n");

"Failed to get H/W revision"

Quoted 18 lines

drivers/mfd/rt4831-core.c new file +18 -0

+		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;
+	}

drivers/mfd/rt4831-core.c line 89

Quoted 10 lines

drivers/mfd/rt4831-core.c new file +10 -0

+
+	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);
+
+	/* Make sure all off before module removal */

"Disable all <thing your disabling> are disabled before ..."

drivers/mfd/rt4831-core.c line 97

Quoted 8 lines

drivers/mfd/rt4831-core.c new file +8 -0

+	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);
+
+	/* Make sure all off before machine shutdown */

As above.

Quoted 22 lines

drivers/mfd/rt4831-core.c new file +22 -0

+	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 <[email protected]>");
+MODULE_LICENSE("GPL v2");

Signature · 4 lines

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

ChiYuan Huang

author

2020-12-01 16:36 UTC #9 lore raw

Lee Jones <[email protected]> 於 2020年11月26日週四上午12:42寫道：

Quoted 25 lines · cy_huang

Quoted 23 lines
From: ChiYuan Huang <[email protected]>
Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.
Signed-off-by ChiYuan Huang <[email protected]>
---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c
drivers/mfd/Kconfig +0 -0
index 8b99a13..a22f002 100644
@@ -1088,6 +1088,17 @@ config MFD_RDC321X
        southbridge which provides access to GPIOs and Watchdog using the
        southbridge PCI device configuration space.
 

Quoted 7 lines

drivers/mfd/Kconfig config MFD_RDC321X · line 1092
Quoted 2 lines
drivers/mfd/Kconfig +2 -0
+config MFD_RT4831
+     tristate "Richtek RT4831 WLED and DSV IC"
Please expand on WLED and DSV.
This is documentation and should leave nothing to the imagination.

Rewrite to "Richtek RT4831 four channel WLED and display bias voltage", is it okay?

Quoted 9 lines

drivers/mfd/Kconfig config MFD_RDC321X · line 1100

Quoted 8 lines

drivers/mfd/Kconfig +8 -0

+     depends on I2C
+     select MFD_CORE
+     select REGMAP_I2C
+     help
+       This enables support for the Richtek RT4831.
+       RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+       It's common used to provide the display power and to drive the
+       display backlight WLED.

Quoted 4 lines

Please don't line-wrap unnecessarily.
Please re-work the last sentence, as it doesn't quite make sense.

Rewrite the whole sentence like as below
"This enables support for the Richtek RT4831. It includes 4 channel WLED driving and Display Bias voltage output. It's commonly used to provide the LCD power and to drive LCD backlight."

Quoted 15 lines

drivers/mfd/Kconfig +0 -0

 config MFD_RT5033
      tristate "Richtek RT5033 Power Management IC"
      depends on I2C

drivers/mfd/Makefile +1 -0

index 1780019..4108141 100644
@@ -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

Quoted 10 lines

drivers/mfd/rt4831-core.c line 1
Quoted 7 lines
drivers/mfd/rt4831-core.c new file +1 -0
new file mode 100644
index 00000000..5342959
@@ -0,0 +1,119 @@
+// SPDX-License-Identifier: GPL-2.0+
No copyright?

Yes, I'll add the copyright like as below
/*
* Copyright (c) 2020 Richtek Inc.
*
* Author: ChiYuan Huang <[email protected]>
*/

Quoted 41 lines

drivers/mfd/rt4831-core.c new file +41 -0

+#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;

Quoted 9 lines

drivers/mfd/rt4831-core.c line 49

Quoted 6 lines

drivers/mfd/rt4831-core.c new file +6 -0

+     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 chip enable gpio\n");

"Failed to get 'enable' GPIO chip"

May I remove "chip" word? It seems redundant. "Failed to get 'enable' GPIO" is better. Because 'enable' is a physical input pin for RT4831.

Quoted 9 lines

drivers/mfd/rt4831-core.c line 55

Quoted 6 lines

drivers/mfd/rt4831-core.c new file +6 -0

+             return PTR_ERR(enable);
+     }
+
+     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
+     if (IS_ERR(regmap)) {
+             dev_err(&client->dev, "Failed to init regmap\n");

"initialise"

Change to verb "initialize"

Quoted 9 lines

drivers/mfd/rt4831-core.c line 61

Quoted 6 lines

drivers/mfd/rt4831-core.c new file +6 -0

+             return PTR_ERR(regmap);
+     }
+
+     ret = regmap_read(regmap, RT4831_REG_REVISION, &val);
+     if (ret) {
+             dev_err(&client->dev, "Fail to get version id\n");

"Failed to get H/W revision"

Ack

Quoted 22 lines

drivers/mfd/rt4831-core.c new file +22 -0

+             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);
+}

Quoted 9 lines

drivers/mfd/rt4831-core.c line 89
Quoted 6 lines
drivers/mfd/rt4831-core.c new file +6 -0
+
+static int rt4831_remove(struct i2c_client *client)
+{
+     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+     /* Make sure all off before module removal */
"Disable all <thing your disabling> are disabled before ..."

May I rewrite it to "Configure WLED driving and DSV output all to be disabled before MFD module removal"?

Quoted 11 lines

drivers/mfd/rt4831-core.c line 97

Quoted 8 lines

drivers/mfd/rt4831-core.c new file +8 -0

+     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);
+
+     /* Make sure all off before machine shutdown */

As above.

like as above ".... before 'machine shutdown'

Quoted 28 lines

Quoted 12 lines

drivers/mfd/rt4831-core.c new file +12 -0

+     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",

drivers/mfd/rt4831-core.c line 119

Quoted 10 lines

drivers/mfd/rt4831-core.c new file +10 -0

+             .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 <[email protected]>");
+MODULE_LICENSE("GPL v2");

--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services

Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Lee Jones

2020-12-02 08:50 UTC #10 lore raw

Quoted 31 lines · ChiYuan Huang

Lee Jones <[email protected]> 於 2020年11月26日週四上午12:42寫道：
Quoted 30 lines · cy_huang
Quoted 20 lines
From: ChiYuan Huang <[email protected]>
Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.
Signed-off-by ChiYuan Huang <[email protected]>
---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c
drivers/mfd/Kconfig +0 -0
index 8b99a13..a22f002 100644
drivers/mfd/Kconfig config MFD_RDC321X · line 1092
Quoted 5 lines
drivers/mfd/Kconfig +2 -0
@@ -1088,6 +1088,17 @@ config MFD_RDC321X
        southbridge which provides access to GPIOs and Watchdog using the
        southbridge PCI device configuration space.
 
+config MFD_RT4831
+     tristate "Richtek RT4831 WLED and DSV IC"
Please expand on WLED and DSV.

Quoted 4 lines

Quoted 2 lines
This is documentation and should leave nothing to the imagination.
Rewrite to "Richtek RT4831 four channel WLED and display bias voltage", is it okay?

I had to look-up WLED, but I guess it's okay.

"Display Bias Voltage"

Quoted 9 lines

Quoted 8 lines

drivers/mfd/Kconfig +8 -0

+     depends on I2C
+     select MFD_CORE
+     select REGMAP_I2C
+     help
+       This enables support for the Richtek RT4831.
+       RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+       It's common used to provide the display power and to drive the
+       display backlight WLED.

Quoted 8 lines

Quoted 4 lines
Please don't line-wrap unnecessarily.
Please re-work the last sentence, as it doesn't quite make sense.
Rewrite the whole sentence like as below
"This enables support for the Richtek RT4831. It includes 4 channel WLED driving and Display Bias voltage output. It's commonly used to provide the LCD power and to drive LCD backlight."

"Display Bias Voltage"

"provide power to the LCD display"

[...]

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
drivers/mfd/Kconfig +4 -0
+static int rt4831_probe(struct i2c_client *client)
+{
+     struct gpio_desc *enable;
+     struct regmap *regmap;
Quoted 9 lines
drivers/mfd/Kconfig config MFD_RDC321X
Quoted 6 lines
drivers/mfd/Kconfig +6 -0
+     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 chip enable gpio\n");
"Failed to get 'enable' GPIO chip"

Quoted 3 lines

May I remove "chip" word? It seems redundant. "Failed to get 'enable' GPIO" is better. Because 'enable' is a physical input pin for RT4831.

Sounds good.

[...]

Quoted 7 lines

Quoted 7 lines
drivers/mfd/Kconfig
Quoted 5 lines
drivers/mfd/Kconfig +5 -0
+static int rt4831_remove(struct i2c_client *client)
+{
+     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+     /* Make sure all off before module removal */
"Disable all <thing your disabling> are disabled before ..."

This should have said:

"Ensure all <thing your disabling> are disabled before ..."

Quoted 2 lines

May I rewrite it to "Configure WLED driving and DSV output all to be disabled before MFD module removal"?

You don't need to mention MFD or modules here since we know how the Device Driver model works and what .remove() does.

What about:

"Disable WLED and DSV outputs"

Quoted 11 lines

drivers/mfd/Kconfig

Quoted 8 lines

drivers/mfd/Kconfig +8 -0

+     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);
+
+     /* Make sure all off before machine shutdown */

As above.

Quoted 1 line

like as above ".... before 'machine shutdown'

"Disable WLED and DSV outputs"

Signature · 4 lines

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

ChiYuan Huang

author

2020-12-02 15:47 UTC #11 lore raw

Lee Jones <[email protected]> 於 2020年12月2日週三下午4:49寫道：

Quoted 34 lines · ChiYuan Huang

Quoted 32 lines
Lee Jones <[email protected]> 於 2020年11月26日週四上午12:42寫道：
Quoted 30 lines · cy_huang
Quoted 20 lines
From: ChiYuan Huang <[email protected]>
Adds support Richtek RT4831 MFD core.
RT4831 includes backlight and DSV part that can provode display panel for postive and negative voltage and WLED driving.
Signed-off-by ChiYuan Huang <[email protected]>
---
 drivers/mfd/Kconfig       |  11 +++++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/rt4831-core.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+)
 create mode 100644 drivers/mfd/rt4831-core.c
drivers/mfd/Kconfig +0 -0
index 8b99a13..a22f002 100644
drivers/mfd/Kconfig config MFD_RDC321X · line 1092
Quoted 5 lines
drivers/mfd/Kconfig +2 -0
@@ -1088,6 +1088,17 @@ config MFD_RDC321X
        southbridge which provides access to GPIOs and Watchdog using the
        southbridge PCI device configuration space.
 
+config MFD_RT4831
+     tristate "Richtek RT4831 WLED and DSV IC"
Please expand on WLED and DSV.

Quoted 9 lines

Quoted 4 lines
Quoted 2 lines
This is documentation and should leave nothing to the imagination.
Rewrite to "Richtek RT4831 four channel WLED and display bias voltage", is it okay?
I had to look-up WLED, but I guess it's okay.
"Display Bias Voltage"

OK, I'll change this line to
"Richtek RT4831 fource channel WLED and Display Bias Voltage"

Quoted 13 lines

Quoted 13 lines
Quoted 13 lines
drivers/mfd/Kconfig config MFD_RDC321X · line 1100
Quoted 8 lines
drivers/mfd/Kconfig +8 -0
+     depends on I2C
+     select MFD_CORE
+     select REGMAP_I2C
+     help
+       This enables support for the Richtek RT4831.
+       RT4831 includes WLED driver and DisplayBias voltage(+/-) regulator.
+       It's common used to provide the display power and to drive the
+       display backlight WLED.
Please don't line-wrap unnecessarily.
Please re-work the last sentence, as it doesn't quite make sense.

Quoted 9 lines

Quoted 4 lines
Rewrite the whole sentence like as below
"This enables support for the Richtek RT4831. It includes 4 channel WLED driving and Display Bias voltage output. It's commonly used to provide the LCD power and to drive LCD backlight."
"Display Bias Voltage"
"provide power to the LCD display"

Thanks. looks better

Quoted 38 lines

[...]
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/mfd/Kconfig +4 -0
+static int rt4831_probe(struct i2c_client *client)
+{
+     struct gpio_desc *enable;
+     struct regmap *regmap;
Quoted 9 lines
drivers/mfd/Kconfig
Quoted 6 lines
drivers/mfd/Kconfig +6 -0
+     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 chip enable gpio\n");
"Failed to get 'enable' GPIO chip"
Quoted 3 lines
May I remove "chip" word? It seems redundant. "Failed to get 'enable' GPIO" is better. Because 'enable' is a physical input pin for RT4831.
Sounds good.
[...]
Quoted 7 lines
Quoted 7 lines
drivers/mfd/Kconfig
Quoted 5 lines
drivers/mfd/Kconfig +5 -0
+static int rt4831_remove(struct i2c_client *client)
+{
+     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+     /* Make sure all off before module removal */
"Disable all <thing your disabling> are disabled before ..."
This should have said:
"Ensure all <thing your disabling> are disabled before ..."
Quoted 2 lines
May I rewrite it to "Configure WLED driving and DSV output all to be disabled before MFD module removal"?

Quoted 6 lines

You don't need to mention MFD or modules here since we know how the Device Driver model works and what .remove() does.
What about:
"Disable WLED and DSV outputs"

Do you mean that only keep this comment line is better? NO more redundant description like "before ....".
If yes, I'll only leave the comment like as you said in remove/shutdown ops.

Quoted 11 lines

drivers/mfd/Kconfig

Quoted 8 lines

drivers/mfd/Kconfig +8 -0

+     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);
+
+     /* Make sure all off before machine shutdown */

As above.

Quoted 3 lines

Quoted 1 line
like as above ".... before 'machine shutdown'
"Disable WLED and DSV outputs"

Same as above.

Thanks for all the suggestion.
If any misunderstanding, please kindly let me know.

Quoted 6 lines

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