Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/11/20 1:24 AM, Gene Chen wrote:
Pavel Machek 於 2020年9月11日 週五 下午3:05寫道：
Hi!
+{
+struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+struct mt6360_priv *priv = led->priv;
+u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+u32 prev = priv->fled_torch_used, curr;
+int ret;
+
+dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+if (priv->fled_strobe_used) {
+dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
Doesn't hardware handle that? IOW, what happens when you have enabled
both torch and flash? If flash just overrides torch mode, than you
should not prevent enabling torch in this case.
Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.
Could I get you to remove code you are not commenting at when
reviewing?
MT6360 FLED register define is STROBE_EN/TORCH_EN/CS1/CS2 (current
source) 4 bits.
The STROBE_EN/TORCH_EN is shared by FLED1 and FLED2.
If I want to enable FLED1 torch mode, I set TORCH_EN and CS1
If I want to enable FLED2 strobe mode, I set STROBE_EN and CS2
For example I set FLED1 torch, then I set FLED2 strobe.
When I set FLED2 strobe, I will see the strobe current is FLED2 add
FLED1 current which is not I want.
So I need disable FLED1 torch first.
Considering every circumstances is complicated when share same H/W
logic control.
And the other problem is torch mode switch to strobe mode needs ramp
time because strobe and torch mode can't be co-exist.
Thank you for the explanation. So we have to keep your guards
but I would return -EBUSY instead of -EINVAL.
This would be also consistent with what
drivers/media/v4l2-core/v4l2-flash-led-class.c
does in its v4l2_flash_s_ctrl(), case V4L2_CID_FLASH_STROBE - it returns
-EBUSY if __software_strobe_mode_inactive() returns false.
The advantage of V4L2 Flash interface is that it has LED_MODE that
can be set to torch or flash, but in LED subsystem we don't have
the counterpart.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi Pavel,
On 9/11/20 9:05 AM, Pavel Machek wrote:
Hi!
+{
+ struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+ struct mt6360_priv *priv = led->priv;
+ u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+ u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+ u32 prev = priv->fled_torch_used, curr;
+ int ret;
+
+ dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+ if (priv->fled_strobe_used) {
+ dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
Doesn't hardware handle that? IOW, what happens when you have enabled
both torch and flash? If flash just overrides torch mode, than you
should not prevent enabling torch in this case.
Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.
What you say now is even more confusing when we look at your ack
under this patch:
commit 7aea8389a77abf9fde254aca2434a605c7704f58
Author: Jacek Anaszewski
Date: Fri Jan 9 07:22:51 2015 -0800
leds: Add LED Flash class extension to the LED subsystem
Some LED devices support two operation modes - torch and flash.
This patch provides support for flash LED devices in the LED subsystem
by introducing new sysfs attributes and kernel internal interface.
The attributes being introduced are: flash_brightness, flash_strobe,
flash_timeout, max_flash_timeout, max_flash_brightness, flash_fault,
flash_sync_strobe and available_sync_leds. All the flash related
features are placed in a separate module.
The modifications aim to be compatible with V4L2 framework requirements
related to the flash devices management. The design assumes that V4L2
sub-device can take of the LED class device control and communicate
with it through the kernel internal interface. When V4L2 Flash
sub-device
file is opened, the LED class device sysfs interface is made
unavailable.
Signed-off-by: Jacek Anaszewski
Acked-by: Kyungmin Park
Cc: Richard Purdie
Acked-by: Pavel Machek
Signed-off-by: Bryan Wu
Could I get you to remove code you are not commenting at when
reviewing?
+MODULE_AUTHOR("Gene Chen ");
+MODULE_DESCRIPTION("MT6360 Led Driver");
Led -> LED.
Pavel
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Pavel Machek 於 2020年9月11日 週五 下午3:05寫道：
>
> Hi!
>
> > >+{
> > >+struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
> > >flash.led_cdev);
> > >+struct mt6360_priv *priv = led->priv;
> > >+u32 enable_mask = MT6360_TORCHEN_MASK |
> > >MT6360_FLCSEN_MASK(led->led_no);
> > >+u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > >+u32 prev = priv->fled_torch_used, curr;
> > >+int ret;
> > >+
> > >+dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > >+if (priv->fled_strobe_used) {
> > >+dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> > >priv->fled_strobe_used);
> >
> > Doesn't hardware handle that? IOW, what happens when you have enabled
> > both torch and flash? If flash just overrides torch mode, than you
> > should not prevent enabling torch in this case.
>
> Yep, this is strange/confusing... and was reason why I asked for not
> supporting strobe from sysfs.
>
> Could I get you to remove code you are not commenting at when
> reviewing?
>
MT6360 FLED register define is STROBE_EN/TORCH_EN/CS1/CS2 (current
source) 4 bits.
The STROBE_EN/TORCH_EN is shared by FLED1 and FLED2.
If I want to enable FLED1 torch mode, I set TORCH_EN and CS1
If I want to enable FLED2 strobe mode, I set STROBE_EN and CS2
For example I set FLED1 torch, then I set FLED2 strobe.
When I set FLED2 strobe, I will see the strobe current is FLED2 add
FLED1 current which is not I want.
So I need disable FLED1 torch first.
Considering every circumstances is complicated when share same H/W
logic control.
And the other problem is torch mode switch to strobe mode needs ramp
time because strobe and torch mode can't be co-exist.
> > >+MODULE_AUTHOR("Gene Chen ");
> > >+MODULE_DESCRIPTION("MT6360 Led Driver");
>
> Led -> LED.
>
ACK
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi!
> >+{
> >+struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
> >flash.led_cdev);
> >+struct mt6360_priv *priv = led->priv;
> >+u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> >+u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> >+u32 prev = priv->fled_torch_used, curr;
> >+int ret;
> >+
> >+dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> >+if (priv->fled_strobe_used) {
> >+dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> >priv->fled_strobe_used);
>
> Doesn't hardware handle that? IOW, what happens when you have enabled
> both torch and flash? If flash just overrides torch mode, than you
> should not prevent enabling torch in this case.
Yep, this is strange/confusing... and was reason why I asked for not
supporting strobe from sysfs.
Could I get you to remove code you are not commenting at when
reviewing?
> >+MODULE_AUTHOR("Gene Chen ");
> >+MODULE_DESCRIPTION("MT6360 Led Driver");
Led -> LED.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi Gene,
Thanks for the update.
On 9/7/20 12:27 PM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
and 4-channel RGB LED support Register/Flash/Breath Mode
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 681 +
3 files changed, 693 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..94a6d83 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,17 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS_FLASH && OF
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on MFD_MT6360
+ help
+ This option enables support for dual Flash LED drivers found on
+ Mediatek MT6360 PMIC.
+ Independent current sources supply for each flash LED support torch
and strobe mode.
+ Includes Low-VF and short protection.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7a..5596427 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532) += leds-rb532.o
obj-$(CONFIG_LEDS_MLXCPLD)+= leds-mlxcpld.o
obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
+obj-$(CONFIG_LEDS_MT6360) += leds-mt6360.o
obj-$(CONFIG_LEDS_NET48XX)+= leds-net48xx.o
obj-$(CONFIG_LEDS_NETXBIG)+= leds-netxbig.o
obj-$(CONFIG_LEDS_NIC78BX)+= leds-nic78bx.o
diff --git a/drivers/leds/leds-mt6360.c b/drivers/leds/leds-mt6360.c
new file mode 100644
index 000..bd6fa48
--- /dev/null
+++ b/drivers/leds/leds-mt6360.c
@@ -0,0 +1,681 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2020 MediaTek Inc.
+//
+// Author: Gene Chen
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+enum {
+ MT6360_LED_ISNK1 = 0,
+ MT6360_LED_ISNK2,
+ MT6360_LED_ISNK3,
+ MT6360_LED_ISNK4,
+ MT6360_LED_FLASH1,
+ MT6360_LED_FLASH2,
+ MT6360_MAX_LEDS,
+};
+
+#define MT6360_REG_RGBEN 0x380
+#define MT6360_REG_ISNK(_led_no) (0x381 + (_led_no))
+#define MT6360_ISNK_ENMASK(_led_no)BIT(7 - (_led_no))
+#define MT6360_ISNK_MASK 0x1F
+#define MT6360_CHRINDSEL_MASK BIT(3)
+
+#define MT6360_REG_FLEDEN 0x37E
+#define MT6360_REG_STRBTO 0x373
+#define MT6360_REG_FLEDBASE(_id) (0x372 + 4 * (_id - MT6360_LED_FLASH1))
+#define MT6360_REG_FLEDISTRB(_id) (MT6360_REG_FLEDBASE(_id) + 2)
+#define MT6360_REG_FLEDITOR(_id) (MT6360_REG_FLEDBASE(_id) + 3)
+#define MT6360_REG_CHGSTAT20x3E1
+#define MT6360_REG_FLEDSTAT1 0x3E9
+#define MT6360_ITORCH_MASK GENMASK(4, 0)
+#define MT6360_ISTROBE_MASKGENMASK(6, 0)
+#define MT6360_STRBTO_MASK GENMASK(6, 0)
+#define MT6360_TORCHEN_MASKBIT(3)
+#define MT6360_STROBEN_MASKBIT(2)
+#define MT6360_FLCSEN_MASK(_id)BIT(MT6360_LED_FLASH2 - _id)
+#define MT6360_FLEDCHGVINOVP_MASK BIT(3)
+#define MT6360_FLED1STRBTO_MASKBIT(11)
+#define MT6360_FLED2STRBTO_MASKBIT(10)
+#define MT6360_FLED1STRB_MASK BIT(9)
+#define MT6360_FLED2STRB_MASK BIT(8)
+#define MT6360_FLED1SHORT_MASK BIT(7)
+#define MT6360_FLED2SHORT_MASK BIT(6)
+#define MT6360_FLEDLVF_MASKBIT(3)
+
+/* 0 means led_off, add one for dummy */
+#define MT6360_ISNK1_MAXLEVEL 13
+#define MT6360_ISNK4_MAXLEVEL 31
+
+#define MT6360_ITORCH_MIN 25000
+#define MT6360_ITORCH_STEP 12500
+#define MT6360_ITORCH_MAX 40
+#define MT6360_ISTRB_MIN 5
+#define MT6360_ISTRB_STEP 12500
+#define MT6360_ISTRB_MAX 150
+#define MT6360_STRBTO_MIN 64000
+#define MT6360_STRBTO_STEP 32000
+#define MT6360_STRBTO_MAX 2432000
+
+#define FLED_TORCH_FLAG_MASK 0x0c
+#define FLED_TORCH_FLAG_SHFT 2
+#define FLED_STROBE_FLAG_MASK 0x03
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct mt6360_led {
+ union {
+ struct led_classdev isnk;
+ struct led_classdev_flash flash;
+ };
+ struct v4l2_flash *v4l2_flash;
+ struct
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/10/20 10:25 PM, Pavel Machek wrote: Hi! 1. set FLED1 brightness # echo 1 > /sys/class/leds/white:flash1/flash_brightness 2. enable FLED1 strobe # echo 1 > /sys/class/leds/white:flash1/flash_strobe 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode flash leds must be turned off) # echo 0 > /sys/class/leds/white:flash1/flash_strobe I believe I'd preffer only exposing torch functionality in /sys/class/leds. .. strobe can be supported using v4l2 APIs. Actually having LED flash class without strobe is pointless. If you looked at led_classdev_flash_register_ext() you would see that it fails with uninitialized strobe_set op. And V4L2 API for strobing flash calls strobe_set from LED flash class beneath. That was the idea behind LED and V4L2 flash API unification - there is one hardware driver needed, the V4L2 Flash layer just takes over control over it when needed. I agree that one driver is enough. But we should not need flash_strobe file in sysfs. Simply use V4L2 for that. Apart from the fact that we can't remove it from LED flash class ABI now, why would you like to prevent the user from exploiting main feature of the hardware? -- Best regards, Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi! > > > 1. set FLED1 brightness > > > # echo 1 > /sys/class/leds/white:flash1/flash_brightness > > > 2. enable FLED1 strobe > > > # echo 1 > /sys/class/leds/white:flash1/flash_strobe > > > 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode > > > flash leds must be turned off) > > > # echo 0 > /sys/class/leds/white:flash1/flash_strobe > > > > I believe I'd preffer only exposing torch functionality in > > /sys/class/leds. .. strobe can be supported using v4l2 APIs. > > Actually having LED flash class without strobe is pointless. > If you looked at led_classdev_flash_register_ext() you would see that > it fails with uninitialized strobe_set op. And V4L2 API for strobing > flash calls strobe_set from LED flash class beneath. > > That was the idea behind LED and V4L2 flash API unification - there > is one hardware driver needed, the V4L2 Flash layer just takes over > control over it when needed. I agree that one driver is enough. But we should not need flash_strobe file in sysfs. Simply use V4L2 for that. Best regards, Pavel
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On 9/10/20 2:29 PM, Pavel Machek wrote:
Hi!
+{
+ struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
flash.led_cdev);
+ struct mt6360_priv *priv = led->priv;
+ u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
+ u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
+ u32 prev = priv->fled_torch_used, curr;
+ int ret;
+
+ dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
+ if (priv->fled_strobe_used) {
+ dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
priv->fled_strobe_used);
+ return -EINVAL;
+ }
So... how does its userland interface look like?
1. set FLED1 brightness
# echo 1 > /sys/class/leds/white:flash1/flash_brightness
2. enable FLED1 strobe
# echo 1 > /sys/class/leds/white:flash1/flash_strobe
3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
flash leds must be turned off)
# echo 0 > /sys/class/leds/white:flash1/flash_strobe
I believe I'd preffer only exposing torch functionality in
/sys/class/leds. .. strobe can be supported using v4l2 APIs.
Actually having LED flash class without strobe is pointless.
If you looked at led_classdev_flash_register_ext() you would see that
it fails with uninitialized strobe_set op. And V4L2 API for strobing
flash calls strobe_set from LED flash class beneath.
That was the idea behind LED and V4L2 flash API unification - there
is one hardware driver needed, the V4L2 Flash layer just takes over
control over it when needed.
--
Best regards,
Jacek Anaszewski
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi!
> > > +{
> > > + struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
> > > flash.led_cdev);
> > > + struct mt6360_priv *priv = led->priv;
> > > + u32 enable_mask = MT6360_TORCHEN_MASK |
> > > MT6360_FLCSEN_MASK(led->led_no);
> > > + u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > > + u32 prev = priv->fled_torch_used, curr;
> > > + int ret;
> > > +
> > > + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > > + if (priv->fled_strobe_used) {
> > > + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> > > priv->fled_strobe_used);
> > > + return -EINVAL;
> > > + }
> >
> > So... how does its userland interface look like?
> >
>
> 1. set FLED1 brightness
> # echo 1 > /sys/class/leds/white:flash1/flash_brightness
> 2. enable FLED1 strobe
> # echo 1 > /sys/class/leds/white:flash1/flash_strobe
> 3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
> flash leds must be turned off)
> # echo 0 > /sys/class/leds/white:flash1/flash_strobe
I believe I'd preffer only exposing torch functionality in
/sys/class/leds. .. strobe can be supported using v4l2 APIs.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: PGP signature
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On Thu 2020-09-10 14:34:54, Andy Shevchenko wrote:
> On Thu, Sep 10, 2020 at 11:18 AM Pavel Machek wrote:
>
> ...
>
> > > > > +enum {
> > > > > + MT6360_LED_ISNK1 = 0,
> > > > > + MT6360_LED_ISNK2,
> > > > > + MT6360_LED_ISNK3,
> > > > > + MT6360_LED_ISNK4,
> > > > > + MT6360_LED_FLASH1,
> > > > > + MT6360_LED_FLASH2,
> > > >
> > > > > + MT6360_MAX_LEDS,
> > > >
> > > > No comma for terminator entry.
> > > >
> > >
> > > ACK
> >
> > Actually, that comma is fine. Its absence would be fine, too.
>
> It is slightly better not to have to prevent (theoretical) rebase or
> other similar issues when a new item can go behind the terminator. In
> such a case compiler can easily tell you if something is wrong.
Okay, I see your point.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: PGP signature
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On Thu, Sep 10, 2020 at 11:11 AM Gene Chen wrote:
> Andy Shevchenko 於 2020年9月9日 週三 下午9:48寫道：
> > On Mon, Sep 7, 2020 at 1:31 PM Gene Chen
> > wrote:
> > > From: Gene Chen
...
> > > + if (priv->fled_strobe_used) {
> > > + dev_warn(lcdev->dev, "Please disable strobe first
> > > [%d]\n", priv->fled_strobe_used);
> > > + return -EINVAL;
> >
> > Hmm... Shouldn't be guaranteed by some framework?
> >
>
> Because both Flash LED use single logically control.
> It doesn't exist one LED is torch mode, and the other is strobe mode.
You mean you have always an attribute for hardware even if it doesn't
support a feature?
Can you consider hiding attributes?
...
> > > + lcdev->max_brightness = (val - MT6360_ITORCH_MIN) /
> > > MT6360_ITORCH_STEP + 1;
> >
> > DIV_ROUND_UP(val - MT6360_ITORCH_MIN, MT6360_ITORCH_STEP) ?
> >
>
> This is mapping 0~val to 1~max_brightness as level.
> I convert val below MT6360_ITORCH_STEP to 1 for ignore max_brightness
> = 0, because 0 means disable.
> There is a little difference from DIV_ROUND_UP.
What div_round_up does is
(x + y - 1) / y
What do you do
x / y + 1 = (x + y)/y = ((x + 1) + y - 1)/y = DIV_ROUND_UP(x+1,y)
So, DIV_ROUND_UP(val - MT6360_ITORCH_MIN + 1, MT6360_ITORCH_STEP) ?
(yes I made classical off-by-one error)
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On Thu, Sep 10, 2020 at 11:18 AM Pavel Machek wrote:
...
> > > > +enum {
> > > > + MT6360_LED_ISNK1 = 0,
> > > > + MT6360_LED_ISNK2,
> > > > + MT6360_LED_ISNK3,
> > > > + MT6360_LED_ISNK4,
> > > > + MT6360_LED_FLASH1,
> > > > + MT6360_LED_FLASH2,
> > >
> > > > + MT6360_MAX_LEDS,
> > >
> > > No comma for terminator entry.
> > >
> >
> > ACK
>
> Actually, that comma is fine. Its absence would be fine, too.
It is slightly better not to have to prevent (theoretical) rebase or
other similar issues when a new item can go behind the terminator. In
such a case compiler can easily tell you if something is wrong.
> > > > +};
...
> > > > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > > > + { .compatible = "mediatek,mt6360-led", },
> > >
> > > > + {},
> > >
> > > No need comma.
> >
> > ACK
>
> It is also no hurting comma.
Same explanation. It doesn't hurt per se, but its absence might serve a purpose.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi!
> > > +enum {
> > > + MT6360_LED_ISNK1 = 0,
> > > + MT6360_LED_ISNK2,
> > > + MT6360_LED_ISNK3,
> > > + MT6360_LED_ISNK4,
> > > + MT6360_LED_FLASH1,
> > > + MT6360_LED_FLASH2,
> >
> > > + MT6360_MAX_LEDS,
> >
> > No comma for terminator entry.
> >
>
> ACK
Actually, that comma is fine. Its absence would be fine, too.
> > > +};
> >
> > ...
> >
> > > +#define MT6360_ISNK_MASK 0x1F
> >
> > GENMASK()
Again, that is fine.
> > > +#define FLED_TORCH_FLAG_MASK 0x0c
> >
> > > +#define FLED_STROBE_FLAG_MASK 0x03
> >
> > GENMASK()
> >
>
> ACK
Again, that is fine.
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > > + { .compatible = "mediatek,mt6360-led", },
> >
> > > + {},
> >
> > No need comma.
>
> ACK
It is also no hurting comma.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Andy Shevchenko 於 2020年9月9日 週三 下午9:48寫道：
>
> On Mon, Sep 7, 2020 at 1:31 PM Gene Chen wrote:
> >
> > From: Gene Chen
> >
> > Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> > and 4-channel RGB LED support Register/Flash/Breath Mode
>
> I'm wondering why you don't use struct led_classdev_flash.
>
> ...
>
Both Flash and RGB LED use led_classdev_flash by
"devm_led_classdev_flash_register_ext".
> > +//
> > +// Copyright (C) 2020 MediaTek Inc.
> > +//
>
> Do you really need these two // lines?
>
ACK, I will remove it
> ...
>
> > +enum {
> > + MT6360_LED_ISNK1 = 0,
> > + MT6360_LED_ISNK2,
> > + MT6360_LED_ISNK3,
> > + MT6360_LED_ISNK4,
> > + MT6360_LED_FLASH1,
> > + MT6360_LED_FLASH2,
>
> > + MT6360_MAX_LEDS,
>
> No comma for terminator entry.
>
ACK
> > +};
>
> ...
>
> > +#define MT6360_ISNK_MASK 0x1F
>
> GENMASK()
>
> ...
>
> > +#define MT6360_ITORCH_MIN 25000
> > +#define MT6360_ITORCH_STEP 12500
> > +#define MT6360_ITORCH_MAX 40
> > +#define MT6360_ISTRB_MIN 5
> > +#define MT6360_ISTRB_STEP 12500
> > +#define MT6360_ISTRB_MAX 150
> > +#define MT6360_STRBTO_MIN 64000
> > +#define MT6360_STRBTO_STEP 32000
> > +#define MT6360_STRBTO_MAX 2432000
>
> Add unit suffixes, please.
>
ACK
> ...
>
> > +#define FLED_TORCH_FLAG_MASK 0x0c
>
> > +#define FLED_STROBE_FLAG_MASK 0x03
>
> GENMASK()
>
ACK
> ...
>
> > + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>
> Not production noise.
>
ACK
> ...
>
> > + ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN,
> > enable_mask, val);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
>
> return regmap...
>
> > + u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
>
> Why parens?
>
ACK
> ...
>
> > + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
>
> Noise.
>
ACK
> ...
>
> > + if (priv->fled_strobe_used) {
> > + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> > priv->fled_strobe_used);
> > + return -EINVAL;
>
> Hmm... Shouldn't be guaranteed by some framework?
>
Because both Flash LED use single logically control.
It doesn't exist one LED is torch mode, and the other is strobe mode.
> ...
>
> > + curr = prev & (~BIT(led->led_no));
>
> Too many parens.
>
ACK
> ...
>
> > +static int mt6360_strobe_brightness_set(struct led_classdev_flash
> > *fl_cdev, u32 brightness)
> > +{
> > + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led,
> > flash);
> > + struct led_classdev *lcdev = _cdev->led_cdev;
> > +
>
> > + dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no,
> > brightness);
>
> Noise. Point of this entire function?
>
ACK, I will remove it, reserve function entry only for register
ledcass_dev check ops exist
> > + return 0;
> > +}
>
> ...
>
> > + dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
>
> Noise.
>
> If you wish to do it right, add trace events to the framework.
>
ACK, I will remove it.
> ...
>
> > + if (priv->fled_torch_used) {
>
> > + dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n",
> > priv->fled_torch_used);
>
> Again, why the warning? Can this be a part of the framework?
>
Same as above.
> > + return -EINVAL;
> > + }
>
> ...
>
> > + curr = prev & (~BIT(led->led_no));
>
> Too many parens.
>
ACK
> ...
>
> > + if (!prev && curr)
> > + usleep_range(5000, 6000);
> > + else if (prev && !curr)
> > + udelay(500);
>
> These delays must be explained.
>
ACK
> ...
>
> > + if (led->led_no == MT6360_LED_FLASH1) {
> > + strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> > + fled_short_mask = MT6360_FLED1SHORT_MASK;
>
> > +
>
> Redundant blank line.
>
ACK
> > + } else {
> > + strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> > + fled_short_mask = MT6360_FLED2SHORT_MASK;
> > + }
>
> ...
>
> > +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash,
> > bool enable)
> > +{
> > + struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> > + struct mt6360_led *led = container_of(flash, struct mt6360_led,
> > flash);
> > + struct mt6360_priv *priv = led->priv;
>
> > + u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
>
> enable_mask -> mask
> u32 value = enable ? mask : 0;
>
> > + int ret;
> > +
> > + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN,
> > enable_mask,
>
> > +enable ? enable_mask : 0);
>
> ret = ... mask, value);
>
ACK
> > + if (ret)
> > + return ret;
> > +
> > + if
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Pavel Machek 於 2020年9月9日 週三 上午6:25寫道：
>
> Hi!
>
> > From: Gene Chen
> >
> > Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> > and 4-channel RGB LED support Register/Flash/Breath Mode
> >
> > Signed-off-by: Gene Chen
> > ---
> > drivers/leds/Kconfig | 11 +
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-mt6360.c | 681
> > +
> > 3 files changed, 693 insertions(+)
> > create mode 100644 drivers/leds/leds-mt6360.c
> >
> > + help
> > + This option enables support for dual Flash LED drivers found on
> > + Mediatek MT6360 PMIC.
> > + Independent current sources supply for each flash LED support torch
> > and strobe mode.
> > + Includes Low-VF and short protection.
> > +
>
> 80 columns. And perhaps user does not need to know about protections... and
> actually
> about independend sources, either.
>
ACK
> "Enable this for RGB LED and flash LED support on..."?
>
> > +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum
> > led_brightness level)
> > +{
> > + struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
> > flash.led_cdev);
> > + struct mt6360_priv *priv = led->priv;
> > + u32 enable_mask = MT6360_TORCHEN_MASK |
> > MT6360_FLCSEN_MASK(led->led_no);
> > + u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> > + u32 prev = priv->fled_torch_used, curr;
> > + int ret;
> > +
> > + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> > + if (priv->fled_strobe_used) {
> > + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> > priv->fled_strobe_used);
> > + return -EINVAL;
> > + }
>
> So... how does its userland interface look like?
>
1. set FLED1 brightness
# echo 1 > /sys/class/leds/white:flash1/flash_brightness
2. enable FLED1 strobe
# echo 1 > /sys/class/leds/white:flash1/flash_strobe
3 . turn off FLED1 strobe (just used to gaurantee the strobe mode
flash leds must be turned off)
# echo 0 > /sys/class/leds/white:flash1/flash_strobe
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
On Mon, Sep 7, 2020 at 1:31 PM Gene Chen wrote:
>
> From: Gene Chen
>
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 4-channel RGB LED support Register/Flash/Breath Mode
I'm wondering why you don't use struct led_classdev_flash.
...
> +//
> +// Copyright (C) 2020 MediaTek Inc.
> +//
Do you really need these two // lines?
...
> +enum {
> + MT6360_LED_ISNK1 = 0,
> + MT6360_LED_ISNK2,
> + MT6360_LED_ISNK3,
> + MT6360_LED_ISNK4,
> + MT6360_LED_FLASH1,
> + MT6360_LED_FLASH2,
> + MT6360_MAX_LEDS,
No comma for terminator entry.
> +};
...
> +#define MT6360_ISNK_MASK 0x1F
GENMASK()
...
> +#define MT6360_ITORCH_MIN 25000
> +#define MT6360_ITORCH_STEP 12500
> +#define MT6360_ITORCH_MAX 40
> +#define MT6360_ISTRB_MIN 5
> +#define MT6360_ISTRB_STEP 12500
> +#define MT6360_ISTRB_MAX 150
> +#define MT6360_STRBTO_MIN 64000
> +#define MT6360_STRBTO_STEP 32000
> +#define MT6360_STRBTO_MAX 2432000
Add unit suffixes, please.
...
> +#define FLED_TORCH_FLAG_MASK 0x0c
> +#define FLED_STROBE_FLAG_MASK 0x03
GENMASK()
...
> + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
Not production noise.
...
> + ret = regmap_update_bits(priv->regmap, MT6360_REG_RGBEN, enable_mask,
> val);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap...
> + u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
Why parens?
...
> + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
Noise.
...
> + if (priv->fled_strobe_used) {
> + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> priv->fled_strobe_used);
> + return -EINVAL;
Hmm... Shouldn't be guaranteed by some framework?
...
> + curr = prev & (~BIT(led->led_no));
Too many parens.
...
> +static int mt6360_strobe_brightness_set(struct led_classdev_flash *fl_cdev,
> u32 brightness)
> +{
> + struct mt6360_led *led = container_of(fl_cdev, struct mt6360_led,
> flash);
> + struct led_classdev *lcdev = _cdev->led_cdev;
> +
> + dev_dbg(lcdev->dev, "[%d] strobe brightness %d\n", led->led_no,
> brightness);
Noise. Point of this entire function?
> + return 0;
> +}
...
> + dev_dbg(lcdev->dev, "[%d] strobe state %d\n", led->led_no, state);
Noise.
If you wish to do it right, add trace events to the framework.
...
> + if (priv->fled_torch_used) {
> + dev_warn(lcdev->dev, "Please disable torch first [0x%x]\n",
> priv->fled_torch_used);
Again, why the warning? Can this be a part of the framework?
> + return -EINVAL;
> + }
...
> + curr = prev & (~BIT(led->led_no));
Too many parens.
...
> + if (!prev && curr)
> + usleep_range(5000, 6000);
> + else if (prev && !curr)
> + udelay(500);
These delays must be explained.
...
> + if (led->led_no == MT6360_LED_FLASH1) {
> + strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> + fled_short_mask = MT6360_FLED1SHORT_MASK;
> +
Redundant blank line.
> + } else {
> + strobe_timeout_mask = MT6360_FLED2STRBTO_MASK;
> + fled_short_mask = MT6360_FLED2SHORT_MASK;
> + }
...
> +static int mt6360_flash_external_strobe_set(struct v4l2_flash *v4l2_flash,
> bool enable)
> +{
> + struct led_classdev_flash *flash = v4l2_flash->fled_cdev;
> + struct mt6360_led *led = container_of(flash, struct mt6360_led,
> flash);
> + struct mt6360_priv *priv = led->priv;
> + u32 enable_mask = MT6360_FLCSEN_MASK(led->led_no);
enable_mask -> mask
u32 value = enable ? mask : 0;
> + int ret;
> +
> + ret = regmap_update_bits(priv->regmap, MT6360_REG_FLEDEN, enable_mask,
> +enable ? enable_mask : 0);
ret = ... mask, value);
> + if (ret)
> + return ret;
> +
> + if (enable)
> + priv->fled_strobe_used |= BIT(led->led_no);
> + else
> + priv->fled_strobe_used &= (~BIT(led->led_no));
Too many parens.
> +
> + return 0;
> +}
...
> + s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
Ditto.
...
> +static void clamp_align(u32 *v, u32 min, u32 max, u32 step)
Can we keep a similar API, i.e. return a new value rather than update old?
> +{
> + *v = clamp_val(*v, min, max);
I would rather use a temporary variable (and it actually will be
required with above).
> + if (step > 1)
> + *v = (*v - min) / step * step + min;
Sounds like open coded rounddown().
> +}
...
> + lcdev->max_brightness = (val - MT6360_ITORCH_MIN) /
> MT6360_ITORCH_STEP + 1;
DIV_ROUND_UP(val -
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Hi!
> From: Gene Chen
>
> Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
> and 4-channel RGB LED support Register/Flash/Breath Mode
>
> Signed-off-by: Gene Chen
> ---
> drivers/leds/Kconfig | 11 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-mt6360.c | 681
> +
> 3 files changed, 693 insertions(+)
> create mode 100644 drivers/leds/leds-mt6360.c
>
> + help
> + This option enables support for dual Flash LED drivers found on
> + Mediatek MT6360 PMIC.
> + Independent current sources supply for each flash LED support torch
> and strobe mode.
> + Includes Low-VF and short protection.
> +
80 columns. And perhaps user does not need to know about protections... and
actually
about independend sources, either.
"Enable this for RGB LED and flash LED support on..."?
> +static int mt6360_torch_brightness_set(struct led_classdev *lcdev, enum
> led_brightness level)
> +{
> + struct mt6360_led *led = container_of(lcdev, struct mt6360_led,
> flash.led_cdev);
> + struct mt6360_priv *priv = led->priv;
> + u32 enable_mask = MT6360_TORCHEN_MASK | MT6360_FLCSEN_MASK(led->led_no);
> + u32 val = (level) ? MT6360_FLCSEN_MASK(led->led_no) : 0;
> + u32 prev = priv->fled_torch_used, curr;
> + int ret;
> +
> + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
> + if (priv->fled_strobe_used) {
> + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> priv->fled_strobe_used);
> + return -EINVAL;
> + }
So... how does its userland interface look like?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Gene
On 9/7/20 5:27 AM, Gene Chen wrote:
From: Gene Chen
Add MT6360 LED driver include 2-channel Flash LED with torch/strobe mode,
and 4-channel RGB LED support Register/Flash/Breath Mode
Signed-off-by: Gene Chen
---
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-mt6360.c | 681 +
3 files changed, 693 insertions(+)
create mode 100644 drivers/leds/leds-mt6360.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c181df..94a6d83 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -271,6 +271,17 @@ config LEDS_MT6323
This option enables support for on-chip LED drivers found on
Mediatek MT6323 PMIC.
+config LEDS_MT6360
+ tristate "LED Support for Mediatek MT6360 PMIC"
+ depends on LEDS_CLASS_FLASH && OF
+ depends on V4L2_FLASH_LED_CLASS || !V4L2_FLASH_LED_CLASS
+ depends on MFD_MT6360
+ help
+ This option enables support for dual Flash LED drivers found on
+ Mediatek MT6360 PMIC.
+ Independent current sources supply for each flash LED support torch
and strobe mode.
+ Includes Low-VF and short protection.
+
config LEDS_S3C24XX
tristate "LED Support for Samsung S3C24XX GPIO LEDs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c2c7d7a..5596427 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_LEDS_MIKROTIK_RB532) += leds-rb532.o
obj-$(CONFIG_LEDS_MLXCPLD)+= leds-mlxcpld.o
obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
+obj-$(CONFIG_LEDS_MT6360) += leds-mt6360.o
obj-$(CONFIG_LEDS_NET48XX)+= leds-net48xx.o
obj-$(CONFIG_LEDS_NETXBIG)+= leds-netxbig.o
obj-$(CONFIG_LEDS_NIC78BX)+= leds-nic78bx.o
diff --git a/drivers/leds/leds-mt6360.c b/drivers/leds/leds-mt6360.c
new file mode 100644
index 000..bd6fa48
--- /dev/null
+++ b/drivers/leds/leds-mt6360.c
@@ -0,0 +1,681 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright (C) 2020 MediaTek Inc.
+//
+// Author: Gene Chen
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+enum {
+ MT6360_LED_ISNK1 = 0,
+ MT6360_LED_ISNK2,
+ MT6360_LED_ISNK3,
+ MT6360_LED_ISNK4,
+ MT6360_LED_FLASH1,
+ MT6360_LED_FLASH2,
+ MT6360_MAX_LEDS,
+};
+
+#define MT6360_REG_RGBEN 0x380
+#define MT6360_REG_ISNK(_led_no) (0x381 + (_led_no))
+#define MT6360_ISNK_ENMASK(_led_no)BIT(7 - (_led_no))
+#define MT6360_ISNK_MASK 0x1F
+#define MT6360_CHRINDSEL_MASK BIT(3)
+
+#define MT6360_REG_FLEDEN 0x37E
+#define MT6360_REG_STRBTO 0x373
+#define MT6360_REG_FLEDBASE(_id) (0x372 + 4 * (_id - MT6360_LED_FLASH1))
+#define MT6360_REG_FLEDISTRB(_id) (MT6360_REG_FLEDBASE(_id) + 2)
+#define MT6360_REG_FLEDITOR(_id) (MT6360_REG_FLEDBASE(_id) + 3)
+#define MT6360_REG_CHGSTAT20x3E1
+#define MT6360_REG_FLEDSTAT1 0x3E9
+#define MT6360_ITORCH_MASK GENMASK(4, 0)
+#define MT6360_ISTROBE_MASKGENMASK(6, 0)
+#define MT6360_STRBTO_MASK GENMASK(6, 0)
+#define MT6360_TORCHEN_MASKBIT(3)
+#define MT6360_STROBEN_MASKBIT(2)
+#define MT6360_FLCSEN_MASK(_id)BIT(MT6360_LED_FLASH2 - _id)
+#define MT6360_FLEDCHGVINOVP_MASK BIT(3)
+#define MT6360_FLED1STRBTO_MASKBIT(11)
+#define MT6360_FLED2STRBTO_MASKBIT(10)
+#define MT6360_FLED1STRB_MASK BIT(9)
+#define MT6360_FLED2STRB_MASK BIT(8)
+#define MT6360_FLED1SHORT_MASK BIT(7)
+#define MT6360_FLED2SHORT_MASK BIT(6)
+#define MT6360_FLEDLVF_MASKBIT(3)
+
+/* 0 means led_off, add one for dummy */
+#define MT6360_ISNK1_MAXLEVEL 13
+#define MT6360_ISNK4_MAXLEVEL 31
+
+#define MT6360_ITORCH_MIN 25000
+#define MT6360_ITORCH_STEP 12500
+#define MT6360_ITORCH_MAX 40
+#define MT6360_ISTRB_MIN 5
+#define MT6360_ISTRB_STEP 12500
+#define MT6360_ISTRB_MAX 150
+#define MT6360_STRBTO_MIN 64000
+#define MT6360_STRBTO_STEP 32000
+#define MT6360_STRBTO_MAX 2432000
+
+#define FLED_TORCH_FLAG_MASK 0x0c
+#define FLED_TORCH_FLAG_SHFT 2
+#define FLED_STROBE_FLAG_MASK 0x03
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct mt6360_led {
+ union {
+ struct led_classdev isnk;
+ struct led_classdev_flash flash;
+ };
+ struct v4l2_flash *v4l2_flash;
+ struct mt6360_priv *priv;
+ u32
