diff mbox series

leds: driver for O2 Micro LED IC

Message ID 20241106015441.995014-1-anishkmr@amazon.com
State New
Headers show
Series leds: driver for O2 Micro LED IC | expand

Commit Message

anishkmr@amazon.com Nov. 6, 2024, 1:54 a.m. UTC
From: Anish Kumar <anishkmr@amazon.com>

LED Driver for O2 Micro LED IC

reviewed-by: Anish Kumar <yesanishhere@gmail.com>
Signed-off-by: Karthik Poduval <kpoduval@lab126.com>
Signed-off-by: Yue Hu <yhuamzn@amazon.com>
---
 .../devicetree/bindings/leds/leds-ozl003.txt  |  23 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/leds/Kconfig                          |   6 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-ozl003.c                    | 306 ++++++++++++++++++
 5 files changed, 338 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-ozl003.txt
 create mode 100644 drivers/leds/leds-ozl003.c

Comments

Krzysztof Kozlowski Nov. 6, 2024, 10:46 a.m. UTC | #1
On 06/11/2024 02:54, anishkmr@amazon.com wrote:
> From: Anish Kumar <anishkmr@amazon.com>
> 
> LED Driver for O2 Micro LED IC
> 
> reviewed-by: Anish Kumar <yesanishhere@gmail.com>
> Signed-off-by: Karthik Poduval <kpoduval@lab126.com>
> Signed-off-by: Yue Hu <yhuamzn@amazon.com>
> ---
>  .../devicetree/bindings/leds/leds-ozl003.txt  |  23 ++

1. Please run scripts/checkpatch.pl and fix reported warnings. Then
please run `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

2. No bindings in TXT. They must com ine DT schema. It is no 2017
anymore. Please reach to your colleagues in Amazon for some internal
guidance on upstreaming. Such big companies should perform basic
internal review instead of asking community to explain that basic stuff.

3. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC. It might happen, that command when run on an
older kernel, gives you outdated entries. Therefore please be sure you
base your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.



...


> +
> +static struct i2c_driver ozl003_driver = {
> +	.driver = {
> +		.owner = THIS_MODULE,

Please do no send downstream, junk code. This was fixed years ago.

> +		.name = "ozl003",
> +		.of_match_table = ozl003_match_table,
> +	},
> +	.id_table = ozl003_id,
> +	.probe = ozl003_probe,
> +	.remove = ozl003_remove,
> +};
> +

...

> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:ozl003-led");

Drop as well. Useless or incorrect.

Best regards,
Krzysztof
anish kumar Nov. 6, 2024, 3:48 p.m. UTC | #2
On Wed, Nov 6, 2024 at 2:46 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 06/11/2024 02:54, anishkmr@amazon.com wrote:
> > From: Anish Kumar <anishkmr@amazon.com>
> >
> > LED Driver for O2 Micro LED IC
> >
> > reviewed-by: Anish Kumar <yesanishhere@gmail.com>
> > Signed-off-by: Karthik Poduval <kpoduval@lab126.com>
> > Signed-off-by: Yue Hu <yhuamzn@amazon.com>
> > ---
> >  .../devicetree/bindings/leds/leds-ozl003.txt  |  23 ++
>
> 1. Please run scripts/checkpatch.pl and fix reported warnings. Then
> please run `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.

It was run on the code but not on this text file. Missed that.
>
> 2. No bindings in TXT. They must com ine DT schema. It is no 2017
> anymore. Please reach to your colleagues in Amazon for some internal
> guidance on upstreaming. Such big companies should perform basic
> internal review instead of asking community to explain that basic stuff.

Will convert this to YAML and send.
>
> 3. Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC. It might happen, that command when run on an
> older kernel, gives you outdated entries. Therefore please be sure you
> base your patches on recent Linux kernel.

We did but our mistake is we cloned the wrong git, we cloned
git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git

>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.

./get_maintainers returned wrong values because of wrong tree.

>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.

will add that.
>
> Please kindly resend and include all necessary To/Cc entries.

Will do. Thanks for review.
>
>
>
> ...
>
>
> > +
> > +static struct i2c_driver ozl003_driver = {
> > +     .driver = {
> > +             .owner = THIS_MODULE,
>
> Please do no send downstream, junk code. This was fixed years ago.

ok.
>
> > +             .name = "ozl003",
> > +             .of_match_table = ozl003_match_table,
> > +     },
> > +     .id_table = ozl003_id,
> > +     .probe = ozl003_probe,
> > +     .remove = ozl003_remove,
> > +};
> > +
>
> ...
>
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:ozl003-led");
>
> Drop as well. Useless or incorrect.

ok.
>
> Best regards,
> Krzysztof
>
Pavel Machek Nov. 6, 2024, 5:49 p.m. UTC | #3
Hi!

