Re: [PATCH v6 7/9] dmaengine: dw-edma: Add support for chip specific flags

[Serge Semin <fancer.lancer@gmail.com>]

On Wed, Apr 13, 2022 at 02:58:08PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Apr 13, 2022 at 12:18:37PM +0300, Serge Semin wrote:
> > On Wed, Apr 06, 2022 at 10:23:45AM -0500, Frank Li wrote:
> > > Add a "flags" field to the "struct dw_edma_chip" so that the controller
> > > drivers can pass flags that are relevant to the platform.
> > > 
> > > DW_EDMA_CHIP_LOCAL - Used by the controller drivers accessing eDMA
> > > locally. Local eDMA access doesn't require generating MSIs to the remote.
> > > 
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > 
> > > Change from v5 to v6
> > >  - use enum instead of define
> > 
> > Hm, why have you decided to do that? I don't see a well justified
> > reason to use the enumeration here, but see my next comment for
> > details.
> 
> It was me who suggested using the enums for flags instead of defines.
> Enums helps with kdoc and it also provides a neat way to group flags together.
> 
> > 
> > > 
> > > Change from v4 to v5
> > >  - split two two patch
> > >  - rework commit message
> > > Change from v3 to v4
> > > none
> > > Change from v2 to v3
> > >  - rework commit message
> > >  - Change to DW_EDMA_CHIP_32BIT_DBI
> > >  - using DW_EDMA_CHIP_LOCAL control msi
> > >  - Apply Bjorn's comments,
> > >         if (!j) {
> > >                control |= DW_EDMA_V0_LIE;
> > >                if (!(chan->chip->flags & DW_EDMA_CHIP_LOCAL))
> > >                                control |= DW_EDMA_V0_RIE;
> > >         }
> > > 
> > >         if ((chan->chip->flags & DW_EDMA_CHIP_REG32BIT) ||
> > >               !IS_ENABLED(CONFIG_64BIT)) {
> > >           SET_CH_32(...);
> > >           SET_CH_32(...);
> > >        } else {
> > >           SET_CH_64(...);
> > >        }
> > > 
> > > 
> > > Change from v1 to v2
> > > - none
> > > 
> > >  drivers/dma/dw-edma/dw-edma-v0-core.c | 9 ++++++---
> > >  include/linux/dma/edma.h              | 9 +++++++++
> > >  2 files changed, 15 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > index 8ddc537d11fd6..30f8bfe6e5712 100644
> > > --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> > > +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
> 
> [...]
> 

> > > +	enum dw_edma_chip_flags	flags;
> > 
> > There is no point in having the named enumeration here since the flags
> > field semantics is actually a bitfield rather than a single value. If
> > you want to stick to the enumerated flags, then please use the
> > anonymous enum like this:
> 
> I agree with using u32 for flags field but I don't agree with anonymous enums.
> Enums with a name conveys information of what the enumerated types represent.
> If you just look at your example below, it is difficult to guess the purpose of
> this enum.

I see your point. Ok, no anonymization then.) @Frank could you please update
the field type to unsigned int or u32 then? Personally I prefer having
"unsigned int" here, since that's the type used by the compiler if no
negative values is enumerated. Though u32 would be ok too.

-Serget

> 
> Thanks,
> Mani
> 
> > +enum {
> > +	DW_EDMA_CHIP_LOCAL	= BIT(0),
> > +};
> > and explicit unsigned int type of the flags field.
> > 
> > -Sergey
> > 
> > >  
> > >  	void __iomem		*reg_base;
> > >  
> > > -- 
> > > 2.35.1
> > >