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@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxx>
> > > > Co-developed-by: Alice Chen <alice_chen@xxxxxxxxxxx>
> > > > Signed-off-by: Alice Chen <alice_chen@xxxxxxxxxxx>
> > > > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > > > Signed-off-by: ChiaEn Wu <chiaen_wu@xxxxxxxxxxx>
> > > > ---
> > > > 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?
> --
> Lee Jones [李琼斯]