> +++ b/Documentation/devicetree/bindings/leds/leds-ozl003.txt
> @@ -0,0 +1,23 @@
> +*O2 Micro Compact LED Strobe Light Controller
> +
> +Compact LED strobe light controller, can be controlled by I2C or via a
> +PWM gpio controlled.
> +
> +Required properties:
> +- compatible : "o2micro,ozl003"

o2micro needs to be registered as a prefix somewhere.

Best regards,
								Pavel
anish kumar Nov. 6, 2024, 10:03 p.m. UTC | #4
On Wed, Nov 6, 2024 at 9:49 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > +++ b/Documentation/devicetree/bindings/leds/leds-ozl003.txt
> > @@ -0,0 +1,23 @@
> > +*O2 Micro Compact LED Strobe Light Controller
> > +
> > +Compact LED strobe light controller, can be controlled by I2C or via a
> > +PWM gpio controlled.
> > +
> > +Required properties:
> > +- compatible : "o2micro,ozl003"
>
> o2micro needs to be registered as a prefix somewhere.

Wondering if adding hereDocumentation/devicetree/bindings/vendor-prefixes.yaml
not sufficient? I added that in the same patch though, I guess
I will have to split the patch to add that first and then the
driver.

>
> Best regards,
>                                                                 Pavel
> --
> People of Russia, stop Putin before his war on Ukraine escalates.
Pavel Machek Nov. 8, 2024, 10:13 a.m. UTC | #5
On Wed 2024-11-06 14:03:24, anish kumar wrote:
> On Wed, Nov 6, 2024 at 9:49 AM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > +++ b/Documentation/devicetree/bindings/leds/leds-ozl003.txt
> > > @@ -0,0 +1,23 @@
> > > +*O2 Micro Compact LED Strobe Light Controller
> > > +
> > > +Compact LED strobe light controller, can be controlled by I2C or via a
> > > +PWM gpio controlled.
> > > +
> > > +Required properties:
> > > +- compatible : "o2micro,ozl003"
> >
> > o2micro needs to be registered as a prefix somewhere.
> 
> Wondering if adding hereDocumentation/devicetree/bindings/vendor-prefixes.yaml
> not sufficient? I added that in the same patch though, I guess
> I will have to split the patch to add that first and then the
> driver.

Doing that in same patch is ok, sorry if I missed that.

