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

[Simon Glass]

Hi,

On 11 April 2016 at 10:10, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 04/11/2016 09:12 AM, Simon Glass wrote:
>>
>> Hi Eric,
>>
>> On 11 April 2016 at 09:10, Eric Nelson <eric at 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 at nelint.com> wrote:
>>>>>
>>>>> On 04/11/2016 07:47 AM, Simon Glass wrote:
>>>>>>
>>>>>> On 10 April 2016 at 08:48, Eric Nelson <eric at nelint.com> wrote:
>>>>>>>
>>>>>>> On 04/09/2016 11:33 AM, Simon Glass wrote:
>>>>>>>>
>>>>>>>> On 4 April 2016 at 11:50, Stephen Warren <swarren at 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.
>
>
> You can avoid that by requiring that ops->xlate always be non-NULL, and
> simply referencing the default function from drivers that want to use it.
> Not a big deal either way though.

I'd prefer not to do that. It just adds cruft, the removal of which is
the purpose of Eric's series.

Regards,
Simon