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@xxxxxxx>
> > ---
>
> > 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.
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
> >
