[PATCH v4 08/12] device core: Introduce multiple dma pfn offsets

[Jim Quinlan]

Hi Andy,

On Sun, Jun 7, 2020 at 12:500f9bfe0fb8840b268af1bbcc51f1cd440514e PM
Andy Shevchenko <andriy.shevchenko at linux.intel.com> wrote:
>
> On Fri, Jun 05, 2020 at 05:26:48PM -0400, Jim Quinlan wrote:
> > The new field in struct device 'dma_pfn_offset_map' is used to facilitate
> > the use of single or multiple pfn offsets between cpu addrs and dma addrs.
> > It subsumes the role of dev->dma_pfn_offset -- a uniform offset.
> >
> > The function of_dma_get_range() has been modified to take two additional
> > arguments: the "map", which is an array that holds the information
> > regarding the pfn offset regions, and map_size, which is the size in bytes
> > of the map array.
> >
> > of_dma_configure() is the typical manner to set pfn offsets but there are a
> > number of ad hoc assignments to dev->dma_pfn_offset in the kernel driver
> > code.  These cases now invoke the function
> > dma_attach_uniform_pfn_offset(dev, pfn_offset).
>
> ...
>
> > +             int ret = dma_attach_uniform_pfn_offset
> > +                             (dev, keystone_dma_pfn_offset);
>
> It's strange indentation. Have you configured your editor correctly?
> Seems to me as fit on one line.
I'm using emacs with the c-style set to linux.  I may have some custom
tweaks; I'll check into it.  But I think I can fix most of your
objections by using the max_line_length of 100.

>
> > +             dev_err(dev, "set dma_pfn_offset%08lx%s\n",
> > +                     dev->dma_pfn_offset, ret ? " failed" : "");
>
> ...
>
> > +     *map_size = (num_ranges + 1) * sizeof(**map);
> > +     r = kzalloc(*map_size, GFP_KERNEL);
>
> kcalloc()
Since I have to calculate the size anyway I thought kzalloc was fine.
I'll switch.
>
> > +     if (!r)
> > +             return -ENOMEM;
>
> ...
>
> > +             r->pfn_offset = PFN_DOWN(range.cpu_addr)
> > +                     - PFN_DOWN(range.bus_addr);
>
> Ditto (indentation).
>
> ...
>
>
> > +             unsigned long dma_pfn_offset
> > +                     = dma_pfn_offset_from_phys_addr(dev, paddr);
>
> Ditto.
>
> ...
>
> > +             unsigned long dma_pfn_offset
> > +                     = dma_pfn_offset_from_dma_addr(dev, dev_addr);
>
> Ditto.
>
> Check entire your series for a such, please!

Will do,
Thanks
Jim Quinlan
>
> --
> With Best Regards,
> Andy Shevchenko
>
>