* [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
@ 2014-01-26 7:15 Manish Badarkhe
2014-01-26 13:22 ` Tomasz Figa
0 siblings, 1 reply; 9+ messages in thread
From: Manish Badarkhe @ 2014-01-26 7:15 UTC (permalink / raw)
To: linux-samsung-soc, linux-kernel; +Cc: dbaryshkov, dwmw2, badarkhe.manish
Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
option for DT code to avoid if-deffery in code.
Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
---
:100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
drivers/power/max8925_power.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
index b4513f2..d353fbc 100644
--- a/drivers/power/max8925_power.c
+++ b/drivers/power/max8925_power.c
@@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct max8925_power_info *info)
return 0;
}
-#ifdef CONFIG_OF
static struct max8925_power_pdata *
max8925_power_dt_init(struct platform_device *pdev)
{
@@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device *pdev)
return pdata;
}
-#else
-static struct max8925_power_pdata *
-max8925_power_dt_init(struct platform_device *pdev)
-{
- return pdev->dev.platform_data;
-}
-#endif
static int max8925_power_probe(struct platform_device *pdev)
{
@@ -483,7 +475,11 @@ static int max8925_power_probe(struct platform_device *pdev)
struct max8925_power_info *info;
int ret;
- pdata = max8925_power_dt_init(pdev);
+ if (IS_ENABLED(CONFIG_OF))
+ pdata = max8925_power_dt_init(pdev);
+ else
+ pdata = pdev->dev.platform_data;
+
if (!pdata) {
dev_err(&pdev->dev, "platform data isn't assigned to "
"power supply\n");
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 7:15 [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code Manish Badarkhe
@ 2014-01-26 13:22 ` Tomasz Figa
2014-01-26 14:01 ` Manish Badarkhe
0 siblings, 1 reply; 9+ messages in thread
From: Tomasz Figa @ 2014-01-26 13:22 UTC (permalink / raw)
To: Manish Badarkhe, linux-samsung-soc, linux-kernel; +Cc: dbaryshkov, dwmw2
Hi Manish,
On 26.01.2014 08:15, Manish Badarkhe wrote:
> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
> option for DT code to avoid if-deffery in code.
>
> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
> ---
> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
> drivers/power/max8925_power.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
> index b4513f2..d353fbc 100644
> --- a/drivers/power/max8925_power.c
> +++ b/drivers/power/max8925_power.c
> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct max8925_power_info *info)
> return 0;
> }
>
> -#ifdef CONFIG_OF
> static struct max8925_power_pdata *
> max8925_power_dt_init(struct platform_device *pdev)
> {
> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device *pdev)
>
> return pdata;
> }
> -#else
> -static struct max8925_power_pdata *
> -max8925_power_dt_init(struct platform_device *pdev)
> -{
> - return pdev->dev.platform_data;
> -}
> -#endif
>
> static int max8925_power_probe(struct platform_device *pdev)
> {
> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct platform_device *pdev)
> struct max8925_power_info *info;
> int ret;
>
> - pdata = max8925_power_dt_init(pdev);
> + if (IS_ENABLED(CONFIG_OF))
> + pdata = max8925_power_dt_init(pdev);
> + else
> + pdata = pdev->dev.platform_data;
> +
This does not look much better than before this patch. Instead of
"if-deffery" outside function bodies you are adding "iffery" (if there
is such a word) inside a function.
What about adding
if (!IS_ENABLED(CONFIG_OF))
return pdev->dev.platform_data;
on top of max8925_power_dt_init() instead or maybe also renaming this
function to max8925_get_pdata()?
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 13:22 ` Tomasz Figa
@ 2014-01-26 14:01 ` Manish Badarkhe
2014-01-26 21:45 ` Dmitry Torokhov
0 siblings, 1 reply; 9+ messages in thread
From: Manish Badarkhe @ 2014-01-26 14:01 UTC (permalink / raw)
To: Tomasz Figa
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
Dmitry Eremin-Solenikov, David Woodhouse
Hi Tomasz,
Thank you for your review comments.
On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> Hi Manish,
>
>
> On 26.01.2014 08:15, Manish Badarkhe wrote:
>>
>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
>> option for DT code to avoid if-deffery in code.
>>
>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
>> ---
>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
>> drivers/power/max8925_power.c | 14 +++++---------
>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
>> index b4513f2..d353fbc 100644
>> --- a/drivers/power/max8925_power.c
>> +++ b/drivers/power/max8925_power.c
>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct max8925_power_info *info)
>> return 0;
>> }
>>
>> -#ifdef CONFIG_OF
>> static struct max8925_power_pdata *
>> max8925_power_dt_init(struct platform_device *pdev)
>> {
>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device *pdev)
>>
>> return pdata;
>> }
>> -#else
>> -static struct max8925_power_pdata *
>> -max8925_power_dt_init(struct platform_device *pdev)
>> -{
>> - return pdev->dev.platform_data;
>> -}
>> -#endif
>>
>> static int max8925_power_probe(struct platform_device *pdev)
>> {
>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct platform_device *pdev)
>> struct max8925_power_info *info;
>> int ret;
>>
>> - pdata = max8925_power_dt_init(pdev);
>> + if (IS_ENABLED(CONFIG_OF))
>> + pdata = max8925_power_dt_init(pdev);
>> + else
>> + pdata = pdev->dev.platform_data;
>> +
>
>
> This does not look much better than before this patch. Instead of "if-deffery" outside function bodies you are adding "iffery" (if there is such a word) inside a function.
> What about adding
>
> if (!IS_ENABLED(CONFIG_OF))
> return pdev->dev.platform_data;
>
> on top of max8925_power_dt_init() instead or maybe also renaming this function to max8925_get_pdata()?
Okay, I will rename function "max8925_power_dt_init()" to "max8925_get_pdata()".
As you suggested, in the body of this function will add a logic to
retrieve data in case
of DT and non-DT platforms.
Regards
Manish Badarkhe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 14:01 ` Manish Badarkhe
@ 2014-01-26 21:45 ` Dmitry Torokhov
2014-01-26 21:49 ` Tomasz Figa
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2014-01-26 21:45 UTC (permalink / raw)
To: Manish Badarkhe
Cc: Tomasz Figa, linux-samsung-soc@vger.kernel.org,
linux-kernel@vger.kernel.org, Dmitry Eremin-Solenikov,
David Woodhouse
On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
> Hi Tomasz,
>
> Thank you for your review comments.
>
> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >
> > Hi Manish,
> >
> >
> > On 26.01.2014 08:15, Manish Badarkhe wrote:
> >>
> >> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
> >> option for DT code to avoid if-deffery in code.
> >>
> >> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
> >> ---
> >> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
> >> drivers/power/max8925_power.c | 14 +++++---------
> >> 1 file changed, 5 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
> >> index b4513f2..d353fbc 100644
> >> --- a/drivers/power/max8925_power.c
> >> +++ b/drivers/power/max8925_power.c
> >> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct max8925_power_info *info)
> >> return 0;
> >> }
> >>
> >> -#ifdef CONFIG_OF
> >> static struct max8925_power_pdata *
> >> max8925_power_dt_init(struct platform_device *pdev)
> >> {
> >> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device *pdev)
> >>
> >> return pdata;
> >> }
> >> -#else
> >> -static struct max8925_power_pdata *
> >> -max8925_power_dt_init(struct platform_device *pdev)
> >> -{
> >> - return pdev->dev.platform_data;
> >> -}
> >> -#endif
> >>
> >> static int max8925_power_probe(struct platform_device *pdev)
> >> {
> >> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct platform_device *pdev)
> >> struct max8925_power_info *info;
> >> int ret;
> >>
> >> - pdata = max8925_power_dt_init(pdev);
> >> + if (IS_ENABLED(CONFIG_OF))
> >> + pdata = max8925_power_dt_init(pdev);
> >> + else
> >> + pdata = pdev->dev.platform_data;
> >> +
> >
> >
> > This does not look much better than before this patch. Instead of "if-deffery" outside function bodies you are adding "iffery" (if there is such a word) inside a function.
> > What about adding
> >
> > if (!IS_ENABLED(CONFIG_OF))
> > return pdev->dev.platform_data;
> >
> > on top of max8925_power_dt_init() instead or maybe also renaming this function to max8925_get_pdata()?
>
> Okay, I will rename function "max8925_power_dt_init()" to "max8925_get_pdata()".
> As you suggested, in the body of this function will add a logic to
> retrieve data in case
> of DT and non-DT platforms.
Should we not always favor platform-supplied data regardless of
CONFIG_OF state and fall back to DT (firmware) supplied data if platform
data is absent? This way one can tweak kernel behavior without needing
to change firmware.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 21:45 ` Dmitry Torokhov
@ 2014-01-26 21:49 ` Tomasz Figa
2014-01-26 22:31 ` Dmitry Eremin-Solenikov
0 siblings, 1 reply; 9+ messages in thread
From: Tomasz Figa @ 2014-01-26 21:49 UTC (permalink / raw)
To: Dmitry Torokhov, Manish Badarkhe
Cc: linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
Dmitry Eremin-Solenikov, David Woodhouse
On 26.01.2014 22:45, Dmitry Torokhov wrote:
> On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
>> Hi Tomasz,
>>
>> Thank you for your review comments.
>>
>> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>>
>>> Hi Manish,
>>>
>>>
>>> On 26.01.2014 08:15, Manish Badarkhe wrote:
>>>>
>>>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
>>>> option for DT code to avoid if-deffery in code.
>>>>
>>>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
>>>> ---
>>>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
>>>> drivers/power/max8925_power.c | 14 +++++---------
>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
>>>> index b4513f2..d353fbc 100644
>>>> --- a/drivers/power/max8925_power.c
>>>> +++ b/drivers/power/max8925_power.c
>>>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct max8925_power_info *info)
>>>> return 0;
>>>> }
>>>>
>>>> -#ifdef CONFIG_OF
>>>> static struct max8925_power_pdata *
>>>> max8925_power_dt_init(struct platform_device *pdev)
>>>> {
>>>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device *pdev)
>>>>
>>>> return pdata;
>>>> }
>>>> -#else
>>>> -static struct max8925_power_pdata *
>>>> -max8925_power_dt_init(struct platform_device *pdev)
>>>> -{
>>>> - return pdev->dev.platform_data;
>>>> -}
>>>> -#endif
>>>>
>>>> static int max8925_power_probe(struct platform_device *pdev)
>>>> {
>>>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct platform_device *pdev)
>>>> struct max8925_power_info *info;
>>>> int ret;
>>>>
>>>> - pdata = max8925_power_dt_init(pdev);
>>>> + if (IS_ENABLED(CONFIG_OF))
>>>> + pdata = max8925_power_dt_init(pdev);
>>>> + else
>>>> + pdata = pdev->dev.platform_data;
>>>> +
>>>
>>>
>>> This does not look much better than before this patch. Instead of "if-deffery" outside function bodies you are adding "iffery" (if there is such a word) inside a function.
>>> What about adding
>>>
>>> if (!IS_ENABLED(CONFIG_OF))
>>> return pdev->dev.platform_data;
>>>
>>> on top of max8925_power_dt_init() instead or maybe also renaming this function to max8925_get_pdata()?
>>
>> Okay, I will rename function "max8925_power_dt_init()" to "max8925_get_pdata()".
>> As you suggested, in the body of this function will add a logic to
>> retrieve data in case
>> of DT and non-DT platforms.
>
> Should we not always favor platform-supplied data regardless of
> CONFIG_OF state and fall back to DT (firmware) supplied data if platform
> data is absent? This way one can tweak kernel behavior without needing
> to change firmware.
I guess we should, but apparently this was not the original behavior
before this patch, so I'm not sure if we should be squashing such
semantic change with this simple refactor.
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 21:49 ` Tomasz Figa
@ 2014-01-26 22:31 ` Dmitry Eremin-Solenikov
2014-01-26 23:14 ` Dmitry Torokhov
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Eremin-Solenikov @ 2014-01-26 22:31 UTC (permalink / raw)
To: Tomasz Figa
Cc: Dmitry Torokhov, Manish Badarkhe,
linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
David Woodhouse
On Mon, Jan 27, 2014 at 1:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 26.01.2014 22:45, Dmitry Torokhov wrote:
>>
>> On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
>>>
>>> Hi Tomasz,
>>>
>>> Thank you for your review comments.
>>>
>>> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com>
>>> wrote:
>>>>
>>>>
>>>> Hi Manish,
>>>>
>>>>
>>>> On 26.01.2014 08:15, Manish Badarkhe wrote:
>>>>>
>>>>>
>>>>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
>>>>> option for DT code to avoid if-deffery in code.
>>>>>
>>>>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
>>>>> ---
>>>>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
>>>>> drivers/power/max8925_power.c | 14 +++++---------
>>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/max8925_power.c
>>>>> b/drivers/power/max8925_power.c
>>>>> index b4513f2..d353fbc 100644
>>>>> --- a/drivers/power/max8925_power.c
>>>>> +++ b/drivers/power/max8925_power.c
>>>>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct
>>>>> max8925_power_info *info)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -#ifdef CONFIG_OF
>>>>> static struct max8925_power_pdata *
>>>>> max8925_power_dt_init(struct platform_device *pdev)
>>>>> {
>>>>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device
>>>>> *pdev)
>>>>>
>>>>> return pdata;
>>>>> }
>>>>> -#else
>>>>> -static struct max8925_power_pdata *
>>>>> -max8925_power_dt_init(struct platform_device *pdev)
>>>>> -{
>>>>> - return pdev->dev.platform_data;
>>>>> -}
>>>>> -#endif
>>>>>
>>>>> static int max8925_power_probe(struct platform_device *pdev)
>>>>> {
>>>>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct
>>>>> platform_device *pdev)
>>>>> struct max8925_power_info *info;
>>>>> int ret;
>>>>>
>>>>> - pdata = max8925_power_dt_init(pdev);
>>>>> + if (IS_ENABLED(CONFIG_OF))
>>>>> + pdata = max8925_power_dt_init(pdev);
>>>>> + else
>>>>> + pdata = pdev->dev.platform_data;
>>>>> +
>>>>
>>>>
>>>>
>>>> This does not look much better than before this patch. Instead of
>>>> "if-deffery" outside function bodies you are adding "iffery" (if there is
>>>> such a word) inside a function.
>>>> What about adding
>>>>
>>>> if (!IS_ENABLED(CONFIG_OF))
>>>> return pdev->dev.platform_data;
>>>>
>>>> on top of max8925_power_dt_init() instead or maybe also renaming this
>>>> function to max8925_get_pdata()?
>>>
>>>
>>> Okay, I will rename function "max8925_power_dt_init()" to
>>> "max8925_get_pdata()".
>>> As you suggested, in the body of this function will add a logic to
>>> retrieve data in case
>>> of DT and non-DT platforms.
>>
>>
>> Should we not always favor platform-supplied data regardless of
>> CONFIG_OF state and fall back to DT (firmware) supplied data if platform
>> data is absent? This way one can tweak kernel behavior without needing
>> to change firmware.
>
>
> I guess we should, but apparently this was not the original behavior before
> this patch, so I'm not sure if we should be squashing such semantic change
> with this simple refactor.
Hmm. Judging from the code, max8925_power_dt_init() function follows exactly
opposite strategy - it uses platform_data if of_node is not populated/available.
So (if dt_init will compile with CONFIG_OF disabled) one can always
use _dt_init()
function to retrieve pdata.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 22:31 ` Dmitry Eremin-Solenikov
@ 2014-01-26 23:14 ` Dmitry Torokhov
2014-01-26 23:46 ` Dmitry Eremin-Solenikov
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Torokhov @ 2014-01-26 23:14 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Tomasz Figa, Manish Badarkhe, linux-samsung-soc@vger.kernel.org,
linux-kernel@vger.kernel.org, David Woodhouse
On Mon, Jan 27, 2014 at 02:31:59AM +0400, Dmitry Eremin-Solenikov wrote:
> On Mon, Jan 27, 2014 at 1:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On 26.01.2014 22:45, Dmitry Torokhov wrote:
> >>
> >> On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
> >>>
> >>> Hi Tomasz,
> >>>
> >>> Thank you for your review comments.
> >>>
> >>> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com>
> >>> wrote:
> >>>>
> >>>>
> >>>> Hi Manish,
> >>>>
> >>>>
> >>>> On 26.01.2014 08:15, Manish Badarkhe wrote:
> >>>>>
> >>>>>
> >>>>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
> >>>>> option for DT code to avoid if-deffery in code.
> >>>>>
> >>>>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
> >>>>> ---
> >>>>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
> >>>>> drivers/power/max8925_power.c | 14 +++++---------
> >>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/power/max8925_power.c
> >>>>> b/drivers/power/max8925_power.c
> >>>>> index b4513f2..d353fbc 100644
> >>>>> --- a/drivers/power/max8925_power.c
> >>>>> +++ b/drivers/power/max8925_power.c
> >>>>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct
> >>>>> max8925_power_info *info)
> >>>>> return 0;
> >>>>> }
> >>>>>
> >>>>> -#ifdef CONFIG_OF
> >>>>> static struct max8925_power_pdata *
> >>>>> max8925_power_dt_init(struct platform_device *pdev)
> >>>>> {
> >>>>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device
> >>>>> *pdev)
> >>>>>
> >>>>> return pdata;
> >>>>> }
> >>>>> -#else
> >>>>> -static struct max8925_power_pdata *
> >>>>> -max8925_power_dt_init(struct platform_device *pdev)
> >>>>> -{
> >>>>> - return pdev->dev.platform_data;
> >>>>> -}
> >>>>> -#endif
> >>>>>
> >>>>> static int max8925_power_probe(struct platform_device *pdev)
> >>>>> {
> >>>>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct
> >>>>> platform_device *pdev)
> >>>>> struct max8925_power_info *info;
> >>>>> int ret;
> >>>>>
> >>>>> - pdata = max8925_power_dt_init(pdev);
> >>>>> + if (IS_ENABLED(CONFIG_OF))
> >>>>> + pdata = max8925_power_dt_init(pdev);
> >>>>> + else
> >>>>> + pdata = pdev->dev.platform_data;
> >>>>> +
> >>>>
> >>>>
> >>>>
> >>>> This does not look much better than before this patch. Instead of
> >>>> "if-deffery" outside function bodies you are adding "iffery" (if there is
> >>>> such a word) inside a function.
> >>>> What about adding
> >>>>
> >>>> if (!IS_ENABLED(CONFIG_OF))
> >>>> return pdev->dev.platform_data;
> >>>>
> >>>> on top of max8925_power_dt_init() instead or maybe also renaming this
> >>>> function to max8925_get_pdata()?
> >>>
> >>>
> >>> Okay, I will rename function "max8925_power_dt_init()" to
> >>> "max8925_get_pdata()".
> >>> As you suggested, in the body of this function will add a logic to
> >>> retrieve data in case
> >>> of DT and non-DT platforms.
> >>
> >>
> >> Should we not always favor platform-supplied data regardless of
> >> CONFIG_OF state and fall back to DT (firmware) supplied data if platform
> >> data is absent? This way one can tweak kernel behavior without needing
> >> to change firmware.
> >
> >
> > I guess we should, but apparently this was not the original behavior before
> > this patch, so I'm not sure if we should be squashing such semantic change
> > with this simple refactor.
>
> Hmm. Judging from the code, max8925_power_dt_init() function follows exactly
> opposite strategy - it uses platform_data if of_node is not populated/available.
> So (if dt_init will compile with CONFIG_OF disabled) one can always
> use _dt_init()
> function to retrieve pdata.
Right, and I question whether this is good behavior or if it should be
corrected.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 23:14 ` Dmitry Torokhov
@ 2014-01-26 23:46 ` Dmitry Eremin-Solenikov
2014-01-27 7:27 ` Manish Badarkhe
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Eremin-Solenikov @ 2014-01-26 23:46 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Tomasz Figa, Manish Badarkhe, linux-samsung-soc@vger.kernel.org,
linux-kernel@vger.kernel.org, David Woodhouse
On Mon, Jan 27, 2014 at 3:14 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jan 27, 2014 at 02:31:59AM +0400, Dmitry Eremin-Solenikov wrote:
>> On Mon, Jan 27, 2014 at 1:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > On 26.01.2014 22:45, Dmitry Torokhov wrote:
>> >>
>> >> On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
>> >>>
>> >>> Hi Tomasz,
>> >>>
>> >>> Thank you for your review comments.
>> >>>
>> >>> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com>
>> >>> wrote:
>> >>>>
>> >>>>
>> >>>> Hi Manish,
>> >>>>
>> >>>>
>> >>>> On 26.01.2014 08:15, Manish Badarkhe wrote:
>> >>>>>
>> >>>>>
>> >>>>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
>> >>>>> option for DT code to avoid if-deffery in code.
>> >>>>>
>> >>>>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
>> >>>>> ---
>> >>>>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
>> >>>>> drivers/power/max8925_power.c | 14 +++++---------
>> >>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>> >>>>>
>> >>>>> diff --git a/drivers/power/max8925_power.c
>> >>>>> b/drivers/power/max8925_power.c
>> >>>>> index b4513f2..d353fbc 100644
>> >>>>> --- a/drivers/power/max8925_power.c
>> >>>>> +++ b/drivers/power/max8925_power.c
>> >>>>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct
>> >>>>> max8925_power_info *info)
>> >>>>> return 0;
>> >>>>> }
>> >>>>>
>> >>>>> -#ifdef CONFIG_OF
>> >>>>> static struct max8925_power_pdata *
>> >>>>> max8925_power_dt_init(struct platform_device *pdev)
>> >>>>> {
>> >>>>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device
>> >>>>> *pdev)
>> >>>>>
>> >>>>> return pdata;
>> >>>>> }
>> >>>>> -#else
>> >>>>> -static struct max8925_power_pdata *
>> >>>>> -max8925_power_dt_init(struct platform_device *pdev)
>> >>>>> -{
>> >>>>> - return pdev->dev.platform_data;
>> >>>>> -}
>> >>>>> -#endif
>> >>>>>
>> >>>>> static int max8925_power_probe(struct platform_device *pdev)
>> >>>>> {
>> >>>>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct
>> >>>>> platform_device *pdev)
>> >>>>> struct max8925_power_info *info;
>> >>>>> int ret;
>> >>>>>
>> >>>>> - pdata = max8925_power_dt_init(pdev);
>> >>>>> + if (IS_ENABLED(CONFIG_OF))
>> >>>>> + pdata = max8925_power_dt_init(pdev);
>> >>>>> + else
>> >>>>> + pdata = pdev->dev.platform_data;
>> >>>>> +
>> >>>>
>> >>>>
>> >>>>
>> >>>> This does not look much better than before this patch. Instead of
>> >>>> "if-deffery" outside function bodies you are adding "iffery" (if there is
>> >>>> such a word) inside a function.
>> >>>> What about adding
>> >>>>
>> >>>> if (!IS_ENABLED(CONFIG_OF))
>> >>>> return pdev->dev.platform_data;
>> >>>>
>> >>>> on top of max8925_power_dt_init() instead or maybe also renaming this
>> >>>> function to max8925_get_pdata()?
>> >>>
>> >>>
>> >>> Okay, I will rename function "max8925_power_dt_init()" to
>> >>> "max8925_get_pdata()".
>> >>> As you suggested, in the body of this function will add a logic to
>> >>> retrieve data in case
>> >>> of DT and non-DT platforms.
>> >>
>> >>
>> >> Should we not always favor platform-supplied data regardless of
>> >> CONFIG_OF state and fall back to DT (firmware) supplied data if platform
>> >> data is absent? This way one can tweak kernel behavior without needing
>> >> to change firmware.
>> >
>> >
>> > I guess we should, but apparently this was not the original behavior before
>> > this patch, so I'm not sure if we should be squashing such semantic change
>> > with this simple refactor.
>>
>> Hmm. Judging from the code, max8925_power_dt_init() function follows exactly
>> opposite strategy - it uses platform_data if of_node is not populated/available.
>> So (if dt_init will compile with CONFIG_OF disabled) one can always
>> use _dt_init()
>> function to retrieve pdata.
>
> Right, and I question whether this is good behavior or if it should be
> corrected.
I'd say, correct it.
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code.
2014-01-26 23:46 ` Dmitry Eremin-Solenikov
@ 2014-01-27 7:27 ` Manish Badarkhe
0 siblings, 0 replies; 9+ messages in thread
From: Manish Badarkhe @ 2014-01-27 7:27 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Dmitry Torokhov, Tomasz Figa, linux-samsung-soc@vger.kernel.org,
linux-kernel@vger.kernel.org, David Woodhouse
Hi
Thank you for review.
On Mon, Jan 27, 2014 at 5:16 AM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> On Mon, Jan 27, 2014 at 3:14 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Mon, Jan 27, 2014 at 02:31:59AM +0400, Dmitry Eremin-Solenikov wrote:
>>> On Mon, Jan 27, 2014 at 1:49 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>>> > On 26.01.2014 22:45, Dmitry Torokhov wrote:
>>> >>
>>> >> On Sun, Jan 26, 2014 at 07:31:50PM +0530, Manish Badarkhe wrote:
>>> >>>
>>> >>> Hi Tomasz,
>>> >>>
>>> >>> Thank you for your review comments.
>>> >>>
>>> >>> On Sun, Jan 26, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com>
>>> >>> wrote:
>>> >>>>
>>> >>>>
>>> >>>> Hi Manish,
>>> >>>>
>>> >>>>
>>> >>>> On 26.01.2014 08:15, Manish Badarkhe wrote:
>>> >>>>>
>>> >>>>>
>>> >>>>> Instead of "#if define CONFIG_OF" use "IS_ENABLED(CONFIG_OF)"
>>> >>>>> option for DT code to avoid if-deffery in code.
>>> >>>>>
>>> >>>>> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
>>> >>>>> ---
>>> >>>>> :100644 100644 b4513f2... d353fbc... M drivers/power/max8925_power.c
>>> >>>>> drivers/power/max8925_power.c | 14 +++++---------
>>> >>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>> >>>>>
>>> >>>>> diff --git a/drivers/power/max8925_power.c
>>> >>>>> b/drivers/power/max8925_power.c
>>> >>>>> index b4513f2..d353fbc 100644
>>> >>>>> --- a/drivers/power/max8925_power.c
>>> >>>>> +++ b/drivers/power/max8925_power.c
>>> >>>>> @@ -427,7 +427,6 @@ static int max8925_deinit_charger(struct
>>> >>>>> max8925_power_info *info)
>>> >>>>> return 0;
>>> >>>>> }
>>> >>>>>
>>> >>>>> -#ifdef CONFIG_OF
>>> >>>>> static struct max8925_power_pdata *
>>> >>>>> max8925_power_dt_init(struct platform_device *pdev)
>>> >>>>> {
>>> >>>>> @@ -468,13 +467,6 @@ max8925_power_dt_init(struct platform_device
>>> >>>>> *pdev)
>>> >>>>>
>>> >>>>> return pdata;
>>> >>>>> }
>>> >>>>> -#else
>>> >>>>> -static struct max8925_power_pdata *
>>> >>>>> -max8925_power_dt_init(struct platform_device *pdev)
>>> >>>>> -{
>>> >>>>> - return pdev->dev.platform_data;
>>> >>>>> -}
>>> >>>>> -#endif
>>> >>>>>
>>> >>>>> static int max8925_power_probe(struct platform_device *pdev)
>>> >>>>> {
>>> >>>>> @@ -483,7 +475,11 @@ static int max8925_power_probe(struct
>>> >>>>> platform_device *pdev)
>>> >>>>> struct max8925_power_info *info;
>>> >>>>> int ret;
>>> >>>>>
>>> >>>>> - pdata = max8925_power_dt_init(pdev);
>>> >>>>> + if (IS_ENABLED(CONFIG_OF))
>>> >>>>> + pdata = max8925_power_dt_init(pdev);
>>> >>>>> + else
>>> >>>>> + pdata = pdev->dev.platform_data;
>>> >>>>> +
>>> >>>>
>>> >>>>
>>> >>>>
>>> >>>> This does not look much better than before this patch. Instead of
>>> >>>> "if-deffery" outside function bodies you are adding "iffery" (if there is
>>> >>>> such a word) inside a function.
>>> >>>> What about adding
>>> >>>>
>>> >>>> if (!IS_ENABLED(CONFIG_OF))
>>> >>>> return pdev->dev.platform_data;
>>> >>>>
>>> >>>> on top of max8925_power_dt_init() instead or maybe also renaming this
>>> >>>> function to max8925_get_pdata()?
>>> >>>
>>> >>>
>>> >>> Okay, I will rename function "max8925_power_dt_init()" to
>>> >>> "max8925_get_pdata()".
>>> >>> As you suggested, in the body of this function will add a logic to
>>> >>> retrieve data in case
>>> >>> of DT and non-DT platforms.
>>> >>
>>> >>
>>> >> Should we not always favor platform-supplied data regardless of
>>> >> CONFIG_OF state and fall back to DT (firmware) supplied data if platform
>>> >> data is absent? This way one can tweak kernel behavior without needing
>>> >> to change firmware.
>>> >
>>> >
>>> > I guess we should, but apparently this was not the original behavior before
>>> > this patch, so I'm not sure if we should be squashing such semantic change
>>> > with this simple refactor.
>>>
>>> Hmm. Judging from the code, max8925_power_dt_init() function follows exactly
>>> opposite strategy - it uses platform_data if of_node is not populated/available.
>>> So (if dt_init will compile with CONFIG_OF disabled) one can always
>>> use _dt_init()
>>> function to retrieve pdata.
>>
>> Right, and I question whether this is good behavior or if it should be
>> corrected.
>
> I'd say, correct it.
Okay, I will update code as per Dmitry's feedback.
Regards
Manish Badarkhe
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-01-27 7:27 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-26 7:15 [PATCH] max8925_power: Use "IS_ENABLED(CONFIG_OF)" for DT code Manish Badarkhe
2014-01-26 13:22 ` Tomasz Figa
2014-01-26 14:01 ` Manish Badarkhe
2014-01-26 21:45 ` Dmitry Torokhov
2014-01-26 21:49 ` Tomasz Figa
2014-01-26 22:31 ` Dmitry Eremin-Solenikov
2014-01-26 23:14 ` Dmitry Torokhov
2014-01-26 23:46 ` Dmitry Eremin-Solenikov
2014-01-27 7:27 ` Manish Badarkhe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox