Hi, On 11 April 2016 at 10:10, Stephen Warren <swar...@wwwdotorg.org> wrote: > On 04/11/2016 09:12 AM, Simon Glass wrote: >> >> Hi Eric, >> >> On 11 April 2016 at 09:10, Eric Nelson <e...@nelint.com> wrote: >>> >>> Hi Simon, >>> >>> On 04/11/2016 07:59 AM, Simon Glass wrote: >>>> >>>> On 11 April 2016 at 08:55, Eric Nelson <e...@nelint.com> wrote: >>>>> >>>>> On 04/11/2016 07:47 AM, Simon Glass wrote: >>>>>> >>>>>> On 10 April 2016 at 08:48, Eric Nelson <e...@nelint.com> wrote: >>>>>>> >>>>>>> On 04/09/2016 11:33 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> On 4 April 2016 at 11:50, Stephen Warren <swar...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
