Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[Bartosz Golaszewski]

2017-02-21 6:03 GMT+01:00 Sekhar Nori <nsekhar@xxxxxx>:
> On Monday 20 February 2017 09:08 PM, Bartosz Golaszewski wrote:
>> 2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar@xxxxxx>:
>>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>>> If we're using the UI board and want vpif capture, we need to select
>>>> the video capture functionality by driving the sel_c pin low on the
>>>> tca6416 expander and sel_a & sel_b pins high. Do it statically by
>>>> hogging relevant GPIOs in the device tree.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
>>>> ---
>>
>> [snip]
>>
>>>>
>>>> +                             sel_a {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <7 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-high;
>>>> +                                     line-name = "sel_a";
>>>> +                             };
>>>> +
>>>> +                             sel_b {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <6 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-high;
>>>> +                                     line-name = "sel_b";
>>>> +                             };
>>>> +
>>>> +                             sel_c {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <5 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-low;
>>>> +                                     line-name = "sel_c";
>>>
>>> I think this is better handled by using an enable-gpios property in vpif
>>> capture device-tree node. So in the vpif capture node you would have:
>>>
>>>         enable-gpios =  <&tca6416 7 GPIO_ACTIVE_HIGH
>>>                          &tca6416 6 GPIO_ACTIVE_HIGH
>>>                          &tca6416 5 GPIO_ACTIVE_LOW>;
>>>
>>> and in the vpif capture driver, you would request each of these gpios
>>> using: devm_gpiod_get_array_optional(.., .., GPIOD_OUT_HIGH);
>>>
>>
>> I'm not sure about this one - the result is the same (function still
>> defined statically in the DT) while now it requires changes to the
>> vpif driver too.
>>
>> Is there any other reason we'd prefer this approach?
>
> The GPIO hog functionality can race with driver probe. Based on probe
> order, you may have a situation where VPIF probes before tca6416 so we
> have an erroneous situation where probe is successful, but hardware is
> not really available.
>
> Using enable-gpios lets you handle probe deferral so VPIF capture probe
> completes only when hardware is ready. So if for some reason tca6416
> driver or hardware is misbehaving, VPIF will know about it through some
> error value rather than just assuming that everything went well.
>
> So, yes, in the "all goes well" scenario, there is not much difference
> in the two approaches. But the difference will be apparent when
> something goes wrong.
>
> Probe order will also influence the shutdown and suspend order. So
> kernel will automatically make sure that shutdown happens in reverse
> probe order. This may or may not matter in this case. But in  general,
> it will be nice to make sure VPIF shuts down before tca6416 does so that
> hardware is available for VPIF to cleanly shutdown (and not disconnected
> behind its back because tca6416 decided to put all its lines to off as
> part of its shutdown).
>
> I think GPIO hog should only be used for pins which are really "system
> level". IOW, not related to any driver functionality.
>
> Thanks,
> Sekhar

Ok, I'll extend the driver then.

Thanks,
Bartosz