Re: [PATCH] gpio: gpio-dwapb: add optional reset

[Alan Tull]

On Wed, Oct 11, 2017 at 5:56 AM, Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote:

Hi Linus, Phillipp,

Thanks for the review.

> Hi Alan, Linus,
>
> On Wed, 2017-10-11 at 10:31 +0200, Linus Walleij wrote:
>> On Tue, Oct 10, 2017 at 11:26 PM, Alan Tull <atull@xxxxxxxxxx> wrote:
>>
>> > Some platforms require reset to be released to allow register
>> > access.
>> >
>> > Signed-off-by: Alan Tull <atull@xxxxxxxxxx>
>>
>> Fair enough.
>>
>> (...)
>> > +       rst = devm_reset_control_get_optional_exclusive(dev, NULL);
>
> The way this reset control is used, it looks like you could use _shared
> instead of _exclusive here. This relaxes the guarantees made by the API
> a bit and may allow this driver to work with more reset controllers.

OK, will use devm_reset_control_get_optional_shared().

>
>> > +       if (IS_ERR(rst)) {
>> > +               if (PTR_ERR(rst) == -EPROBE_DEFER)
>> > +                       return PTR_ERR(rst);
>
> The _optional variant of reset_control_get returns NULL if no reset is
> specified in the device tree. If an error value is returned, it is
> always an actual error (invalid device tree contents, reset is specified
> in the device tree but the driver returns an error, etc.).
> This should just be:
>
>         if (IS_ERR(rst))
>                 return PTR_ERR(rst);

That's great!

>
>> > +       } else {
>> > +               reset_control_deassert(rst);
>> > +               gpio->rst = rst;
>
> And this should be made unconditional. reset_control_deassert just
> ignores rst == NULL.

Nice.

>
>> > +       }
>>
>> I do not see why any error other than -EPROBE_DEFER
>> should be ignored?
>>
>> I guess the _optional API returns NULL if there is no
>> reset line so it should be fine to just return the error on
>> any error.
>
> Correct. The _optional API together with NULL reset control handles
> allows to simplify handling of optional resets in the consumer drivers.
>
>> > +       if (gpio->rst)
>> > +               reset_control_assert(gpio->rst);
>>
>> Is this the right way to handle an optional reset line?
>
> Just as the deassert above, this should be made unconditional.

I've made the requested changes which shrinks the patch to be even smaller :)

Will send up v2 soon.

Thanks,
Alan

>
> regards
> Philipp