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

From: ChiYuan Huang <cy_huang@richtek.com>

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 <cy_huang@richtek.com>
---
 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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b99a13..a22f002 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -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
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..5342959
--- /dev/null
+++ b/drivers/mfd/rt4831-core.c
@@ -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 <cy_huang@richtek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


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

* [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-02  3:13 [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
@ 2020-11-02  3:13 ` cy_huang
  2020-11-02  5:53   ` ChiYuan Huang
  2020-11-02 17:21   ` Rob Herring
  2020-11-06 16:29 ` [PATCH v1 1/2] mfd: rt4831: Adds support " ChiYuan Huang
  2020-11-25 16:42 ` Lee Jones
  2 siblings, 2 replies; 12+ messages in thread
From: cy_huang @ 2020-11-02  3:13 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>
---
 .../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

diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
new file mode 100644
index 00000000..c602d50
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
@@ -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 <cy_huang@richtek.com>
+
+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>;
+        };
+      };
+    };
diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
new file mode 100644
index 00000000..7084906
--- /dev/null
+++ b/include/dt-bindings/leds/rt4831-backlight.h
@@ -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 */
-- 
2.7.4


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

* Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-02  3:13 ` [PATCH v1 2/2] mfd: rt4505: Adds DT binding document " cy_huang
@ 2020-11-02  5:53   ` ChiYuan Huang
  2020-11-02 17:21   ` Rob Herring
  1 sibling, 0 replies; 12+ messages in thread
From: ChiYuan Huang @ 2020-11-02  5:53 UTC (permalink / raw)
  To: Lee Jones, robh+dt; +Cc: cy_huang, lkml, devicetree

Hi,
  I seems I typo the wrong comment headline, not RT4505. It's RT4831.
Please just review the contents, I'll fix it in next series patch.


cy_huang <u0084500@gmail.com> 於 2020年11月2日 週一 上午11:13寫道:
>
> 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>
> ---
>  .../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
>
> diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> new file mode 100644
> index 00000000..c602d50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
> @@ -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 <cy_huang@richtek.com>
> +
> +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>;
> +        };
> +      };
> +    };
> diff --git a/include/dt-bindings/leds/rt4831-backlight.h b/include/dt-bindings/leds/rt4831-backlight.h
> new file mode 100644
> index 00000000..7084906
> --- /dev/null
> +++ b/include/dt-bindings/leds/rt4831-backlight.h
> @@ -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 */
> --
> 2.7.4
>

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

* Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-02  3:13 ` [PATCH v1 2/2] mfd: rt4505: Adds DT binding document " cy_huang
  2020-11-02  5:53   ` ChiYuan Huang
@ 2020-11-02 17:21   ` Rob Herring
  2020-11-03  1:14     ` ChiYuan Huang
  1 sibling, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-11-02 17:21 UTC (permalink / raw)
  To: cy_huang; +Cc: devicetree, robh+dt, cy_huang, linux-kernel, lee.jones

On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote:
> 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>
> ---
>  .../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.


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

* Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-02 17:21   ` Rob Herring
@ 2020-11-03  1:14     ` ChiYuan Huang
  2020-11-03  1:58       ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: ChiYuan Huang @ 2020-11-03  1:14 UTC (permalink / raw)
  To: Rob Herring; +Cc: devicetree, robh+dt, cy_huang, lkml, Lee Jones

Rob Herring <robh@kernel.org> 於 2020年11月3日 週二 上午1:21寫道:
>
> On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote:
> > 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>
> > ---
> >  .../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.
>
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.

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

* Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-03  1:14     ` ChiYuan Huang
@ 2020-11-03  1:58       ` Rob Herring
  2020-11-03  5:48         ` ChiYuan Huang
  0 siblings, 1 reply; 12+ messages in thread
From: Rob Herring @ 2020-11-03  1:58 UTC (permalink / raw)
  To: ChiYuan Huang; +Cc: devicetree, cy_huang, lkml, Lee Jones

On Mon, Nov 2, 2020 at 7:14 PM ChiYuan Huang <u0084500@gmail.com> wrote:
>
> Rob Herring <robh@kernel.org> 於 2020年11月3日 週二 上午1:21寫道:
> >
> > On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote:
> > > 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>
> > > ---
> > >  .../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.
> >
> 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

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

* Re: [PATCH v1 2/2] mfd: rt4505: Adds DT binding document for Richtek RT4831 MFD core
  2020-11-03  1:58       ` Rob Herring
@ 2020-11-03  5:48         ` ChiYuan Huang
  0 siblings, 0 replies; 12+ messages in thread
From: ChiYuan Huang @ 2020-11-03  5:48 UTC (permalink / raw)
  To: Rob Herring; +Cc: devicetree, cy_huang, lkml, Lee Jones

Rob Herring <robh@kernel.org> 於 2020年11月3日 週二 上午9:58寫道:
>
> On Mon, Nov 2, 2020 at 7:14 PM ChiYuan Huang <u0084500@gmail.com> wrote:
> >
> > Rob Herring <robh@kernel.org> 於 2020年11月3日 週二 上午1:21寫道:
> > >
> > > On Mon, 02 Nov 2020 11:13:23 +0800, cy_huang wrote:
> > > > 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>
> > > > ---
> > > >  .../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.
> > >
> > 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.
>
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".


> Rob

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

* Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-11-02  3:13 [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
  2020-11-02  3:13 ` [PATCH v1 2/2] mfd: rt4505: Adds DT binding document " cy_huang
