Re: [PATCH] leds: mt6360: Add LED driver for MT6360

[Pavel Machek]

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