Re: [PATCH v3 1/9] dmaengine: Support bus widths of 32 bytes and above

["Nuno Sá" <nuno.sa@analog.com>]

On Mon, Aug 31, 2026 at 04:45:15PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 31, 2026 at 12:46:38PM +0100, Nuno Sá wrote:
> > The src_addr_widths and dst_addr_widths capability masks encode each
> > supported width as a bit whose position equals the corresponding
> > enum dma_slave_buswidth value (e.g. DMA_SLAVE_BUSWIDTH_4_BYTES sets bit
> > 4). As these masks are plain u32, widths of 32 bytes and above
> > (DMA_SLAVE_BUSWIDTH_32/64/128_BYTES map to bits 32, 64 and 128) cannot
> > be represented at all.
> > 
> > Introduce bitmap-based bus width capabilities that span the full enum
> > range, through a new dma_buswidth_mask_t type modeled after
> > dma_cap_mask_t. To allow DMA controller drivers to be converted
> > incrementally, the legacy dma_device u32 fields are kept alongside the
> > new masks and the core folds a legacy-only driver's u32 into the mask
> > when the device is registered, so consumers only ever have to look at
> > the mask.
> > 
> > The new interface lives in two new headers under a new
> > include/linux/dma/engine/ directory instead of growing
> > linux/dmaengine.h, which is included nearly everywhere:
> > 
> >  - dma/engine/types.h holds enum dma_slave_buswidth and the new
> >    dma_buswidth_mask_t type. Like dma_cap_mask_t, the type only needs
> >    DECLARE_BITMAP();
> > 
> >  - dma/engine/widthmask.h holds the accessors which are based on the new
> >    dma_buswidth_mask_t type. This gives us freedom to change the core
> >    without affecting consumers as they only see (and should only use) the
> >    new type.
> > 
> > Note the fold only has to happen in one direction on the producer side:
> > nothing outside a controller driver reads the legacy dma_device fields,
> > so a converted driver's mask is not mirrored back into them. The legacy
> > dma_slave_caps fields are different, as consumers not converted yet
> > still read them: dma_get_slave_caps() derives them from the mask when a
> > device_caps() callback adjusted it. Both go away with the legacy fields.
> 
> ...
> 
> >  F:	drivers/dma/
> >  F:	include/dt-bindings/dma/
> >  F:	include/linux/dma/
> > +F:	include/linux/dma/engine/
> >  F:	include/linux/dmaengine.h
> >  F:	include/linux/of_dma.h
> 
> Unneeded, previous entry includes recursively.
> 

Checkpatch complains so I guess we assume it's a tool issue?

> ...
> 
> > +/*
> > + * Basic types shared by the DMA engine interfaces.
> > + */
> > +#ifndef LINUX_DMA_ENGINE_TYPES_H
> > +#define LINUX_DMA_ENGINE_TYPES_H
> > +
> > +#include <linux/bitops.h>
> 
> Not yet? Perhaps next changes will use it, then they can add it.

Should be dropped!

> 
> > +#include <linux/types.h>
> > +
> > +/**
> > + * enum dma_slave_buswidth - defines bus width of the DMA slave
> > + * device, source or target buses
> 
> I would describe the _UNDEFINED case, it might require some clarification on
> what behaviour is to expect with this one.

Yeah, here I can of just copy pasted what we had. Also not completely
sure what's the undefined case is about. I can do some search though.

> 
> > + */
> 
> > +enum dma_slave_buswidth {
> > +	DMA_SLAVE_BUSWIDTH_UNDEFINED = 0,
> > +	DMA_SLAVE_BUSWIDTH_1_BYTE = 1,
> > +	DMA_SLAVE_BUSWIDTH_2_BYTES = 2,
> > +	DMA_SLAVE_BUSWIDTH_3_BYTES = 3,
> > +	DMA_SLAVE_BUSWIDTH_4_BYTES = 4,
> > +	DMA_SLAVE_BUSWIDTH_8_BYTES = 8,
> > +	DMA_SLAVE_BUSWIDTH_16_BYTES = 16,
> > +	DMA_SLAVE_BUSWIDTH_32_BYTES = 32,
> > +	DMA_SLAVE_BUSWIDTH_64_BYTES = 64,
> > +	DMA_SLAVE_BUSWIDTH_128_BYTES = 128,
> > +	DMA_SLAVE_BUSWIDTH_MAX
> > +};
> 
> > +/**
> > + * typedef dma_buswidth_mask_t - bus width capabilities bitmap modeled after
> > + * dma_cap_mask_t.
> > + *
> > + * Each supported bus width is represented by the bit whose position equals the
> > + * corresponding enum dma_slave_buswidth value, e.g. a device supporting a bus
> > + * width of 4 bytes has bit 4 set.
> > + */
> > +typedef struct {
> > +	DECLARE_BITMAP(bits, DMA_SLAVE_BUSWIDTH_MAX);
> > +} dma_buswidth_mask_t;
> > +
> > +#endif /* LINUX_DMA_ENGINE_TYPES_H */
> 
> ...
> 
> > +/*
> > + * Bus width capabilities of DMA engine devices and channels.
> > + */
> > +#ifndef LINUX_DMA_ENGINE_WIDTHMASK_H
> > +#define LINUX_DMA_ENGINE_WIDTHMASK_H
> > +
> > +#include <linux/bitmap.h>
> > +#include <linux/dma/engine/types.h>
> 
> + errno.h

ack

> 
> > +#include <linux/types.h>
> 
> I would group subsystem ones.
> 
> #include <linux/bitmap.h>
> #include <linux/errno.h>
> #include <linux/types.h>
> 
> #include <linux/dma/engine/types.h>

Can do that, yes.

> 
> ...
> 
> > +static inline enum dma_slave_buswidth
> > +__dma_bus_width_min(const dma_buswidth_mask_t *mask)
> > +{
> > +	enum dma_slave_buswidth width = find_first_bit(mask->bits,
> > +						       DMA_SLAVE_BUSWIDTH_MAX);
> > +
> 
> For easier maintenance better to split the assignment.
> 
> 	enum dma_slave_buswidth width;
> 
> 	width = find_first_bit(mask->bits, DMA_SLAVE_BUSWIDTH_MAX);
> 

No strong feelings so sure.

> > +	if (width == DMA_SLAVE_BUSWIDTH_MAX)
> > +		return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> > +
> > +	return width;
> > +}
> 
> ...
> 
> >  #ifndef LINUX_DMAENGINE_H
> >  #define LINUX_DMAENGINE_H
> >  
> > +#include <linux/bitops.h>
> >  #include <linux/device.h>
> 
> > +#include <linux/dma/engine/types.h>
> 
> Same, group them after generic linux/*.h.
> 
> >  #include <linux/err.h>
> >  #include <linux/uio.h>
> >  #include <linux/bug.h>
> 
> ...
> 
> It's possible to split this patch to two:
> - move the existing type into a new types.h header
> - add support for the new API

Yeah I thought about that but this still looked simple enough to go
together but I can split in the next version.

- Nuno Sá
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>