Re: [PATCH v17 RESEND 2/3] leds: flash: mt6370: Add MediaTek MT6370 flashlight support

[ChiYuan Huang]

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 [李琼斯]