lkml.org 
[lkml]   [2019]   [Sep]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 05/11] of: Ratify of_dma_configure() interface
    On Mon, Sep 30, 2019 at 8:32 AM Nicolas Saenz Julienne
    <nsaenzjulienne@suse.de> wrote:
    >
    > On Mon, 2019-09-30 at 05:57 -0700, Christoph Hellwig wrote:
    > > On Thu, Sep 26, 2019 at 07:24:49PM -0500, Rob Herring wrote:
    > > > -int of_dma_configure(struct device *dev, struct device_node *np, bool
    > > > force_dma)
    > > > +int of_dma_configure(struct device *dev, struct device_node *parent, bool
    > > > force_dma)
    > >
    > > This creates a > 80 char line.
    > >
    > > > {
    > > > u64 dma_addr, paddr, size = 0;
    > > > int ret;
    > > > bool coherent;
    > > > unsigned long offset;
    > > > const struct iommu_ops *iommu;
    > > > + struct device_node *np;
    > > > u64 mask;
    > > >
    > > > + np = dev->of_node;
    > > > + if (!np)
    > > > + np = parent;
    > > > + if (!np)
    > > > + return -ENODEV;
    > >
    > > I have to say I find the older calling convention simpler to understand.
    > > If we want to enforce the invariant I'd rather do that explicitly:
    > >
    > > if (dev->of_node && np != dev->of_node)
    > > return -EINVAL;
    >
    > As is, this would break Freescale Layerscape fsl-mc bus' dma_configure():

    This may break PCI too for devices that have a DT node.

    > static int fsl_mc_dma_configure(struct device *dev)
    > {
    > struct device *dma_dev = dev;
    >
    > while (dev_is_fsl_mc(dma_dev))
    > dma_dev = dma_dev->parent;
    >
    > return of_dma_configure(dev, dma_dev->of_node, 0);
    > }
    >
    > But I think that with this series, given the fact that we now treat the lack of
    > dma-ranges as a 1:1 mapping instead of an error, we could rewrite the function
    > like this:

    Now, I'm reconsidering allowing this abuse... It's better if the code
    which understands the bus structure in DT for a specific bus passes in
    the right thing. Maybe I should go back to Robin's version (below).
    OTOH, the existing assumption that 'dma-ranges' was in the immediate
    parent was an assumption on the bus structure which maybe doesn't
    always apply.

    diff --git a/drivers/of/device.c b/drivers/of/device.c
    index a45261e21144..6951450bb8f3 100644
    --- a/drivers/of/device.c
    +++ b/drivers/of/device.c
    @@ -98,12 +98,15 @@ int of_dma_configure(struct device *dev, struct
    device_node *parent, bool force_
    u64 mask;

    np = dev->of_node;
    - if (!np)
    - np = parent;
    + if (np)
    + parent = of_get_dma_parent(np);
    + else
    + np = of_node_get(parent);
    if (!np)
    return -ENODEV;

    - ret = of_dma_get_range(np, &dma_addr, &paddr, &size);
    + ret = of_dma_get_range(parent, &dma_addr, &paddr, &size);
    + of_node_put(parent);
    if (ret < 0) {
    /*
    * For legacy reasons, we have to assume some devices need
    \
     
     \ /
      Last update: 2019-09-30 23:25    [W:26.120 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site