backlight: aat2870_bl: questions about setting max_current

backlight: aat2870_bl: questions about setting max_current

[Axel Lin]

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:

diff --git a/drivers/video/backlight/aat2870_bl.c b/drivers/video/backlight/aat2870_bl.c
index f13a3f7..a8a2545 100644
--- a/drivers/video/backlight/aat2870_bl.c
+++ b/drivers/video/backlight/aat2870_bl.c
@@ -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

Re: backlight: aat2870_bl: questions about setting max_current

[Jinyoung Park]

Hi, Axel

 > 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.

 > 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.

diff --git a/drivers/video/backlight/aat2870_bl.c 
b/drivers/video/backlight/aat2870_bl.c
index 4952a61..c0e2ce7 100644
--- a/drivers/video/backlight/aat2870_bl.c
+++ b/drivers/video/backlight/aat2870_bl.c
@@ -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;
diff --git a/include/linux/mfd/aat2870.h b/include/linux/mfd/aat2870.h
index 89212df..f8b7e8e 100644
--- a/include/linux/mfd/aat2870.h
+++ b/include/linux/mfd/aat2870.h
@@ -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.

Re: backlight: aat2870_bl: questions about setting max_current

[Axel Lin]

2011/7/6 Jinyoung Park <jinyoungp@nvidia.com>:
> Hi, Axel
>
>> 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.
>
>> 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?
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
> If there is no problem, could you submit this change with first change as
> one patch?
> because two changes are related.
>
> diff --git a/drivers/video/backlight/aat2870_bl.c
> b/drivers/video/backlight/aat2870_bl.c
> index 4952a61..c0e2ce7 100644
> --- a/drivers/video/backlight/aat2870_bl.c
> +++ b/drivers/video/backlight/aat2870_bl.c
> @@ -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;
> diff --git a/include/linux/mfd/aat2870.h b/include/linux/mfd/aat2870.h
> index 89212df..f8b7e8e 100644
> --- a/include/linux/mfd/aat2870.h
> +++ b/include/linux/mfd/aat2870.h
> @@ -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.
>

Re: backlight: aat2870_bl: questions about setting max_current

[Jinyoung Park]

Hi, Axel

 > 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.