From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eric Nelson <eric@nelint.com>
Date: Mon, 11 Apr 2016 07:55:39 -0700
Subject: [U-Boot] [PATCH] dm: gpio: handle GPIO_ACTIVE_LOW flag in DT
In-Reply-To: <CAPnjgZ3QM-MVkkfy_nFLAJ87jGttXcRn+nw2pZVOtwCbpA2BZg@mail.gmail.com>
References: <1458936731-13223-1-git-send-email-eric@nelint.com>
 <20160329045729.GA32493@linux-7smt.suse> <56FD8B60.8060103@nelint.com>
 <20160402054612.GA27255@linux-7smt.suse> <56FFE1B3.6070608@nelint.com>
 <57008FE7.7000904@wwwdotorg.org> <57012392.9070505@nelint.com>
 <5702A980.3040400@wwwdotorg.org>
 <CAPnjgZ3PS55_Nf1n4YzX8YUfjM-VCxS1otPCV6hdD3DHVdRjNw@mail.gmail.com>
 <570A67B4.9000009@nelint.com>
 <CAPnjgZ3QM-MVkkfy_nFLAJ87jGttXcRn+nw2pZVOtwCbpA2BZg@mail.gmail.com>
Message-ID: <570BBAEB.90904@nelint.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On 04/11/2016 07:47 AM, Simon Glass wrote:
> Hi Eric,
> 
> On 10 April 2016 at 08:48, Eric Nelson <eric@nelint.com> wrote:
>> Hi Simon,
>>
>> 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.

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.

Regards,


Eric