Hi!
> +
> +#define MT6360_LED_DESC(_id) { \
> + .cdev = { \
> + .name = "mt6360_isink" #_id, \
> + .max_brightness = MT6360_SINKCUR_MAX##_id, \
> + .brightness_set_blocking = mt6360_led_brightness_set, \
> + .brightness_get = mt6360_led_brightness_get, \
> + .blink_set = mt6360_led_blink_set, \
> + }, \
> + .index = MT6360_LED_ISINK##_id, \
> + .currsel_reg = MT6360_CURRSEL_REG##_id, \
> + .currsel_mask = MT6360_CURRSEL_MASK##_id, \
> + .enable_mask = MT6360_LEDEN_MASK##_id, \
> + .mode_reg = MT6360_LEDMODE_REG##_id, \
> + .mode_mask = MT6360_LEDMODE_MASK##_id, \
> + .pwmduty_reg = MT6360_PWMDUTY_REG##_id, \
> + .pwmduty_mask = MT6360_PWMDUTY_MASK##_id, \
> + .pwmfreq_reg = MT6360_PWMFREQ_REG##_id, \
> + .pwmfreq_mask = MT6360_PWMFREQ_MASK##_id, \
> + .breath_regbase = MT6360_BREATH_REGBASE##_id, \
> +}
> +
> +/* ISINK 1/2/3 for RGBLED, ISINK4 for MoonLight */
> +static const struct mt6360_led_classdev def_led_classdev[MT6360_LED_MAX] = {
> + MT6360_LED_DESC(1),
> + MT6360_LED_DESC(2),
> + MT6360_LED_DESC(3),
> + MT6360_LED_DESC(4),
> +};
While this is pretty interesting abuse of the macros, please get rid
of it. I'm sure code will look better as a result.
> +static int mt6360_fled_strobe_set(
> + struct led_classdev_flash *fled_cdev, bool state)
> +{
> + struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> + struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> + struct mt6360_fled_classdev *mtfled_cdev = (void *)fled_cdev;
> + int id = mtfled_cdev->index, ret;
> +
> + if (!(state ^ test_bit(id, &mld->fl_strobe_flags)))
> + return 0;
Yup, and you can abuse xor operator too. Don't do that. I believe you
wanted to say:
+ if (state == test_bit(id, &mld->fl_strobe_flags))
+ return 0;
> + if (mt6360_fled_check_flags_if_any(&mld->fl_torch_flags)) {
> + dev_err(led_cdev->dev,
> + "Disable all leds torch [%lu]\n", mld->fl_torch_flags);
> + return -EINVAL;
> + }
> + ret = regmap_update_bits(mld->regmap, mtfled_cdev->cs_enable_reg,
> + mtfled_cdev->cs_enable_mask, state ? 0xff : 0);
> + if (ret < 0) {
> + dev_err(led_cdev->dev, "Fail to set cs enable [%d]\n", state);
> + goto out_strobe_set;
> + }
Just return ret; no need for goto. (Please fix globally).
> +static int mt6360_fled_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct led_classdev_flash *lcf = lcdev_to_flcdev(led_cdev);
> + struct mt6360_led_data *mld = dev_get_drvdata(led_cdev->dev->parent);
> + struct mt6360_fled_classdev *mtfled_cdev = (void *)lcf;
> + int id = mtfled_cdev->index, shift, keep, ret;
> +
> + if (mt6360_fled_check_flags_if_any(&mld->fl_strobe_flags)) {
> + dev_err(led_cdev->dev,
> + "Disable all leds strobe [%lu]\n", mld->fl_strobe_flags);
> + return -EINVAL;
> + }
> + if (brightness == LED_OFF) {
> + clear_bit(id, &mld->fl_torch_flags);
> + keep = mt6360_fled_check_flags_if_any(&mld->fl_torch_flags);
> + ret = regmap_update_bits(mld->regmap,
> + mtfled_cdev->torch_enable_reg,
> + mtfled_cdev->torch_enable_mask,
> + keep ? 0xff : 0);
> + if (ret < 0) {
> + dev_err(led_cdev->dev, "Fail to set torch disable\n");
> + goto out_bright_set;
> + }
> + ret = regmap_update_bits(mld->regmap,
> + mtfled_cdev->cs_enable_reg,
> + mtfled_cdev->cs_enable_mask, 0);
> + if (ret < 0)
> + dev_err(led_cdev->dev, "Fail to set torch disable\n");
> + goto out_bright_set;
> + }
Should turning off the led go to separate function?
> +#define MT6360_FLED_DESC(_id) { \
> + .fl_cdev = { \
> + .led_cdev = { \
> + .name = "mt6360_fled_ch" #_id, \
> + .max_brightness = MT6360_TORBRIGHT_MAX##_id, \
> + .brightness_set_blocking = mt6360_fled_brightness_set, \
> + .brightness_get = mt6360_fled_brightness_get, \
> + .flags = LED_DEV_CAP_FLASH, \
> + }, \
> + .brightness = { \
> + .min = MT6360_STROBECUR_MIN, \
> + .step = MT6360_STROBECUR_STEP, \
> + .max = MT6360_STROBECUR_MAX, \
> + .val = MT6360_STROBECUR_MIN, \
> + }, \
> + .timeout = { \
> + .min = MT6360_STRBTIMEOUT_MIN, \
> + .step = MT6360_STRBTIMEOUT_STEP, \
> + .max = MT6360_STRBTIMEOUT_MAX, \
> + .val = MT6360_STRBTIMEOUT_MIN, \
> + }, \
> + .ops = &mt6360_fled_ops, \
> + }, \
> + .index = MT6360_FLED_CH##_id, \
> + .cs_enable_reg = MT6360_CSENABLE_REG##_id, \
> + .cs_enable_mask = MT6360_CSENABLE_MASK##_id, \
> + .torch_bright_reg = MT6360_TORBRIGHT_REG##_id, \
> + .torch_bright_mask = MT6360_TORBRIGHT_MASK##_id, \
> + .torch_enable_reg = MT6360_TORENABLE_REG##_id, \
> + .torch_enable_mask = MT6360_TORENABLE_MASK##_id, \
> + .strobe_bright_reg = MT6360_STRBRIGHT_REG##_id, \
> + .strobe_bright_mask = MT6360_STRBRIGHT_MASK##_id, \
> + .strobe_enable_reg = MT6360_STRBENABLE_REG##_id, \
> + .strobe_enable_mask = MT6360_STRBENABLE_MASK##_id, \
> +}
> +
> +static const struct mt6360_fled_classdev def_fled_classdev[MT6360_FLED_MAX] = {
> + MT6360_FLED_DESC(1),
> + MT6360_FLED_DESC(2),
> +};
Yeah, well, no.
> @@ -236,5 +241,4 @@ struct mt6360_pmu_data {
> #define CHIP_VEN_MASK (0xF0)
> #define CHIP_VEN_MT6360 (0x50)
> #define CHIP_REV_MASK (0x0F)
> -
> #endif /* __MT6360_H__ */
This one is unrelated and not really an improvement.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Attachment:
signature.asc
Description: PGP signature
