[U-Boot] [PATCH] dm: gpio: handle GPIO_ACTIVE_LOW flag in DT

[Simon Glass <sjg@chromium.org>]

Hi Eric,

On 11 April 2016 at 09:10, Eric Nelson <eric@nelint.com> wrote:
> Hi Simon,
>
> On 04/11/2016 07:59 AM, Simon Glass wrote:
>> On 11 April 2016 at 08:55, Eric Nelson <eric@nelint.com> wrote:
>>> On 04/11/2016 07:47 AM, Simon Glass wrote:
>>>> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote:
>>>>> On 04/09/2016 11:33 AM, Simon Glass wrote:
>>>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>>> On 04/03/2016 08:07 AM, Eric Nelson wrote:
>>>>>>>> On 04/02/2016 08:37 PM, Stephen Warren wrote:
>>>>>>>>> On 04/02/2016 09:13 AM, Eric Nelson wrote:
>>>>>>>>>> On 04/01/2016 10:46 PM, Peng Fan wrote:
>>>>>>>>>>> On Thu, Mar 31, 2016 at 01:41:04PM -0700, Eric Nelson wrote:
>>>>>>>>>>>> On 03/28/2016 09:57 PM, Peng Fan wrote:
>>>>>>>>>>>>> On Fri, Mar 25, 2016 at 01:12:11PM -0700, Eric Nelson wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Device tree parsing of GPIO nodes is currently ignoring flags.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Add support for GPIO_ACTIVE_LOW by checking for the presence
>>>>>>>>>>>>>> of the flag and setting the desc->flags field to the driver
>>>>>>>>>>>>>> model constant GPIOD_ACTIVE_LOW.
>>>>>>>>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>>>
>>>>>>> The intent of the change is good.
>>>>>>>
>>>>>>> I'm not sure why we need to remove gpio_find_and_xlate(); it provides an API
>>>>>>> for clients so they don't need to know how to access driver functionality
>>>>>>> through the ops pointer, which I think is an internal/private implementation
>>>>>>> detail. Is that detail exposed to clients in other places? If so, removing
>>>>>>> the wrapper seems fine. If not, I suspect it's a deliberate abstraction.
>>>>>>
>>>>>> This seems a bit pedantic, but since Linux does it this way I think we
>>>>>> should follow along.
>>>>>>
>>>>>> Eric you still get to remove the code from all the GPIO drivers - the
>>>>>> difference is just creating a common function to call when no xlate()
>>>>>> method is available.
>>>>>>
>>>>>> Can you please take a look at what Stephen suggests?
>>>>>>
>>>>>
>>>>> Got it. I'm just not sure about where to start (before or after
>>>>> the patch set you sent) and whether to also remove offset parsing
>>>>> from gpio_find_and_xlate().
>>>>>
>>>>
>>>> Which patch did I send? My understanding is:
>>>>
>>>
>>> At the time I sent this, you had just submitted the patch set adding
>>> more driver-model support for block devices.
>>>
>>>         http://lists.denx.de/pipermail/u-boot/2016-April/251095.html
>>>
>>>> - Add my review/ack tag to the patches as necessary
>>>> - Drop the tegra patch
>>>> - Update gpio_find_and_xlate() to call a default function if there is
>>>> no xlate() method
>>>> - Resend the series
>>>>
>>>> I'm not sure about removing the existing functionality from
>>>> gpio_find_and_xlate(), but my guess is that it is best to move it to
>>>> your default function, so that gpio_find_and_xlate() doesn't include
>>>> any default behaviour in the case where there is a xlate() method.
>>>>
>>>
>>> Reviewing the use of the offset field did yield some information about
>>> the broken sunxi support and also that Vybrid was also missing
>>> the xlate routine.
>>>
>>> Since reviewing your patch sets (driver model updates for blk and also
>>> driver model updates for mmc) will take some time, so I'll base an
>>> updated patch set on master. My guess is that any merge issues will
>>> be trivial.
>>
>> Yes, that's right.
>>>
>>> I'll remove your acks in the updated patch set, since the updates
>>> to the drivers won't drop the xlate field, but will connect them
>>> to the common (__maybe_unused) routine. This will prevent the code
>>> from leaking into machines like Tegra that don't need the common code.
>>
>> I'm pretty sure you can drop the xlate() implementations from the
>> functions, though, and those at the patches I acked.
>>
>> I don't think you need __maybe_unused
>>
>> static int gpio_find_and_xlate(...)
>> {
>>    get ops...
>>
>>    if (ops->xlate)
>>       return ops->xlate(....)
>>    else
>>       return gpio_default_xlate()...
>> }
>>
>> gpio_default_xlate() (or whatever name you use) should be exported so
>> drivers can use it.
>>
>
> This will leak gpio_default_xlate (locally named gpio_xlate_offs_flags)
> into machines that don't need it.
>
> I can go the route you suggest above, but it will cost the tegra
> and sandbox builds ~64 bytes ;)
>

Sure, but we can live with that.

Regards,
Simon