On Mon, Sep 7, 2020 at 1:31 PM Gene Chen <gene.chen.richtek@xxxxxxxxx> wrote:
>
> From: Gene Chen <gene_chen@xxxxxxxxxxx>
>
> 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.
...
> +//
> +// Copyright (C) 2020 MediaTek Inc.
> +//
Do you really need these two // lines?
...
> +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.
> +};
...
> +#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.
...
> +#define FLED_TORCH_FLAG_MASK 0x0c
> +#define FLED_STROBE_FLAG_MASK 0x03
GENMASK()
...
> + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
Not production noise.
...
> + 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?
...
> + dev_dbg(lcdev->dev, "[%d] brightness %d\n", led->led_no, level);
Noise.
...
> + 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?
...
> + curr = prev & (~BIT(led->led_no));
Too many parens.
...
> +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?
> + 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.
...
> + 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?
> + return -EINVAL;
> + }
...
> + curr = prev & (~BIT(led->led_no));
Too many parens.
...
> + if (!prev && curr)
> + usleep_range(5000, 6000);
> + else if (prev && !curr)
> + udelay(500);
These delays must be explained.
...
> + if (led->led_no == MT6360_LED_FLASH1) {
> + strobe_timeout_mask = MT6360_FLED1STRBTO_MASK;
> + fled_short_mask = MT6360_FLED1SHORT_MASK;
> +
Redundant blank line.
> + } 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);
> + 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.
> +
> + return 0;
> +}
...
> + s->val = s->max = (s->min) + (lcdev->max_brightness - 1) * s->step;
Ditto.
...
> +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().
> +}
...
> + lcdev->max_brightness = (val - MT6360_ITORCH_MIN) / MT6360_ITORCH_STEP + 1;
DIV_ROUND_UP(val - MT6360_ITORCH_MIN, MT6360_ITORCH_STEP) ?
...
> +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.
> + 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;
> +}
...
> +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: ?
> + 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.
> +
> + return 0;
> +}
> +
> +static const struct of_device_id __maybe_unused mt6360_led_of_id[] = {
> + { .compatible = "mediatek,mt6360-led", },
> + {},
No need comma.
> +};
--
With Best Regards,
Andy Shevchenko
