2017-03-15 11:34:40

by Philipp Zabel

[permalink] [raw]
Subject: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

As of commit bb475230b8e5 ("reset: make optional functions really
optional"), the reset framework API calls use NULL pointers to describe
optional, non-present reset controls.

This allows to return errors from devm_reset_control_get_optional and to
call reset_control_(de)assert unconditionally.

Signed-off-by: Philipp Zabel <[email protected]>
---
drivers/mmc/host/dw_mmc.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index a9ac0b4573131..3d62b0a1f81cb 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)

/* find reset controller when exist */
pdata->rstc = devm_reset_control_get_optional(dev, "reset");
- if (IS_ERR(pdata->rstc)) {
- if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
- return ERR_PTR(-EPROBE_DEFER);
- }
+ if (IS_ERR(pdata->rstc))
+ return ERR_CAST(pdata->rstc);

/* find out number of slots supported */
of_property_read_u32(np, "num-slots", &pdata->num_slots);
@@ -3100,7 +3098,7 @@ int dw_mci_probe(struct dw_mci *host)
}
}

- if (!IS_ERR(host->pdata->rstc)) {
+ if (host->pdata->rstc) {
reset_control_assert(host->pdata->rstc);
usleep_range(10, 50);
reset_control_deassert(host->pdata->rstc);
@@ -3257,8 +3255,7 @@ int dw_mci_probe(struct dw_mci *host)
if (host->use_dma && host->dma_ops->exit)
host->dma_ops->exit(host);

- if (!IS_ERR(host->pdata->rstc))
- reset_control_assert(host->pdata->rstc);
+ reset_control_assert(host->pdata->rstc);

err_clk_ciu:
clk_disable_unprepare(host->ciu_clk);
@@ -3290,8 +3287,7 @@ void dw_mci_remove(struct dw_mci *host)
if (host->use_dma && host->dma_ops->exit)
host->dma_ops->exit(host);

- if (!IS_ERR(host->pdata->rstc))
- reset_control_assert(host->pdata->rstc);
+ reset_control_assert(host->pdata->rstc);

clk_disable_unprepare(host->ciu_clk);
clk_disable_unprepare(host->biu_clk);
--
2.11.0


2017-03-16 14:46:59

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On 15 March 2017 at 12:31, Philipp Zabel <[email protected]> wrote:
> As of commit bb475230b8e5 ("reset: make optional functions really
> optional"), the reset framework API calls use NULL pointers to describe
> optional, non-present reset controls.
>
> This allows to return errors from devm_reset_control_get_optional and to
> call reset_control_(de)assert unconditionally.
>
> Signed-off-by: Philipp Zabel <[email protected]>

Thanks, applied for next!

Kind regards
Uffe

> ---
> drivers/mmc/host/dw_mmc.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a9ac0b4573131..3d62b0a1f81cb 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>
> /* find reset controller when exist */
> pdata->rstc = devm_reset_control_get_optional(dev, "reset");
> - if (IS_ERR(pdata->rstc)) {
> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> - return ERR_PTR(-EPROBE_DEFER);
> - }
> + if (IS_ERR(pdata->rstc))
> + return ERR_CAST(pdata->rstc);
>
> /* find out number of slots supported */
> of_property_read_u32(np, "num-slots", &pdata->num_slots);
> @@ -3100,7 +3098,7 @@ int dw_mci_probe(struct dw_mci *host)
> }
> }
>
> - if (!IS_ERR(host->pdata->rstc)) {
> + if (host->pdata->rstc) {
> reset_control_assert(host->pdata->rstc);
> usleep_range(10, 50);
> reset_control_deassert(host->pdata->rstc);
> @@ -3257,8 +3255,7 @@ int dw_mci_probe(struct dw_mci *host)
> if (host->use_dma && host->dma_ops->exit)
> host->dma_ops->exit(host);
>
> - if (!IS_ERR(host->pdata->rstc))
> - reset_control_assert(host->pdata->rstc);
> + reset_control_assert(host->pdata->rstc);
>
> err_clk_ciu:
> clk_disable_unprepare(host->ciu_clk);
> @@ -3290,8 +3287,7 @@ void dw_mci_remove(struct dw_mci *host)
> if (host->use_dma && host->dma_ops->exit)
> host->dma_ops->exit(host);
>
> - if (!IS_ERR(host->pdata->rstc))
> - reset_control_assert(host->pdata->rstc);
> + reset_control_assert(host->pdata->rstc);
>
> clk_disable_unprepare(host->ciu_clk);
> clk_disable_unprepare(host->biu_clk);
> --
> 2.11.0
>

2017-03-20 09:24:19

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

Hi Philipp,

Todays next branch does not work with exynos5433-tm2 board.
I guess this patch causes regression. On MMC without reset controller I
get errors:
[ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available
[ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22
[ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available
[ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22

Commenting out reset controller get and error checks 'fixes' the issue.

On 15.03.2017 12:31, Philipp Zabel wrote:
> As of commit bb475230b8e5 ("reset: make optional functions really
> optional"), the reset framework API calls use NULL pointers to describe
> optional, non-present reset controls.
>
> This allows to return errors from devm_reset_control_get_optional and to
> call reset_control_(de)assert unconditionally.
>
> Signed-off-by: Philipp Zabel <[email protected]>
> ---
> drivers/mmc/host/dw_mmc.c | 14 +++++---------
> 1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index a9ac0b4573131..3d62b0a1f81cb 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>
> /* find reset controller when exist */
> pdata->rstc = devm_reset_control_get_optional(dev, "reset");
> - if (IS_ERR(pdata->rstc)) {
> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> - return ERR_PTR(-EPROBE_DEFER);
> - }
> + if (IS_ERR(pdata->rstc))
> + return ERR_CAST(pdata->rstc);


With three lines above commented out it works.

Regards
Andrzej



2017-03-20 09:54:08

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On Mon, 2017-03-20 at 10:22 +0100, Andrzej Hajda wrote:
> Hi Philipp,
>
> Todays next branch does not work with exynos5433-tm2 board.
> I guess this patch causes regression. On MMC without reset controller I
> get errors:
> [ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available
> [ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22
> [ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available
> [ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22
>
> Commenting out reset controller get and error checks 'fixes' the issue.
>
> On 15.03.2017 12:31, Philipp Zabel wrote:
> > As of commit bb475230b8e5 ("reset: make optional functions really
> > optional"), the reset framework API calls use NULL pointers to describe
> > optional, non-present reset controls.
> >
> > This allows to return errors from devm_reset_control_get_optional and to
> > call reset_control_(de)assert unconditionally.
> >
> > Signed-off-by: Philipp Zabel <[email protected]>
> > ---
> > drivers/mmc/host/dw_mmc.c | 14 +++++---------
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> > index a9ac0b4573131..3d62b0a1f81cb 100644
> > --- a/drivers/mmc/host/dw_mmc.c
> > +++ b/drivers/mmc/host/dw_mmc.c
> > @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> >
> > /* find reset controller when exist */
> > pdata->rstc = devm_reset_control_get_optional(dev, "reset");
> > - if (IS_ERR(pdata->rstc)) {
> > - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> > - return ERR_PTR(-EPROBE_DEFER);
> > - }
> > + if (IS_ERR(pdata->rstc))
> > + return ERR_CAST(pdata->rstc);
>
>
> With three lines above commented out it works.

So devm_reset_control_get_optional returns -EINVAL. Why?

The mshc@15560000 node is compatible to "samsung,exynos7-dw-mshc-smu",
so that's dw_mmc-exynos.c calling dw_mci_pltfm_register, which then
calls dw_mci_probe, passing the original platform device as
host->dev = &pdev->dev, and I expect __of_reset_control_get being called
with a valid DT node (dev->of_node).
Since id is set to "reset", of_property_match_string(node,
"reset-names", id) should then be called and return -EINVAL because the
"reset-names" property does not exist. Then __of_reset_control_get
should return NULL because optional == true.
Some of this obviously doesn't happen, where am I wrong?

regards
Philipp

2017-03-20 10:06:46

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

Hi Philipp,

On 20.03.2017 10:53, Philipp Zabel wrote:
> On Mon, 2017-03-20 at 10:22 +0100, Andrzej Hajda wrote:
>> Hi Philipp,
>>
>> Todays next branch does not work with exynos5433-tm2 board.
>> I guess this patch causes regression. On MMC without reset controller I
>> get errors:
>> [ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available
>> [ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22
>> [ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available
>> [ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22
>>
>> Commenting out reset controller get and error checks 'fixes' the issue.
>>
>> On 15.03.2017 12:31, Philipp Zabel wrote:
>>> As of commit bb475230b8e5 ("reset: make optional functions really
>>> optional"), the reset framework API calls use NULL pointers to describe
>>> optional, non-present reset controls.
>>>
>>> This allows to return errors from devm_reset_control_get_optional and to
>>> call reset_control_(de)assert unconditionally.
>>>
>>> Signed-off-by: Philipp Zabel <[email protected]>
>>> ---
>>> drivers/mmc/host/dw_mmc.c | 14 +++++---------
>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index a9ac0b4573131..3d62b0a1f81cb 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>>
>>> /* find reset controller when exist */
>>> pdata->rstc = devm_reset_control_get_optional(dev, "reset");
>>> - if (IS_ERR(pdata->rstc)) {
>>> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>> - return ERR_PTR(-EPROBE_DEFER);
>>> - }
>>> + if (IS_ERR(pdata->rstc))
>>> + return ERR_CAST(pdata->rstc);
>>
>> With three lines above commented out it works.
> So devm_reset_control_get_optional returns -EINVAL. Why?
>
> The mshc@15560000 node is compatible to "samsung,exynos7-dw-mshc-smu",
> so that's dw_mmc-exynos.c calling dw_mci_pltfm_register, which then
> calls dw_mci_probe, passing the original platform device as
> host->dev = &pdev->dev, and I expect __of_reset_control_get being called
> with a valid DT node (dev->of_node).
> Since id is set to "reset", of_property_match_string(node,
> "reset-names", id) should then be called and return -EINVAL because the
> "reset-names" property does not exist. Then __of_reset_control_get
> should return NULL because optional == true.
> Some of this obviously doesn't happen, where am I wrong?


When RESET_CONTROLLER is not enabled dummy stubs return -ENOSUPP error [1].

[1]: http://lxr.free-electrons.com/source/include/linux/reset.h#L77

Regards
Andrzej




>
> regards
> Philipp
>
>
>
>

2017-03-20 10:29:03

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

Hi Andrzej,

On Mon, 2017-03-20 at 11:03 +0100, Andrzej Hajda wrote:
> Hi Philipp,
>
> On 20.03.2017 10:53, Philipp Zabel wrote:
> > On Mon, 2017-03-20 at 10:22 +0100, Andrzej Hajda wrote:
> >> Hi Philipp,
> >>
> >> Todays next branch does not work with exynos5433-tm2 board.
> >> I guess this patch causes regression. On MMC without reset controller I
> >> get errors:
> >> [ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available
> >> [ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22

I was thrown off by this. Should maybe dw_mci_probe return the error
value reported by dw_mci_parse_dt instead of always returning -EINVAL?

> >> [ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available
> >> [ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22
> >>
> >> Commenting out reset controller get and error checks 'fixes' the issue.
> >>
> >> On 15.03.2017 12:31, Philipp Zabel wrote:
> >>> As of commit bb475230b8e5 ("reset: make optional functions really
> >>> optional"), the reset framework API calls use NULL pointers to describe
> >>> optional, non-present reset controls.
> >>>
> >>> This allows to return errors from devm_reset_control_get_optional and to
> >>> call reset_control_(de)assert unconditionally.
> >>>
> >>> Signed-off-by: Philipp Zabel <[email protected]>
> >>> ---
> >>> drivers/mmc/host/dw_mmc.c | 14 +++++---------
> >>> 1 file changed, 5 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> >>> index a9ac0b4573131..3d62b0a1f81cb 100644
> >>> --- a/drivers/mmc/host/dw_mmc.c
> >>> +++ b/drivers/mmc/host/dw_mmc.c
> >>> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
> >>>
> >>> /* find reset controller when exist */
> >>> pdata->rstc = devm_reset_control_get_optional(dev, "reset");
> >>> - if (IS_ERR(pdata->rstc)) {
> >>> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
> >>> - return ERR_PTR(-EPROBE_DEFER);
> >>> - }
> >>> + if (IS_ERR(pdata->rstc))
> >>> + return ERR_CAST(pdata->rstc);
> >>
> >> With three lines above commented out it works.
> > So devm_reset_control_get_optional returns -EINVAL. Why?
> >
> > The mshc@15560000 node is compatible to "samsung,exynos7-dw-mshc-smu",
> > so that's dw_mmc-exynos.c calling dw_mci_pltfm_register, which then
> > calls dw_mci_probe, passing the original platform device as
> > host->dev = &pdev->dev, and I expect __of_reset_control_get being called
> > with a valid DT node (dev->of_node).
> > Since id is set to "reset", of_property_match_string(node,
> > "reset-names", id) should then be called and return -EINVAL because the
> > "reset-names" property does not exist. Then __of_reset_control_get
> > should return NULL because optional == true.
> > Some of this obviously doesn't happen, where am I wrong?
>
>
> When RESET_CONTROLLER is not enabled dummy stubs return -ENOSUPP error [1].
>
> [1]: http://lxr.free-electrons.com/source/include/linux/reset.h#L77

Thanks, I suppose that issue should be fixed by:

----------8<----------
diff --git a/include/linux/reset.h b/include/linux/reset.h
index 86b4ed75359e8..c905ff1c21ec6 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
const char *id, int index, bool shared,
bool optional)
{
- return ERR_PTR(-ENOTSUPP);
+ return optional ? NULL : ERR_PTR(-ENOTSUPP);
}

static inline struct reset_control *__devm_reset_control_get(
struct device *dev, const char *id,
int index, bool shared, bool optional)
{
- return ERR_PTR(-ENOTSUPP);
+ return optional ? NULL : ERR_PTR(-ENOTSUPP);
}

#endif /* CONFIG_RESET_CONTROLLER */
---------->8----------

regards
Philipp

2017-03-20 10:50:37

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On 20.03.2017 11:27, Philipp Zabel wrote:
> Hi Andrzej,
>
> On Mon, 2017-03-20 at 11:03 +0100, Andrzej Hajda wrote:
>> Hi Philipp,
>>
>> On 20.03.2017 10:53, Philipp Zabel wrote:
>>> On Mon, 2017-03-20 at 10:22 +0100, Andrzej Hajda wrote:
>>>> Hi Philipp,
>>>>
>>>> Todays next branch does not work with exynos5433-tm2 board.
>>>> I guess this patch causes regression. On MMC without reset controller I
>>>> get errors:
>>>> [ 4.938222] dwmmc_exynos 15540000.mshc: platform data not available
>>>> [ 4.943268] dwmmc_exynos: probe of 15540000.mshc failed with error -22
> I was thrown off by this. Should maybe dw_mci_probe return the error
> value reported by dw_mci_parse_dt instead of always returning -EINVAL?
>
>>>> [ 4.950184] dwmmc_exynos 15560000.mshc: platform data not available
>>>> [ 4.955962] dwmmc_exynos: probe of 15560000.mshc failed with error -22
>>>>
>>>> Commenting out reset controller get and error checks 'fixes' the issue.
>>>>
>>>> On 15.03.2017 12:31, Philipp Zabel wrote:
>>>>> As of commit bb475230b8e5 ("reset: make optional functions really
>>>>> optional"), the reset framework API calls use NULL pointers to describe
>>>>> optional, non-present reset controls.
>>>>>
>>>>> This allows to return errors from devm_reset_control_get_optional and to
>>>>> call reset_control_(de)assert unconditionally.
>>>>>
>>>>> Signed-off-by: Philipp Zabel <[email protected]>
>>>>> ---
>>>>> drivers/mmc/host/dw_mmc.c | 14 +++++---------
>>>>> 1 file changed, 5 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>>>> index a9ac0b4573131..3d62b0a1f81cb 100644
>>>>> --- a/drivers/mmc/host/dw_mmc.c
>>>>> +++ b/drivers/mmc/host/dw_mmc.c
>>>>> @@ -2968,10 +2968,8 @@ static struct dw_mci_board *dw_mci_parse_dt(struct dw_mci *host)
>>>>>
>>>>> /* find reset controller when exist */
>>>>> pdata->rstc = devm_reset_control_get_optional(dev, "reset");
>>>>> - if (IS_ERR(pdata->rstc)) {
>>>>> - if (PTR_ERR(pdata->rstc) == -EPROBE_DEFER)
>>>>> - return ERR_PTR(-EPROBE_DEFER);
>>>>> - }
>>>>> + if (IS_ERR(pdata->rstc))
>>>>> + return ERR_CAST(pdata->rstc);
>>>> With three lines above commented out it works.
>>> So devm_reset_control_get_optional returns -EINVAL. Why?
>>>
>>> The mshc@15560000 node is compatible to "samsung,exynos7-dw-mshc-smu",
>>> so that's dw_mmc-exynos.c calling dw_mci_pltfm_register, which then
>>> calls dw_mci_probe, passing the original platform device as
>>> host->dev = &pdev->dev, and I expect __of_reset_control_get being called
>>> with a valid DT node (dev->of_node).
>>> Since id is set to "reset", of_property_match_string(node,
>>> "reset-names", id) should then be called and return -EINVAL because the
>>> "reset-names" property does not exist. Then __of_reset_control_get
>>> should return NULL because optional == true.
>>> Some of this obviously doesn't happen, where am I wrong?
>>
>> When RESET_CONTROLLER is not enabled dummy stubs return -ENOSUPP error [1].
>>
>> [1]: http://lxr.free-electrons.com/source/include/linux/reset.h#L77
> Thanks, I suppose that issue should be fixed by:
>
> ----------8<----------
> diff --git a/include/linux/reset.h b/include/linux/reset.h
> index 86b4ed75359e8..c905ff1c21ec6 100644
> --- a/include/linux/reset.h
> +++ b/include/linux/reset.h
> @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
> const char *id, int index, bool shared,
> bool optional)
> {
> - return ERR_PTR(-ENOTSUPP);
> + return optional ? NULL : ERR_PTR(-ENOTSUPP);
> }
>
> static inline struct reset_control *__devm_reset_control_get(
> struct device *dev, const char *id,
> int index, bool shared, bool optional)
> {
> - return ERR_PTR(-ENOTSUPP);
> + return optional ? NULL : ERR_PTR(-ENOTSUPP);
> }
>
> #endif /* CONFIG_RESET_CONTROLLER */
> ---------->8----------

In dw_mmc.c file there are also unconditional calls to
reset_control_assert, with disabled RESET_CONTROLLER it will cause
unexpected WARNs.
Anyway if you change reset API as above I think you should remove all
warns from reset stubs, because NULL reset is valid, but these warns are
there for reason - contradiction.

Regards
Andrzej

>
> regards
> Philipp
>
>
>
>

2017-03-20 11:02:40

by Philipp Zabel

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On Mon, 2017-03-20 at 11:49 +0100, Andrzej Hajda wrote:
> On 20.03.2017 11:27, Philipp Zabel wrote:
[...]
> > diff --git a/include/linux/reset.h b/include/linux/reset.h
> > index 86b4ed75359e8..c905ff1c21ec6 100644
> > --- a/include/linux/reset.h
> > +++ b/include/linux/reset.h
> > @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
> > const char *id, int index, bool shared,
> > bool optional)
> > {
> > - return ERR_PTR(-ENOTSUPP);
> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
> > }
> >
> > static inline struct reset_control *__devm_reset_control_get(
> > struct device *dev, const char *id,
> > int index, bool shared, bool optional)
> > {
> > - return ERR_PTR(-ENOTSUPP);
> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
> > }
> >
> > #endif /* CONFIG_RESET_CONTROLLER */
> > ---------->8----------
>
> In dw_mmc.c file there are also unconditional calls to
> reset_control_assert, with disabled RESET_CONTROLLER it will cause
> unexpected WARNs.
> Anyway if you change reset API as above I think you should remove all
> warns from reset stubs, because NULL reset is valid, but these warns are
> there for reason - contradiction.

You are right, I have to let go of those, too.

regards
Philipp

2017-03-20 18:09:34

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On 20 March 2017 at 12:00, Philipp Zabel <[email protected]> wrote:
> On Mon, 2017-03-20 at 11:49 +0100, Andrzej Hajda wrote:
>> On 20.03.2017 11:27, Philipp Zabel wrote:
> [...]
>> > diff --git a/include/linux/reset.h b/include/linux/reset.h
>> > index 86b4ed75359e8..c905ff1c21ec6 100644
>> > --- a/include/linux/reset.h
>> > +++ b/include/linux/reset.h
>> > @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
>> > const char *id, int index, bool shared,
>> > bool optional)
>> > {
>> > - return ERR_PTR(-ENOTSUPP);
>> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>> > }
>> >
>> > static inline struct reset_control *__devm_reset_control_get(
>> > struct device *dev, const char *id,
>> > int index, bool shared, bool optional)
>> > {
>> > - return ERR_PTR(-ENOTSUPP);
>> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>> > }
>> >
>> > #endif /* CONFIG_RESET_CONTROLLER */
>> > ---------->8----------
>>
>> In dw_mmc.c file there are also unconditional calls to
>> reset_control_assert, with disabled RESET_CONTROLLER it will cause
>> unexpected WARNs.
>> Anyway if you change reset API as above I think you should remove all
>> warns from reset stubs, because NULL reset is valid, but these warns are
>> there for reason - contradiction.
>
> You are right, I have to let go of those, too.
>
> regards
> Philipp
>

2017-03-20 18:11:03

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

On 20 March 2017 at 12:00, Philipp Zabel <[email protected]> wrote:
> On Mon, 2017-03-20 at 11:49 +0100, Andrzej Hajda wrote:
>> On 20.03.2017 11:27, Philipp Zabel wrote:
> [...]
>> > diff --git a/include/linux/reset.h b/include/linux/reset.h
>> > index 86b4ed75359e8..c905ff1c21ec6 100644
>> > --- a/include/linux/reset.h
>> > +++ b/include/linux/reset.h
>> > @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
>> > const char *id, int index, bool shared,
>> > bool optional)
>> > {
>> > - return ERR_PTR(-ENOTSUPP);
>> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>> > }
>> >
>> > static inline struct reset_control *__devm_reset_control_get(
>> > struct device *dev, const char *id,
>> > int index, bool shared, bool optional)
>> > {
>> > - return ERR_PTR(-ENOTSUPP);
>> > + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>> > }
>> >
>> > #endif /* CONFIG_RESET_CONTROLLER */
>> > ---------->8----------
>>
>> In dw_mmc.c file there are also unconditional calls to
>> reset_control_assert, with disabled RESET_CONTROLLER it will cause
>> unexpected WARNs.
>> Anyway if you change reset API as above I think you should remove all
>> warns from reset stubs, because NULL reset is valid, but these warns are
>> there for reason - contradiction.
>
> You are right, I have to let go of those, too.


Until fixed, I have dropped the three changes from my next branch
related to this. Please re-post when fixed.

Kind regards
Uffe

>
> regards
> Philipp
>

2017-03-21 12:12:36

by Jaehoon Chung

[permalink] [raw]
Subject: Re: [PATCH v2 06/14] mmc: dw_mmc: simplify optional reset handling

Hi All,

On 03/21/2017 03:10 AM, Ulf Hansson wrote:
> On 20 March 2017 at 12:00, Philipp Zabel <[email protected]> wrote:
>> On Mon, 2017-03-20 at 11:49 +0100, Andrzej Hajda wrote:
>>> On 20.03.2017 11:27, Philipp Zabel wrote:
>> [...]
>>>> diff --git a/include/linux/reset.h b/include/linux/reset.h
>>>> index 86b4ed75359e8..c905ff1c21ec6 100644
>>>> --- a/include/linux/reset.h
>>>> +++ b/include/linux/reset.h
>>>> @@ -74,14 +74,14 @@ static inline struct reset_control *__of_reset_control_get(
>>>> const char *id, int index, bool shared,
>>>> bool optional)
>>>> {
>>>> - return ERR_PTR(-ENOTSUPP);
>>>> + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>>>> }
>>>>
>>>> static inline struct reset_control *__devm_reset_control_get(
>>>> struct device *dev, const char *id,
>>>> int index, bool shared, bool optional)
>>>> {
>>>> - return ERR_PTR(-ENOTSUPP);
>>>> + return optional ? NULL : ERR_PTR(-ENOTSUPP);
>>>> }
>>>>
>>>> #endif /* CONFIG_RESET_CONTROLLER */
>>>> ---------->8----------
>>>
>>> In dw_mmc.c file there are also unconditional calls to
>>> reset_control_assert, with disabled RESET_CONTROLLER it will cause
>>> unexpected WARNs.
>>> Anyway if you change reset API as above I think you should remove all
>>> warns from reset stubs, because NULL reset is valid, but these warns are
>>> there for reason - contradiction.
>>
>> You are right, I have to let go of those, too.
>
>
> Until fixed, I have dropped the three changes from my next branch
> related to this. Please re-post when fixed.

I missed this patch. If resend the patch, i will check.

Best Regards,
Jaehoon Chung

>
> Kind regards
> Uffe
>
>>
>> regards
>> Philipp
>>
>
>
>