2018-04-03 17:00 GMT+09:00 Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>:
> On Thu, 2018-03-29 at 15:07 +0900, Masahiro Yamada wrote:
>> This driver handles the reset control in a common manner; deassert
>> resets before use, assert them after use. There is no good reason
>> why it should be exclusive.
>
> Is this preemptive cleanup, or do you have hardware on the horizon that
> shares these reset lines with other peripherals?
This patch is necessary for Socionext SoCs.
The same reset lines are shared between
this dwc3-of_simple and other glue circuits.
>> Also, use devm_ for clean-up.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> ---
>>
>> CCing Philipp Zabel.
>> I see his sob in commit 06c47e6286d5.
>
> At the time I was concerned with the reset_control_array addition and
> didn't look closely at the exclusive vs shared issue.
>> drivers/usb/dwc3/dwc3-of-simple.c | 7 ++-----
>> 1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index e54c362..bd6ab65 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -91,7 +91,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>> platform_set_drvdata(pdev, simple);
>> simple->dev = dev;
>>
>> - simple->resets = of_reset_control_array_get_optional_exclusive(np);
>> + simple->resets = devm_reset_control_array_get_optional_shared(dev);
>
> From the usage in the driver, it does indeed look like _shared reset
> usage is appropriate. I assume that the hardware has no need for the
> reset to be asserted right before probe or after remove, it just
> requires that the reset line is kept deasserted while the driver is
> probed.
>
>> if (IS_ERR(simple->resets)) {
>> ret = PTR_ERR(simple->resets);
>> dev_err(dev, "failed to get device resets, err=%d\n", ret);
>> @@ -100,7 +100,7 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>
>> ret = reset_control_deassert(simple->resets);
>> if (ret)
>> - goto err_resetc_put;
>> + return ret;
>>
>> ret = dwc3_of_simple_clk_init(simple, of_count_phandle_with_args(np,
>> "clocks", "#clock-cells"));
>> @@ -126,8 +126,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>> err_resetc_assert:
>> reset_control_assert(simple->resets);
>>
>> -err_resetc_put:
>> - reset_control_put(simple->resets);
>> return ret;
>> }
>>
>> @@ -146,7 +144,6 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>> simple->num_clocks = 0;
>>
>> reset_control_assert(simple->resets);
>> - reset_control_put(simple->resets);
>>
>> pm_runtime_put_sync(dev);
>> pm_runtime_disable(dev);
>
> Changing to devm_ changes the order here. Whether or not it could be a
> problem to assert the reset only after pm_runtime_put (or potentially
> never), I can't say. I assume this is a non-issue, but somebody who
> knows the hardware better would have to decide.
I do not understand what you mean.
Can you describe your concern in more details?
I am not touching reset_control_assert() here.
I am delaying the call for reset_control_put().
If I understand reset_control_put() correctly,
the effects of this change are:
- The ref_count and module ownership for the reset controller
driver will be held a little longer
- The call for kfree() will be a little bit delayed.
Why do you need knowledge about this hardware?
--
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