BR,
								Pavel
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-ozl003.txt b/Documentation/devicetree/bindings/leds/leds-ozl003.txt
new file mode 100644
index 000000000000..9dbd78ed1093
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-ozl003.txt
@@ -0,0 +1,23 @@ 
+*O2 Micro Compact LED Strobe Light Controller
+
+Compact LED strobe light controller, can be controlled by I2C or via a
+PWM gpio controlled.
+
+Required properties:
+- compatible : "o2micro,ozl003"
+- #address-cells: must be 1
+- #size-cells: must be 0
+- reg: I2C slave address. depends on the model.
+
+Optional properties:
+- gpio-mode: if set then controlled via gpio
+
+Examples:
+
+irled: ozl003@4b {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+	compatible = "o2micro,ozl003";
+	reg = <0x4B>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index f6064d84a424..06453649c0e7 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -796,6 +796,8 @@  patternProperties:
     description: NVIDIA
   "^nxp,.*":
     description: NXP Semiconductors
+  "^o2micro,.*":
+    description: O2Micro Ltd.
   "^oceanic,.*":
     description: Oceanic Systems (UK) Ltd.
   "^oct,.*":
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..ddc5bc0886af 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -732,6 +732,12 @@  config LEDS_OT200
 	  This option enables support for the LEDs on the Bachmann OT200.
 	  Say Y to enable LEDs on the Bachmann OT200.
 
+config LEDS_OZL003
+	tristate "O2 Micro OZL003 Compact LED Strobe Light Controller"
+	depends on LEDS_CLASS && I2C
+	help
+	  This option enables support for O2 Micro LED IC.
+
 config LEDS_MENF21BMC
 	tristate "LED support for the MEN 14F021P00 BMC"
 	depends on LEDS_CLASS && MFD_MENF21BMC
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..b89c8747466d 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -73,6 +73,7 @@  obj-$(CONFIG_LEDS_NETXBIG)		+= leds-netxbig.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
 obj-$(CONFIG_LEDS_NS2)			+= leds-ns2.o
 obj-$(CONFIG_LEDS_OT200)		+= leds-ot200.o
+obj-$(CONFIG_LEDS_OZL003)		+= leds-ozl003.o
 obj-$(CONFIG_LEDS_PCA9532)		+= leds-pca9532.o
 obj-$(CONFIG_LEDS_PCA955X)		+= leds-pca955x.o
 obj-$(CONFIG_LEDS_PCA963X)		+= leds-pca963x.o
diff --git a/drivers/leds/leds-ozl003.c b/drivers/leds/leds-ozl003.c
new file mode 100644
index 000000000000..0b6a98a2ab19
--- /dev/null
+++ b/drivers/leds/leds-ozl003.c
@@ -0,0 +1,306 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+#include <linux/of_gpio.h>
+
+/* Register mapping */
+#define OPERATION_MODE			(0x00)
+#define ISEN1_REG_SETTING		(0x01)
+#define ISEN2_REG_SETTING		(0x02)
+#define DURATION_WDT			(0x03)
+#define STATUS_REG			(0x04)
+#define PROTECTION_THRES		(0x05)
+
+/* Register bit masks */
+#define OPERATION_MODE_CNTRL_MSK	(0x01)
+#define OPERATION_MODE_ENA_MSK		(0x02)
+#define OPERATION_MODE_VLED_MSK		(0xFC)
+#define ISEN_REG_MSK			(0x80)
+#define DURATION_WDT_LED_MSK		(0x80)
+
+/* Default Register values */
+#define ISEN1_CURRENT			(0x7D)
+#define ISEN2_CURRENT			(0x7D)
+#define DEFAULT_STROBE_OP		(0x00)
+#define DEFAULT_VLED_OUTPUT		(0x00)
+#define DEFAULT_PROT_THRES		(0x06)
+#define DEFAULT_WDT_AND_I2C_DISABLE	(0x00)
+
+#define OZL003_MAX_BRIGHTNESS		(127)
+
+struct ozl003 {
+	struct i2c_client *client;
+	bool gpio_mode;
+	struct led_classdev led_dev;
+	enum led_brightness brightness;
+	struct mutex lock;
+	struct work_struct work;
+};
+
+static int ozl003_i2c_read_byte(struct ozl003 *ozl003, u8 addr, u8 *val)
+{
+	int retval;
+	struct i2c_client *client = ozl003->client;
+	struct i2c_adapter *adap = client->adapter;
+	struct i2c_msg msg[2];
+
+	msg[0].addr = client->addr;
+	msg[0].len = 1;
+	msg[0].flags = 0;
+	msg[0].buf = &addr;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 1;
+	msg[1].buf = val;
+
+	retval = i2c_transfer(adap, msg, 2);
+	if (retval < 0)
+		return retval;
+	return (retval == 2) ? 0 : -EIO;
+}
+
+static int ozl003_i2c_write_byte(struct ozl003 *ozl003, u8 addr, u8 val)
+{
+	int retval;
+	u8 buf[2];
+	struct i2c_client *client = ozl003->client;
+
+	buf[0] = addr;
+	buf[1] = val;
+
+	retval = i2c_master_send(client, buf, 2);
+	if (retval < 0)
+		return retval;
+	return (retval == 2) ? 0 : -EIO;
+}
+
+static int ozl003_set_led_operation(struct ozl003 *ozl003, bool on)
+{
+	int ret;
+	u8 val;
+
+	/* If we are using gpio to toggle LED, no need to set register */
+	if (ozl003->gpio_mode)
+		return 0;
+
+	ret = ozl003_i2c_read_byte(ozl003, DURATION_WDT, &val);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed getting WDT register ret=%d\n", ret);
+		return ret;
+	}
+	if (on)
+		val |= DURATION_WDT_LED_MSK;
+	else
+		val &= (~DURATION_WDT_LED_MSK);
+	ret = ozl003_i2c_write_byte(ozl003, DURATION_WDT, val);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting WDT register ret=%d\n", ret);
+	}
+	return ret;
+}
+
+static int ozl003_set_led_brightness(struct ozl003 *ozl003, enum led_brightness brt_val)
+{
+	int ret = 0;
+	u8 s1_current;
+	u8 s2_current;
+
+	s1_current = s2_current = brt_val;
+
+	ret = ozl003_i2c_write_byte(ozl003, ISEN1_REG_SETTING, s1_current);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting SEN1 current register ret=%d\n", ret);
+		return ret;
+	}
+	ret = ozl003_i2c_write_byte(ozl003, ISEN2_REG_SETTING, s2_current);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting SEN2 current ret=%d\n", ret);
+		return ret;
+	}
+	return ret;
+
+}
+
+static void ozl003_led_brightness_work(struct work_struct *work)
+{
+	struct ozl003 *ozl003 = container_of(work, struct ozl003, work);
+
+	mutex_lock(&ozl003->lock);
+
+	if (ozl003->brightness == LED_OFF) {
+		ozl003_set_led_operation(ozl003, false);
+	} else {
+		ozl003_set_led_brightness(ozl003, ozl003->brightness);
+		ozl003_set_led_operation(ozl003, true);
+	}
+
+	mutex_unlock(&ozl003->lock);
+}
+
+static void ozl003_brightness_set(struct led_classdev *led_cdev,
+		enum led_brightness brt_val)
+{
+	struct ozl003 *ozl003 = container_of(led_cdev, struct ozl003, led_dev);
+	bool update_brightness = false;
+
+	mutex_lock(&ozl003->lock);
+
+	if (brt_val != ozl003->brightness)
+		update_brightness = true;
+
+	ozl003->brightness = brt_val;
+
+	mutex_unlock(&ozl003->lock);
+
+	if (update_brightness)
+		schedule_work(&ozl003->work);
+}
+
+static int ozl003_init(struct ozl003 *ozl003)
+{
+	int ret;
+	u8 mode = DEFAULT_VLED_OUTPUT;
+	u8 s1_current = 0;
+	u8 s2_current = 0;
+
+	if (ozl003->gpio_mode) {
+		mode &= (~OPERATION_MODE_CNTRL_MSK);
+	} else { /* I2C mode */
+		mode |= OPERATION_MODE_CNTRL_MSK;
+	}
+
+	ret = ozl003_i2c_write_byte(ozl003, OPERATION_MODE, mode);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting OP register ret=%d\n", ret);
+		return ret;
+	}
+
+	ret = ozl003_i2c_write_byte(ozl003, ISEN1_REG_SETTING, s1_current);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting SEN1 current register ret=%d\n", ret);
+		return ret;
+	}
+
+	ret = ozl003_i2c_write_byte(ozl003, ISEN2_REG_SETTING, s2_current);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting SEN2 current ret=%d\n", ret);
+		return ret;
+	}
+
+	/* disable the delay timer */
+	ret = ozl003_i2c_write_byte(ozl003, DURATION_WDT, DEFAULT_WDT_AND_I2C_DISABLE);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting WDT register ret=%d\n", ret);
+	}
+
+	/* enable the IC */
+	mode |= OPERATION_MODE_ENA_MSK;
+	ret = ozl003_i2c_write_byte(ozl003, OPERATION_MODE, mode);
+	if (unlikely(ret)) {
+		dev_err(&(ozl003->client->dev),
+				"Failed setting OP register ret=%d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int ozl003_probe(struct i2c_client *client,
+		const struct i2c_device_id *id)
+{
+	struct ozl003 *ozl003;
+	struct device_node *np = client->dev.of_node;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		dev_err(&client->dev, "i2c_check_functionality error\n");
+		return -EIO;
+	}
+
+	ozl003 = devm_kzalloc(&client->dev, sizeof(struct ozl003), GFP_KERNEL);
+	if (!ozl003)
+		return -ENOMEM;
+	ozl003->client = client;
+	i2c_set_clientdata(client, ozl003);
+	mutex_init(&ozl003->lock);
+	INIT_WORK(&ozl003->work, ozl003_led_brightness_work);
+
+	if (client->dev.of_node) {
+		ozl003->gpio_mode = of_property_read_bool(np, "gpio-mode");
+		dev_info(&client->dev, "gpio-mode %d\n", ozl003->gpio_mode);
+	}
+
+
+	ozl003->led_dev.max_brightness = OZL003_MAX_BRIGHTNESS;
+	ozl003->led_dev.brightness_set = ozl003_brightness_set;
+	ozl003->led_dev.name = "ozl003";
+
+	ret = ozl003_init(ozl003);
+	if (ret)
+		goto err;
+
+	ret = led_classdev_register(&client->dev, &ozl003->led_dev);
+	if (ret) {
+		dev_err(&client->dev, "led register err: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	return ret;
+}
+
+static int ozl003_remove(struct i2c_client *client)
+{
+	struct ozl003 *ozl003 = i2c_get_clientdata(client);
+
+	led_classdev_unregister(&ozl003->led_dev);
+	return 0;
+}
+
+static const struct i2c_device_id ozl003_id[] = {
+	{ "ozl003", 0 },
+	{ }
+};
+
+static const struct of_device_id ozl003_match_table[] = {
+	{.compatible = "o2micro,ozl003",},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, ozl003_id);
+
+static struct i2c_driver ozl003_driver = {
+	.driver = {
+		.owner = THIS_MODULE,
+		.name = "ozl003",
+		.of_match_table = ozl003_match_table,
+	},
+	.id_table = ozl003_id,
+	.probe = ozl003_probe,
+	.remove = ozl003_remove,
+};
+
+module_i2c_driver(ozl003_driver);
+
+MODULE_AUTHOR("Yue Hu <yhuamzn@amazon.com>");
+MODULE_AUTHOR("Karthik Poduval <kpoduval@lab126.com>");
+MODULE_DESCRIPTION("O2 Micro LED Controller driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ozl003-led");