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

Simon Glass sjg at chromium.org
Mon Apr 11 18:53:20 CEST 2016


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


More information about the U-Boot mailing list