| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230309172448.GU9667@google.com>
Date: Thu, 9 Mar 2023 17:24:48 +0000
From: Lee Jones <lee@...nel.org>
To: ChiYuan Huang <cy_huang@...htek.com>
Cc: ChiaEn Wu <chiaen_wu@...htek.com>, corbet@....net, pavel@....cz,
matthias.bgg@...il.com, andriy.shevchenko@...ux.intel.com,
jacek.anaszewski@...il.com,
angelogioacchino.delregno@...labora.com, linux-doc@...r.kernel.org,
peterwu.pub@...il.com, linux-leds@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
szunichen@...il.com
Subject: Re: [PATCH v17 RESEND 2/3] leds: flash: mt6370: Add MediaTek MT6370
flashlight support
On Thu, 09 Mar 2023, ChiYuan Huang wrote:
> On Wed, Mar 08, 2023 at 01:54:33PM +0000, Lee Jones wrote:
> Hi, Lee:
> > On Tue, 07 Mar 2023, ChiYuan Huang wrote:
> >
> > > Hi, Lee:
> > > Reply below the comments.
> > >
> > > On Sun, Mar 05, 2023 at 10:06:08AM +0000, Lee Jones wrote:
> > > > On Thu, 23 Feb 2023, ChiaEn Wu wrote:
> > > >
> > > > > From: ChiYuan Huang <cy_huang@...htek.com>
> > > > >
> > > > > The MediaTek MT6370 is a highly-integrated smart power management IC,
> > > > > which includes a single cell Li-Ion/Li-Polymer switching battery
> > > > > charger, a USB Type-C & Power Delivery (PD) controller, dual Flash
> > > > > LED current sources, a RGB LED driver, a backlight WLED driver,
> > > > > a display bias driver and a general LDO for portable devices.
> > > > >
> > > > > Add support for the MT6370 Flash LED driver. Flash LED in MT6370
> > > > > has 2 channels and support torch/strobe mode.
> > > > >
> > > > > Acked-by: Jacek Anaszewski <jacek.anaszewski@...il.com>
> > > > > Co-developed-by: Alice Chen <alice_chen@...htek.com>
> > > > > Signed-off-by: Alice Chen <alice_chen@...htek.com>
> > > > > Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> > > > > Signed-off-by: ChiaEn Wu <chiaen_wu@...htek.com>
> > > > > ---
> > > > > v17
> > > > > - Update the year of Copyright from 2022 to 2023
> > > > >
> > > > > ---
> > > > > drivers/leds/flash/Kconfig | 13 +
> > > > > drivers/leds/flash/Makefile | 1 +
> > > > > drivers/leds/flash/leds-mt6370-flash.c | 596 +++++++++++++++++++++++++++++++++
> > > > > 3 files changed, 610 insertions(+)
> > > > > create mode 100644 drivers/leds/flash/leds-mt6370-flash.c
> >
> > [...]
> >
> > > > > +static int _mt6370_flash_brightness_set(struct led_classdev_flash *fl_cdev,
> > > > > + u32 brightness)
> > > > > +{
> > > > > + struct mt6370_led *led = to_mt6370_led(fl_cdev, flash);
> > > > > + struct mt6370_priv *priv = led->priv;
> > > > > + struct led_flash_setting *setting = &fl_cdev->brightness;
> > > > > + u32 val = (brightness - setting->min) / setting->step;
> > > > > + int ret, i;
> > > > > +
> > > > > + if (led->led_no == MT6370_LED_JOINT) {
> > > >
> > > > What is a "JOINT"?
> > > >
> > > Since MT6370 has two flash led channels. Per channel can drive the current up to 1.5A.
> > > 'JOINT' case is used if 1.5A driving current is not enough, like as flash current 2A.
> > > They can use two channels to drive 'one' flash led by the HW application.
> > > This will make the driving current larger than the capability of one channel.
> >
> > Is "joint" the term used in the datasheet?
> >
> Nope, this term is not clearly defined in the datasheet. but this kind of HW application is
> allowed.
> > Please make this definition clear in the code.
> >
> > If I'm asking, others are likely to too.
> >
> Thanks, I'll add more comments to clearly describe what the 'JOINT' code did.
> > [...]
> >
> > > > > +static int mt6370_init_flash_properties(struct device *dev,
> > > > > + struct mt6370_led *led,
> > > > > + struct fwnode_handle *fwnode)
> > > > > +{
> > > > > + struct led_classdev_flash *flash = &led->flash;
> > > > > + struct led_classdev *lcdev = &flash->led_cdev;
> > > > > + struct mt6370_priv *priv = led->priv;
> > > > > + struct led_flash_setting *s;
> > > > > + u32 sources[MT6370_MAX_LEDS];
> > > > > + u32 max_ua, val;
> > > > > + int i, ret, num;
> > > > > +
> > > > > + num = fwnode_property_count_u32(fwnode, "led-sources");
> > > > > + if (num < 1)
> > > > > + return dev_err_probe(dev, -EINVAL,
> > > > > + "Not specified or wrong number of led-sources\n");
> > > > > +
> > > > > + ret = fwnode_property_read_u32_array(fwnode, "led-sources", sources, num);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + for (i = 0; i < num; i++) {
> > > > > + if (sources[i] >= MT6370_MAX_LEDS)
> > > > > + return -EINVAL;
> > > > > + if (priv->leds_active & BIT(sources[i]))
> > > > > + return -EINVAL;
> > > > > + priv->leds_active |= BIT(sources[i]);
> > > > > + }
> > > > > +
> > > > > + led->led_no = num == 2 ? MT6370_LED_JOINT : sources[0];
> > > > > +
> > > > > + max_ua = num == 2 ? MT6370_ITORCH_DOUBLE_MAX_uA : MT6370_ITORCH_MAX_uA;
> > > > > + val = MT6370_ITORCH_MIN_uA;
> > > >
> > > > In what scenario does this not get overwritten?
> > > >
> > > Only if the property is missing. This will make the value keep in minimum.
> >
> > If the property is missing, fwnode_property_read_u32() returns an errno, no?
> >
> > If that's the case, val will be over-written in the if() clause?
> >
> In this funciton, three properties needs to be paresed from DT. Each one need to clamp the value.
> There're two ways to write the code.
>
> [Option 1]
> ret = fwnode_property_read_u32(...)
> if (ret)
> val = MIM_uA;
>
> val = mt6370_clamp(val, MIN, MAX);
>
> [Option 2]
> val = MIN_uA;
> if (!ret)
> val = mt6370_clamp(val, MIN, MAX);
>
>
> From the above, the sencond one can save more LOC, no?
> But it seems the first one is more preferable by you, right?
I see now that 'val' is used in clamp() before being overwritten now.
--
Lee Jones [李琼斯]
Powered by blists - more mailing lists