* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Julia Lawall @ 2012-08-01 5:13 UTC (permalink / raw)
To: Jingoo Han
Cc: 'Sachin Kamat', 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel
In-Reply-To: <003f01cd6fa2$36463bd0$a2d2b370$%han@samsung.com>
On Wed, 1 Aug 2012, Jingoo Han wrote:
> On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote:
>>
>> On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
>>> On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>>>>
>>>> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
>>>>> On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>>>>>>
>>>>>> From: Damien Cassou <damien.cassou@lifl.fr>
>>>>>>
>>>>>> The various devm_ functions allocate memory that is released when a driver
>>>>>> detaches. This patch uses these functions for data that is allocated in
>>>>>> the probe function of a platform device and is only freed in the remove
>>>>>> function.
>>>>>>
>>>>>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>>>>>
>>>>>> ---
>>>>>> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>>>>>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>>>>>> index c6c016a..00fe4f0 100644
>>>>>> --- a/drivers/video/exynos/exynos_dp_core.c
>>>>>> +++ b/drivers/video/exynos/exynos_dp_core.c
>>>>>> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>>>>>
>>>>>> dp->dev = &pdev->dev;
>>>>>>
>>>>>> - dp->clock = clk_get(&pdev->dev, "dp");
>>>>>> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>>>>>> if (IS_ERR(dp->clock)) {
>>>>>> dev_err(&pdev->dev, "failed to get clock\n");
>>>>>> return PTR_ERR(dp->clock);
>>>>>> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>>>>> clk_enable(dp->clock);
>>>>>>
>>>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>> - if (!res) {
>>>>>> - dev_err(&pdev->dev, "failed to get registers\n");
>>>>>> - ret = -EINVAL;
>>>>>> - goto err_clock;
>>>>>> - }
>>>>>
>>>>> Why do you remove this return check?
>>>>> If there is no reason, please, do it as follows:
>>>>>
>>>>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>> if (!res) {
>>>>> dev_err(&pdev->dev, "failed to get registers\n");
>>>>> - ret = -EINVAL;
>>>>> - goto err_clock;
>>>>> + return -EINVAL;
>>>>> }
>>>>>
>>>>>
>>>>
>>>> devm_request_and_ioremap function checks the validity of res. Hence
>>>> this check above is redundant and can be removed.
>>>
>>>
>>> I don't think so.
>>> Even though function called next checks the NULL value,
>>> for robustness, the return value of platform_get_resource() should be
>>> checked.
>>>
>>> It is possible that devm_request_and_ioremap() can be changed in the future,
>>> as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
>>
>> They are not changed. They still exist. devm_request_and_ioremap() is
>> an additional function provided for device managed resources.
>
>
> OK, I see. I accept it.
> Anyway it is simpler.
This thread contains a discussion about the issue
http://lkml.org/lkml/2012/1/28/10
Look for the comments by Wolfram Sang, who
implemented devm_request_and_ioremap, and who suggests that the NULL test
be removed.
I rather agree with the desire to be safe and uniform, but these
initialization functions are really large, and with error handling code
(although not in this case) there is always the danger of jumping to the
wrong place, and thus making more of a mess. It would be nice if the
platform_get_resource could be merged with devm_request_and_ioremap, but I
think that I looked once and there were not enough calls that were similar
enough to make that compelling.
julia
^ permalink raw reply
* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Jingoo Han @ 2012-08-01 5:08 UTC (permalink / raw)
To: 'Damien Cassou', 'Sachin Kamat'
Cc: kernel-janitors, 'Florian Tobias Schandinat', linux-fbdev,
linux-kernel, 'Jingoo Han'
In-Reply-To: <CAK9yfHzje6NLqi5ixxA3J4aT1a9p6wJNJePzTi4xG9LN6AeLVw@mail.gmail.com>
On Wednesday, August 01, 2012 1:00 PM Sachin Kamat wrote:
>
> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >>
> >> From: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> The various devm_ functions allocate memory that is released when a driver
> >> detaches. This patch uses these functions for data that is allocated in
> >> the probe function of a platform device and is only freed in the remove
> >> function.
> >>
> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> ---
> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> index c6c016a..00fe4f0 100644
> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >>
> >> dp->dev = &pdev->dev;
> >>
> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> if (IS_ERR(dp->clock)) {
> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> return PTR_ERR(dp->clock);
> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> clk_enable(dp->clock);
> >>
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> - if (!res) {
> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> - ret = -EINVAL;
> >> - goto err_clock;
> >> - }
> >
> > Why do you remove this return check?
> > If there is no reason, please, do it as follows:
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > dev_err(&pdev->dev, "failed to get registers\n");
> > - ret = -EINVAL;
> > - goto err_clock;
> > + return -EINVAL;
> > }
> >
> >
>
> devm_request_and_ioremap function checks the validity of res. Hence
> this check above is redundant and can be removed.
>
> Damien,
> This patch only adds devm_clk_get() function. Hence you could make the
> subject line more specific.
To Damien,
As Sachin Kamat mentioned, please change the subject more specific. For example,
video: exynos_dp: use devm_clk_get function
Best regards,
Jingoo Han
>
>
>
>
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> if (!dp->reg_base) {
> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> - ret = -ENOMEM;
> >> - goto err_clock;
> >> + return -ENOMEM;
> >> }
> >>
> >> dp->irq = platform_get_irq(pdev, 0);
> >> if (!dp->irq) {
> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> - ret = -ENODEV;
> >> - goto err_clock;
> >> + return -ENODEV;
> >> }
> >>
> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> "exynos-dp", dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> dp->video_info = pdata->video_info;
> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_detect_hpd(dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_handle_edid(dp);
> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> dp->video_info->link_rate);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_enable_scramble(dp, 1);
> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to config video\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> platform_set_drvdata(pdev, dp);
> >>
> >> return 0;
> >> -
> >> -err_clock:
> >> - clk_put(dp->clock);
> >> -
> >> - return ret;
> >> }
> >>
> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> pdata->phy_exit();
> >>
> >> clk_disable(dp->clock);
> >> - clk_put(dp->clock);
> >>
> >> return 0;
> >> }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
>
>
> --
> With warm regards,
> Sachin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Jingoo Han @ 2012-08-01 4:57 UTC (permalink / raw)
To: 'Sachin Kamat'
Cc: 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel,
'Jingoo Han'
In-Reply-To: <CAK9yfHyvHEPQRFvuX6q5CZEEN_6xeTqWvitdtayib6W8t0qCWw@mail.gmail.com>
On Wednesday, August 01, 2012 1:38 PM Sachin Kamat wrote:
>
> On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
> >>
> >> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> >> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >> >>
> >> >> From: Damien Cassou <damien.cassou@lifl.fr>
> >> >>
> >> >> The various devm_ functions allocate memory that is released when a driver
> >> >> detaches. This patch uses these functions for data that is allocated in
> >> >> the probe function of a platform device and is only freed in the remove
> >> >> function.
> >> >>
> >> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >> >>
> >> >> ---
> >> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >> >>
> >> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> >> index c6c016a..00fe4f0 100644
> >> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >>
> >> >> dp->dev = &pdev->dev;
> >> >>
> >> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> >> if (IS_ERR(dp->clock)) {
> >> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> >> return PTR_ERR(dp->clock);
> >> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> clk_enable(dp->clock);
> >> >>
> >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> >> - if (!res) {
> >> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> >> - ret = -EINVAL;
> >> >> - goto err_clock;
> >> >> - }
> >> >
> >> > Why do you remove this return check?
> >> > If there is no reason, please, do it as follows:
> >> >
> >> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> > if (!res) {
> >> > dev_err(&pdev->dev, "failed to get registers\n");
> >> > - ret = -EINVAL;
> >> > - goto err_clock;
> >> > + return -EINVAL;
> >> > }
> >> >
> >> >
> >>
> >> devm_request_and_ioremap function checks the validity of res. Hence
> >> this check above is redundant and can be removed.
> >
> >
> > I don't think so.
> > Even though function called next checks the NULL value,
> > for robustness, the return value of platform_get_resource() should be
> > checked.
> >
> > It is possible that devm_request_and_ioremap() can be changed in the future,
> > as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
>
> They are not changed. They still exist. devm_request_and_ioremap() is
> an additional function provided for device managed resources.
OK, I see. I accept it.
Anyway it is simpler.
>
>
> >
> >
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> Damien,
> >> This patch only adds devm_clk_get() function. Hence you could make the
> >> subject line more specific.
> >>
> >>
> >>
> >>
> >> > Best regards,
> >> > Jingoo Han
> >> >
> >> >
> >> >>
> >> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> >> if (!dp->reg_base) {
> >> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> >> - ret = -ENOMEM;
> >> >> - goto err_clock;
> >> >> + return -ENOMEM;
> >> >> }
> >> >>
> >> >> dp->irq = platform_get_irq(pdev, 0);
> >> >> if (!dp->irq) {
> >> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> >> - ret = -ENODEV;
> >> >> - goto err_clock;
> >> >> + return -ENODEV;
> >> >> }
> >> >>
> >> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> >> "exynos-dp", dp);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> dp->video_info = pdata->video_info;
> >> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> ret = exynos_dp_detect_hpd(dp);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> exynos_dp_handle_edid(dp);
> >> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> dp->video_info->link_rate);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> exynos_dp_enable_scramble(dp, 1);
> >> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> >> if (ret) {
> >> >> dev_err(&pdev->dev, "unable to config video\n");
> >> >> - goto err_clock;
> >> >> + return ret;
> >> >> }
> >> >>
> >> >> platform_set_drvdata(pdev, dp);
> >> >>
> >> >> return 0;
> >> >> -
> >> >> -err_clock:
> >> >> - clk_put(dp->clock);
> >> >> -
> >> >> - return ret;
> >> >> }
> >> >>
> >> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> >> pdata->phy_exit();
> >> >>
> >> >> clk_disable(dp->clock);
> >> >> - clk_put(dp->clock);
> >> >>
> >> >> return 0;
> >> >> }
> >> >
> >> > --
> >> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> > the body of a message to majordomo@vger.kernel.org
> >> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> > Please read the FAQ at http://www.tux.org/lkml/
> >>
> >>
> >>
> >> --
> >> With warm regards,
> >> Sachin
> >
>
>
>
> --
> With warm regards,
> Sachin
^ permalink raw reply
* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Sachin Kamat @ 2012-08-01 4:50 UTC (permalink / raw)
To: Jingoo Han
Cc: Damien Cassou, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
In-Reply-To: <003e01cd6f9e$5a1146d0$0e33d470$%han@samsung.com>
On 1 August 2012 10:00, Jingoo Han <jg1.han@samsung.com> wrote:
> On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>>
>> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
>> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>> >>
>> >> From: Damien Cassou <damien.cassou@lifl.fr>
>> >>
>> >> The various devm_ functions allocate memory that is released when a driver
>> >> detaches. This patch uses these functions for data that is allocated in
>> >> the probe function of a platform device and is only freed in the remove
>> >> function.
>> >>
>> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>> >>
>> >> ---
>> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>> >> 1 file changed, 7 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>> >> index c6c016a..00fe4f0 100644
>> >> --- a/drivers/video/exynos/exynos_dp_core.c
>> >> +++ b/drivers/video/exynos/exynos_dp_core.c
>> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >>
>> >> dp->dev = &pdev->dev;
>> >>
>> >> - dp->clock = clk_get(&pdev->dev, "dp");
>> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>> >> if (IS_ERR(dp->clock)) {
>> >> dev_err(&pdev->dev, "failed to get clock\n");
>> >> return PTR_ERR(dp->clock);
>> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> clk_enable(dp->clock);
>> >>
>> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> - if (!res) {
>> >> - dev_err(&pdev->dev, "failed to get registers\n");
>> >> - ret = -EINVAL;
>> >> - goto err_clock;
>> >> - }
>> >
>> > Why do you remove this return check?
>> > If there is no reason, please, do it as follows:
>> >
>> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> > if (!res) {
>> > dev_err(&pdev->dev, "failed to get registers\n");
>> > - ret = -EINVAL;
>> > - goto err_clock;
>> > + return -EINVAL;
>> > }
>> >
>> >
>>
>> devm_request_and_ioremap function checks the validity of res. Hence
>> this check above is redundant and can be removed.
>
>
> I don't think so.
> Even though function called next checks the NULL value,
> for robustness, the return value of platform_get_resource() should be
> checked.
>
> It is possible that devm_request_and_ioremap() can be changed in the future,
> as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
They are not changed. They still exist. devm_request_and_ioremap() is
an additional function provided for device managed resources.
>
>
> Best regards,
> Jingoo Han
>
>
>>
>> Damien,
>> This patch only adds devm_clk_get() function. Hence you could make the
>> subject line more specific.
>>
>>
>>
>>
>> > Best regards,
>> > Jingoo Han
>> >
>> >
>> >>
>> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>> >> if (!dp->reg_base) {
>> >> dev_err(&pdev->dev, "failed to ioremap\n");
>> >> - ret = -ENOMEM;
>> >> - goto err_clock;
>> >> + return -ENOMEM;
>> >> }
>> >>
>> >> dp->irq = platform_get_irq(pdev, 0);
>> >> if (!dp->irq) {
>> >> dev_err(&pdev->dev, "failed to get irq\n");
>> >> - ret = -ENODEV;
>> >> - goto err_clock;
>> >> + return -ENODEV;
>> >> }
>> >>
>> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
>> >> "exynos-dp", dp);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "failed to request irq\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> dp->video_info = pdata->video_info;
>> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> ret = exynos_dp_detect_hpd(dp);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to detect hpd\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> exynos_dp_handle_edid(dp);
>> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> dp->video_info->link_rate);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to do link train\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> exynos_dp_enable_scramble(dp, 1);
>> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> >> ret = exynos_dp_config_video(dp, dp->video_info);
>> >> if (ret) {
>> >> dev_err(&pdev->dev, "unable to config video\n");
>> >> - goto err_clock;
>> >> + return ret;
>> >> }
>> >>
>> >> platform_set_drvdata(pdev, dp);
>> >>
>> >> return 0;
>> >> -
>> >> -err_clock:
>> >> - clk_put(dp->clock);
>> >> -
>> >> - return ret;
>> >> }
>> >>
>> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> >> pdata->phy_exit();
>> >>
>> >> clk_disable(dp->clock);
>> >> - clk_put(dp->clock);
>> >>
>> >> return 0;
>> >> }
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at http://www.tux.org/lkml/
>>
>>
>>
>> --
>> With warm regards,
>> Sachin
>
--
With warm regards,
Sachin
^ permalink raw reply
* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Jingoo Han @ 2012-08-01 4:30 UTC (permalink / raw)
To: 'Sachin Kamat'
Cc: 'Damien Cassou', kernel-janitors,
'Florian Tobias Schandinat', linux-fbdev, linux-kernel,
'Jingoo Han'
In-Reply-To: <CAK9yfHzje6NLqi5ixxA3J4aT1a9p6wJNJePzTi4xG9LN6AeLVw@mail.gmail.com>
On Wednesday, August 01, 2012 1:00 PMSachin Kamat wrote:
>
> On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> > On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
> >>
> >> From: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> The various devm_ functions allocate memory that is released when a driver
> >> detaches. This patch uses these functions for data that is allocated in
> >> the probe function of a platform device and is only freed in the remove
> >> function.
> >>
> >> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
> >>
> >> ---
> >> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> >> 1 file changed, 7 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> >> index c6c016a..00fe4f0 100644
> >> --- a/drivers/video/exynos/exynos_dp_core.c
> >> +++ b/drivers/video/exynos/exynos_dp_core.c
> >> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >>
> >> dp->dev = &pdev->dev;
> >>
> >> - dp->clock = clk_get(&pdev->dev, "dp");
> >> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> >> if (IS_ERR(dp->clock)) {
> >> dev_err(&pdev->dev, "failed to get clock\n");
> >> return PTR_ERR(dp->clock);
> >> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> clk_enable(dp->clock);
> >>
> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> - if (!res) {
> >> - dev_err(&pdev->dev, "failed to get registers\n");
> >> - ret = -EINVAL;
> >> - goto err_clock;
> >> - }
> >
> > Why do you remove this return check?
> > If there is no reason, please, do it as follows:
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > if (!res) {
> > dev_err(&pdev->dev, "failed to get registers\n");
> > - ret = -EINVAL;
> > - goto err_clock;
> > + return -EINVAL;
> > }
> >
> >
>
> devm_request_and_ioremap function checks the validity of res. Hence
> this check above is redundant and can be removed.
I don't think so.
Even though function called next checks the NULL value,
for robustness, the return value of platform_get_resource() should be
checked.
It is possible that devm_request_and_ioremap() can be changed in the future,
as request_mem_region() & ioremap() were changed to devm_request_and_ioremap().
Best regards,
Jingoo Han
>
> Damien,
> This patch only adds devm_clk_get() function. Hence you could make the
> subject line more specific.
>
>
>
>
> > Best regards,
> > Jingoo Han
> >
> >
> >>
> >> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> >> if (!dp->reg_base) {
> >> dev_err(&pdev->dev, "failed to ioremap\n");
> >> - ret = -ENOMEM;
> >> - goto err_clock;
> >> + return -ENOMEM;
> >> }
> >>
> >> dp->irq = platform_get_irq(pdev, 0);
> >> if (!dp->irq) {
> >> dev_err(&pdev->dev, "failed to get irq\n");
> >> - ret = -ENODEV;
> >> - goto err_clock;
> >> + return -ENODEV;
> >> }
> >>
> >> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> >> "exynos-dp", dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "failed to request irq\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> dp->video_info = pdata->video_info;
> >> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_detect_hpd(dp);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to detect hpd\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_handle_edid(dp);
> >> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> dp->video_info->link_rate);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to do link train\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> exynos_dp_enable_scramble(dp, 1);
> >> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> >> ret = exynos_dp_config_video(dp, dp->video_info);
> >> if (ret) {
> >> dev_err(&pdev->dev, "unable to config video\n");
> >> - goto err_clock;
> >> + return ret;
> >> }
> >>
> >> platform_set_drvdata(pdev, dp);
> >>
> >> return 0;
> >> -
> >> -err_clock:
> >> - clk_put(dp->clock);
> >> -
> >> - return ret;
> >> }
> >>
> >> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> >> pdata->phy_exit();
> >>
> >> clk_disable(dp->clock);
> >> - clk_put(dp->clock);
> >>
> >> return 0;
> >> }
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
>
>
> --
> With warm regards,
> Sachin
^ permalink raw reply
* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Sachin Kamat @ 2012-08-01 4:11 UTC (permalink / raw)
To: Jingoo Han
Cc: Damien Cassou, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
In-Reply-To: <002c01cd6f73$4252b090$c6f811b0$%han@samsung.com>
On 1 August 2012 04:51, Jingoo Han <jg1.han@samsung.com> wrote:
> On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>>
>> From: Damien Cassou <damien.cassou@lifl.fr>
>>
>> The various devm_ functions allocate memory that is released when a driver
>> detaches. This patch uses these functions for data that is allocated in
>> the probe function of a platform device and is only freed in the remove
>> function.
>>
>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>
>> ---
>> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
>> 1 file changed, 7 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
>> index c6c016a..00fe4f0 100644
>> --- a/drivers/video/exynos/exynos_dp_core.c
>> +++ b/drivers/video/exynos/exynos_dp_core.c
>> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>>
>> dp->dev = &pdev->dev;
>>
>> - dp->clock = clk_get(&pdev->dev, "dp");
>> + dp->clock = devm_clk_get(&pdev->dev, "dp");
>> if (IS_ERR(dp->clock)) {
>> dev_err(&pdev->dev, "failed to get clock\n");
>> return PTR_ERR(dp->clock);
>> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> clk_enable(dp->clock);
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - if (!res) {
>> - dev_err(&pdev->dev, "failed to get registers\n");
>> - ret = -EINVAL;
>> - goto err_clock;
>> - }
>
> Why do you remove this return check?
> If there is no reason, please, do it as follows:
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res) {
> dev_err(&pdev->dev, "failed to get registers\n");
> - ret = -EINVAL;
> - goto err_clock;
> + return -EINVAL;
> }
>
>
devm_request_and_ioremap function checks the validity of res. Hence
this check above is redundant and can be removed.
Damien,
This patch only adds devm_clk_get() function. Hence you could make the
subject line more specific.
> Best regards,
> Jingoo Han
>
>
>>
>> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
>> if (!dp->reg_base) {
>> dev_err(&pdev->dev, "failed to ioremap\n");
>> - ret = -ENOMEM;
>> - goto err_clock;
>> + return -ENOMEM;
>> }
>>
>> dp->irq = platform_get_irq(pdev, 0);
>> if (!dp->irq) {
>> dev_err(&pdev->dev, "failed to get irq\n");
>> - ret = -ENODEV;
>> - goto err_clock;
>> + return -ENODEV;
>> }
>>
>> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
>> "exynos-dp", dp);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to request irq\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> dp->video_info = pdata->video_info;
>> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> ret = exynos_dp_detect_hpd(dp);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to detect hpd\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> exynos_dp_handle_edid(dp);
>> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> dp->video_info->link_rate);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to do link train\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> exynos_dp_enable_scramble(dp, 1);
>> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>> ret = exynos_dp_config_video(dp, dp->video_info);
>> if (ret) {
>> dev_err(&pdev->dev, "unable to config video\n");
>> - goto err_clock;
>> + return ret;
>> }
>>
>> platform_set_drvdata(pdev, dp);
>>
>> return 0;
>> -
>> -err_clock:
>> - clk_put(dp->clock);
>> -
>> - return ret;
>> }
>>
>> static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
>> pdata->phy_exit();
>>
>> clk_disable(dp->clock);
>> - clk_put(dp->clock);
>>
>> return 0;
>> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
With warm regards,
Sachin
^ permalink raw reply
* Re: [PATCH V2 5/5] arm: samsung: delete frame buffer header files from platform
From: Leela Krishna Amudala @ 2012-08-01 3:32 UTC (permalink / raw)
To: Kukjin Kim, Sylwester Nawrocki
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, joshi, jg1.han, m.szyprowski
In-Reply-To: <02ee01cd6f89$f218b2c0$d64a1840$%kim@samsung.com>
Hello Kgene,
On Wed, Aug 1, 2012 at 7:34 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Leela Krishna Amudala wrote:
>>
>> The FIMD register headers are moved to include/video/
>> hence, deleting these files from platform side
>>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> ---
>> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 ---------
>> arch/arm/plat-samsung/include/plat/regs-fb.h | 403
> -------------------
>> ----
>> 2 files changed, 0 insertions(+), 562 deletions(-)
>> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
>> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
>>
> No. This should be squashed into first patch on this series. See below.
>
> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159
> --------------------
> .../plat/regs-fb.h => include/video/samsung_fimd.h | 145
> ++++++++++++++++--
> 2 files changed, 134 insertions(+), 170 deletions(-)
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> rename arch/arm/plat-samsung/include/plat/regs-fb.h =>
> include/video/samsung_fimd.h (74%)
>
If I squash it with the first patch and if somebody set that as a head
commit, it will break the build. Hence, splitted it up from the first
patch.
Thank you Sylwester for suggesting this split up change.
Regards,
Leela Krishna
> Thanks.
>
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-01 2:50 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Simon Glass, Grant Likely, Rob Herring,
Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120731101931.GB16155-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On 07/31/2012 07:19 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
>> On 07/30/2012 08:33 PM, Thierry Reding wrote:
>>>> +You will need an instance of power_seq_resources to keep track of the resources
>>>> +that are already allocated. On success, the function returns a devm allocated
>>>> +resolved sequence that is ready to be passed to power_seq_run(). In case of
>>>> +failure, and error code is returned.
>>>
>>> I don't quite understand why the struct power_seq_resources is needed.
>>> Can this not be stored within power_seq?
>>
>> power_seq_resources serves two purposes:
>> 1) When parsing sequences, it keeps track of the resources we have
>> already allocated to avoid getting the same resource twice
>> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
>> those that are not devm-handled).
>>
>> 2) can certainly be removed either by enforcing use of devm, or by
>> doing reference counting. 1) seems more difficult to avoid - we need
>> to keep track of the resources we already own between calls to
>> power_seq_build(). I'd certainly be glad to remove that structure
>> from public view and simplify the code if that is possible though.
>
> I still don't see the problem. Managing the resources should be part of
> the power_seq core and shouldn't be visible to users. Maybe what you are
> worried about is that you may need the same resource both for a power-up
> and a power-down sequence? I can see how that would require a global
> list of resources.
Yes, that is precisely my concern. Sorry for not stating that more clearly.
> However I still think it would be easier to encapsulate that completely.
> Maybe another level of abstraction is required. You could for example
> add another type to encapsulate several power sequences and that could
> keep a list of used resources. I can't think of a good name, but maybe
> the following DT snippet clarifies what I mean:
>
> power-sequences {
> #address-cells = <1>;
> #size-cells = <0>;
>
> sequence@0 {
> name = "up";
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> step@0 {
> ...
> };
>
> ...
> };
>
> sequence@1 {
> name = "down";
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> step@0 {
> ...
> };
>
> ...
> };
> };
>
> If you add a name property like this, you could extend the API to
> support running a named sequence:
>
> power_seq_run(seq, "up");
> ...
> power_seq_run(seq, "down);
Mmm, that's something to consider. Forcing power sequences to be grouped
within a "power-sequences" node would also make parsing easier from the
driver side since it would not have to explicitly parse every sequence.
We could even imagine some tighter integration with the device subsystem
to automatically run specifically-named sequences during suspend/resume.
But maybe I'm thinking too much here.
>
>>> Also, is there some way we can make the id property for GPIOs not
>>> require the -gpio suffix? If the resource type is already GPIO, then it
>>> seems redundant to add -gpio to the ID.
>>
>> There is unfortunately an inconsistency between the way regulators
>> and GPIOs are gotten by name. regulator_get(id) will expect to find
>> a property named "id-supply", while gpio_request_one(id) expects a
>> property named exactly "id". To workaround this we could sprintf the
>> correct property name from a non-suffixed property name within the
>> driver, but I think this actually speaks more in favor of having
>> phandles directly into the sequences.
>
> Yes, if it can be made to work by specifying the phandle directly that
> is certainly better.
Let's do that then - as for the PWM issue I had, let's address that by
clearly stating in the documentation that phandles referring to a same
device *must* be identical.
>>>> + if (!seq) return 0;
>>>
>>> I don't think this is acceptable according to the coding style. Also,
>>> perhaps returning -EINVAL would be more meaningful?
>>
>> I neglected running checkpatch before submitting, apologies for
>> that. The return value seems correct to me, a NULL sequence has no
>> effect.
>
> But seq = NULL should never happen anyway, right?
It could if you are parsing a NULL node. It seems safe to me to consider
that a NULL sequence is an empty sequence, but if I go for your solution
involving another data structure to encapsulate the sequence, then this
might change.
>>> Perhaps this should check for POWER_SEQ_STOP instead?
>>
>> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
>> is used instead.
>
> Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
> a sequence, so intuitively I'd be looking for that as a stop condition.
That is for platform data - resolved sequences get their type from their
resource, and a STOP sequence does not have a resource. But the STOP
type will go away too since we will have a steps count in the platform
data instead.
>> I would like to do that actually. The issue is that it did not work
>> go well with the legacy pwm_backlight behavior: a power sequence
>> needs to be constructed out of a PWM obtained through
>> pwm_request(int pwm_id, char *label) and this behavior cannot be
>> emulated using the new platform data interface (which only works
>> with pwm_get()). But if I remove this old behavior, then I could
>> make power_seq opaque. I don't think many drivers are using it. What
>> do you think?
>
> I don't see how that is relevant here, since this power-sequencing code
> is supposed to be generic and not tied to any specific implementation.
> Can you explain further?
>
> In any case you shouldn't be using pwm_request() in new code.
Power sequences only rely on pwm_get, and never call pwm_request since
it is, as you stated, deprecated. However there are still boards that
use the old pwm_id member of the pwm_backlight_platform_data. For these,
we must call pwm_request from the pwm_backlight driver in order to
resolve the PWM (see pwm_backlight_legacy_probe of the seconds patch).
As the PWM is being resolved by the backlight driver, and not within the
power sequences parser, the resolved data structure must be visible to
pwm_backlight so it can construct it. There are two ways to solve this
and keep the power sequences structure private:
1) Add a way to resolve a PWM by id using pwm_request in the power
sequences (we probably should not do that)
2) Port the old platform pwm_request code to use pwm_add_table and
pwm_get instead.
Do I get points if I do 2)? :)
Thanks,
Alex.
^ permalink raw reply
* RE: [PATCH V2 0/5] arm: samsung: Move FIMD headers to include/video/
From: Kukjin Kim @ 2012-08-01 2:28 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski, 'Florian Tobias Schandinat'
In-Reply-To: <1343737385-23337-1-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> This patchset moves the contents of regs-fb-v4.h and regs-fb.h from arch
> side
> to include/video/samsung_fimd.h
>
> This patchset is created and rebased against master branch of torvalds
> tree.
> Tested on smdk5250 board, build tested for other boards.
>
(Cc'ed Florian Tobias Schandinat)
Yeah, since it's merge window, this series could be created against on
mainline. And IMO, would be helpful to us if this series could be sent to
upstream via samsung tree after reviewing, because this touches too many
files in samsung tree and just adds include/video/samsung_fimd.h. But I'm
not sure the added inclusion will be used in other file of drivers/video. If
so, I can provide some topic branch can be merged into Florian's tree.
Florian, how about?
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
> Changes from version 1:
> - Split the patches as per Sylwester comments
> - Changed FIMD_V8_xxx macro to EXYNOS5_xxx
>
> Leela Krishna Amudala (5):
> include/video: Add samsung FIMD register header
> include/video: Add Exynos5 specific FIMD register offsets
> arm: samsung: Include the modified FIMD header file
> driver: Include the modified FIMD header file
> arm: samsung: delete frame buffer header files from platform
>
> arch/arm/mach-exynos/mach-nuri.c | 2 +-
> arch/arm/mach-exynos/mach-origen.c | 2 +-
> arch/arm/mach-exynos/mach-smdk4x12.c | 2 +-
> arch/arm/mach-exynos/mach-smdkv310.c | 2 +-
> arch/arm/mach-exynos/mach-universal_c210.c | 2 +-
> arch/arm/mach-exynos/setup-fimd0.c | 2 +-
> arch/arm/mach-s3c24xx/mach-smdk2416.c | 2 +-
> arch/arm/mach-s3c64xx/mach-anw6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-crag6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
> arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-ncp.c | 2 +-
> arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq5.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq7.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smdk6410.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6440.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6450.c | 2 +-
> arch/arm/mach-s5pc100/mach-smdkc100.c | 2 +-
> arch/arm/mach-s5pv210/mach-aquila.c | 2 +-
> arch/arm/mach-s5pv210/mach-goni.c | 2 +-
> arch/arm/mach-s5pv210/mach-smdkv210.c | 2 +-
> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159
----------------
> ----
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
> drivers/video/s3c-fb.c | 2 +-
> .../plat/regs-fb.h => include/video/samsung_fimd.h | 152
> +++++++++++++++++--
> 26 files changed, 165 insertions(+), 194 deletions(-)
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> rename arch/arm/plat-samsung/include/plat/regs-fb.h =>
> include/video/samsung_fimd.h (74%)
^ permalink raw reply
* RE: [PATCH V2 4/5] driver: Include the modified FIMD header file
From: Kukjin Kim @ 2012-08-01 2:20 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski
In-Reply-To: <1343737385-23337-5-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> The fimd register headers have been moved to include/video/
> hence, modifying the driver files accordingly.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
> drivers/video/s3c-fb.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
[...]
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -26,7 +26,7 @@
> #include <linux/pm_runtime.h>
>
> #include <mach/map.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/fb.h>
>
See my previous comments.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH V2 3/5] arm: samsung: Include the modified FIMD header file
From: Kukjin Kim @ 2012-08-01 2:19 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski
In-Reply-To: <1343737385-23337-4-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> The fimd register headers have been moved to include/video/
> hence, modifying the machine files accordingly.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> arch/arm/mach-exynos/mach-nuri.c | 2 +-
> arch/arm/mach-exynos/mach-origen.c | 2 +-
> arch/arm/mach-exynos/mach-smdk4x12.c | 2 +-
> arch/arm/mach-exynos/mach-smdkv310.c | 2 +-
> arch/arm/mach-exynos/mach-universal_c210.c | 2 +-
> arch/arm/mach-exynos/setup-fimd0.c | 2 +-
> arch/arm/mach-s3c24xx/mach-smdk2416.c | 2 +-
> arch/arm/mach-s3c64xx/mach-anw6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-crag6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
> arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-ncp.c | 2 +-
> arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq5.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq7.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smdk6410.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6440.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6450.c | 2 +-
> arch/arm/mach-s5pc100/mach-smdkc100.c | 2 +-
> arch/arm/mach-s5pv210/mach-aquila.c | 2 +-
> arch/arm/mach-s5pv210/mach-goni.c | 2 +-
> arch/arm/mach-s5pv210/mach-smdkv210.c | 2 +-
> 22 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-
> nuri.c
> index f98a83a..573a0c4 100644
> --- a/arch/arm/mach-exynos/mach-nuri.c
> +++ b/arch/arm/mach-exynos/mach-nuri.c
> @@ -39,7 +39,7 @@
> #include <asm/mach-types.h>
>
> #include <plat/adc.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/regs-serial.h>
> #include <plat/cpu.h>
> #include <plat/devs.h>
NO! Don't just replace it. Please put the inclusions with the same kind like
following.
diff --git a/arch/arm/mach-exynos/mach-nuri.c
b/arch/arm/mach-exynos/mach-nuri.c
index ea785fc..90d8daa 100644
--- a/arch/arm/mach-exynos/mach-nuri.c
+++ b/arch/arm/mach-exynos/mach-nuri.c
@@ -29,6 +29,7 @@
#include <drm/exynos_drm.h>
#include <video/platform_lcd.h>
+#include <video/samsung_fimd.h>
#include <media/m5mols.h>
#include <media/s5k6aa.h>
#include <media/s5p_fimc.h>
@@ -39,7 +40,6 @@
#include <asm/mach-types.h>
#include <plat/adc.h>
-#include <plat/regs-fb-v4.h>
#include <plat/regs-serial.h>
#include <plat/cpu.h>
#include <plat/devs.h>
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply related
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Mitch Bradley @ 2012-08-01 2:15 UTC (permalink / raw)
To: Alex Courbot
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Brown,
Stephen Warren, Greg Kroah-Hartman,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <50188ABB.2060304-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 8/1/2012 9:47 AM, Alex Courbot wrote:
> On 07/31/2012 09:55 PM, Mitch Bradley wrote:
>> On 7/31/2012 8:38 PM, Thierry Reding wrote:
>>> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
>>>> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>>>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>>>> I wonder if using the same structure/array as input and output would
>>>>>>> simplify the API; the platform data would fill in the fields mentioned
>>>>>>> above, and power_seq_build() would parse those, then set other fields in
>>>>>>> the same structs to the looked-up handle values?
>>>>>>
>>>>>> The thing is that I am not sure what happens to the platform data
>>>>>> once probe() is done. Isn't it customary to mark it with __devinit
>>>>>> and have it freed after probing is successful?
>>>>>
>>>>> No, platform data should stay around forever. Otherwise, consider what
>>>>> would happen if your driver is built as a module and you unload and load
>>>>> it again.
>>>>>
>>>>>> More generally, I think it is a good practice to have data
>>>>>> structures tailored right for what they need to do - code with
>>>>>> members that are meaningful only at given points of an instance's
>>>>>> life tends to be more confusing.
>>>>>
>>>>> I agree. Furthermore the driver unload/reload would be another reason
>>>>> not to reuse platform data as the output of the build() function.
>>>>>
>>>>> But maybe what Stephen meant was more like filling a structure with data
>>>>> taken from the platform data and pass that to a resolve() function which
>>>>> would fill in the missing pieces like pointers to actual resources. I
>>>>> imagine a managed interface would become a little trickier to do using
>>>>> such an approach.
>>>>>
>>>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>>>>> have to if all named "step" and there's more than one of them, then they
>>>>>>> will need a matching reg property. Equally, the parent node will need
>>>>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>>>>
>>>>>>> power-on-sequence {
>>>>>>> #address-cells = <1>;
>>>>>>> #size-cells = <0>;
>>>>>>> step@0 {
>>>>>>> reg = <0>;
>>>>>>
>>>>>> That's precisely what I would like to avoid - I don't need the steps
>>>>>> to be numbered and I certainly have no use for a reg property. Isn't
>>>>>> there a way to make it simpler?
>>>>>
>>>>> It's not technically valid to not have the reg property. Or
>>>>> #address-cells and #size-cells properties for that matter.
>>>>
>>>> I'm not keen on this representation where individual steps are nodes.
>>>> That seems like it could end up being too "heavyweight" for a long sequence.
>>>
>>> The other alternative would involve using a single property to encode
>>> one sequence. I think that was the initial proposal, though using proper
>>> phandle encoding it could probably be enhanced a bit. However anything
>>> that involves a single property has the problem that we need to encode
>>> the type of resource as an integer, and that makes things very hard to
>>> read.
>>>
>>> So it would look something like this:
>>>
>>> power-on = <1 &gpio 6 0 1
>>> 0 10000
>>> 2 ® 1
>>> 3 &pwm 0 5000000 1>;
>>>
>>> power-off = <3 &pwm 0 5000000 0
>>> 2 ® 0
>>> 0 10000
>>> 1 &gpio 6 0 0>;
>>>
>>> So the first cell would encode the type:
>>> 0: delay
>>> 1: gpio
>>> 2: regulator
>>> 3: PWM
>>>
>>> The next n cells would be the phandle and the specifier, while the last
>>> cell would encode a resource-specific parameter:
>>> delay: time in microseconds
>>> gpio: set level (0: low, 1: high)
>>> regulator: 0: disable, 1: enable
>>> pwm: 0: disable, 1: enable
>>>
>>> I guess this would be more compact, but it is also very hard to read. Is
>>> that something you would be happier with? Perhaps you were thinking of
>>> something completely different?
>>
>>
>> Perhaps a compact/flexible encoding could be designed, with a textual
>> encoding that is easy to read. A separate tool could convert the text
>> encoding to the integer format, annotated with comments containing
>> the "source text". A file containing that output could be #included
>> into the dts file.
>
> Do you mean having a external compiler that would run before dtc just
> for producing the power sequences? That sounds a little bit overkill for
> something that ough to remain simple.
>
> Also, although I admit I don't have the whole picture of where they
> could be used, I don't expect the power sequences to grow to sizes that
> would make us bother about their footprint.
It is axiomatic that every "language", if it succeeds at all, eventually
grows into a complete programming language. The more special-purpose
that it starts as, the uglier that it ends up.
>
> Alex.
>
^ permalink raw reply
* RE: [PATCH V2 2/5] include/video: Add Exynos5 specific FIMD register offsets
From: Kukjin Kim @ 2012-08-01 2:14 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski
In-Reply-To: <1343737385-23337-3-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> Exynos5 has VIDTCON and VIDCON registers at different offsets
> from the previous SOCs. Hence, adding the macros.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> include/video/samsung_fimd.h | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
> index e979f42..820f190 100644
> --- a/include/video/samsung_fimd.h
> +++ b/include/video/samsung_fimd.h
> @@ -524,3 +524,10 @@
> * 1110 -none- -none- -none- -none- -none-
> * 1111 -none- -none- -none- -none- -none-
> */
> +
> +/*EXYNOS5 FIMD REG OFFSET */
> +#define EXYNOS5_VIDTCON0 (0x20010)
> +#define EXYNOS5_VIDTCON1 (0x20014)
> +#define EXYNOS5_VIDTCON2 (0x20018)
> +#define EXYNOS5_VIDTCON3 (0x2001C)
> +#define EXYNOS5_VIDCON1 (0x20004)
> --
> 1.7.0.4
All of EXYNOS5 SoCs including upcoming SoCs _really_ have same address like
above?
I don't think so. You need to consider its flexibility or some compatibility
when definitions are added.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH V2 1/5] include/video: Add samsung FIMD register header
From: Kukjin Kim @ 2012-08-01 2:10 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski
In-Reply-To: <1343737385-23337-2-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> This patch copies the contents from regs-fb-v4.h and regs-fb.h to
> include/video/samsung_fimd.h
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> include/video/samsung_fimd.h | 526
> ++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 526 insertions(+), 0 deletions(-)
> create mode 100644 include/video/samsung_fimd.h
>
Firstly, please check my comment on your previous patches.
And I don't see following definitions are needed and used now. If some
definitions are not needed, they can be removed.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH V2 5/5] arm: samsung: delete frame buffer header files from platform
From: Kukjin Kim @ 2012-08-01 2:04 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han,
m.szyprowski
In-Reply-To: <1343737385-23337-6-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> The FIMD register headers are moved to include/video/
> hence, deleting these files from platform side
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 ---------
> arch/arm/plat-samsung/include/plat/regs-fb.h | 403
-------------------
> ----
> 2 files changed, 0 insertions(+), 562 deletions(-)
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
>
No. This should be squashed into first patch on this series. See below.
arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159
--------------------
.../plat/regs-fb.h => include/video/samsung_fimd.h | 145
++++++++++++++++--
2 files changed, 134 insertions(+), 170 deletions(-)
delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
rename arch/arm/plat-samsung/include/plat/regs-fb.h =>
include/video/samsung_fimd.h (74%)
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Kukjin Kim @ 2012-08-01 1:56 UTC (permalink / raw)
To: 'Marek Szyprowski', 'Jingoo Han',
'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, joshi
In-Reply-To: <033c01cd6ef6$fb514570$f1f3d050$%szyprowski@samsung.com>
Marek Szyprowski wrote:
>
> Hello,
>
Hi all,
[...]
> > static struct s3c_fb_driverdata s3c_fb_data_exynos5 = {
> > .variant = {
> > .nr_windows = 5,
> > - .vidtcon = VIDTCON0,
> > + .vidtcon = FIMD_V8_VIDTCON0,
>
> Yes, this method looks good imo. Maybe even having something like
> vidtcon_base in
> variant structure will be enough to cover all VIDTCON0-3 registers.
>
Yes, I agree. And how about changing the name of s3c_fb_drvierdata to
xxx_fimd_v8 not xxx_fimd_exynos5? It's time to use independent from SoC name
because we don't have any guarantee of same address on all of EXYNOS5 SoCs.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Kukjin Kim @ 2012-08-01 1:50 UTC (permalink / raw)
To: 'Jingoo Han', 'Leela Krishna Amudala'
Cc: linux-arm-kernel, linux-samsung-soc, dri-devel, linux-fbdev,
ben-linux, inki.dae, joshi, 'Marek Szyprowski'
In-Reply-To: <000a01cd6eb6$2113a050$633ae0f0$%han@samsung.com>
Jingoo Han wrote:
>
> On Monday, July 30, 2012 8:16 PM, Leela Krishna Amudala wrote:
> >
> > Hello Jingoo Han,
> >
> > On Mon, Jul 30, 2012 at 2:23 PM, Jingoo Han <jg1.han@samsung.com> wrote:
> > > On Monday, July 30, 2012 5:45 PM, Leela Krishna Amudala wrote:
> > >>
> > >> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> > >> to include/video/samsung_fimd.h
> > >>
> > >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> > >> ---
> > >> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> > >> arch/arm/plat-samsung/include/plat/regs-fb.h | 403
---------------
> --
> > >> include/video/samsung_fimd.h | 533
> +++++++++++++++++++++++
> > >> 3 files changed, 533 insertions(+), 562 deletions(-)
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> > >> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> > >> create mode 100644 include/video/samsung_fimd.h
> > >>
> > >> +*/
> > >> +
> > >> +/*FIMD V8 REG OFFSET */
> > >> +#define FIMD_V8_VIDTCON0 (0x20010)
> > >> +#define FIMD_V8_VIDTCON1 (0x20014)
> > >> +#define FIMD_V8_VIDTCON2 (0x20018)
> > >> +#define FIMD_V8_VIDTCON3 (0x2001C)
> > >> +#define FIMD_V8_VIDCON1 (0x20004)
>
>
> How about using soc_is_exynos5250()?
>
> +#define VIDTCON0 (soc_is_exynos5250() ? \
> + (0x20010) : (0x10))
>
> In this case, the FIMD driver does not need to change.
> Also, one binary is available.
>
I don't agree. Do you _really_ think the address will be fixed on other SoCs
including upcoming SoCs? I'd prefer to use the version or something for it.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* RE: [PATCH V2 2/5] include/video: Add Exynos5 specific FIMD register offsets
From: Jingoo Han @ 2012-08-01 1:49 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
m.szyprowski, 'Sylwester Nawrocki', 'Jingoo Han'
In-Reply-To: <1343737385-23337-3-git-send-email-l.krishna@samsung.com>
On Tuesday, July 31, 2012 9:23 PM Leela Krishna Amudala wrote:
>
> Exynos5 has VIDTCON and VIDCON registers at different offsets
> from the previous SOCs. Hence, adding the macros.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> include/video/samsung_fimd.h | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
> index e979f42..820f190 100644
> --- a/include/video/samsung_fimd.h
> +++ b/include/video/samsung_fimd.h
> @@ -524,3 +524,10 @@
> * 1110 -none- -none- -none- -none- -none-
> * 1111 -none- -none- -none- -none- -none-
> */
> +
> +/*EXYNOS5 FIMD REG OFFSET */
^
Please insert one space between '/*' and 'EXYNOS5'.
As Sylwester Nawrocki mentioned, the following seems to be better.
/* EXYNOS5 specific register offset definitions */
Best regards,
Jingoo Han
> +#define EXYNOS5_VIDTCON0 (0x20010)
> +#define EXYNOS5_VIDTCON1 (0x20014)
> +#define EXYNOS5_VIDTCON2 (0x20018)
> +#define EXYNOS5_VIDTCON3 (0x2001C)
> +#define EXYNOS5_VIDCON1 (0x20004)
> --
> 1.7.0.4
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-01 1:47 UTC (permalink / raw)
To: Mitch Bradley
Cc: Thierry Reding, linux-fbdev@vger.kernel.org, Stephen Warren,
Greg Kroah-Hartman, Mark Brown, linux-kernel@vger.kernel.org,
Rob Herring, linux-tegra@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org
In-Reply-To: <5017D5C9.3070905@firmworks.com>
On 07/31/2012 09:55 PM, Mitch Bradley wrote:
> On 7/31/2012 8:38 PM, Thierry Reding wrote:
>> On Tue, Jul 31, 2012 at 08:22:17PM +0800, Mitch Bradley wrote:
>>> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>>>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>>> I wonder if using the same structure/array as input and output would
>>>>>> simplify the API; the platform data would fill in the fields mentioned
>>>>>> above, and power_seq_build() would parse those, then set other fields in
>>>>>> the same structs to the looked-up handle values?
>>>>>
>>>>> The thing is that I am not sure what happens to the platform data
>>>>> once probe() is done. Isn't it customary to mark it with __devinit
>>>>> and have it freed after probing is successful?
>>>>
>>>> No, platform data should stay around forever. Otherwise, consider what
>>>> would happen if your driver is built as a module and you unload and load
>>>> it again.
>>>>
>>>>> More generally, I think it is a good practice to have data
>>>>> structures tailored right for what they need to do - code with
>>>>> members that are meaningful only at given points of an instance's
>>>>> life tends to be more confusing.
>>>>
>>>> I agree. Furthermore the driver unload/reload would be another reason
>>>> not to reuse platform data as the output of the build() function.
>>>>
>>>> But maybe what Stephen meant was more like filling a structure with data
>>>> taken from the platform data and pass that to a resolve() function which
>>>> would fill in the missing pieces like pointers to actual resources. I
>>>> imagine a managed interface would become a little trickier to do using
>>>> such an approach.
>>>>
>>>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>>>> have to if all named "step" and there's more than one of them, then they
>>>>>> will need a matching reg property. Equally, the parent node will need
>>>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>>>
>>>>>> power-on-sequence {
>>>>>> #address-cells = <1>;
>>>>>> #size-cells = <0>;
>>>>>> step@0 {
>>>>>> reg = <0>;
>>>>>
>>>>> That's precisely what I would like to avoid - I don't need the steps
>>>>> to be numbered and I certainly have no use for a reg property. Isn't
>>>>> there a way to make it simpler?
>>>>
>>>> It's not technically valid to not have the reg property. Or
>>>> #address-cells and #size-cells properties for that matter.
>>>
>>> I'm not keen on this representation where individual steps are nodes.
>>> That seems like it could end up being too "heavyweight" for a long sequence.
>>
>> The other alternative would involve using a single property to encode
>> one sequence. I think that was the initial proposal, though using proper
>> phandle encoding it could probably be enhanced a bit. However anything
>> that involves a single property has the problem that we need to encode
>> the type of resource as an integer, and that makes things very hard to
>> read.
>>
>> So it would look something like this:
>>
>> power-on = <1 &gpio 6 0 1
>> 0 10000
>> 2 ® 1
>> 3 &pwm 0 5000000 1>;
>>
>> power-off = <3 &pwm 0 5000000 0
>> 2 ® 0
>> 0 10000
>> 1 &gpio 6 0 0>;
>>
>> So the first cell would encode the type:
>> 0: delay
>> 1: gpio
>> 2: regulator
>> 3: PWM
>>
>> The next n cells would be the phandle and the specifier, while the last
>> cell would encode a resource-specific parameter:
>> delay: time in microseconds
>> gpio: set level (0: low, 1: high)
>> regulator: 0: disable, 1: enable
>> pwm: 0: disable, 1: enable
>>
>> I guess this would be more compact, but it is also very hard to read. Is
>> that something you would be happier with? Perhaps you were thinking of
>> something completely different?
>
>
> Perhaps a compact/flexible encoding could be designed, with a textual
> encoding that is easy to read. A separate tool could convert the text
> encoding to the integer format, annotated with comments containing
> the "source text". A file containing that output could be #included
> into the dts file.
Do you mean having a external compiler that would run before dtc just
for producing the power sequences? That sounds a little bit overkill for
something that ough to remain simple.
Also, although I admit I don't have the whole picture of where they
could be used, I don't expect the power sequences to grow to sizes that
would make us bother about their footprint.
Alex.
^ permalink raw reply
* RE: [PATCH 1/3] Move FIMD register headers to include/video/
From: Kukjin Kim @ 2012-08-01 1:46 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, joshi, jg1.han
In-Reply-To: <1343637905-17764-2-git-send-email-l.krishna@samsung.com>
Leela Krishna Amudala wrote:
>
> Moved the contents of regs-fb-v4.h and regs-fb.h from arch side
> to include/video/samsung_fimd.h
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> arch/arm/plat-samsung/include/plat/regs-fb-v4.h | 159 -------
> arch/arm/plat-samsung/include/plat/regs-fb.h | 403 -----------------
> include/video/samsung_fimd.h | 533
+++++++++++++++++++++++
> 3 files changed, 533 insertions(+), 562 deletions(-)
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb-v4.h
> delete mode 100644 arch/arm/plat-samsung/include/plat/regs-fb.h
> create mode 100644 include/video/samsung_fimd.h
>
+ I have some comments on this.
[...]
> +/* VIDCON0 */
I don't think above comment is needed because everybody knows that from the
name of VIDCON0_.
> +
> +#define VIDCON0 (0x00)
> +#define VIDCON0_INTERLACE (1 << 29)
> +#define VIDCON0_VIDOUT_MASK (0x3 << 26)
> +#define VIDCON0_VIDOUT_SHIFT (26)
I think, you can use either of the following.
+#define VIDCON0_VIDOUT_SHIFT (26)
+#define VIDCON0_VIDOUT_MASK (0x3 <<
VIDCON0_VIDOUT_SHIFT)
Or just without XXX_SHIFT if the XXX_SHIFT is not used somewhere else,
+#define VIDCON0_VIDOUT_MASK (0x3 << 26)
[...]
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply
* Re: [PATCH V2 3/5] arm: samsung: Include the modified FIMD header file
From: Jingoo Han @ 2012-08-01 1:42 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
m.szyprowski, 'Jingoo Han'
In-Reply-To: <1343737385-23337-4-git-send-email-l.krishna@samsung.com>
On Tuesday, July 31, 2012 9:23 PM Leela Krishna Amudala wrote:
>
> The fimd register headers have been moved to include/video/
> hence, modifying the machine files accordingly.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> arch/arm/mach-exynos/mach-nuri.c | 2 +-
> arch/arm/mach-exynos/mach-origen.c | 2 +-
> arch/arm/mach-exynos/mach-smdk4x12.c | 2 +-
> arch/arm/mach-exynos/mach-smdkv310.c | 2 +-
> arch/arm/mach-exynos/mach-universal_c210.c | 2 +-
> arch/arm/mach-exynos/setup-fimd0.c | 2 +-
> arch/arm/mach-s3c24xx/mach-smdk2416.c | 2 +-
> arch/arm/mach-s3c64xx/mach-anw6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-crag6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-hmt.c | 2 +-
> arch/arm/mach-s3c64xx/mach-mini6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-ncp.c | 2 +-
> arch/arm/mach-s3c64xx/mach-real6410.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq5.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smartq7.c | 2 +-
> arch/arm/mach-s3c64xx/mach-smdk6410.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6440.c | 2 +-
> arch/arm/mach-s5p64x0/mach-smdk6450.c | 2 +-
> arch/arm/mach-s5pc100/mach-smdkc100.c | 2 +-
> arch/arm/mach-s5pv210/mach-aquila.c | 2 +-
> arch/arm/mach-s5pv210/mach-goni.c | 2 +-
> arch/arm/mach-s5pv210/mach-smdkv210.c | 2 +-
> 22 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/mach-nuri.c b/arch/arm/mach-exynos/mach-nuri.c
> index f98a83a..573a0c4 100644
> --- a/arch/arm/mach-exynos/mach-nuri.c
> +++ b/arch/arm/mach-exynos/mach-nuri.c
> @@ -39,7 +39,7 @@
> #include <asm/mach-types.h>
>
> #include <plat/adc.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/regs-serial.h>
> #include <plat/cpu.h>
> #include <plat/devs.h>
Please move header include to proper place as bellows, including other
machine files such as origen, smdk, and so on.
#include <linux/...>
#include <video/samsung_fimd.h>
#include <plat/...>
#include <mach/...>
Best regards,
Jingoo Han
> diff --git a/arch/arm/mach-exynos/mach-origen.c b/arch/arm/mach-exynos/mach-origen.c
> index 5a12dc2..c69707e 100644
> --- a/arch/arm/mach-exynos/mach-origen.c
> +++ b/arch/arm/mach-exynos/mach-origen.c
> @@ -31,7 +31,7 @@
> #include <video/platform_lcd.h>
>
> #include <plat/regs-serial.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/cpu.h>
> #include <plat/devs.h>
> #include <plat/sdhci.h>
> diff --git a/arch/arm/mach-exynos/mach-smdk4x12.c b/arch/arm/mach-exynos/mach-smdk4x12.c
> index b26beb1..8a8acff 100644
> --- a/arch/arm/mach-exynos/mach-smdk4x12.c
> +++ b/arch/arm/mach-exynos/mach-smdk4x12.c
> @@ -35,7 +35,7 @@
> #include <plat/iic.h>
> #include <plat/keypad.h>
> #include <plat/mfc.h>
> -#include <plat/regs-fb.h>
> +#include <video/samsung_fimd.h>
> #include <plat/regs-serial.h>
> #include <plat/sdhci.h>
>
> diff --git a/arch/arm/mach-exynos/mach-smdkv310.c b/arch/arm/mach-exynos/mach-smdkv310.c
> index 3cfa688..c2df6e8 100644
> --- a/arch/arm/mach-exynos/mach-smdkv310.c
> +++ b/arch/arm/mach-exynos/mach-smdkv310.c
> @@ -28,7 +28,7 @@
> #include <video/platform_lcd.h>
> #include <plat/regs-serial.h>
> #include <plat/regs-srom.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/cpu.h>
> #include <plat/devs.h>
> #include <plat/fb.h>
> diff --git a/arch/arm/mach-exynos/mach-universal_c210.c b/arch/arm/mach-exynos/mach-universal_c210.c
> index 4d1f40d..e6fb471 100644
> --- a/arch/arm/mach-exynos/mach-universal_c210.c
> +++ b/arch/arm/mach-exynos/mach-universal_c210.c
> @@ -39,7 +39,7 @@
> #include <plat/fb.h>
> #include <plat/mfc.h>
> #include <plat/sdhci.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/fimc-core.h>
> #include <plat/s5p-time.h>
> #include <plat/camport.h>
> diff --git a/arch/arm/mach-exynos/setup-fimd0.c b/arch/arm/mach-exynos/setup-fimd0.c
> index 07a6dbe..53c4c51 100644
> --- a/arch/arm/mach-exynos/setup-fimd0.c
> +++ b/arch/arm/mach-exynos/setup-fimd0.c
> @@ -14,7 +14,7 @@
> #include <linux/gpio.h>
>
> #include <plat/gpio-cfg.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include <mach/map.h>
>
> diff --git a/arch/arm/mach-s3c24xx/mach-smdk2416.c b/arch/arm/mach-s3c24xx/mach-smdk2416.c
> index c3100a0..c8d5f51 100644
> --- a/arch/arm/mach-s3c24xx/mach-smdk2416.c
> +++ b/arch/arm/mach-s3c24xx/mach-smdk2416.c
> @@ -52,7 +52,7 @@
> #include <plat/udc.h>
> #include <linux/platform_data/s3c-hsudc.h>
>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/fb.h>
>
> #include <plat/common-smdk.h>
> diff --git a/arch/arm/mach-s3c64xx/mach-anw6410.c b/arch/arm/mach-s3c64xx/mach-anw6410.c
> index ffa29dd..27e3087 100644
> --- a/arch/arm/mach-s3c64xx/mach-anw6410.c
> +++ b/arch/arm/mach-s3c64xx/mach-anw6410.c
> @@ -44,7 +44,7 @@
> #include <plat/regs-serial.h>
> #include <plat/iic.h>
> #include <plat/fb.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include <plat/clock.h>
> #include <plat/devs.h>
> diff --git a/arch/arm/mach-s3c64xx/mach-crag6410.c b/arch/arm/mach-s3c64xx/mach-crag6410.c
> index 09cd812..66e8c69 100644
> --- a/arch/arm/mach-s3c64xx/mach-crag6410.c
> +++ b/arch/arm/mach-s3c64xx/mach-crag6410.c
> @@ -57,7 +57,7 @@
> #include <mach/regs-gpio-memport.h>
>
> #include <plat/regs-serial.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/fb.h>
> #include <plat/sdhci.h>
> #include <plat/gpio-cfg.h>
> diff --git a/arch/arm/mach-s3c64xx/mach-hmt.c b/arch/arm/mach-s3c64xx/mach-hmt.c
> index 6890881..ab78c5e 100644
> --- a/arch/arm/mach-s3c64xx/mach-hmt.c
> +++ b/arch/arm/mach-s3c64xx/mach-hmt.c
> @@ -41,7 +41,7 @@
> #include <plat/clock.h>
> #include <plat/devs.h>
> #include <plat/cpu.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
>
> diff --git a/arch/arm/mach-s3c64xx/mach-mini6410.c b/arch/arm/mach-s3c64xx/mach-mini6410.c
> index 5539a25..4b9a9ff 100644
> --- a/arch/arm/mach-s3c64xx/mach-mini6410.c
> +++ b/arch/arm/mach-s3c64xx/mach-mini6410.c
> @@ -41,7 +41,7 @@
> #include <plat/nand.h>
> #include <plat/regs-serial.h>
> #include <plat/ts.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include <video/platform_lcd.h>
>
> diff --git a/arch/arm/mach-s3c64xx/mach-ncp.c b/arch/arm/mach-s3c64xx/mach-ncp.c
> index cad2e05..d4c8af0 100644
> --- a/arch/arm/mach-s3c64xx/mach-ncp.c
> +++ b/arch/arm/mach-s3c64xx/mach-ncp.c
> @@ -43,7 +43,7 @@
> #include <plat/clock.h>
> #include <plat/devs.h>
> #include <plat/cpu.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
>
> diff --git a/arch/arm/mach-s3c64xx/mach-real6410.c b/arch/arm/mach-s3c64xx/mach-real6410.c
> index 326b216..05f05d2 100644
> --- a/arch/arm/mach-s3c64xx/mach-real6410.c
> +++ b/arch/arm/mach-s3c64xx/mach-real6410.c
> @@ -42,7 +42,7 @@
> #include <plat/nand.h>
> #include <plat/regs-serial.h>
> #include <plat/ts.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include <video/platform_lcd.h>
>
> diff --git a/arch/arm/mach-s3c64xx/mach-smartq5.c b/arch/arm/mach-s3c64xx/mach-smartq5.c
> index d6266d8..aa225ff 100644
> --- a/arch/arm/mach-s3c64xx/mach-smartq5.c
> +++ b/arch/arm/mach-s3c64xx/mach-smartq5.c
> @@ -28,7 +28,7 @@
> #include <plat/devs.h>
> #include <plat/fb.h>
> #include <plat/gpio-cfg.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
> #include "mach-smartq.h"
> diff --git a/arch/arm/mach-s3c64xx/mach-smartq7.c b/arch/arm/mach-s3c64xx/mach-smartq7.c
> index 0957d2a..6e9c070 100644
> --- a/arch/arm/mach-s3c64xx/mach-smartq7.c
> +++ b/arch/arm/mach-s3c64xx/mach-smartq7.c
> @@ -28,7 +28,7 @@
> #include <plat/devs.h>
> #include <plat/fb.h>
> #include <plat/gpio-cfg.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
> #include "mach-smartq.h"
> diff --git a/arch/arm/mach-s3c64xx/mach-smdk6410.c b/arch/arm/mach-s3c64xx/mach-smdk6410.c
> index 0fe4f15..1803192 100644
> --- a/arch/arm/mach-s3c64xx/mach-smdk6410.c
> +++ b/arch/arm/mach-s3c64xx/mach-smdk6410.c
> @@ -72,7 +72,7 @@
> #include <plat/ts.h>
> #include <plat/keypad.h>
> #include <plat/backlight.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
>
> diff --git a/arch/arm/mach-s5p64x0/mach-smdk6440.c b/arch/arm/mach-s5p64x0/mach-smdk6440.c
> index 92fefad..0db0bdd 100644
> --- a/arch/arm/mach-s5p64x0/mach-smdk6440.c
> +++ b/arch/arm/mach-s5p64x0/mach-smdk6440.c
> @@ -52,7 +52,7 @@
> #include <plat/s5p-time.h>
> #include <plat/backlight.h>
> #include <plat/fb.h>
> -#include <plat/regs-fb.h>
> +#include <video/smasung_fimd.h>
> #include <plat/sdhci.h>
>
> #include "common.h"
> diff --git a/arch/arm/mach-s5p64x0/mach-smdk6450.c b/arch/arm/mach-s5p64x0/mach-smdk6450.c
> index e2335ec..c641d33 100644
> --- a/arch/arm/mach-s5p64x0/mach-smdk6450.c
> +++ b/arch/arm/mach-s5p64x0/mach-smdk6450.c
> @@ -52,7 +52,7 @@
> #include <plat/s5p-time.h>
> #include <plat/backlight.h>
> #include <plat/fb.h>
> -#include <plat/regs-fb.h>
> +#include <video/samsung_fimd.h>
> #include <plat/sdhci.h>
>
> #include "common.h"
> diff --git a/arch/arm/mach-s5pc100/mach-smdkc100.c b/arch/arm/mach-s5pc100/mach-smdkc100.c
> index 0c3ae38..e4df3d0 100644
> --- a/arch/arm/mach-s5pc100/mach-smdkc100.c
> +++ b/arch/arm/mach-s5pc100/mach-smdkc100.c
> @@ -51,7 +51,7 @@
> #include <plat/ts.h>
> #include <plat/audio.h>
> #include <plat/backlight.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
>
> diff --git a/arch/arm/mach-s5pv210/mach-aquila.c b/arch/arm/mach-s5pv210/mach-aquila.c
> index 78028df..8cbe4ac 100644
> --- a/arch/arm/mach-s5pv210/mach-aquila.c
> +++ b/arch/arm/mach-s5pv210/mach-aquila.c
> @@ -39,7 +39,7 @@
> #include <plat/fimc-core.h>
> #include <plat/sdhci.h>
> #include <plat/s5p-time.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "common.h"
>
> diff --git a/arch/arm/mach-s5pv210/mach-goni.c b/arch/arm/mach-s5pv210/mach-goni.c
> index 822a559..e799a02 100644
> --- a/arch/arm/mach-s5pv210/mach-goni.c
> +++ b/arch/arm/mach-s5pv210/mach-goni.c
> @@ -49,7 +49,7 @@
> #include <plat/clock.h>
> #include <plat/s5p-time.h>
> #include <plat/mfc.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/camport.h>
>
> #include <media/v4l2-mediabus.h>
> diff --git a/arch/arm/mach-s5pv210/mach-smdkv210.c b/arch/arm/mach-s5pv210/mach-smdkv210.c
> index 918b23d..3c569e5 100644
> --- a/arch/arm/mach-s5pv210/mach-smdkv210.c
> +++ b/arch/arm/mach-s5pv210/mach-smdkv210.c
> @@ -46,7 +46,7 @@
> #include <plat/fb.h>
> #include <plat/s5p-time.h>
> #include <plat/backlight.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/mfc.h>
> #include <plat/clock.h>
>
> --
> 1.7.0.4
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-01 1:42 UTC (permalink / raw)
To: Mitch Bradley
Cc: Thierry Reding,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, Greg Kroah-Hartman, Mark Brown,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <5017CDF9.2060304-D5eQfiDGL7eakBO8gow8eQ@public.gmane.org>
On 07/31/2012 09:22 PM, Mitch Bradley wrote:
> On 7/31/2012 6:56 PM, Thierry Reding wrote:
>> On Tue, Jul 31, 2012 at 07:32:20PM +0900, Alex Courbot wrote:
>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>> I wonder if using the same structure/array as input and output would
>>>> simplify the API; the platform data would fill in the fields mentioned
>>>> above, and power_seq_build() would parse those, then set other fields in
>>>> the same structs to the looked-up handle values?
>>>
>>> The thing is that I am not sure what happens to the platform data
>>> once probe() is done. Isn't it customary to mark it with __devinit
>>> and have it freed after probing is successful?
>>
>> No, platform data should stay around forever. Otherwise, consider what
>> would happen if your driver is built as a module and you unload and load
>> it again.
>>
>>> More generally, I think it is a good practice to have data
>>> structures tailored right for what they need to do - code with
>>> members that are meaningful only at given points of an instance's
>>> life tends to be more confusing.
>>
>> I agree. Furthermore the driver unload/reload would be another reason
>> not to reuse platform data as the output of the build() function.
>>
>> But maybe what Stephen meant was more like filling a structure with data
>> taken from the platform data and pass that to a resolve() function which
>> would fill in the missing pieces like pointers to actual resources. I
>> imagine a managed interface would become a little trickier to do using
>> such an approach.
>>
>>>> If the nodes have a unit address (i.e. end in "@n"), which they will
>>>> have to if all named "step" and there's more than one of them, then they
>>>> will need a matching reg property. Equally, the parent node will need
>>>> #address-cells and #size-cells too. So, the last couple lines would be:
>>>>
>>>> power-on-sequence {
>>>> #address-cells = <1>;
>>>> #size-cells = <0>;
>>>> step@0 {
>>>> reg = <0>;
>>>
>>> That's precisely what I would like to avoid - I don't need the steps
>>> to be numbered and I certainly have no use for a reg property. Isn't
>>> there a way to make it simpler?
>>
>> It's not technically valid to not have the reg property. Or
>> #address-cells and #size-cells properties for that matter.
>
> I'm not keen on this representation where individual steps are nodes.
> That seems like it could end up being too "heavyweight" for a long sequence.
Using nodes has a big advantage though: we can use any arbitrary
property to add extra parameters to the resource being controlled. Right
now we only use enable/disable, but for example one can imagine an
optional voltage setting for a regulator. It is much more future-proof
than a design where the number of parameters would be fixed and could
not be extended without breaking compatibility.
I experimented encoding the whole sequence within a single property. It
works of course, but is not really flexible, hard to read, and quite
error-prone overall. The memory footprint gain was not so obvious
neither (although it was indeed more compact).
What bothers me is to have to specify the cells layout and reg property
for *every single sequence*, but well, I guess we can live with that.
Alex.
^ permalink raw reply
* Re: [PATCH V2 4/5] driver: Include the modified FIMD header file
From: Jingoo Han @ 2012-08-01 1:36 UTC (permalink / raw)
To: 'Leela Krishna Amudala', linux-arm-kernel,
linux-samsung-soc
Cc: dri-devel, linux-fbdev, ben-linux, inki.dae, kgene.kim, joshi,
m.szyprowski, 'Jingoo Han'
In-Reply-To: <1343737385-23337-5-git-send-email-l.krishna@samsung.com>
On Tuesday, July 31, 2012 9:23 PM Leela Krishna Amudala wrote:
>
> The fimd register headers have been moved to include/video/
> hence, modifying the driver files accordingly.
>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> ---
> drivers/gpu/drm/exynos/exynos_drm_fimd.c | 2 +-
> drivers/video/s3c-fb.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 29fdbfe..8da90f9 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -20,7 +20,7 @@
> #include <linux/pm_runtime.h>
>
> #include <drm/exynos_drm.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
>
> #include "exynos_drm_drv.h"
> #include "exynos_drm_fbdev.h"
> diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
> index 69bf9d0..1226fdd 100644
> --- a/drivers/video/s3c-fb.c
> +++ b/drivers/video/s3c-fb.c
> @@ -26,7 +26,7 @@
> #include <linux/pm_runtime.h>
>
> #include <mach/map.h>
> -#include <plat/regs-fb-v4.h>
> +#include <video/samsung_fimd.h>
> #include <plat/fb.h>
Please move header include to proper place as bellows:
#include <linux/pm_runtime.h>
#include <video/samsung_fimd.h>
#include <mach/map.h>
#include <plat/fb.h>
Best regards,
Jingoo Han
>
> /* This driver will export a number of framebuffer interfaces depending
> --
> 1.7.0.4
^ permalink raw reply
* Re: Gethering power management/policy hw drivers under drivers/power/? (Re: [RFC][PATCH v3 1/3] runt
From: Anton Vorontsov @ 2012-08-01 0:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Alex Courbot, Jean Pihet, Greg Kroah-Hartman, David Woodhouse,
Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Liam Girdwood, MyungJoo Ham, linux-pm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <201207302259.39396.rjw-KKrjLPT3xs0@public.gmane.org>
On Mon, Jul 30, 2012 at 10:59:39PM +0200, Rafael J. Wysocki wrote:
[...]
> > Well, currently drivers/power/ is indeed just for power supply class
> > subsystem and drivers. But if the trend is to gather power management
> > ("policy") stuff under one directory, i.e.
> >
> > drivers/
> > power/
> > supplies/ <- former "power supply class and drivers"
> > regulators/
> > idle/
> > cpuidle/
> > cpufreq/
> > devfreq/
> > avs/
> > ...
> >
> > That would probably make sense, we could easily see the big picture.
> > But if we're not going to do this long-term,
>
> Yes, we may do this eventually, but surely not right now.
OK, great! Then I'll probably start thinking about moving power supply
drivers into its own directory, maybe in 3.7.
Thanks!
--
Anton Vorontsov
Email: cbouatmailru@gmail.com
^ permalink raw reply
* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Jingoo Han @ 2012-07-31 23:21 UTC (permalink / raw)
To: 'Damien Cassou'
Cc: kernel-janitors, 'Florian Tobias Schandinat', linux-fbdev,
linux-kernel, 'Jingoo Han'
In-Reply-To: <1343752762-16861-6-git-send-email-damien.cassou@lifl.fr>
On Wednesday, August 01, 2012 1:39 AM Damien Cassou wrote:
>
> From: Damien Cassou <damien.cassou@lifl.fr>
>
> The various devm_ functions allocate memory that is released when a driver
> detaches. This patch uses these functions for data that is allocated in
> the probe function of a platform device and is only freed in the remove
> function.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>
> ---
> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> 1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index c6c016a..00fe4f0 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>
> dp->dev = &pdev->dev;
>
> - dp->clock = clk_get(&pdev->dev, "dp");
> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> if (IS_ERR(dp->clock)) {
> dev_err(&pdev->dev, "failed to get clock\n");
> return PTR_ERR(dp->clock);
> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> clk_enable(dp->clock);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to get registers\n");
> - ret = -EINVAL;
> - goto err_clock;
> - }
Why do you remove this return check?
If there is no reason, please, do it as follows:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
dev_err(&pdev->dev, "failed to get registers\n");
- ret = -EINVAL;
- goto err_clock;
+ return -EINVAL;
}
Best regards,
Jingoo Han
>
> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> if (!dp->reg_base) {
> dev_err(&pdev->dev, "failed to ioremap\n");
> - ret = -ENOMEM;
> - goto err_clock;
> + return -ENOMEM;
> }
>
> dp->irq = platform_get_irq(pdev, 0);
> if (!dp->irq) {
> dev_err(&pdev->dev, "failed to get irq\n");
> - ret = -ENODEV;
> - goto err_clock;
> + return -ENODEV;
> }
>
> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> "exynos-dp", dp);
> if (ret) {
> dev_err(&pdev->dev, "failed to request irq\n");
> - goto err_clock;
> + return ret;
> }
>
> dp->video_info = pdata->video_info;
> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> dev_err(&pdev->dev, "unable to detect hpd\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_handle_edid(dp);
> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> dp->video_info->link_rate);
> if (ret) {
> dev_err(&pdev->dev, "unable to do link train\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_enable_scramble(dp, 1);
> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_config_video(dp, dp->video_info);
> if (ret) {
> dev_err(&pdev->dev, "unable to config video\n");
> - goto err_clock;
> + return ret;
> }
>
> platform_set_drvdata(pdev, dp);
>
> return 0;
> -
> -err_clock:
> - clk_put(dp->clock);
> -
> - return ret;
> }
>
> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> pdata->phy_exit();
>
> clk_disable(dp->clock);
> - clk_put(dp->clock);
>
> return 0;
> }
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox