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

[Lee Jones <lee@kernel.org>]

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@richtek.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@gmail.com>
> > > > > Co-developed-by: Alice Chen <alice_chen@richtek.com>
> > > > > Signed-off-by: Alice Chen <alice_chen@richtek.com>
> > > > > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > > > > Signed-off-by: ChiaEn Wu <chiaen_wu@richtek.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 [李琼斯]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel