Skip to contentSkip to conversation

backlight: aat2870_bl: questions about setting max_current

PATCH Axel Lin 2 participants 4 messages Jul 6, 2011

Subsystems (3)
Framebuffer layer Multifunction devices (MFD) Backlight class/subsystem
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

hi Jin,
Just found some questions about setting max_current in current implementation of aat2870_bl:

1. Seems the implementation tests wrong value for setting aat2870_bl->max_current. I think you mean:

drivers/video/backlight/aat2870_bl.c +1 -1
index f13a3f7..a8a2545 100644
@@ -175,7 +175,7 @@ static int aat2870_bl_probe(struct platform_device *pdev)
        else
                aat2870_bl->channels = AAT2870_BL_CH_ALL;
 
-       if (pdata->max_brightness > 0)
+       if (pdata->max_current > 0)
                aat2870_bl->max_current = pdata->max_current;
        else
                aat2870_bl->max_current = AAT2870_CURRENT_27_9;

2. Even above issue is fixed, you still cannot differentiate below 2 cases:

        a) if pdata->max_current is not set , or
        b) pdata->max_current is set to AAT2870_CURRENT_0_45 ( which is also 0 ).

In either case, current implementation will set max_current to AAT2870_CURRENT_27_9.

Regards,
Axel

#1 lore raw

Hi, Axel

Quoted 2 lines

1. Seems the implementation tests wrong value for setting aat2870_bl->max_current. I think you mean:

Right, that's my mean. This is my mistyping bug.

Quoted 4 lines

2. Even above issue is fixed, you still cannot differentiate below 2 cases:

         a) if pdata->max_current is not set , or
         b) pdata->max_current is set to AAT2870_CURRENT_0_45 ( which is also 0 ).
    In either case, current implementation will set max_current to AAT2870_CURRENT_27_9.

I didn't find it when verifying driver on real device and my self review. Thanks for your kindly review.

Below is fixed codes. How about this change?
If there is no problem, could you submit this change with first change as one patch? because two changes are related.

drivers/video/backlight/aat2870_bl.c +1 -1
index 4952a61..c0e2ce7 100644
@@ -44,7 +44,7 @@ static inline int aat2870_brightness(struct aat2870_bl_driver_data *aat2870_bl,
        struct backlight_device *bd = aat2870_bl->bd;
        int val;
 
-       val = brightness * aat2870_bl->max_current;
+       val = brightness * (aat2870_bl->max_current - 1);
        val /= bd->props.max_brightness;
 
        return val;
include/linux/mfd/aat2870.h +1 -0
index 89212df..f8b7e8e 100644
@@ -89,6 +89,7 @@ enum aat2870_id {
 
 /* Backlight current magnitude (mA) */
 enum aat2870_current {
+       AAT2870_CURRENT_0_00,
        AAT2870_CURRENT_0_45,
        AAT2870_CURRENT_0_90,
        AAT2870_CURRENT_1_80,

Thanks,
Jin.

author
#2 lore raw

2011/7/6 Jinyoung Park <[email protected]>:

Quoted 14 lines

Hi, Axel

Quoted 3 lines

1. Seems the implementation tests wrong value for setting aat2870_bl->max_current.
    I think you mean:

Right, that's my mean. This is my mistyping bug.

Quoted 7 lines

2. Even above issue is fixed, you still cannot differentiate below 2 cases:
         a) if pdata->max_current is not set , or
         b) pdata->max_current is set to AAT2870_CURRENT_0_45 ( which is also 0 ).
    In either case, current implementation will set max_current to AAT2870_CURRENT_27_9.

Quoted 4 lines

I didn't find it when verifying driver on real device and my self review. Thanks for your kindly review.

Below is fixed codes. How about this change?

It looks fine in fixing this issue.

Yet another question:
(Just a quick check at the datasheet.)
Why not just set max_current to AAT2870_CURRENT_27_9? Why do you want max_current to be configurable by pdata?

Regards,
Axel

Quoted 35 lines

If there is no problem, could you submit this change with first change as one patch?
because two changes are related.

drivers/video/backlight/aat2870_bl.c +1 -1
index 4952a61..c0e2ce7 100644
@@ -44,7 +44,7 @@ static inline int aat2870_brightness(struct
 aat2870_bl_driver_data *aat2870_bl,
        struct backlight_device *bd = aat2870_bl->bd;
        int val;
 
-       val = brightness * aat2870_bl->max_current;
+       val = brightness * (aat2870_bl->max_current - 1);
        val /= bd->props.max_brightness;

       return val;

include/linux/mfd/aat2870.h +1 -0
index 89212df..f8b7e8e 100644
@@ -89,6 +89,7 @@ enum aat2870_id {
 
  /* Backlight current magnitude (mA) */
  enum aat2870_current {
+       AAT2870_CURRENT_0_00,
        AAT2870_CURRENT_0_45,
        AAT2870_CURRENT_0_90,
        AAT2870_CURRENT_1_80,

Thanks,
Jin.

#3 lore raw

Hi, Axel

Quoted 4 lines

Yet another question:
(Just a quick check at the datasheet.)
Why not just set max_current to AAT2870_CURRENT_27_9? Why do you want max_current to be configurable by pdata?

I think that the max backlight current should be configured according to required power consumption and it could be different for each platform. So the max_current value be controlled.

Thanks,
Jin.