@ 2020-11-06 16:29 ` ChiYuan Huang
  2020-11-25 16:42 ` Lee Jones
  2 siblings, 0 replies; 12+ messages in thread
From: ChiYuan Huang @ 2020-11-06 16:29 UTC (permalink / raw)
  To: Lee Jones, robh+dt; +Cc: cy_huang, lkml, devicetree

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

cy_huang <u0084500@gmail.com> 於 2020年11月2日 週一 上午11:13寫道:
>
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> 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 <cy_huang@richtek.com>
> ---
>  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
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13..a22f002 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -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
> 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..5342959
> --- /dev/null
> +++ b/drivers/mfd/rt4831-core.c
> @@ -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 <cy_huang@richtek.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

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

* Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-11-02  3:13 [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
  2020-11-02  3:13 ` [PATCH v1 2/2] mfd: rt4505: Adds DT binding document " cy_huang
  2020-11-06 16:29 ` [PATCH v1 1/2] mfd: rt4831: Adds support " ChiYuan Huang
@ 2020-11-25 16:42 ` Lee Jones
  2020-12-01 16:35   ` ChiYuan Huang
  2 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2020-11-25 16:42 UTC (permalink / raw)
  To: cy_huang; +Cc: robh+dt, cy_huang, linux-kernel, devicetree

On Mon, 02 Nov 2020, cy_huang wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> 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 <cy_huang@richtek.com>
> ---
>  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
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13..a22f002 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -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.

> +	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.

>  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..5342959
> --- /dev/null
> +++ b/drivers/mfd/rt4831-core.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0+

No copyright?

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

"Failed to get 'enable' GPIO chip"

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

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

> +		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 */

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

> +	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.

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

-- 
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] 12+ messages in thread

* Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-11-25 16:42 ` Lee Jones
@ 2020-12-01 16:35   ` ChiYuan Huang
  2020-12-02  8:49     ` Lee Jones
  0 siblings, 1 reply; 12+ messages in thread
From: ChiYuan Huang @ 2020-12-01 16:35 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年11月26日 週四 上午12:42寫道:
>
> On Mon, 02 Nov 2020, cy_huang wrote:
>
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > 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 <cy_huang@richtek.com>
> > ---
> >  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
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..a22f002 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -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.
>
Rewrite to "Richtek RT4831 four channel WLED and display bias
voltage", is it okay?
> > +     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.
>
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."

> >  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..5342959
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831-core.c
> > @@ -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 <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 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.
> > +             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"
> > +             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
> > +             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 */
>
> "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"?
> > +     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'
> > +     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");
>
> --
> 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] 12+ messages in thread

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

On Wed, 02 Dec 2020, ChiYuan Huang wrote:

> Lee Jones <lee.jones@linaro.org> 於 2020年11月26日 週四 上午12:42寫道:
> >
> > On Mon, 02 Nov 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > 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 <cy_huang@richtek.com>
> > > ---
> > >  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
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..a22f002 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -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.
> >
> 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"

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

[...]

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

[...]

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

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

> > > +     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'

  "Disable WLED and DSV outputs"

-- 
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] 12+ messages in thread

* Re: [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core
  2020-12-02  8:49     ` Lee Jones
@ 2020-12-02 15:45       ` ChiYuan Huang
  0 siblings, 0 replies; 12+ messages in thread
From: ChiYuan Huang @ 2020-12-02 15:45 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月2日 週三 下午4:49寫道:
>
> On Wed, 02 Dec 2020, ChiYuan Huang wrote:
>
> > Lee Jones <lee.jones@linaro.org> 於 2020年11月26日 週四 上午12:42寫道:
> > >
> > > On Mon, 02 Nov 2020, cy_huang wrote:
> > >
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > 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 <cy_huang@richtek.com>
> > > > ---
> > > >  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
> > > >
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index 8b99a13..a22f002 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -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.
> > >
> > 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"
> > > > +     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.
> > >
> > 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
> [...]
>
> > > > +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"
> > >
> > 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.
>
> [...]
>
> > > > +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 ..."
>
> > 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"
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.
> > > > +     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'
>
>   "Disable WLED and DSV outputs"
Same as above.

Thanks for all the suggestion.
If any misunderstanding, please kindly let me know.
>
> --
> 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] 12+ messages in thread

end of thread, other threads:[~2020-12-02 15:47 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02  3:13 [PATCH v1 1/2] mfd: rt4831: Adds support for Richtek RT4831 MFD core cy_huang
2020-11-02  3:13 ` [PATCH v1 2/2] mfd: rt4505: Adds DT binding document " cy_huang
2020-11-02  5:53   ` ChiYuan Huang
2020-11-02 17:21   ` Rob Herring
2020-11-03  1:14     ` ChiYuan Huang
2020-11-03  1:58       ` Rob Herring
2020-11-03  5:48         ` ChiYuan Huang
2020-11-06 16:29 ` [PATCH v1 1/2] mfd: rt4831: Adds support " ChiYuan Huang
2020-11-25 16:42 ` Lee Jones
2020-12-01 16:35   ` ChiYuan Huang
2020-12-02  8:49     ` Lee Jones
2020-12-02 15:45       ` ChiYuan Huang

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).