linux-fbdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
@ 2020-12-17 15:00 cy_huang
  2020-12-17 15:00 ` [PATCH v5 2/6] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
                   ` (6 more replies)
  0 siblings, 7 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

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

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
since v5
- Rename file name from rt4831-core.c to rt4831.c
- Change RICHTEK_VID to RICHTEK_VENDOR_ID.
- Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
- Change variable 'val' to the meaningful name 'chip_id'.
- Refine the error log when vendor id is not matched.
- Remove of_match_ptr.

since v2
- Refine Kconfig descriptions.
- Add copyright.
- Refine error logs in probe.
- Refine comment lines in remove and shutdown.
---
 drivers/mfd/Kconfig  |  10 +++++
 drivers/mfd/Makefile |   1 +
 drivers/mfd/rt4831.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 135 insertions(+)
 create mode 100644 drivers/mfd/rt4831.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b99a13..dfb2640 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1088,6 +1088,16 @@ config MFD_RDC321X
 	  southbridge which provides access to GPIOs and Watchdog using the
 	  southbridge PCI device configuration space.
 
+config MFD_RT4831
+	tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This enables support for the Richtek RT4831 that includes 4 channel
+	  WLED driving and Display Bias Voltage. It's commonly used to provide
+	  power to the LCD display and LCD backlight.
+
 config MFD_RT5033
 	tristate "Richtek RT5033 Power Management IC"
 	depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 1780019..28d247b 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.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
