From: Gene Chen <gene.chen.richtek@gmail.com>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>,
Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Dan Murphy <dmurphy@ti.com>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Gene Chen <gene_chen@richtek.com>,
Wilma.Wu@mediatek.com, shufan_lee@richtek.com,
cy_huang@richtek.com, benjamin.chao@mediatek.com
Subject: Re: [PATCH v3 1/2] leds: mt6360: Add LED driver for MT6360
Date: Thu, 10 Sep 2020 08:11:06 +0800 [thread overview]
Message-ID: <CAE+NS35FETQ9ASJeYP=Sa8dm7ohRBcdAwUioCAnHPY2TiD4pNA@mail.gmail.com> (raw)
In-Reply-To: <CAHp75VdLDvoQicP1nLnjOiit6qjaw9n7+LuJ-J3MtaoHUOa_2g@mail.gmail.com>
Andy Shevchenko <andy.shevchenko@gmail.com> 於 2020年9月9日 週三 下午9:48寫道:
>
> On Mon, Sep 7, 2020 at 1:31 PM Gene Chen <gene.chen.richtek@gmail.com> wrote:
> >
> > From: Gene Chen <gene_chen@richtek.com>
> >
> > 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 400000
> > +#define MT6360_ISTRB_MIN 50000
> > +#define MT6360_ISTRB_STEP 12500
> > +#define MT6360_ISTRB_MAX 1500000
> > +#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 = &fl_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 (enable)
> > + priv->fled_strobe_used |= BIT(led->led_no);
> > + else
> > + priv->fled_strobe_used &= (~BIT(led->led_no));
>
> Too many parens.
>
ACK
> > +
> > + return 0;
> > +}
>
> ...
>
> > + s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
>
> Ditto.
>
ACK
> ...
>
> > +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().
>
ACK
> > +}
>
> ...
>
> > + 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.
> ...
>
> > +static int mt6360_init_common_properties(struct mt6360_led *led, struct led_init_data *init_data)
> > +{
> > + const char *str;
> > +
> > + if (!fwnode_property_read_string(init_data->fwnode, "default-state", &str)) {
> > + if (!strcmp(str, "on"))
> > + led->default_state = STATE_ON;
> > + else if (!strcmp(str, "keep"))
> > + led->default_state = STATE_KEEP;
>
> > + else
>
> I wouldn't allow some garbage to be off.
>
ACK
> > + led->default_state = STATE_OFF;
> > + }
>
> What about
>
> static const char * const states = { "on", "keep", "off" };
>
> int ret;
>
> ret = match_string(states, ARRAY_SIZE(states), str);
> if (ret)
> ...
>
> default_state = ret;
>
> ?
>
> > + return 0;
> > +}
>
ACK
> ...
>
> > +static int mt6360_led_probe(struct platform_device *pdev)
> > +{
> > + struct mt6360_priv *priv;
> > + struct fwnode_handle *child;
> > + int i, ret;
> > +
>
> > + priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > + if (!priv->regmap) {
> > + dev_err(&pdev->dev, "Failed to get parent regmap\n");
> > + return -ENODEV;
> > + }
>
> ...
>
> > +out:
>
> out_flash_leds_release: ?
>
ACK
> > + for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> > + struct mt6360_led *led = priv->leds[i];
> > +
> > + if (led && led->v4l2_flash)
> > + v4l2_flash_release(led->v4l2_flash);
> > +
> > + }
>
> ...
>
> > +static int mt6360_led_remove(struct platform_device *pdev)
> > +{
> > + struct mt6360_priv *priv = platform_get_drvdata(pdev);
> > + int i;
> > +
> > + for (i = MT6360_LED_FLASH1; i <= MT6360_LED_FLASH2; i++) {
> > + struct mt6360_led *led = priv->leds[i];
> > +
> > + if (led && led->v4l2_flash)
> > + v4l2_flash_release(led->v4l2_flash);
> > +
> > + }
>
> Looks like a code duplication.
>
ACK
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> > + { .compatible = "mediatek,mt6360-led", },
>
> > + {},
>
> No need comma.
>
ACK
> > +};
>
>
> --
> With Best Regards,
> Andy Shevchenko
next prev parent reply other threads:[~2020-09-10 8:11 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-07 10:27 [PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360 Gene Chen
2020-09-07 10:27 ` [PATCH v3 1/2] " Gene Chen
2020-09-08 14:13 ` Dan Murphy
2020-09-08 22:25 ` Pavel Machek
2020-09-09 23:49 ` Gene Chen
2020-09-10 12:29 ` Pavel Machek
2020-09-10 20:23 ` Jacek Anaszewski
2020-09-10 20:25 ` Pavel Machek
2020-09-10 20:31 ` Jacek Anaszewski
2020-09-09 13:48 ` Andy Shevchenko
2020-09-10 0:11 ` Gene Chen [this message]
2020-09-10 8:18 ` Pavel Machek
2020-09-10 11:34 ` Andy Shevchenko
2020-09-10 12:28 ` Pavel Machek
2020-09-10 11:46 ` Andy Shevchenko
2020-09-10 21:42 ` Jacek Anaszewski
2020-09-11 7:05 ` Pavel Machek
2020-09-10 23:24 ` Gene Chen
2020-09-11 21:21 ` Jacek Anaszewski
2020-09-11 20:56 ` Jacek Anaszewski
2020-09-07 10:27 ` [PATCH v3 2/2] dt-bindings: leds: Add bindings for MT6360 LED Gene Chen
2020-09-08 13:55 ` Dan Murphy
2020-09-08 22:22 ` Pavel Machek
2020-09-15 15:51 ` Rob Herring
2020-09-08 14:14 ` [PATCH v3 0/2] leds: mt6360: Add LED driver for MT6360 Dan Murphy
2020-09-09 0:00 ` Gene Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAE+NS35FETQ9ASJeYP=Sa8dm7ohRBcdAwUioCAnHPY2TiD4pNA@mail.gmail.com' \
--to=gene.chen.richtek@gmail.com \
--cc=Wilma.Wu@mediatek.com \
--cc=andy.shevchenko@gmail.com \
--cc=benjamin.chao@mediatek.com \
--cc=cy_huang@richtek.com \
--cc=devicetree@vger.kernel.org \
--cc=dmurphy@ti.com \
--cc=gene_chen@richtek.com \
--cc=jacek.anaszewski@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=shufan_lee@richtek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).