new file mode 100644
index 00000000..2bf8364
--- /dev/null
+++ b/drivers/mfd/rt4831.c
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020 Richtek Technology Corp.
+ *
+ * Author: ChiYuan Huang <cy_huang@richtek.com>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define RT4831_REG_REVISION	0x01
+#define RT4831_REG_ENABLE	0x08
+#define RT4831_REG_I2CPROT	0x15
+
+#define RICHTEK_VENDOR_ID	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_gpio;
+	struct regmap *regmap;
+	unsigned int chip_id;
+	int ret;
+
+	enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(enable_gpio)) {
+		dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
+		return PTR_ERR(enable_gpio);
+	}
+
+	regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "Failed to initialize regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	ret = regmap_read(regmap, RT4831_REG_REVISION, &chip_id);
+	if (ret) {
+		dev_err(&client->dev, "Failed to get H/W revision\n");
+		return ret;
+	}
+
+	if ((chip_id & RT4831_VID_MASK) != RICHTEK_VENDOR_ID) {
+		dev_err(&client->dev, "Chip vendor ID 0x%02x not matched\n", chip_id);
+		return -ENODEV;
+	}
+
+	/*
+	 * Used to prevent the abnormal shutdown.
+	 * If SCL/SDA both keep low for one second to reset HW.
+	 */
+	ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
+				 RT4831_I2CSAFETMR_MASK);
+	if (ret) {
+		dev_err(&client->dev, "Failed to enable I2C safety timer\n");
+		return ret;
+	}
+
+	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
+				    ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
+}
+
+static int rt4831_remove(struct i2c_client *client)
+{
+	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+	/* Disable WLED and DSV outputs */
+	return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
+}
+
+static void rt4831_shutdown(struct i2c_client *client)
+{
+	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
+
+	/* Disable WLED and DSV outputs */
+	regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
+}
+
+static const struct of_device_id __maybe_unused rt4831_of_match[] = {
+	{ .compatible = "richtek,rt4831", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rt4831_of_match);
+
+static struct i2c_driver rt4831_driver = {
+	.driver = {
+		.name = "rt4831",
+		.of_match_table = 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] 14+ messages in thread

* [PATCH v5 2/6] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
@ 2020-12-17 15:00 ` cy_huang
  2020-12-17 15:00 ` [PATCH v5 3/6] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator cy_huang
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

Adds DT binding document for Richtek RT4831 backlight.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
since v5
- Drop the example in dt-binding. Aready full example in mfd dt-binding.

since v3
- Move inlcude/dt-bindings/leds/rt4831-backlight.h from patch 0004 to here.
- Add dual license tag in header and backlight binding document.
- Left backlight dt-binding example only.
---
 .../leds/backlight/richtek,rt4831-backlight.yaml   | 65 ++++++++++++++++++++++
 include/dt-bindings/leds/rt4831-backlight.h        | 23 ++++++++
 2 files changed, 88 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/richtek,rt4831-backlight.yaml
 create mode 100644 include/dt-bindings/leds/rt4831-backlight.h

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


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

* [PATCH v5 3/6] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
  2020-12-17 15:00 ` [PATCH v5 2/6] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
@ 2020-12-17 15:00 ` cy_huang
  2020-12-17 15:00 ` [PATCH v5 4/6] mfd: rt4831: Adds DT binding document for Richtek RT4831 core cy_huang
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

Adds DT binding document for Richtek RT4831 DSV regulator.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
since v5
- Revert it back from v3 patch.
- Drop the example in dt-binding, Already full example in mfd dt-binding.
---
 .../regulator/richtek,rt4831-regulator.yaml        | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml

diff --git a/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml b/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
new file mode 100644
index 00000000..d9c2333
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/richtek,rt4831-regulator.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/regulator/richtek,rt4831-regulator.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT4831 Display Bias Voltage Regulator
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  RT4831 is a multifunctional device that can provide power to the LCD display
+  and LCD backlight.
+
+  For Display Bias Voltage DSVP and DSVN, the output range is about 4V to 6.5V.
+  It is sufficient to meet the current LCD power requirement.
+
+  DSVLCM is a boost regulator in IC internal as DSVP and DSVN input power.
+  Its voltage should be configured above 0.15V to 0.2V gap larger than the
+  voltage needed for DSVP and DSVN. Too much voltage gap could improve the
+  voltage drop from the heavy loading scenario. But it also make the power
+  efficiency worse. It's a trade-off.
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
+
+patternProperties:
+  "^DSV(LCM|P|N)$":
+    type: object
+    $ref: regulator.yaml#
+    description:
+      Properties for single Display Bias Voltage regulator.
+
+additionalProperties: false
-- 
2.7.4


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

* [PATCH v5 4/6] mfd: rt4831: Adds DT binding document for Richtek RT4831 core
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
  2020-12-17 15:00 ` [PATCH v5 2/6] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
  2020-12-17 15:00 ` [PATCH v5 3/6] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator cy_huang
@ 2020-12-17 15:00 ` cy_huang
  2020-12-17 15:00 ` [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight cy_huang
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

Adds DT binding document for Richtek RT4831 core.

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

since v5
- Revert mfd dt-binding to v3 patch.

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

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

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

diff --git a/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
new file mode 100644
index 00000000..4762eb1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/richtek,rt4831.yaml
@@ -0,0 +1,90 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/richtek,rt4831.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Richtek RT4831 DSV and Backlight Integrated IC
+
+maintainers:
+  - ChiYuan Huang <cy_huang@richtek.com>
+
+description: |
+  RT4831 is a multifunctional device that can provide power to the LCD display
+  and LCD backlight.
+
+  For Display Bias Voltage DSVP and DSVN, the output range is about 4V to 6.5V.
+  It's sufficient to meet the current LCD power requirement.
+
+  For the LCD backlight, it can provide four channel WLED driving capability.
+  Each channel driving current is up to 30mA
+
+  Datasheet is available at
+  https://www.richtek.com/assets/product_file/RT4831A/DS4831A-05.pdf
+
+properties:
+  compatible:
+    const: richtek,rt4831
+
+  reg:
+    description: I2C device address.
+    maxItems: 1
+
+  enable-gpios:
+    description: |
+      GPIO to enable/disable the chip. It is optional.
+      Some usage directly tied this pin to follow VIO 1.8V power on sequence.
+    maxItems: 1
+
+  regulators:
+    $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>;
+            regulator-allow-bypass;
+          };
+          DSVP {
+            regulator-name = "rt4831-dsvp";
+            regulator-min-microvolt = <4000000>;
+            regulator-max-microvolt = <6500000>;
+            regulator-boot-on;
+          };
+          DSVN {
+            regulator-name = "rt4831-dsvn";
+            regulator-min-microvolt = <4000000>;
+            regulator-max-microvolt = <6500000>;
+            regulator-boot-on;
+          };
+        };
+
+        backlight {
+          compatible = "richtek,rt4831-backlight";
+          default-brightness = <1024>;
+          max-brightness = <2048>;
+          richtek,bled-ovp-sel = /bits/ 8 <RT4831_BLOVPLVL_21V>;
+          richtek,channel-use = /bits/ 8 <RT4831_BLED_ALLCHEN>;
+        };
+      };
+    };
-- 
2.7.4


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

* [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
                   ` (2 preceding siblings ...)
  2020-12-17 15:00 ` [PATCH v5 4/6] mfd: rt4831: Adds DT binding document for Richtek RT4831 core cy_huang
@ 2020-12-17 15:00 ` cy_huang
  2021-03-25  8:22   ` ChiYuan Huang
  2021-03-25 11:53   ` Daniel Thompson
  2020-12-17 15:00 ` [PATCH v5 6/6] regulator: rt4831: Adds support for Richtek RT4831 DSV regulator cy_huang
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

Adds support for Richtek RT4831 backlight.

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/video/backlight/Kconfig            |   8 ++
 drivers/video/backlight/Makefile           |   1 +
 drivers/video/backlight/rt4831-backlight.c | 219 +++++++++++++++++++++++++++++
 3 files changed, 228 insertions(+)
 create mode 100644 drivers/video/backlight/rt4831-backlight.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index d83c87b..666bdb0 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
 	  If you have the Qualcomm PMIC, say Y to enable a driver for the
 	  WLED block. Currently it supports PM8941 and PMI8998.
 
+config BACKLIGHT_RT4831
+	tristate "Richtek RT4831 Backlight Driver"
+	depends on MFD_RT4831
+	help
+	  This enables support for Richtek RT4831 Backlight driver.
+	  It's commont used to drive the display WLED. There're four channels
+	  inisde, and each channel can provide up to 30mA current.
+
 config BACKLIGHT_SAHARA
 	tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
 	depends on X86
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 685f3f1..cae2c83 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_BACKLIGHT_PANDORA)		+= pandora_bl.o
 obj-$(CONFIG_BACKLIGHT_PCF50633)	+= pcf50633-backlight.o
 obj-$(CONFIG_BACKLIGHT_PWM)		+= pwm_bl.o
 obj-$(CONFIG_BACKLIGHT_QCOM_WLED)	+= qcom-wled.o
+obj-$(CONFIG_BACKLIGHT_RT4831)		+= rt4831-backlight.o
 obj-$(CONFIG_BACKLIGHT_SAHARA)		+= kb3886_bl.o
 obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
diff --git a/drivers/video/backlight/rt4831-backlight.c b/drivers/video/backlight/rt4831-backlight.c
new file mode 100644
index 00000000..816c4d6
--- /dev/null
+++ b/drivers/video/backlight/rt4831-backlight.c
@@ -0,0 +1,219 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <dt-bindings/leds/rt4831-backlight.h>
+#include <linux/backlight.h>
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+
+#define RT4831_REG_BLCFG	0x02
+#define RT4831_REG_BLDIML	0x04
+#define RT4831_REG_ENABLE	0x08
+
+#define BL_MAX_BRIGHTNESS	2048
+
+#define RT4831_BLOVP_MASK	GENMASK(7, 5)
+#define RT4831_BLOVP_SHIFT	5
+#define RT4831_BLPWMEN_MASK	BIT(0)
+#define RT4831_BLEN_MASK	BIT(4)
+#define RT4831_BLCH_MASK	GENMASK(3, 0)
+#define RT4831_BLDIML_MASK	GENMASK(2, 0)
+#define RT4831_BLDIMH_MASK	GENMASK(10, 3)
+#define RT4831_BLDIMH_SHIFT	3
+
+struct rt4831_priv {
+	struct regmap *regmap;
+	struct mutex lock;
+	struct backlight_device *bl;
+};
+
+static int rt4831_bl_update_status(struct backlight_device *bl_dev)
+{
+	struct rt4831_priv *priv = bl_get_data(bl_dev);
+	int brightness = backlight_get_brightness(bl_dev);
+	unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
+	u8 v[2];
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	if (brightness) {
+		v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
+		v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >> RT4831_BLDIMH_SHIFT;
+
+		ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
+		if (ret)
+			goto unlock;
+	}
+
+	ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLEN_MASK, enable);
+
+unlock:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
+{
+	struct rt4831_priv *priv = bl_get_data(bl_dev);
+	unsigned int val;
+	u8 v[2];
+	int ret;
+
+	mutex_lock(&priv->lock);
+
+	ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, &val);
+	if (ret)
+		return ret;
+
+	if (!(val & RT4831_BLEN_MASK)) {
+		ret = 0;
+		goto unlock;
+	}
+
+	ret = regmap_raw_read(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
+	if (ret)
+		goto unlock;
+
+	ret = (v[1] << RT4831_BLDIMH_SHIFT) + (v[0] & RT4831_BLDIML_MASK) + 1;
+
+unlock:
+	mutex_unlock(&priv->lock);
+	return ret;
+}
+
+static const struct backlight_ops rt4831_bl_ops = {
+	.options = BL_CORE_SUSPENDRESUME,
+	.update_status = rt4831_bl_update_status,
+	.get_brightness = rt4831_bl_get_brightness,
+};
+
+static int rt4831_init_device_properties(struct rt4831_priv *priv, struct device *dev,
+					  struct backlight_properties *bl_props)
+{
+	u8 propval;
+	u32 brightness;
+	unsigned int val = 0;
+	int ret;
+
+	/* common properties */
+	ret = device_property_read_u32(dev, "max-brightness", &brightness);
+	if (ret) {
+		dev_warn(dev, "max-brightness DT property missing, use HW max as default\n");
+		brightness = BL_MAX_BRIGHTNESS;
+	}
+
+	bl_props->max_brightness = min_t(u32, brightness, BL_MAX_BRIGHTNESS);
+
+	ret = device_property_read_u32(dev, "default-brightness", &brightness);
+	if (ret) {
+		dev_warn(dev, "default-brightness DT property missing, use max limit as default\n");
+		brightness = bl_props->max_brightness;
+	}
+
+	bl_props->brightness = min_t(u32, brightness, bl_props->max_brightness);
+
+	/* vendor properties */
+	if (device_property_read_bool(dev, "richtek,pwm-enable"))
+		val = RT4831_BLPWMEN_MASK;
+
+	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLPWMEN_MASK, val);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u8(dev, "richtek,bled-ovp-sel", &propval);
+	if (ret) {
+		dev_warn(dev, "richtek,bled-ovp-sel DT property missing, use default 21V\n");
+		propval = RT4831_BLOVPLVL_21V;
+	}
+
+	propval = min_t(u8, propval, RT4831_BLOVPLVL_29V);
+	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLOVP_MASK,
+				 propval << RT4831_BLOVP_SHIFT);
+	if (ret)
+		return ret;
+
+	ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
+	if (ret) {
+		dev_err(dev, "richtek,channel-use DT property missing\n");
+		return ret;
+	}
+
+	if (!(propval & RT4831_BLCH_MASK)) {
+		dev_err(dev, "No channel specified\n");
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLCH_MASK, propval);
+}
+
+static int rt4831_bl_probe(struct platform_device *pdev)
+{
+	struct rt4831_priv *priv;
+	struct backlight_properties bl_props = { .type = BACKLIGHT_RAW, };
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	mutex_init(&priv->lock);
+
+	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (IS_ERR(priv->regmap)) {
+		dev_err(&pdev->dev, "Failed to init regmap\n");
+		return PTR_ERR(priv->regmap);
+	}
+
+	ret = rt4831_init_device_properties(priv, &pdev->dev, &bl_props);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to init device properties\n");
+		return ret;
+	}
+
+	priv->bl = devm_backlight_device_register(&pdev->dev, pdev->name, &pdev->dev, priv,
+						  &rt4831_bl_ops, &bl_props);
+	if (IS_ERR(priv->bl)) {
+		dev_err(&pdev->dev, "Failed to register backlight\n");
+		return PTR_ERR(priv->bl);
+	}
+
+	backlight_update_status(priv->bl);
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int rt4831_bl_remove(struct platform_device *pdev)
+{
+	struct rt4831_priv *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl_dev = priv->bl;
+
+	bl_dev->props.brightness = 0;
+	backlight_update_status(priv->bl);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused rt4831_bl_of_match[] = {
+	{ .compatible = "richtek,rt4831-backlight", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rt4831_bl_of_match);
+
+static struct platform_driver rt4831_bl_driver = {
+	.driver = {
+		.name = "rt4831-backlight",
+		.of_match_table = rt4831_bl_of_match,
+	},
+	.probe = rt4831_bl_probe,
+	.remove = rt4831_bl_remove,
+};
+module_platform_driver(rt4831_bl_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


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

* [PATCH v5 6/6] regulator: rt4831: Adds support for Richtek RT4831 DSV regulator
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
                   ` (3 preceding siblings ...)
  2020-12-17 15:00 ` [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight cy_huang
@ 2020-12-17 15:00 ` cy_huang
  2020-12-28 16:13 ` (subset) [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core Mark Brown
  2021-01-13 12:21 ` Lee Jones
  6 siblings, 0 replies; 14+ messages in thread
From: cy_huang @ 2020-12-17 15:00 UTC (permalink / raw)
  To: lee.jones, lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie
  Cc: dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

From: ChiYuan Huang <cy_huang@richtek.com>

Adds support for Richtek RT4831 DSV Regulator

Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
---
 drivers/regulator/Kconfig            |  10 ++
 drivers/regulator/Makefile           |   1 +
 drivers/regulator/rt4831-regulator.c | 198 +++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/regulator/rt4831-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 020a00d..3e875ad 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -931,6 +931,16 @@ config REGULATOR_RT4801
 	  This adds support for voltage regulators in Richtek RT4801 Display Bias IC.
 	  The device supports two regulators (DSVP/DSVN).
 
+config REGULATOR_RT4831
+	tristate "Richtek RT4831 DSV Regulators"
+	depends on MFD_RT4831
+	help
+	  This adds support for voltage regulators in Richtek RT4831.
+	  There are three regulators (VLCM/DSVP/DSVN).
+	  VLCM is a virtual voltage input for DSVP/DSVN inside IC.
+	  And DSVP/DSVN is the real Vout range from 4V to 6.5V.
+	  It's common used to provide the power for the display panel.
+
 config REGULATOR_RT5033
 	tristate "Richtek RT5033 Regulators"
 	depends on MFD_RT5033
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6ebae51..eb587d4 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -115,6 +115,7 @@ obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
 obj-$(CONFIG_REGULATOR_RN5T618) += rn5t618-regulator.o
 obj-$(CONFIG_REGULATOR_ROHM)	+= rohm-regulator.o
 obj-$(CONFIG_REGULATOR_RT4801)	+= rt4801-regulator.o
+obj-$(CONFIG_REGULATOR_RT4831)	+= rt4831-regulator.o
 obj-$(CONFIG_REGULATOR_RT5033)	+= rt5033-regulator.o
 obj-$(CONFIG_REGULATOR_RTMV20)	+= rtmv20-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPA01) += s2mpa01.o
diff --git a/drivers/regulator/rt4831-regulator.c b/drivers/regulator/rt4831-regulator.c
new file mode 100644
index 00000000..3d4695d
--- /dev/null
+++ b/drivers/regulator/rt4831-regulator.c
@@ -0,0 +1,198 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/regulator/driver.h>
+
+enum {
+	DSV_OUT_VLCM = 0,
+	DSV_OUT_VPOS,
+	DSV_OUT_VNEG,
+	DSV_OUT_MAX
+};
+
+#define RT4831_REG_DSVEN	0x09
+#define RT4831_REG_VLCM		0x0c
+#define RT4831_REG_VPOS		0x0d
+#define RT4831_REG_VNEG		0x0e
+#define RT4831_REG_FLAGS	0x0f
+
+#define RT4831_VOLT_MASK	GENMASK(5, 0)
+#define RT4831_DSVMODE_SHIFT	5
+#define RT4831_DSVMODE_MASK	GENMASK(7, 5)
+#define RT4831_POSADEN_MASK	BIT(4)
+#define RT4831_NEGADEN_MASK	BIT(3)
+#define RT4831_POSEN_MASK	BIT(2)
+#define RT4831_NEGEN_MASK	BIT(1)
+
+#define RT4831_OTP_MASK		BIT(6)
+#define RT4831_LCMOVP_MASK	BIT(5)
+#define RT4831_VPOSSCP_MASK	BIT(3)
+#define RT4831_VNEGSCP_MASK	BIT(2)
+
+#define DSV_MODE_NORMAL		(0x4 << RT4831_DSVMODE_SHIFT)
+#define DSV_MODE_BYPASS		(0x6 << RT4831_DSVMODE_SHIFT)
+#define STEP_UV			50000
+#define VLCM_MIN_UV		4000000
+#define VLCM_MAX_UV		7150000
+#define VLCM_N_VOLTAGES		((VLCM_MAX_UV - VLCM_MIN_UV) / STEP_UV + 1)
+#define VPN_MIN_UV		4000000
+#define VPN_MAX_UV		6500000
+#define VPN_N_VOLTAGES		((VPN_MAX_UV - VPN_MIN_UV) / STEP_UV + 1)
+
+static int rt4831_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
+{
+	struct regmap *regmap = rdev_get_regmap(rdev);
+	int rid = rdev_get_id(rdev);
+	unsigned int val, events = 0;
+	int ret;
+
+	ret = regmap_read(regmap, RT4831_REG_FLAGS, &val);
+	if (ret)
+		return ret;
+
+	if (val & RT4831_OTP_MASK)
+		events |= REGULATOR_ERROR_OVER_TEMP;
+
+	if (rid == DSV_OUT_VLCM && (val & RT4831_LCMOVP_MASK))
+		events |= REGULATOR_ERROR_OVER_CURRENT;
+
+	if (rid == DSV_OUT_VPOS && (val & RT4831_VPOSSCP_MASK))
+		events |= REGULATOR_ERROR_OVER_CURRENT;
+
+	if (rid == DSV_OUT_VNEG && (val & RT4831_VNEGSCP_MASK))
+		events |= REGULATOR_ERROR_OVER_CURRENT;
+
+	*flags = events;
+	return 0;
+}
+
+static const struct regulator_ops rt4831_dsvlcm_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.set_bypass = regulator_set_bypass_regmap,
+	.get_bypass = regulator_get_bypass_regmap,
+	.get_error_flags = rt4831_get_error_flags,
+};
+
+static const struct regulator_ops rt4831_dsvpn_ops = {
+	.list_voltage = regulator_list_voltage_linear,
+	.set_voltage_sel = regulator_set_voltage_sel_regmap,
+	.get_voltage_sel = regulator_get_voltage_sel_regmap,
+	.enable = regulator_enable_regmap,
+	.disable = regulator_disable_regmap,
+	.is_enabled = regulator_is_enabled_regmap,
+	.set_active_discharge = regulator_set_active_discharge_regmap,
+	.get_error_flags = rt4831_get_error_flags,
+};
+
+static const struct regulator_desc rt4831_regulator_descs[] = {
+	{
+		.name = "DSVLCM",
+		.ops = &rt4831_dsvlcm_ops,
+		.of_match = of_match_ptr("DSVLCM"),
+		.regulators_node = of_match_ptr("regulators"),
+		.type = REGULATOR_VOLTAGE,
+		.id = DSV_OUT_VLCM,
+		.n_voltages = VLCM_N_VOLTAGES,
+		.min_uV = VLCM_MIN_UV,
+		.uV_step = STEP_UV,
+		.vsel_reg = RT4831_REG_VLCM,
+		.vsel_mask = RT4831_VOLT_MASK,
+		.bypass_reg = RT4831_REG_DSVEN,
+		.bypass_val_on = DSV_MODE_BYPASS,
+		.bypass_val_off = DSV_MODE_NORMAL,
+	},
+	{
+		.name = "DSVP",
+		.ops = &rt4831_dsvpn_ops,
+		.of_match = of_match_ptr("DSVP"),
+		.regulators_node = of_match_ptr("regulators"),
+		.type = REGULATOR_VOLTAGE,
+		.id = DSV_OUT_VPOS,
+		.n_voltages = VPN_N_VOLTAGES,
+		.min_uV = VPN_MIN_UV,
+		.uV_step = STEP_UV,
+		.vsel_reg = RT4831_REG_VPOS,
+		.vsel_mask = RT4831_VOLT_MASK,
+		.enable_reg = RT4831_REG_DSVEN,
+		.enable_mask = RT4831_POSEN_MASK,
+		.active_discharge_reg = RT4831_REG_DSVEN,
+		.active_discharge_mask = RT4831_POSADEN_MASK,
+	},
+	{
+		.name = "DSVN",
+		.ops = &rt4831_dsvpn_ops,
+		.of_match = of_match_ptr("DSVN"),
+		.regulators_node = of_match_ptr("regulators"),
+		.type = REGULATOR_VOLTAGE,
+		.id = DSV_OUT_VNEG,
+		.n_voltages = VPN_N_VOLTAGES,
+		.min_uV = VPN_MIN_UV,
+		.uV_step = STEP_UV,
+		.vsel_reg = RT4831_REG_VNEG,
+		.vsel_mask = RT4831_VOLT_MASK,
+		.enable_reg = RT4831_REG_DSVEN,
+		.enable_mask = RT4831_NEGEN_MASK,
+		.active_discharge_reg = RT4831_REG_DSVEN,
+		.active_discharge_mask = RT4831_NEGADEN_MASK,
+	}
+};
+
+static int rt4831_regulator_probe(struct platform_device *pdev)
+{
+	struct regmap *regmap;
+	struct regulator_dev *rdev;
+	struct regulator_config config = {};
+	int i, ret;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (IS_ERR(regmap)) {
+		dev_err(&pdev->dev, "Failed to init regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	/* Configure DSV mode to normal by default */
+	ret = regmap_update_bits(regmap, RT4831_REG_DSVEN, RT4831_DSVMODE_MASK, DSV_MODE_NORMAL);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to configure dsv mode to normal\n");
+		return ret;
+	}
+
+	config.dev = pdev->dev.parent;
+	config.regmap = regmap;
+
+	for (i = 0; i < DSV_OUT_MAX; i++) {
+		rdev = devm_regulator_register(&pdev->dev, rt4831_regulator_descs + i, &config);
+		if (IS_ERR(rdev)) {
+			dev_err(&pdev->dev, "Failed to register %d regulator\n", i);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id rt4831_regulator_match[] = {
+	{ "rt4831-regulator", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, rt4831_regulator_match);
+
+static struct platform_driver rt4831_regulator_driver = {
+	.driver = {
+		.name = "rt4831-regulator",
+	},
+	.id_table = rt4831_regulator_match,
+	.probe = rt4831_regulator_probe,
+};
+module_platform_driver(rt4831_regulator_driver);
+
+MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


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

* Re: (subset) [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
                   ` (4 preceding siblings ...)
  2020-12-17 15:00 ` [PATCH v5 6/6] regulator: rt4831: Adds support for Richtek RT4831 DSV regulator cy_huang
@ 2020-12-28 16:13 ` Mark Brown
  2021-01-13 12:21 ` Lee Jones
  6 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2020-12-28 16:13 UTC (permalink / raw)
  To: lgirdwood, jingoohan1, cy_huang, lee.jones, b.zolnierkie,
	daniel.thompson
  Cc: linux-fbdev, linux-kernel, dri-devel, ChiYuan Huang

On Thu, 17 Dec 2020 23:00:39 +0800, cy_huang wrote:
> This adds support Richtek RT4831 core. It includes four channel WLED driver
> and Display Bias Voltage outputs.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[3/6] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator
      commit: 934b05e818620e922151734b2d0e070e388e3c53
[6/6] regulator: rt4831: Adds support for Richtek RT4831 DSV regulator
      commit: 9351ab8b0cb61ffbef30343d28d1855e329c98fb

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

* Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
  2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
                   ` (5 preceding siblings ...)
  2020-12-28 16:13 ` (subset) [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core Mark Brown
@ 2021-01-13 12:21 ` Lee Jones
  2021-01-13 14:09   ` ChiYuan Huang
  6 siblings, 1 reply; 14+ messages in thread
From: Lee Jones @ 2021-01-13 12:21 UTC (permalink / raw)
  To: cy_huang
  Cc: lgirdwood, broonie, daniel.thompson, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

On Thu, 17 Dec 2020, cy_huang wrote:

> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> This adds support Richtek RT4831 core. It includes four channel WLED driver
> and Display Bias Voltage outputs.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
> since v5
> - Rename file name from rt4831-core.c to rt4831.c
> - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> - Change variable 'val' to the meaningful name 'chip_id'.
> - Refine the error log when vendor id is not matched.
> - Remove of_match_ptr.
> 
> since v2
> - Refine Kconfig descriptions.
> - Add copyright.
> - Refine error logs in probe.
> - Refine comment lines in remove and shutdown.
> ---
>  drivers/mfd/Kconfig  |  10 +++++
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/rt4831.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 135 insertions(+)
>  create mode 100644 drivers/mfd/rt4831.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b99a13..dfb2640 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1088,6 +1088,16 @@ config MFD_RDC321X
>  	  southbridge which provides access to GPIOs and Watchdog using the
>  	  southbridge PCI device configuration space.
>  
> +config MFD_RT4831
> +	tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  This enables support for the Richtek RT4831 that includes 4 channel
> +	  WLED driving and Display Bias Voltage. It's commonly used to provide
> +	  power to the LCD display and LCD backlight.
> +
>  config MFD_RT5033
>  	tristate "Richtek RT5033 Power Management IC"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 1780019..28d247b 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.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> new file mode 100644
> index 00000000..2bf8364
> --- /dev/null
> +++ b/drivers/mfd/rt4831.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020 Richtek Technology Corp.

Nit: If you respin this, please bump the date.

> + * 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_VENDOR_ID	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_gpio;
> +	struct regmap *regmap;
> +	unsigned int chip_id;
> +	int ret;
> +
> +	enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(enable_gpio)) {
> +		dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> +		return PTR_ERR(enable_gpio);
> +	}
> +
> +	regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "Failed to initialize regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	ret = regmap_read(regmap, RT4831_REG_REVISION, &chip_id);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to get H/W revision\n");
> +		return ret;
> +	}
> +
> +	if ((chip_id & RT4831_VID_MASK) != RICHTEK_VENDOR_ID) {
> +		dev_err(&client->dev, "Chip vendor ID 0x%02x not matched\n", chip_id);
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Used to prevent the abnormal shutdown.
> +	 * If SCL/SDA both keep low for one second to reset HW.
> +	 */
> +	ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> +				 RT4831_I2CSAFETMR_MASK);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> +		return ret;
> +	}
> +
> +	return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> +				    ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> +}
> +
> +static int rt4831_remove(struct i2c_client *client)
> +{
> +	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> +
> +	/* Disable WLED and DSV outputs */
> +	return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> +}
> +
> +static void rt4831_shutdown(struct i2c_client *client)
> +{
> +	struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> +
> +	/* Disable WLED and DSV outputs */
> +	regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> +}

What is your reason for providing a .shutdown() routine?

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

* Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
  2021-01-13 12:21 ` Lee Jones
@ 2021-01-13 14:09   ` ChiYuan Huang
  2021-03-25  8:25     ` ChiYuan Huang
  0 siblings, 1 reply; 14+ messages in thread
From: ChiYuan Huang @ 2021-01-13 14:09 UTC (permalink / raw)
  To: Lee Jones
  Cc: lgirdwood, Mark Brown, Daniel Thompson, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, lkml, ChiYuan Huang

Lee Jones <lee.jones@linaro.org> 於 2021年1月13日 週三 下午8:21寫道:
>
> On Thu, 17 Dec 2020, cy_huang wrote:
>
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > This adds support Richtek RT4831 core. It includes four channel WLED driver
> > and Display Bias Voltage outputs.
> >
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > since v5
> > - Rename file name from rt4831-core.c to rt4831.c
> > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > - Change variable 'val' to the meaningful name 'chip_id'.
> > - Refine the error log when vendor id is not matched.
> > - Remove of_match_ptr.
> >
> > since v2
> > - Refine Kconfig descriptions.
> > - Add copyright.
> > - Refine error logs in probe.
> > - Refine comment lines in remove and shutdown.
> > ---
> >  drivers/mfd/Kconfig  |  10 +++++
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/rt4831.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 135 insertions(+)
> >  create mode 100644 drivers/mfd/rt4831.c
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 8b99a13..dfb2640 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> >         southbridge which provides access to GPIOs and Watchdog using the
> >         southbridge PCI device configuration space.
> >
> > +config MFD_RT4831
> > +     tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > +     depends on I2C
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     help
> > +       This enables support for the Richtek RT4831 that includes 4 channel
> > +       WLED driving and Display Bias Voltage. It's commonly used to provide
> > +       power to the LCD display and LCD backlight.
> > +
> >  config MFD_RT5033
> >       tristate "Richtek RT5033 Power Management IC"
> >       depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 1780019..28d247b 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.o
> >  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
> >  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
> >
> > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > new file mode 100644
> > index 00000000..2bf8364
> > --- /dev/null
> > +++ b/drivers/mfd/rt4831.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2020 Richtek Technology Corp.
>
> Nit: If you respin this, please bump the date.
>
Okay.
> > + * 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_VENDOR_ID    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_gpio;
> > +     struct regmap *regmap;
> > +     unsigned int chip_id;
> > +     int ret;
> > +
> > +     enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(enable_gpio)) {
> > +             dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> > +             return PTR_ERR(enable_gpio);
> > +     }
> > +
> > +     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> > +     if (IS_ERR(regmap)) {
> > +             dev_err(&client->dev, "Failed to initialize regmap\n");
> > +             return PTR_ERR(regmap);
> > +     }
> > +
> > +     ret = regmap_read(regmap, RT4831_REG_REVISION, &chip_id);
> > +     if (ret) {
> > +             dev_err(&client->dev, "Failed to get H/W revision\n");
> > +             return ret;
> > +     }
> > +
> > +     if ((chip_id & RT4831_VID_MASK) != RICHTEK_VENDOR_ID) {
> > +             dev_err(&client->dev, "Chip vendor ID 0x%02x not matched\n", chip_id);
> > +             return -ENODEV;
> > +     }
> > +
> > +     /*
> > +      * Used to prevent the abnormal shutdown.
> > +      * If SCL/SDA both keep low for one second to reset HW.
> > +      */
> > +     ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> > +                              RT4831_I2CSAFETMR_MASK);
> > +     if (ret) {
> > +             dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> > +             return ret;
> > +     }
> > +
> > +     return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> > +                                 ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> > +}
> > +
> > +static int rt4831_remove(struct i2c_client *client)
> > +{
> > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > +
> > +     /* Disable WLED and DSV outputs */
> > +     return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > +}
> > +
> > +static void rt4831_shutdown(struct i2c_client *client)
> > +{
> > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > +
> > +     /* Disable WLED and DSV outputs */
> > +     regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > +}
>
> What is your reason for providing a .shutdown() routine?
>
Just like as remove routine to make sure all output are off for the safety.
This chip only have one 'enable' pin and I2C as the control signal.
As normal shutdown, it can be make sure 'enable' will be pull low.
But for some case, if 'enable' always tied to high, like as ARM reset,
chip reset only triggered during next booting phase.
The period from arm  reset to next probe, if user doesn't call DSV and
WLED off before machine shutdown/reboot, the WLED/DSV voltage boost
circuit will be kept as on.
That's why I also put shutdown routine in the driver to reset the whole chip.
> > +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 = 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] 14+ messages in thread

* Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
  2020-12-17 15:00 ` [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight cy_huang
@ 2021-03-25  8:22   ` ChiYuan Huang
  2021-03-25 11:54     ` Daniel Thompson
  2021-03-25 11:53   ` Daniel Thompson
  1 sibling, 1 reply; 14+ messages in thread
From: ChiYuan Huang @ 2021-03-25  8:22 UTC (permalink / raw)
  To: Lee Jones, lgirdwood, Mark Brown, Daniel Thompson, jingoohan1,
	b.zolnierkie
  Cc: dri-devel, linux-fbdev, lkml, ChiYuan Huang

Dear reviewers:

           Didn't get any response about this backlight patch.
Is there any part need to be refined?

cy_huang <u0084500@gmail.com> 於 2020年12月17日 週四 下午11:01寫道:
>
> From: ChiYuan Huang <cy_huang@richtek.com>
>
> Adds support for Richtek RT4831 backlight.
>
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> ---
>  drivers/video/backlight/Kconfig            |   8 ++
>  drivers/video/backlight/Makefile           |   1 +
>  drivers/video/backlight/rt4831-backlight.c | 219 +++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
>  create mode 100644 drivers/video/backlight/rt4831-backlight.c
>
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d83c87b..666bdb0 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
>           If you have the Qualcomm PMIC, say Y to enable a driver for the
>           WLED block. Currently it supports PM8941 and PMI8998.
>
> +config BACKLIGHT_RT4831
> +       tristate "Richtek RT4831 Backlight Driver"
> +       depends on MFD_RT4831
> +       help
> +         This enables support for Richtek RT4831 Backlight driver.
> +         It's commont used to drive the display WLED. There're four channels
> +         inisde, and each channel can provide up to 30mA current.
> +
>  config BACKLIGHT_SAHARA
>         tristate "Tabletkiosk Sahara Touch-iT Backlight Driver"
>         depends on X86
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 685f3f1..cae2c83 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_BACKLIGHT_PANDORA)               += pandora_bl.o
>  obj-$(CONFIG_BACKLIGHT_PCF50633)       += pcf50633-backlight.o
>  obj-$(CONFIG_BACKLIGHT_PWM)            += pwm_bl.o
>  obj-$(CONFIG_BACKLIGHT_QCOM_WLED)      += qcom-wled.o
> +obj-$(CONFIG_BACKLIGHT_RT4831)         += rt4831-backlight.o
>  obj-$(CONFIG_BACKLIGHT_SAHARA)         += kb3886_bl.o
>  obj-$(CONFIG_BACKLIGHT_SKY81452)       += sky81452-backlight.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)           += tosa_bl.o
> diff --git a/drivers/video/backlight/rt4831-backlight.c b/drivers/video/backlight/rt4831-backlight.c
> new file mode 100644
> index 00000000..816c4d6
> --- /dev/null
> +++ b/drivers/video/backlight/rt4831-backlight.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <dt-bindings/leds/rt4831-backlight.h>
> +#include <linux/backlight.h>
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define RT4831_REG_BLCFG       0x02
> +#define RT4831_REG_BLDIML      0x04
> +#define RT4831_REG_ENABLE      0x08
> +
> +#define BL_MAX_BRIGHTNESS      2048
> +
> +#define RT4831_BLOVP_MASK      GENMASK(7, 5)
> +#define RT4831_BLOVP_SHIFT     5
> +#define RT4831_BLPWMEN_MASK    BIT(0)
> +#define RT4831_BLEN_MASK       BIT(4)
> +#define RT4831_BLCH_MASK       GENMASK(3, 0)
> +#define RT4831_BLDIML_MASK     GENMASK(2, 0)
> +#define RT4831_BLDIMH_MASK     GENMASK(10, 3)
> +#define RT4831_BLDIMH_SHIFT    3
> +
> +struct rt4831_priv {
> +       struct regmap *regmap;
> +       struct mutex lock;
> +       struct backlight_device *bl;
> +};
> +
> +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> +{
> +       struct rt4831_priv *priv = bl_get_data(bl_dev);
> +       int brightness = backlight_get_brightness(bl_dev);
> +       unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> +       u8 v[2];
> +       int ret;
> +
> +       mutex_lock(&priv->lock);
> +
> +       if (brightness) {
> +               v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
> +               v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >> RT4831_BLDIMH_SHIFT;
> +
> +               ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
> +               if (ret)
> +                       goto unlock;
> +       }
> +
> +       ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLEN_MASK, enable);
> +
> +unlock:
> +       mutex_unlock(&priv->lock);
> +       return ret;
> +}
> +
> +static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
> +{
> +       struct rt4831_priv *priv = bl_get_data(bl_dev);
> +       unsigned int val;
> +       u8 v[2];
> +       int ret;
> +
> +       mutex_lock(&priv->lock);
> +
> +       ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (!(val & RT4831_BLEN_MASK)) {
> +               ret = 0;
> +               goto unlock;
> +       }
> +
> +       ret = regmap_raw_read(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
> +       if (ret)
> +               goto unlock;
> +
> +       ret = (v[1] << RT4831_BLDIMH_SHIFT) + (v[0] & RT4831_BLDIML_MASK) + 1;
> +
> +unlock:
> +       mutex_unlock(&priv->lock);
> +       return ret;
> +}
> +
> +static const struct backlight_ops rt4831_bl_ops = {
> +       .options = BL_CORE_SUSPENDRESUME,
> +       .update_status = rt4831_bl_update_status,
> +       .get_brightness = rt4831_bl_get_brightness,
> +};
> +
> +static int rt4831_init_device_properties(struct rt4831_priv *priv, struct device *dev,
> +                                         struct backlight_properties *bl_props)
> +{
> +       u8 propval;
> +       u32 brightness;
> +       unsigned int val = 0;
> +       int ret;
> +
> +       /* common properties */
> +       ret = device_property_read_u32(dev, "max-brightness", &brightness);
> +       if (ret) {
> +               dev_warn(dev, "max-brightness DT property missing, use HW max as default\n");
> +               brightness = BL_MAX_BRIGHTNESS;
> +       }
> +
> +       bl_props->max_brightness = min_t(u32, brightness, BL_MAX_BRIGHTNESS);
> +
> +       ret = device_property_read_u32(dev, "default-brightness", &brightness);
> +       if (ret) {
> +               dev_warn(dev, "default-brightness DT property missing, use max limit as default\n");
> +               brightness = bl_props->max_brightness;
> +       }
> +
> +       bl_props->brightness = min_t(u32, brightness, bl_props->max_brightness);
> +
> +       /* vendor properties */
> +       if (device_property_read_bool(dev, "richtek,pwm-enable"))
> +               val = RT4831_BLPWMEN_MASK;
> +
> +       ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLPWMEN_MASK, val);
> +       if (ret)
> +               return ret;
> +
> +       ret = device_property_read_u8(dev, "richtek,bled-ovp-sel", &propval);
> +       if (ret) {
> +               dev_warn(dev, "richtek,bled-ovp-sel DT property missing, use default 21V\n");
> +               propval = RT4831_BLOVPLVL_21V;
> +       }
> +
> +       propval = min_t(u8, propval, RT4831_BLOVPLVL_29V);
> +       ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLOVP_MASK,
> +                                propval << RT4831_BLOVP_SHIFT);
> +       if (ret)
> +               return ret;
> +
> +       ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
> +       if (ret) {
> +               dev_err(dev, "richtek,channel-use DT property missing\n");
> +               return ret;
> +       }
> +
> +       if (!(propval & RT4831_BLCH_MASK)) {
> +               dev_err(dev, "No channel specified\n");
> +               return -EINVAL;
> +       }
> +
> +       return regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLCH_MASK, propval);
> +}
> +
> +static int rt4831_bl_probe(struct platform_device *pdev)
> +{
> +       struct rt4831_priv *priv;
> +       struct backlight_properties bl_props = { .type = BACKLIGHT_RAW, };
> +       int ret;
> +
> +       priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       mutex_init(&priv->lock);
> +
> +       priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +       if (IS_ERR(priv->regmap)) {
> +               dev_err(&pdev->dev, "Failed to init regmap\n");
> +               return PTR_ERR(priv->regmap);
> +       }
> +
> +       ret = rt4831_init_device_properties(priv, &pdev->dev, &bl_props);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to init device properties\n");
> +               return ret;
> +       }
> +
> +       priv->bl = devm_backlight_device_register(&pdev->dev, pdev->name, &pdev->dev, priv,
> +                                                 &rt4831_bl_ops, &bl_props);
> +       if (IS_ERR(priv->bl)) {
> +               dev_err(&pdev->dev, "Failed to register backlight\n");
> +               return PTR_ERR(priv->bl);
> +       }
> +
> +       backlight_update_status(priv->bl);
> +       platform_set_drvdata(pdev, priv);
> +
> +       return 0;
> +}
> +
> +static int rt4831_bl_remove(struct platform_device *pdev)
> +{
> +       struct rt4831_priv *priv = platform_get_drvdata(pdev);
> +       struct backlight_device *bl_dev = priv->bl;
> +
> +       bl_dev->props.brightness = 0;
> +       backlight_update_status(priv->bl);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused rt4831_bl_of_match[] = {
> +       { .compatible = "richtek,rt4831-backlight", },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, rt4831_bl_of_match);
> +
> +static struct platform_driver rt4831_bl_driver = {
> +       .driver = {
> +               .name = "rt4831-backlight",
> +               .of_match_table = rt4831_bl_of_match,
> +       },
> +       .probe = rt4831_bl_probe,
> +       .remove = rt4831_bl_remove,
> +};
> +module_platform_driver(rt4831_bl_driver);
> +
> +MODULE_AUTHOR("ChiYuan Huang <cy_huang@richtek.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

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

* Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
  2021-01-13 14:09   ` ChiYuan Huang
@ 2021-03-25  8:25     ` ChiYuan Huang
  2021-03-25 14:31       ` Lee Jones
  0 siblings, 1 reply; 14+ messages in thread
From: ChiYuan Huang @ 2021-03-25  8:25 UTC (permalink / raw)
  To: Lee Jones
  Cc: lgirdwood, Mark Brown, Daniel Thompson, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, lkml, ChiYuan Huang

HI, Lee:

ChiYuan Huang <u0084500@gmail.com> 於 2021年1月13日 週三 下午10:09寫道:
>
> Lee Jones <lee.jones@linaro.org> 於 2021年1月13日 週三 下午8:21寫道:
> >
> > On Thu, 17 Dec 2020, cy_huang wrote:
> >
> > > From: ChiYuan Huang <cy_huang@richtek.com>
> > >
> > > This adds support Richtek RT4831 core. It includes four channel WLED driver
> > > and Display Bias Voltage outputs.
> > >
> > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > ---
> > > since v5
> > > - Rename file name from rt4831-core.c to rt4831.c
> > > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > > - Change variable 'val' to the meaningful name 'chip_id'.
> > > - Refine the error log when vendor id is not matched.
> > > - Remove of_match_ptr.
> > >
> > > since v2
> > > - Refine Kconfig descriptions.
> > > - Add copyright.
> > > - Refine error logs in probe.
> > > - Refine comment lines in remove and shutdown.
> > > ---
> > >  drivers/mfd/Kconfig  |  10 +++++
> > >  drivers/mfd/Makefile |   1 +
> > >  drivers/mfd/rt4831.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 135 insertions(+)
> > >  create mode 100644 drivers/mfd/rt4831.c
> > >
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 8b99a13..dfb2640 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > >         southbridge which provides access to GPIOs and Watchdog using the
> > >         southbridge PCI device configuration space.
> > >
> > > +config MFD_RT4831
> > > +     tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > > +     depends on I2C
> > > +     select MFD_CORE
> > > +     select REGMAP_I2C
> > > +     help
> > > +       This enables support for the Richtek RT4831 that includes 4 channel
> > > +       WLED driving and Display Bias Voltage. It's commonly used to provide
> > > +       power to the LCD display and LCD backlight.
> > > +
> > >  config MFD_RT5033
> > >       tristate "Richtek RT5033 Power Management IC"
> > >       depends on I2C
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 1780019..28d247b 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.o
> > >  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
> > >  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
> > >
> > > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > > new file mode 100644
> > > index 00000000..2bf8364
> > > --- /dev/null
> > > +++ b/drivers/mfd/rt4831.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (c) 2020 Richtek Technology Corp.
> >
> > Nit: If you respin this, please bump the date.
> >
> Okay.
> > > + * 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_VENDOR_ID    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_gpio;
> > > +     struct regmap *regmap;
> > > +     unsigned int chip_id;
> > > +     int ret;
> > > +
> > > +     enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > > +     if (IS_ERR(enable_gpio)) {
> > > +             dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> > > +             return PTR_ERR(enable_gpio);
> > > +     }
> > > +
> > > +     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> > > +     if (IS_ERR(regmap)) {
> > > +             dev_err(&client->dev, "Failed to initialize regmap\n");
> > > +             return PTR_ERR(regmap);
> > > +     }
> > > +
> > > +     ret = regmap_read(regmap, RT4831_REG_REVISION, &chip_id);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "Failed to get H/W revision\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     if ((chip_id & RT4831_VID_MASK) != RICHTEK_VENDOR_ID) {
> > > +             dev_err(&client->dev, "Chip vendor ID 0x%02x not matched\n", chip_id);
> > > +             return -ENODEV;
> > > +     }
> > > +
> > > +     /*
> > > +      * Used to prevent the abnormal shutdown.
> > > +      * If SCL/SDA both keep low for one second to reset HW.
> > > +      */
> > > +     ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> > > +                              RT4831_I2CSAFETMR_MASK);
> > > +     if (ret) {
> > > +             dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> > > +                                 ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> > > +}
> > > +
> > > +static int rt4831_remove(struct i2c_client *client)
> > > +{
> > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > +     /* Disable WLED and DSV outputs */
> > > +     return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > +}
> > > +
> > > +static void rt4831_shutdown(struct i2c_client *client)
> > > +{
> > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > +
> > > +     /* Disable WLED and DSV outputs */
> > > +     regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > +}
> >
> > What is your reason for providing a .shutdown() routine?
> >
> Just like as remove routine to make sure all output are off for the safety.
> This chip only have one 'enable' pin and I2C as the control signal.
> As normal shutdown, it can be make sure 'enable' will be pull low.
> But for some case, if 'enable' always tied to high, like as ARM reset,
> chip reset only triggered during next booting phase.
> The period from arm  reset to next probe, if user doesn't call DSV and
> WLED off before machine shutdown/reboot, the WLED/DSV voltage boost
> circuit will be kept as on.
> That's why I also put shutdown routine in the driver to reset the whole chip.

If the shutdown routine is not suitable, I think it can be removed.
There's the HWEN pin inside this IC to make sure all function will be disabled.

There're changed part in my note
1. respin the header date.
2. Remove the shutdown routine

Anything else?


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

* Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
  2020-12-17 15:00 ` [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight cy_huang
  2021-03-25  8:22   ` ChiYuan Huang
@ 2021-03-25 11:53   ` Daniel Thompson
  1 sibling, 0 replies; 14+ messages in thread
From: Daniel Thompson @ 2021-03-25 11:53 UTC (permalink / raw)
  To: cy_huang
  Cc: lee.jones, lgirdwood, broonie, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, linux-kernel, ChiYuan Huang

On Thu, Dec 17, 2020 at 11:00:43PM +0800, cy_huang wrote:
> From: ChiYuan Huang <cy_huang@richtek.com>
> 
> Adds support for Richtek RT4831 backlight.
> 
> Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>

Looks ok but there are a few minor niggles.
> ---
>  drivers/video/backlight/Kconfig            |   8 ++
>  drivers/video/backlight/Makefile           |   1 +
>  drivers/video/backlight/rt4831-backlight.c | 219 +++++++++++++++++++++++++++++
>  3 files changed, 228 insertions(+)
>  create mode 100644 drivers/video/backlight/rt4831-backlight.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index d83c87b..666bdb0 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -289,6 +289,14 @@ config BACKLIGHT_QCOM_WLED
>  	  If you have the Qualcomm PMIC, say Y to enable a driver for the
>  	  WLED block. Currently it supports PM8941 and PMI8998.
>  
> +config BACKLIGHT_RT4831
> +	tristate "Richtek RT4831 Backlight Driver"
> +	depends on MFD_RT4831
> +	help
> +	  This enables support for Richtek RT4831 Backlight driver.
> +	  It's commont used to drive the display WLED. There're four channels
                    ^^^

> diff --git a/drivers/video/backlight/rt4831-backlight.c b/drivers/video/backlight/rt4831-backlight.c
> new file mode 100644
> index 00000000..816c4d6
> --- /dev/null
> +++ b/drivers/video/backlight/rt4831-backlight.c
> @@ -0,0 +1,219 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <dt-bindings/leds/rt4831-backlight.h>
> +#include <linux/backlight.h>
> +#include <linux/bitops.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define RT4831_REG_BLCFG	0x02
> +#define RT4831_REG_BLDIML	0x04
> +#define RT4831_REG_ENABLE	0x08
> +
> +#define BL_MAX_BRIGHTNESS	2048

Would be better with a prefix.
> +
> +#define RT4831_BLOVP_MASK	GENMASK(7, 5)
> +#define RT4831_BLOVP_SHIFT	5
> +#define RT4831_BLPWMEN_MASK	BIT(0)
> +#define RT4831_BLEN_MASK	BIT(4)
> +#define RT4831_BLCH_MASK	GENMASK(3, 0)
> +#define RT4831_BLDIML_MASK	GENMASK(2, 0)
> +#define RT4831_BLDIMH_MASK	GENMASK(10, 3)
> +#define RT4831_BLDIMH_SHIFT	3
> +
> +struct rt4831_priv {
> +	struct regmap *regmap;
> +	struct mutex lock;

Locks aren't very common in backlight drivers. Why isn't the ops_lock
sufficient?


> +	struct backlight_device *bl;
> +};
> +
> +static int rt4831_bl_update_status(struct backlight_device *bl_dev)
> +{
> +	struct rt4831_priv *priv = bl_get_data(bl_dev);
> +	int brightness = backlight_get_brightness(bl_dev);
> +	unsigned int enable = brightness ? RT4831_BLEN_MASK : 0;
> +	u8 v[2];
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	if (brightness) {
> +		v[0] = (brightness - 1) & RT4831_BLDIML_MASK;
> +		v[1] = ((brightness - 1) & RT4831_BLDIMH_MASK) >> RT4831_BLDIMH_SHIFT;
> +
> +		ret = regmap_raw_write(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
> +		if (ret)
> +			goto unlock;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLEN_MASK, enable);
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static int rt4831_bl_get_brightness(struct backlight_device *bl_dev)
> +{
> +	struct rt4831_priv *priv = bl_get_data(bl_dev);
> +	unsigned int val;
> +	u8 v[2];
> +	int ret;
> +
> +	mutex_lock(&priv->lock);
> +
> +	ret = regmap_read(priv->regmap, RT4831_REG_ENABLE, &val);
> +	if (ret)
> +		return ret;

Deadlock.


> +
> +	if (!(val & RT4831_BLEN_MASK)) {
> +		ret = 0;
> +		goto unlock;
> +	}
> +
> +	ret = regmap_raw_read(priv->regmap, RT4831_REG_BLDIML, v, sizeof(v));
> +	if (ret)
> +		goto unlock;
> +
> +	ret = (v[1] << RT4831_BLDIMH_SHIFT) + (v[0] & RT4831_BLDIML_MASK) + 1;
> +
> +unlock:
> +	mutex_unlock(&priv->lock);
> +	return ret;
> +}
> +
> +static const struct backlight_ops rt4831_bl_ops = {
> +	.options = BL_CORE_SUSPENDRESUME,
> +	.update_status = rt4831_bl_update_status,
> +	.get_brightness = rt4831_bl_get_brightness,
> +};
> +
> +static int rt4831_init_device_properties(struct rt4831_priv *priv, struct device *dev,

This is not the idiomatic name usually used for this type of function.
In fact since this driver purely uses device properties then this code
could just be merged into the probe function.


> +					  struct backlight_properties *bl_props)
> +{
> +	u8 propval;
> +	u32 brightness;
> +	unsigned int val = 0;
> +	int ret;
> +
> +	/* common properties */
> +	ret = device_property_read_u32(dev, "max-brightness", &brightness);
> +	if (ret) {
> +		dev_warn(dev, "max-brightness DT property missing, use HW max as default\n");

Does there need to be a warning on this?

It's code pattern is common but the DT docs have formalized a lot
recently. The DT docs in patch 1 say these are optional... so
why does it justify a warning of they are omitted? There is nothing
wrong! Is it better to specify the defaults in the bindings and
then the kernel can say nothing when the defaults are adopted.


> +		brightness = BL_MAX_BRIGHTNESS;
> +	}
> +
> +	bl_props->max_brightness = min_t(u32, brightness, BL_MAX_BRIGHTNESS);
> +
> +	ret = device_property_read_u32(dev, "default-brightness", &brightness);
> +	if (ret) {
> +		dev_warn(dev, "default-brightness DT property missing, use max limit as default\n");

Ditto.


> +		brightness = bl_props->max_brightness;
> +	}
> +
> +	bl_props->brightness = min_t(u32, brightness, bl_props->max_brightness);
> +
> +	/* vendor properties */
> +	if (device_property_read_bool(dev, "richtek,pwm-enable"))
> +		val = RT4831_BLPWMEN_MASK;
> +
> +	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLPWMEN_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u8(dev, "richtek,bled-ovp-sel", &propval);
> +	if (ret) {
> +		dev_warn(dev, "richtek,bled-ovp-sel DT property missing,
> use default 21V\n");o

Ditto.

> +		propval = RT4831_BLOVPLVL_21V;
> +	}
> +
> +	propval = min_t(u8, propval, RT4831_BLOVPLVL_29V);
> +	ret = regmap_update_bits(priv->regmap, RT4831_REG_BLCFG, RT4831_BLOVP_MASK,
> +				 propval << RT4831_BLOVP_SHIFT);
> +	if (ret)
> +		return ret;
> +
> +	ret = device_property_read_u8(dev, "richtek,channel-use", &propval);
> +	if (ret) {
> +		dev_err(dev, "richtek,channel-use DT property missing\n");
> +		return ret;
> +	}
> +
> +	if (!(propval & RT4831_BLCH_MASK)) {
> +		dev_err(dev, "No channel specified\n");
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(priv->regmap, RT4831_REG_ENABLE, RT4831_BLCH_MASK, propval);
> +}
> +
> +static int rt4831_bl_probe(struct platform_device *pdev)
> +{
> +	struct rt4831_priv *priv;
> +	struct backlight_properties bl_props = { .type = BACKLIGHT_RAW, };

In new drivers please make sure to correctly set props.scale so that the
backlight slider can be mapped correctly (see
Documentation/ABI/testing/sysfs-class-backlight for description of the
options).
 
 
Daniel.

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

* Re: [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight
  2021-03-25  8:22   ` ChiYuan Huang
@ 2021-03-25 11:54     ` Daniel Thompson
  0 siblings, 0 replies; 14+ messages in thread
From: Daniel Thompson @ 2021-03-25 11:54 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: Lee Jones, lgirdwood, Mark Brown, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, lkml, ChiYuan Huang

On Thu, Mar 25, 2021 at 04:22:11PM +0800, ChiYuan Huang wrote:
> Dear reviewers:
> 
>            Didn't get any response about this backlight patch.
> Is there any part need to be refined?

Thanks for the reminders and sorry for the delay. Have just replied
to the original message!


Daniel.

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

* Re: [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core
  2021-03-25  8:25     ` ChiYuan Huang
@ 2021-03-25 14:31       ` Lee Jones
  0 siblings, 0 replies; 14+ messages in thread
From: Lee Jones @ 2021-03-25 14:31 UTC (permalink / raw)
  To: ChiYuan Huang
  Cc: lgirdwood, Mark Brown, Daniel Thompson, jingoohan1, b.zolnierkie,
	dri-devel, linux-fbdev, lkml, ChiYuan Huang

On Thu, 25 Mar 2021, ChiYuan Huang wrote:

> HI, Lee:
> 
> ChiYuan Huang <u0084500@gmail.com> 於 2021年1月13日 週三 下午10:09寫道:
> >
> > Lee Jones <lee.jones@linaro.org> 於 2021年1月13日 週三 下午8:21寫道:
> > >
> > > On Thu, 17 Dec 2020, cy_huang wrote:
> > >
> > > > From: ChiYuan Huang <cy_huang@richtek.com>
> > > >
> > > > This adds support Richtek RT4831 core. It includes four channel WLED driver
> > > > and Display Bias Voltage outputs.
> > > >
> > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > ---
> > > > since v5
> > > > - Rename file name from rt4831-core.c to rt4831.c
> > > > - Change RICHTEK_VID to RICHTEK_VENDOR_ID.
> > > > - Change gpio_desc nameing from 'enable' to 'enable_gpio' in probe.
> > > > - Change variable 'val' to the meaningful name 'chip_id'.
> > > > - Refine the error log when vendor id is not matched.
> > > > - Remove of_match_ptr.
> > > >
> > > > since v2
> > > > - Refine Kconfig descriptions.
> > > > - Add copyright.
> > > > - Refine error logs in probe.
> > > > - Refine comment lines in remove and shutdown.
> > > > ---
> > > >  drivers/mfd/Kconfig  |  10 +++++
> > > >  drivers/mfd/Makefile |   1 +
> > > >  drivers/mfd/rt4831.c | 124 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 135 insertions(+)
> > > >  create mode 100644 drivers/mfd/rt4831.c
> > > >
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index 8b99a13..dfb2640 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -1088,6 +1088,16 @@ config MFD_RDC321X
> > > >         southbridge which provides access to GPIOs and Watchdog using the
> > > >         southbridge PCI device configuration space.
> > > >
> > > > +config MFD_RT4831
> > > > +     tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
> > > > +     depends on I2C
> > > > +     select MFD_CORE
> > > > +     select REGMAP_I2C
> > > > +     help
> > > > +       This enables support for the Richtek RT4831 that includes 4 channel
> > > > +       WLED driving and Display Bias Voltage. It's commonly used to provide
> > > > +       power to the LCD display and LCD backlight.
> > > > +
> > > >  config MFD_RT5033
> > > >       tristate "Richtek RT5033 Power Management IC"
> > > >       depends on I2C
> > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > > index 1780019..28d247b 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.o
> > > >  obj-$(CONFIG_MFD_RT5033)     += rt5033.o
> > > >  obj-$(CONFIG_MFD_SKY81452)   += sky81452.o
> > > >
> > > > diff --git a/drivers/mfd/rt4831.c b/drivers/mfd/rt4831.c
> > > > new file mode 100644
> > > > index 00000000..2bf8364
> > > > --- /dev/null
> > > > +++ b/drivers/mfd/rt4831.c
> > > > @@ -0,0 +1,124 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright (c) 2020 Richtek Technology Corp.
> > >
> > > Nit: If you respin this, please bump the date.
> > >
> > Okay.
> > > > + * 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_VENDOR_ID    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_gpio;
> > > > +     struct regmap *regmap;
> > > > +     unsigned int chip_id;
> > > > +     int ret;
> > > > +
> > > > +     enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_HIGH);
> > > > +     if (IS_ERR(enable_gpio)) {
> > > > +             dev_err(&client->dev, "Failed to get 'enable' GPIO\n");
> > > > +             return PTR_ERR(enable_gpio);
> > > > +     }
> > > > +
> > > > +     regmap = devm_regmap_init_i2c(client, &rt4831_regmap_config);
> > > > +     if (IS_ERR(regmap)) {
> > > > +             dev_err(&client->dev, "Failed to initialize regmap\n");
> > > > +             return PTR_ERR(regmap);
> > > > +     }
> > > > +
> > > > +     ret = regmap_read(regmap, RT4831_REG_REVISION, &chip_id);
> > > > +     if (ret) {
> > > > +             dev_err(&client->dev, "Failed to get H/W revision\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     if ((chip_id & RT4831_VID_MASK) != RICHTEK_VENDOR_ID) {
> > > > +             dev_err(&client->dev, "Chip vendor ID 0x%02x not matched\n", chip_id);
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     /*
> > > > +      * Used to prevent the abnormal shutdown.
> > > > +      * If SCL/SDA both keep low for one second to reset HW.
> > > > +      */
> > > > +     ret = regmap_update_bits(regmap, RT4831_REG_I2CPROT, RT4831_I2CSAFETMR_MASK,
> > > > +                              RT4831_I2CSAFETMR_MASK);
> > > > +     if (ret) {
> > > > +             dev_err(&client->dev, "Failed to enable I2C safety timer\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_AUTO, rt4831_subdevs,
> > > > +                                 ARRAY_SIZE(rt4831_subdevs), NULL, 0, NULL);
> > > > +}
> > > > +
> > > > +static int rt4831_remove(struct i2c_client *client)
> > > > +{
> > > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > > +
> > > > +     /* Disable WLED and DSV outputs */
> > > > +     return regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > > +}
> > > > +
> > > > +static void rt4831_shutdown(struct i2c_client *client)
> > > > +{
> > > > +     struct regmap *regmap = dev_get_regmap(&client->dev, NULL);
> > > > +
> > > > +     /* Disable WLED and DSV outputs */
> > > > +     regmap_update_bits(regmap, RT4831_REG_ENABLE, RT4831_RESET_MASK, RT4831_RESET_MASK);
> > > > +}
> > >
> > > What is your reason for providing a .shutdown() routine?
> > >
> > Just like as remove routine to make sure all output are off for the safety.
> > This chip only have one 'enable' pin and I2C as the control signal.
> > As normal shutdown, it can be make sure 'enable' will be pull low.
> > But for some case, if 'enable' always tied to high, like as ARM reset,
> > chip reset only triggered during next booting phase.
> > The period from arm  reset to next probe, if user doesn't call DSV and
> > WLED off before machine shutdown/reboot, the WLED/DSV voltage boost
> > circuit will be kept as on.
> > That's why I also put shutdown routine in the driver to reset the whole chip.
> 
> If the shutdown routine is not suitable, I think it can be removed.
> There's the HWEN pin inside this IC to make sure all function will be disabled.
> 
> There're changed part in my note
> 1. respin the header date.
> 2. Remove the shutdown routine
> 
> Anything else?

Just respin and resend.

I will do a full review once it's back on the list.

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

end of thread, other threads:[~2021-03-25 14:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 15:00 [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core cy_huang
2020-12-17 15:00 ` [PATCH v5 2/6] backlight: rt4831: Adds DT binding document for Richtek RT4831 backlight cy_huang
2020-12-17 15:00 ` [PATCH v5 3/6] regulator: rt4831: Adds DT binding document for Richtek RT4831 DSV regulator cy_huang
2020-12-17 15:00 ` [PATCH v5 4/6] mfd: rt4831: Adds DT binding document for Richtek RT4831 core cy_huang
2020-12-17 15:00 ` [PATCH v5 5/6] backlight: rt4831: Adds support for Richtek RT4831 backlight cy_huang
2021-03-25  8:22   ` ChiYuan Huang
2021-03-25 11:54     ` Daniel Thompson
2021-03-25 11:53   ` Daniel Thompson
2020-12-17 15:00 ` [PATCH v5 6/6] regulator: rt4831: Adds support for Richtek RT4831 DSV regulator cy_huang
2020-12-28 16:13 ` (subset) [PATCH v5 1/6] mfd: rt4831: Adds support for Richtek RT4831 core Mark Brown
2021-01-13 12:21 ` Lee Jones
2021-01-13 14:09   ` ChiYuan Huang
2021-03-25  8:25     ` ChiYuan Huang
2021-03-25 14:31       ` Lee Jones

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