Re: [PATCH v4 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

2020-10-16 Thread Nicolin Chen
On Tue, Oct 13, 2020 at 03:17:32PM +0300, Viorel Suman (OSS) wrote:
> From: Viorel Suman 
> 
> XCVR (Audio Transceiver) is a on-chip functional module found
> on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
> 
> Signed-off-by: Viorel Suman 

Acked-by: Nicolin Chen 


Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

2020-09-17 Thread Nicolin Chen
Hi Viorel,

It looks pretty clean to me, though some small comments inline.

On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> From: Viorel Suman 
> 
> XCVR (Audio Transceiver) is a on-chip functional module found
> on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
> 
> Signed-off-by: Viorel Suman 

> +static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /* one bit 
> 6, 12 ? */

What's the meaning of the comments?

> +static const int fsl_xcvr_phy_arc_cfg[] = {
> + FSL_XCVR_PHY_CTRL_ARC_MODE_SE_EN, FSL_XCVR_PHY_CTRL_ARC_MODE_CM_EN,
> +};

Nit: better be u32 vs. int?

> +/** phy: true => phy, false => pll */
> +static int fsl_xcvr_ai_write(struct fsl_xcvr *xcvr, u8 reg, u32 data, bool 
> phy)
> +{
> + u32 val, idx, tidx;
> +
> + idx  = BIT(phy ? 26 : 24);
> + tidx = BIT(phy ? 27 : 25);
> +
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_CLR, 0xFF);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_SET, reg);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_WDATA, data);
> + regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_TOG, idx);
> +
> + do {
> + regmap_read(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL, );
> + } while ((val & idx) != ((val & tidx) >> 1));

Might regmap_read_poll_timeout() be better? And it seems to poll
intentionally with no sleep nor timeout -- would be nice to have
a line of comments to explain why.

> > +static int fsl_xcvr_runtime_resume(struct device *dev)
> +{
> + struct fsl_xcvr *xcvr = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(xcvr->ipg_clk);
> + if (ret) {
> + dev_err(dev, "failed to start IPG clock.\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->pll_ipg_clk);
> + if (ret) {
> + dev_err(dev, "failed to start PLL IPG clock.\n");

Should it disable ipg_clk?

> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->phy_clk);
> + if (ret) {
> + dev_err(dev, "failed to start PHY clock: %d\n", ret);
> + clk_disable_unprepare(xcvr->ipg_clk);

Should it disable pll_ipg_clk?

> + return ret;
> + }
> +
> + ret = clk_prepare_enable(xcvr->spba_clk);
> + if (ret) {
> + dev_err(dev, "failed to start SPBA clock.\n");
> + clk_disable_unprepare(xcvr->phy_clk);
> + clk_disable_unprepare(xcvr->ipg_clk);

Ditto

> + return ret;
> + }
> +
> + regcache_cache_only(xcvr->regmap, false);
> + regcache_mark_dirty(xcvr->regmap);
> + ret = regcache_sync(xcvr->regmap);
> +
> + if (ret) {
> + dev_err(dev, "failed to sync regcache.\n");
> + return ret;

What about those clocks? Probably better to have some error-out
labels at the end of the function?

> + }
> +
> + reset_control_assert(xcvr->reset);
> + reset_control_deassert(xcvr->reset);
> +
> + ret = fsl_xcvr_load_firmware(xcvr);
> + if (ret) {
> + dev_err(dev, "failed to load firmware.\n");
> + return ret;

Ditto

> + }
> +
> + /* Release M0+ reset */
> + ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> +  FSL_XCVR_EXT_CTRL_CORE_RESET, 0);
> + if (ret < 0) {
> + dev_err(dev, "M0+ core release failed: %d\n", ret);
> + return ret;

Ditto

> + }
> + mdelay(50);

Any reason to use mdelay over msleep for a 50ms wait? May add a
line of comments if mdelay is a must?


Re: [PATCH] ASoC: fsl_audmix: make clock and output src write only

2020-09-16 Thread Nicolin Chen
On Mon, Sep 14, 2020 at 08:24:34PM +0300, Viorel Suman (OSS) wrote:
> From: Viorel Suman 
> 
> "alsactl -f state.conf store/restore" sequence fails because setting
> "mixing clock source" and "output source" requires active TDM clock
> being started for configuration propagation. Make these two controls
> write only so that their values are not stored at "alsactl store".
> 
> Signed-off-by: Viorel Suman 

Acked-by: Nicolin Chen 

> ---
>  sound/soc/fsl/fsl_audmix.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
> index a447baf..7ad5925 100644
> --- a/sound/soc/fsl/fsl_audmix.c
> +++ b/sound/soc/fsl/fsl_audmix.c
> @@ -199,10 +199,18 @@ static int fsl_audmix_put_out_src(struct snd_kcontrol 
> *kcontrol,
>  
>  static const struct snd_kcontrol_new fsl_audmix_snd_controls[] = {
>   /* FSL_AUDMIX_CTR controls */
> - SOC_ENUM_EXT("Mixing Clock Source", fsl_audmix_enum[0],
> -  snd_soc_get_enum_double, fsl_audmix_put_mix_clk_src),
> - SOC_ENUM_EXT("Output Source", fsl_audmix_enum[1],
> -  snd_soc_get_enum_double, fsl_audmix_put_out_src),
> + {   .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> + .name = "Mixing Clock Source",
> + .info = snd_soc_info_enum_double,
> + .access = SNDRV_CTL_ELEM_ACCESS_WRITE,
> + .put = fsl_audmix_put_mix_clk_src,
> + .private_value = (unsigned long)_audmix_enum[0] },
> + {   .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
> + .name = "Output Source",
> + .info = snd_soc_info_enum_double,
> + .access = SNDRV_CTL_ELEM_ACCESS_WRITE,
> + .put = fsl_audmix_put_out_src,
> + .private_value = (unsigned long)_audmix_enum[1] },
>   SOC_ENUM("Output Width", fsl_audmix_enum[2]),
>   SOC_ENUM("Frame Rate Diff Error", fsl_audmix_enum[3]),
>   SOC_ENUM("Clock Freq Diff Error", fsl_audmix_enum[4]),
> -- 
> 2.7.4
> 


Re: [PATCH] ASoC: fsl_sai: Set SAI Channel Mode to Output Mode

2020-09-02 Thread Nicolin Chen
On Thu, Sep 03, 2020 at 11:09:15AM +0800, Shengjiu Wang wrote:
> Transmit data pins will output zero when slots are masked or channels
> are disabled. In CHMOD TDM mode, transmit data pins are tri-stated when
> slots are masked or channels are disabled. When data pins are tri-stated,
> there is noise on some channels when FS clock value is high and data is
> read while fsclk is transitioning from high to low.
> 
> Signed-off-by: Cosmin-Gabriel Samoila 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 

Though one nit inline:

> ---
>  sound/soc/fsl/fsl_sai.c | 12 ++--
>  sound/soc/fsl/fsl_sai.h |  2 ++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index 62c5fdb678fc..33b194a5c1dc 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -486,6 +486,12 @@ static int fsl_sai_hw_params(struct snd_pcm_substream 
> *substream,
>  
>   val_cr4 |= FSL_SAI_CR4_FRSZ(slots);
>  
> + /* Output Mode - data pins transmit 0 when slots are masked
> +  * or channels are disabled
> +  */

Coding style for multi-line comments. Yet, probably can simplify?

/* Set to output mode to avoid tri-stated data pins */


Re: [PATCH 0/2] dma-mapping: update default segment_boundary_mask

2020-09-02 Thread Nicolin Chen
On Wed, Sep 02, 2020 at 10:13:12AM +0200, Niklas Schnelle wrote:
> On 9/2/20 12:16 AM, Nicolin Chen wrote:
> > These two patches are to update default segment_boundary_mask.
> > 
> > PATCH-1 fixes overflow issues in callers of dma_get_seg_boundary.
> > Previous version was a series: https://lkml.org/lkml/2020/8/31/1026
> > 
> > Then PATCH-2 sets default segment_boundary_mask to ULONG_MAX.
> > 
> > Nicolin Chen (2):
> >   dma-mapping: introduce dma_get_seg_boundary_nr_pages()
> >   dma-mapping: set default segment_boundary_mask to ULONG_MAX
> 
> I gave both of your patches a quick test ride on a couple of dev mainframes,
> both NVMe, ConnectX and virtio-pci devices all seems to work fine.
> I already commented on Christoph's mail that I like the helper approach,
> so as for s390 you can add my
> 
> Acked-by: Niklas Schnelle 
 
Thanks for testing and the ack! 


[PATCH 2/2] dma-mapping: set default segment_boundary_mask to ULONG_MAX

2020-09-01 Thread Nicolin Chen
The default segment_boundary_mask was set to DMA_BIT_MAKS(32)
a decade ago by referencing SCSI/block subsystem, as a 32-bit
mask was good enough for most of the devices.

Now more and more drivers set dma_masks above DMA_BIT_MAKS(32)
while only a handful of them call dma_set_seg_boundary(). This
means that most drivers have a 4GB segmention boundary because
DMA API returns a 32-bit default value, though they might not
really have such a limit.

The default segment_boundary_mask should mean "no limit" since
the device doesn't explicitly set the mask. But a 32-bit mask
certainly limits those devices capable of 32+ bits addressing.

So this patch sets default segment_boundary_mask to ULONG_MAX.

Signed-off-by: Nicolin Chen 
---
 include/linux/dma-mapping.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index faab0a8210b9..df0bff2ea750 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -629,7 +629,7 @@ static inline unsigned long dma_get_seg_boundary(struct 
device *dev)
 {
if (dev->dma_parms && dev->dma_parms->segment_boundary_mask)
return dev->dma_parms->segment_boundary_mask;
-   return DMA_BIT_MASK(32);
+   return ULONG_MAX;
 }
 
 /**
-- 
2.17.1



[PATCH 0/2] dma-mapping: update default segment_boundary_mask

2020-09-01 Thread Nicolin Chen
These two patches are to update default segment_boundary_mask.

PATCH-1 fixes overflow issues in callers of dma_get_seg_boundary.
Previous version was a series: https://lkml.org/lkml/2020/8/31/1026

Then PATCH-2 sets default segment_boundary_mask to ULONG_MAX.

Nicolin Chen (2):
  dma-mapping: introduce dma_get_seg_boundary_nr_pages()
  dma-mapping: set default segment_boundary_mask to ULONG_MAX

 arch/alpha/kernel/pci_iommu.c|  7 +--
 arch/ia64/hp/common/sba_iommu.c  |  3 +--
 arch/powerpc/kernel/iommu.c  |  9 ++---
 arch/s390/pci/pci_dma.c  |  6 ++
 arch/sparc/kernel/iommu-common.c | 10 +++---
 arch/sparc/kernel/iommu.c|  3 +--
 arch/sparc/kernel/pci_sun4v.c|  3 +--
 arch/x86/kernel/amd_gart_64.c|  3 +--
 drivers/parisc/ccio-dma.c|  3 +--
 drivers/parisc/sba_iommu.c   |  3 +--
 include/linux/dma-mapping.h  | 21 -
 11 files changed, 34 insertions(+), 37 deletions(-)

-- 
2.17.1



[PATCH 1/2] dma-mapping: introduce dma_get_seg_boundary_nr_pages()

2020-09-01 Thread Nicolin Chen
We found that callers of dma_get_seg_boundary mostly do an ALIGN
with page mask and then do a page shift to get number of pages:
ALIGN(boundary + 1, 1 << shift) >> shift

However, the boundary might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1" or
passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here into a helper function doing:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

This patch introduces and applies dma_get_seg_boundary_nr_pages()
as an overflow-free helper for the dma_get_seg_boundary() callers
to get numbers of pages. It also takes care of the NULL dev case
for non-DMA API callers.

Suggested-by: Christoph Hellwig 
Signed-off-by: Nicolin Chen 
---
 arch/alpha/kernel/pci_iommu.c|  7 +--
 arch/ia64/hp/common/sba_iommu.c  |  3 +--
 arch/powerpc/kernel/iommu.c  |  9 ++---
 arch/s390/pci/pci_dma.c  |  6 ++
 arch/sparc/kernel/iommu-common.c | 10 +++---
 arch/sparc/kernel/iommu.c|  3 +--
 arch/sparc/kernel/pci_sun4v.c|  3 +--
 arch/x86/kernel/amd_gart_64.c|  3 +--
 drivers/parisc/ccio-dma.c|  3 +--
 drivers/parisc/sba_iommu.c   |  3 +--
 include/linux/dma-mapping.h  | 19 +++
 11 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index 81037907268d..6f7de4f4e191 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -141,12 +141,7 @@ iommu_arena_find_pages(struct device *dev, struct 
pci_iommu_arena *arena,
unsigned long boundary_size;
 
base = arena->dma_base >> PAGE_SHIFT;
-   if (dev) {
-   boundary_size = dma_get_seg_boundary(dev) + 1;
-   boundary_size >>= PAGE_SHIFT;
-   } else {
-   boundary_size = 1UL << (32 - PAGE_SHIFT);
-   }
+   boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
 
/* Search forward for the first mask-aligned sequence of N free ptes */
ptes = arena->ptes;
diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
index 656a4888c300..b49b73a95067 100644
--- a/arch/ia64/hp/common/sba_iommu.c
+++ b/arch/ia64/hp/common/sba_iommu.c
@@ -485,8 +485,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) 
== 0);
ASSERT(res_ptr < res_end);
 
-   boundary_size = (unsigned long long)dma_get_seg_boundary(dev) + 1;
-   boundary_size = ALIGN(boundary_size, 1ULL << iovp_shift) >> iovp_shift;
+   boundary_size = dma_get_seg_boundary_nr_pages(dev, iovp_shift);
 
BUG_ON(ioc->ibase & ~iovp_mask);
shift = ioc->ibase >> iovp_shift;
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 9704f3f76e63..cbc2e62db597 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -236,15 +236,10 @@ static unsigned long iommu_range_alloc(struct device *dev,
}
}
 
-   if (dev)
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << tbl->it_page_shift);
-   else
-   boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
-   /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
+   boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
 
n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
-boundary_size >> tbl->it_page_shift, align_mask);
+boundary_size, align_mask);
if (n == -1) {
if (likely(pass == 0)) {
/* First try the pool from the start */
diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
index 64b1399a73f0..4a37d8f4de9d 100644
--- a/arch/s390/pci/pci_dma.c
+++ b/arch/s390/pci/pci_dma.c
@@ -261,13 +261,11 @@ static unsigned long __dma_alloc_iommu(struct device *dev,
   unsigned long start, int size)
 {
struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
-   unsigned long boundary_size;
 
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- PAGE_SIZE) >> PAGE_SHIFT;
return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
start, size, zdev->start_dma &g

Re: [RESEND][PATCH 1/7] powerpc/iommu: Avoid overflow at boundary_size

2020-09-01 Thread Nicolin Chen
On Tue, Sep 01, 2020 at 11:27:36PM +1000, Michael Ellerman wrote:
> Nicolin Chen  writes:
> > The boundary_size might be as large as ULONG_MAX, which means
> > that a device has no specific boundary limit. So either "+ 1"
> > or passing it to ALIGN() would potentially overflow.
> >
> > According to kernel defines:
> > #define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
> > #define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
> >
> > We can simplify the logic here:
> >   ALIGN(boundary + 1, 1 << shift) >> shift
> > = ALIGN_MASK(b + 1, (1 << s) - 1) >> s
> > = {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
> > = [b + 1 + (1 << s) - 1] >> s
> > = [b + (1 << s)] >> s
> > = (b >> s) + 1
> >
> > So fixing a potential overflow with the safer shortcut.
> >
> > Reported-by: Stephen Rothwell 
> > Signed-off-by: Nicolin Chen 
> > Cc: Christoph Hellwig 
> > ---
> >  arch/powerpc/kernel/iommu.c | 11 +--
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> Are you asking for acks, or for maintainers to merge the patches
> individually?

I was expecting that but Christoph just suggested me to squash them
into one so he would merge it: https://lkml.org/lkml/2020/9/1/159

Though I feel it'd be nice to get maintainers' acks before merging.

> > diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> > index 9704f3f76e63..c01ccbf8afdd 100644
> > --- a/arch/powerpc/kernel/iommu.c
> > +++ b/arch/powerpc/kernel/iommu.c
> > @@ -236,15 +236,14 @@ static unsigned long iommu_range_alloc(struct device 
> > *dev,
> > }
> > }
> >  
> > -   if (dev)
> > -   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
> > - 1 << tbl->it_page_shift);
> > -   else
> > -   boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
> > /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> > +   boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> 
> Is there any path that passes a NULL dev anymore?
> 
> Both iseries_hv_alloc() and iseries_hv_map() were removed years ago.
> See:
>   8ee3e0d69623 ("powerpc: Remove the main legacy iSerie platform code")
> 
> 
> So maybe we should do a lead-up patch that drops the NULL dev support,
> which will then make this patch simpler.

The next version of this change will follow Christoph's suggestion
by having a helper function that takes care of !dev internally.

Thanks
Nic

> 
> 
> > +   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> > +   boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
> >  
> > n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
> > -boundary_size >> tbl->it_page_shift, align_mask);
> > +boundary_size, align_mask);
> > if (n == -1) {
> > if (likely(pass == 0)) {
> > /* First try the pool from the start */
> > -- 
> > 2.17.1


Re: [RESEND][PATCH 0/7] Avoid overflow at boundary_size

2020-09-01 Thread Nicolin Chen
Hi Christoph,

On Tue, Sep 01, 2020 at 09:36:23AM +0200, Christoph Hellwig wrote:
> I really don't like all the open coded smarts in the various drivers.
> What do you think about a helper like the one in the untested patch

A helper function will be actually better. I was thinking of
one yet not very sure about the naming and where to put it.

> below (on top of your series).  Also please include the original
> segment boundary patch with the next resend so that the series has
> the full context.

I will use your change instead and resend with the ULONG_MAX
change. But in that case, should I make separate changes for
different files like this series, or just one single change
like yours?

Asking this as I was expecting that those changes would get
applied by different maintainers. But now it feels like you
will merge it to your tree at once?

Thanks
Nic

> diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
> index 1ef2c647bd3ec2..6f7de4f4e191e7 100644
> --- a/arch/alpha/kernel/pci_iommu.c
> +++ b/arch/alpha/kernel/pci_iommu.c
> @@ -141,10 +141,7 @@ iommu_arena_find_pages(struct device *dev, struct 
> pci_iommu_arena *arena,
>   unsigned long boundary_size;
>  
>   base = arena->dma_base >> PAGE_SHIFT;
> -
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> PAGE_SHIFT) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT);
>  
>   /* Search forward for the first mask-aligned sequence of N free ptes */
>   ptes = arena->ptes;
> diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
> index 945954903bb0ba..b49b73a95067d2 100644
> --- a/arch/ia64/hp/common/sba_iommu.c
> +++ b/arch/ia64/hp/common/sba_iommu.c
> @@ -485,8 +485,7 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
>   ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) 
> == 0);
>   ASSERT(res_ptr < res_end);
>  
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> iovp_shift) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, iovp_shift);
>  
>   BUG_ON(ioc->ibase & ~iovp_mask);
>   shift = ioc->ibase >> iovp_shift;
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index c01ccbf8afdd42..cbc2e62db597cf 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -236,11 +236,7 @@ static unsigned long iommu_range_alloc(struct device 
> *dev,
>   }
>   }
>  
> - /* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> -
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
> + boundary_size = dma_get_seg_boundary_nr_pages(dev, tbl->it_page_shift);
>  
>   n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
>boundary_size, align_mask);
> diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
> index ecb067acc6d532..4a37d8f4de9d9d 100644
> --- a/arch/s390/pci/pci_dma.c
> +++ b/arch/s390/pci/pci_dma.c
> @@ -261,13 +261,11 @@ static unsigned long __dma_alloc_iommu(struct device 
> *dev,
>  unsigned long start, int size)
>  {
>   struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
> - unsigned long boundary_size;
>  
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
>   return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
>   start, size, zdev->start_dma >> PAGE_SHIFT,
> - boundary_size, 0);
> + dma_get_seg_boundary_nr_pages(dev, PAGE_SHIFT),
> + 0);
>  }
>  
>  static dma_addr_t dma_alloc_address(struct device *dev, int size)
> diff --git a/arch/sparc/kernel/iommu-common.c 
> b/arch/sparc/kernel/iommu-common.c
> index 843e71894d0482..e6139c99762e11 100644
> --- a/arch/sparc/kernel/iommu-common.c
> +++ b/arch/sparc/kernel/iommu-common.c
> @@ -166,10 +166,6 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
>   }
>   }
>  
> - boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
> -
> - /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
> - boundary_size = (boundary_size >> iommu->table_shift) + 1;
>   /*
>* if the skip_span_boundary_check had been set during init, we set
>* things up so that iommu_is_span_boundary() merely checks if the
> @@ -178,7 +174,11 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
>   if ((iommu->flags & IOMMU_NO_SPAN_BOUND) != 0) {
>   shift = 0;
>   

[RESEND][PATCH 7/7] parisc: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 drivers/parisc/ccio-dma.c  | 4 ++--
 drivers/parisc/sba_iommu.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/parisc/ccio-dma.c b/drivers/parisc/ccio-dma.c
index a5507f75b524..c667d6aba764 100644
--- a/drivers/parisc/ccio-dma.c
+++ b/drivers/parisc/ccio-dma.c
@@ -356,8 +356,8 @@ ccio_alloc_range(struct ioc *ioc, struct device *dev, 
size_t size)
** ggg sacrifices another 710 to the computer gods.
*/
 
-   boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
- 1ULL << IOVP_SHIFT) >> IOVP_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
 
if (pages_needed <= 8) {
/*
diff --git a/drivers/parisc/sba_iommu.c b/drivers/parisc/sba_iommu.c
index d4314fba0269..96bc2c617cbd 100644
--- a/drivers/parisc/sba_iommu.c
+++ b/drivers/parisc/sba_iommu.c
@@ -342,8 +342,8 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
unsigned long shift;
int ret;
 
-   boundary_size = ALIGN((unsigned long long)dma_get_seg_boundary(dev) + 1,
- 1ULL << IOVP_SHIFT) >> IOVP_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (dma_get_seg_boundary(dev) >> IOVP_SHIFT) + 1;
 
 #if defined(ZX1_SUPPORT)
BUG_ON(ioc->ibase & ~IOVP_MASK);
-- 
2.17.1



[RESEND][PATCH 6/7] x86/amd_gart: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/x86/kernel/amd_gart_64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/amd_gart_64.c b/arch/x86/kernel/amd_gart_64.c
index e89031e9c847..7fa0bb490065 100644
--- a/arch/x86/kernel/amd_gart_64.c
+++ b/arch/x86/kernel/amd_gart_64.c
@@ -96,8 +96,8 @@ static unsigned long alloc_iommu(struct device *dev, int size,
 
base_index = ALIGN(iommu_bus_base & dma_get_seg_boundary(dev),
   PAGE_SIZE) >> PAGE_SHIFT;
-   boundary_size = ALIGN((u64)dma_get_seg_boundary(dev) + 1,
- PAGE_SIZE) >> PAGE_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
 
spin_lock_irqsave(_bitmap_lock, flags);
offset = iommu_area_alloc(iommu_gart_bitmap, iommu_pages, next_bit,
-- 
2.17.1



[RESEND][PATCH 5/7] sparc: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/sparc/kernel/iommu-common.c | 9 +++--
 arch/sparc/kernel/iommu.c| 4 ++--
 arch/sparc/kernel/pci_sun4v.c| 4 ++--
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/sparc/kernel/iommu-common.c b/arch/sparc/kernel/iommu-common.c
index 59cb16691322..843e71894d04 100644
--- a/arch/sparc/kernel/iommu-common.c
+++ b/arch/sparc/kernel/iommu-common.c
@@ -166,13 +166,10 @@ unsigned long iommu_tbl_range_alloc(struct device *dev,
}
}
 
-   if (dev)
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << iommu->table_shift);
-   else
-   boundary_size = ALIGN(1ULL << 32, 1 << iommu->table_shift);
+   boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
 
-   boundary_size = boundary_size >> iommu->table_shift;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (boundary_size >> iommu->table_shift) + 1;
/*
 * if the skip_span_boundary_check had been set during init, we set
 * things up so that iommu_is_span_boundary() merely checks if the
diff --git a/arch/sparc/kernel/iommu.c b/arch/sparc/kernel/iommu.c
index 4ae7388b1bff..d981c37305ae 100644
--- a/arch/sparc/kernel/iommu.c
+++ b/arch/sparc/kernel/iommu.c
@@ -472,8 +472,8 @@ static int dma_4u_map_sg(struct device *dev, struct 
scatterlist *sglist,
outs->dma_length = 0;
 
max_seg_size = dma_get_max_seg_size(dev);
-   seg_boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
base_shift = iommu->tbl.table_map_base >> IO_PAGE_SHIFT;
for_each_sg(sglist, s, nelems, i) {
unsigned long paddr, npages, entry, out_entry = 0, slen;
diff --git a/arch/sparc/kernel/pci_sun4v.c b/arch/sparc/kernel/pci_sun4v.c
index 14b93c5564e3..233fe8a20cec 100644
--- a/arch/sparc/kernel/pci_sun4v.c
+++ b/arch/sparc/kernel/pci_sun4v.c
@@ -508,8 +508,8 @@ static int dma_4v_map_sg(struct device *dev, struct 
scatterlist *sglist,
iommu_batch_start(dev, prot, ~0UL);
 
max_seg_size = dma_get_max_seg_size(dev);
-   seg_boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- IO_PAGE_SIZE) >> IO_PAGE_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   seg_boundary_size = (dma_get_seg_boundary(dev) >> IO_PAGE_SHIFT) + 1;
 
mask = *dev->dma_mask;
if (!iommu_use_atu(iommu, mask))
-- 
2.17.1



[RESEND][PATCH 4/7] s390/pci_dma: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/s390/pci/pci_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci_dma.c b/arch/s390/pci/pci_dma.c
index 64b1399a73f0..ecb067acc6d5 100644
--- a/arch/s390/pci/pci_dma.c
+++ b/arch/s390/pci/pci_dma.c
@@ -263,8 +263,8 @@ static unsigned long __dma_alloc_iommu(struct device *dev,
struct zpci_dev *zdev = to_zpci(to_pci_dev(dev));
unsigned long boundary_size;
 
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- PAGE_SIZE) >> PAGE_SHIFT;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (dma_get_seg_boundary(dev) >> PAGE_SHIFT) + 1;
return iommu_area_alloc(zdev->iommu_bitmap, zdev->iommu_pages,
start, size, zdev->start_dma >> PAGE_SHIFT,
boundary_size, 0);
-- 
2.17.1



[RESEND][PATCH 0/7] Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
 For this resend 
The original series have not been acked at any patch. So I am
resending them, being suggested by Niklas.

 Coverletter 
We are expending the default DMA segmentation boundary to its
possible maximum value (ULONG_MAX) to indicate that a device
doesn't specify a boundary limit. So all dma_get_seg_boundary
callers should take a precaution with the return values since
it would easily get overflowed.

I scanned the entire kernel tree for all the existing callers
and found that most of callers may get overflowed in two ways:
either "+ 1" or passing it to ALIGN() that does "+ mask".

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So this series of patches fix the potential overflow with this
overflow-free shortcut.

As I don't have these platforms, testings/comments are welcome.

Thanks
Nic

Nicolin Chen (7):
  powerpc/iommu: Avoid overflow at boundary_size
  alpha: Avoid overflow at boundary_size
  ia64/sba_iommu: Avoid overflow at boundary_size
  s390/pci_dma: Avoid overflow at boundary_size
  sparc: Avoid overflow at boundary_size
  x86/amd_gart: Avoid overflow at boundary_size
  parisc: Avoid overflow at boundary_size

 arch/alpha/kernel/pci_iommu.c| 10 --
 arch/ia64/hp/common/sba_iommu.c  |  4 ++--
 arch/powerpc/kernel/iommu.c  | 11 +--
 arch/s390/pci/pci_dma.c  |  4 ++--
 arch/sparc/kernel/iommu-common.c |  9 +++--
 arch/sparc/kernel/iommu.c|  4 ++--
 arch/sparc/kernel/pci_sun4v.c|  4 ++--
 arch/x86/kernel/amd_gart_64.c|  4 ++--
 drivers/parisc/ccio-dma.c|  4 ++--
 drivers/parisc/sba_iommu.c   |  4 ++--
 10 files changed, 26 insertions(+), 32 deletions(-)

-- 
2.17.1



[RESEND][PATCH 3/7] ia64/sba_iommu: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/ia64/hp/common/sba_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/ia64/hp/common/sba_iommu.c b/arch/ia64/hp/common/sba_iommu.c
index 656a4888c300..945954903bb0 100644
--- a/arch/ia64/hp/common/sba_iommu.c
+++ b/arch/ia64/hp/common/sba_iommu.c
@@ -485,8 +485,8 @@ sba_search_bitmap(struct ioc *ioc, struct device *dev,
ASSERT(((unsigned long) ioc->res_hint & (sizeof(unsigned long) - 1UL)) 
== 0);
ASSERT(res_ptr < res_end);
 
-   boundary_size = (unsigned long long)dma_get_seg_boundary(dev) + 1;
-   boundary_size = ALIGN(boundary_size, 1ULL << iovp_shift) >> iovp_shift;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (dma_get_seg_boundary(dev) >> iovp_shift) + 1;
 
BUG_ON(ioc->ibase & ~iovp_mask);
shift = ioc->ibase >> iovp_shift;
-- 
2.17.1



[RESEND][PATCH 1/7] powerpc/iommu: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Reported-by: Stephen Rothwell 
Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/powerpc/kernel/iommu.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 9704f3f76e63..c01ccbf8afdd 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -236,15 +236,14 @@ static unsigned long iommu_range_alloc(struct device *dev,
}
}
 
-   if (dev)
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << tbl->it_page_shift);
-   else
-   boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
/* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
+   boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
+
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
 
n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
-boundary_size >> tbl->it_page_shift, align_mask);
+boundary_size, align_mask);
if (n == -1) {
if (likely(pass == 0)) {
/* First try the pool from the start */
-- 
2.17.1



[RESEND][PATCH 2/7] alpha: Avoid overflow at boundary_size

2020-08-31 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So "+ 1" would
potentially overflow.

Also, by following other places in the kernel, boundary_size
should align with the PAGE_SIZE before right shifting by the
PAGE_SHIFT. However, passing it to ALIGN() would potentially
overflow too.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/alpha/kernel/pci_iommu.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/alpha/kernel/pci_iommu.c b/arch/alpha/kernel/pci_iommu.c
index 81037907268d..1ef2c647bd3e 100644
--- a/arch/alpha/kernel/pci_iommu.c
+++ b/arch/alpha/kernel/pci_iommu.c
@@ -141,12 +141,10 @@ iommu_arena_find_pages(struct device *dev, struct 
pci_iommu_arena *arena,
unsigned long boundary_size;
 
base = arena->dma_base >> PAGE_SHIFT;
-   if (dev) {
-   boundary_size = dma_get_seg_boundary(dev) + 1;
-   boundary_size >>= PAGE_SHIFT;
-   } else {
-   boundary_size = 1UL << (32 - PAGE_SHIFT);
-   }
+
+   boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (boundary_size >> PAGE_SHIFT) + 1;
 
/* Search forward for the first mask-aligned sequence of N free ptes */
ptes = arena->ptes;
-- 
2.17.1



Re: [RFT][PATCH 0/7] Avoid overflow at boundary_size

2020-08-25 Thread Nicolin Chen
Hi Niklas,

On Tue, Aug 25, 2020 at 12:16:27PM +0200, Niklas Schnelle wrote:
> On 8/21/20 1:19 AM, Nicolin Chen wrote:
> > We are expending the default DMA segmentation boundary to its
> > possible maximum value (ULONG_MAX) to indicate that a device
> > doesn't specify a boundary limit. So all dma_get_seg_boundary
> > callers should take a precaution with the return values since
> > it would easily get overflowed.
> > 
> > I scanned the entire kernel tree for all the existing callers
> > and found that most of callers may get overflowed in two ways:
> > either "+ 1" or passing it to ALIGN() that does "+ mask".
> > 
> > According to kernel defines:
> > #define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
> > #define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
> > 
> > We can simplify the logic here:
> >   ALIGN(boundary + 1, 1 << shift) >> shift
> > = ALIGN_MASK(b + 1, (1 << s) - 1) >> s
> > = {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
> > = [b + 1 + (1 << s) - 1] >> s
> > = [b + (1 << s)] >> s
> > = (b >> s) + 1
> > 
> > So this series of patches fix the potential overflow with this
> > overflow-free shortcut.
 
> haven't seen any other feedback from other maintainers,

I am wondering this too...whether I sent correctly or not.

> so I guess you will resend this?

Do I need to? Though I won't mind doing so if it's necessary..

> On first glance it seems to make sense.
> I'm a little confused why it is only a "potential overflow"
> while this part
> 
> "We are expending the default DMA segmentation boundary to its
>  possible maximum value (ULONG_MAX) to indicate that a device
>  doesn't specify a boundary limit"
> 
> sounds to me like ULONG_MAX is actually used, does that
> mean there are currently no devices which do not specify a
> boundary limit?

Sorry for the confusion. We actually applied ULONG_MAX change
last week but reverted it right after, due to a bug report at
one of these "potential" overflows. So at this moment the top
of the tree doesn't set default boundary to ULONG_MAX yet.

Thanks
Nic


[RFT][PATCH 1/7] powerpc/iommu: Avoid overflow at boundary_size

2020-08-20 Thread Nicolin Chen
The boundary_size might be as large as ULONG_MAX, which means
that a device has no specific boundary limit. So either "+ 1"
or passing it to ALIGN() would potentially overflow.

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So fixing a potential overflow with the safer shortcut.

Reported-by: Stephen Rothwell 
Signed-off-by: Nicolin Chen 
Cc: Christoph Hellwig 
---
 arch/powerpc/kernel/iommu.c | 11 +--
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index 9704f3f76e63..c01ccbf8afdd 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -236,15 +236,14 @@ static unsigned long iommu_range_alloc(struct device *dev,
}
}
 
-   if (dev)
-   boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
- 1 << tbl->it_page_shift);
-   else
-   boundary_size = ALIGN(1UL << 32, 1 << tbl->it_page_shift);
/* 4GB boundary for iseries_hv_alloc and iseries_hv_map */
+   boundary_size = dev ? dma_get_seg_boundary(dev) : U32_MAX;
+
+   /* Overflow-free shortcut for: ALIGN(b + 1, 1 << s) >> s */
+   boundary_size = (boundary_size >> tbl->it_page_shift) + 1;
 
n = iommu_area_alloc(tbl->it_map, limit, start, npages, tbl->it_offset,
-boundary_size >> tbl->it_page_shift, align_mask);
+boundary_size, align_mask);
if (n == -1) {
if (likely(pass == 0)) {
/* First try the pool from the start */
-- 
2.17.1



[RFT][PATCH 0/7] Avoid overflow at boundary_size

2020-08-20 Thread Nicolin Chen
We are expending the default DMA segmentation boundary to its
possible maximum value (ULONG_MAX) to indicate that a device
doesn't specify a boundary limit. So all dma_get_seg_boundary
callers should take a precaution with the return values since
it would easily get overflowed.

I scanned the entire kernel tree for all the existing callers
and found that most of callers may get overflowed in two ways:
either "+ 1" or passing it to ALIGN() that does "+ mask".

According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)

We can simplify the logic here:
  ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1

So this series of patches fix the potential overflow with this
overflow-free shortcut.

As I don't think that I have these platforms, marking RFT.

Thanks
Nic

Nicolin Chen (7):
  powerpc/iommu: Avoid overflow at boundary_size
  alpha: Avoid overflow at boundary_size
  ia64/sba_iommu: Avoid overflow at boundary_size
  s390/pci_dma: Avoid overflow at boundary_size
  sparc: Avoid overflow at boundary_size
  x86/amd_gart: Avoid overflow at boundary_size
  parisc: Avoid overflow at boundary_size

 arch/alpha/kernel/pci_iommu.c| 10 --
 arch/ia64/hp/common/sba_iommu.c  |  4 ++--
 arch/powerpc/kernel/iommu.c  | 11 +--
 arch/s390/pci/pci_dma.c  |  4 ++--
 arch/sparc/kernel/iommu-common.c |  9 +++--
 arch/sparc/kernel/iommu.c|  4 ++--
 arch/sparc/kernel/pci_sun4v.c|  4 ++--
 arch/x86/kernel/amd_gart_64.c|  4 ++--
 drivers/parisc/ccio-dma.c|  4 ++--
 drivers/parisc/sba_iommu.c   |  4 ++--
 10 files changed, 26 insertions(+), 32 deletions(-)

-- 
2.17.1



Re: [PATCH v2] ASoC: fsl-asoc-card: Get "extal" clock rate by clk_get_rate

2020-08-10 Thread Nicolin Chen
On Mon, Aug 10, 2020 at 04:11:43PM +0800, Shengjiu Wang wrote:
> On some platform(.e.g. i.MX8QM MEK), the "extal" clock is different
> with the mclk of codec, then the clock rate is also different.
> So it is better to get clock rate of "extal" rate by clk_get_rate,
> don't reuse the clock rate of mclk.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_sai: Clean code for synchronize mode

2020-08-04 Thread Nicolin Chen
On Tue, Aug 04, 2020 at 12:03:46AM -0700, Nicolin Chen wrote:
> On Tue, Aug 04, 2020 at 12:22:53PM +0800, Shengjiu Wang wrote:
> 
> > > > Btw, do we need similar change for TRIGGER_STOP?
> > >
> > > This is a good question. It is better to do change for STOP,
> > > but I am afraid that there is no much test to guarantee the result.
> 
> > Maybe we can do this change for STOP.
> 
> The idea looks good to me...(check inline comments)
>  
> > diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> > index 1c0e06bb3783..6e4be398eaee 100644
> > --- a/sound/soc/fsl/fsl_sai.c
> > +++ b/sound/soc/fsl/fsl_sai.c
> > @@ -517,6 +517,37 @@ static int fsl_sai_hw_free(struct
> > snd_pcm_substream *substream,
> > return 0;
> >  }
> > 
> > +static void fsl_sai_config_disable(struct fsl_sai *sai, bool tx)
> > +{
> > +   unsigned int ofs = sai->soc_data->reg_offset;
> > +   u32 xcsr, count = 100;
> > +
> > +   regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> > +  FSL_SAI_CSR_TERE, 0);
> > +
> > +   /* TERE will remain set till the end of current frame */
> > +   do {
> > +   udelay(10);
> > +   regmap_read(sai->regmap, FSL_SAI_xCSR(tx, ofs), );
> > +   } while (--count && xcsr & FSL_SAI_CSR_TERE);
> > +
> > +   regmap_update_bits(sai->regmap, FSL_SAI_xCSR(tx, ofs),
> > +  FSL_SAI_CSR_FR, FSL_SAI_CSR_FR);
> > +
> > +   /*
> > +* For sai master mode, after several open/close sai,
> > +* there will be no frame clock, and can't recover
> > +* anymore. Add software reset to fix this issue.
> > +* This is a hardware bug, and will be fix in the
> > +* next sai version.
> > +*/
> > +   if (!sai->is_slave_mode) {
> > +   /* Software Reset for both Tx and Rx */
> 
> Remove "for both Tx and Rx"
> 
> > /* Check if the opposite FRDE is also disabled */
> > regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), );
> > +   if (sai->synchronous[tx] && !sai->synchronous[!tx] && 
> > !(xcsr & FSL_SAI_CSR_FRDE))
> > +   fsl_sai_config_disable(sai, !tx);
> 
> > +   if (sai->synchronous[tx] || !sai->synchronous[!tx] || 
> > !(xcsr & FSL_SAI_CSR_FRDE))
> > +   fsl_sai_config_disable(sai, tx);
> 
> The first "||" should probably be "&&".
> 
> The trigger() logic is way more complicated than a simple cleanup
> in my opinion. Would suggest to split RMR part out of this change.
> 
> And for conditions like "sync[tx] && !sync[!tx]", it'd be better
> to have a helper function to improve readability:
> 
> /**
>  * fsl_sai_dir_is_synced - Check if stream is synced by the opposite stream
>  *
>  * SAI supports synchronous mode using bit/frame clocks of either 
> Transmitter's
>  * or Receiver's for both streams. This function is used to check if clocks of
>  * current stream's are synced by the opposite stream.

Aww..this should be a generic function, so drop "current":

  * or Receiver's for both streams. This function is used to check if clocks of
  * the stream's are synced by the opposite stream.

>  *
>  * @sai: SAI context
>  * @dir: direction of current stream

Ditto:
   * @dir: stream direction

Thanks.
-
   
>  */
> static inline bool fsl_sai_dir_is_synced(struct fsl_sai *sai, int dir)
> {
>   int adir = (dir == TX) ? RX : TX;
> 
>   /* current dir in async mode while opposite dir in sync mode */
>   return !sai->synchronous[dir] && sai->synchronous[adir];
> }
> 
> Then add more comments in trigger:
> 
> static ...trigger()
> {
>   bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
>   int adir = tx ? RX : TX;
>   int dir = tx ? TX : RX;
> 
>   // 
>   {
>   // ...
> 
>   /* Check if the opposite FRDE is also disabled */
>   regmap_read(sai->regmap, FSL_SAI_xCSR(!tx, ofs), );
> 
>   /*
>* If opposite stream provides clocks for synchronous mode and
>* it is inactive, disable it before disabling the current one
>*/
>   if (fsl_sai_dir_is_synced(adir) && !(xcsr & FSL_SAI_CSR_FRDE))
>   fsl_sai_config_disable(sai, adir);
> 
>   /*
>* Disable current stream if either of:
>* 1. current stream doesn't provide clocks for synchronous mode
>* 2. current stream provides clocks for synchronous mode but no
>*more stream is active.
>*/
>   if (!fsl_sai_dir_is_synced(dir) || !(xcsr & FSL_SAI_CSR_FRDE))
>   fsl_sai_config_disable(sai, dir);
> 
>   // ...
>   }
>   // 
> }
> 
> Note, in fsl_sai_config_disable(sai, dir):
>   bool tx = dir == TX;
> 
> Above all, I am just drafting, so please test it thoroughly :)


Re: [PATCH 0/4] ASoC: fsl_asrc: allow selecting arbitrary clocks

2020-07-22 Thread Nicolin Chen
On Fri, Jul 17, 2020 at 01:16:42PM +0200, Arnaud Ferraris wrote:
> Hi Nic,
> 
> Le 02/07/2020 à 20:42, Nicolin Chen a écrit :
> > Hi Arnaud,
> > 
> > On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
> >> The current ASRC driver hardcodes the input and output clocks used for
> >> sample rate conversions. In order to allow greater flexibility and to
> >> cover more use cases, it would be preferable to select the clocks using
> >> device-tree properties.
> > 
> > We recent just merged a new change that auto-selecting internal
> > clocks based on sample rates as the first option -- ideal ratio
> > mode is the fallback mode now. Please refer to:
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702=d0250cf4f2abfbea64ed247230f08f5ae23979f0
> 
> While working on fixing the automatic clock selection (see my v3), I
> came across another potential issue, which would be better explained
> with an example:
>   - Input has sample rate 8kHz and uses clock SSI1 with rate 512kHz
>   - Output has sample rate 16kHz and uses clock SSI2 with rate 1024kHz
> 
> Let's say my v3 patch is merged, then the selected input clock will be
> SSI1, while the selected output clock will be SSI2. In that case, it's
> all good, as the driver will calculate the dividers right.
> 
> Now, suppose a similar board has the input wired to SSI2 and output to
> SSI1, meaning we're now in the following case:
>   - Input has sample rate 8kHz and uses clock SSI2 with rate 512kHz
>   - Output has sample rate 16kHz and uses clock SSI1 with rate 1024kHz
> (the same result is achieved during capture with the initial example
> setup, as input and output properties are then swapped)
> 
> In that case, the selected clocks will still be SSI1 for input (just
> because it appears first in the clock table), and SSI2 for output,
> meaning the calculated dividers will be:
>   - input: 512 / 16 => 32 (should be 64)
>   - output: 1024 / 8 => 128 (should be 64 here too)

I don't get the 32, 128 and 64 parts. Would you please to elaborate
a bit? What you said sounds to me like the driver calculates wrong
dividers?


Re: [PATCH v2 1/1] ASoC: fsl: fsl-asoc-card: Trivial: Fix misspelling of 'exists'

2020-07-15 Thread Nicolin Chen
On Wed, Jul 15, 2020 at 04:00:09PM +0100, Lee Jones wrote:
> Cc: Timur Tabi 
> Cc: Nicolin Chen 

Acked-by: Nicolin Chen 

> Cc: Xiubo Li 
> Cc: Fabio Estevam 
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Lee Jones 
> ---
>  sound/soc/fsl/fsl-asoc-card.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
> index faac6ce9a82cb..dbacdd25dfe76 100644
> --- a/sound/soc/fsl/fsl-asoc-card.c
> +++ b/sound/soc/fsl/fsl-asoc-card.c
> @@ -92,7 +92,7 @@ struct fsl_asoc_card_priv {
>  };
>  
>  /*
> - * This dapm route map exits for DPCM link only.
> + * This dapm route map exists for DPCM link only.
>   * The other routes shall go through Device Tree.
>   *
>   * Note: keep all ASRC routes in the second half
> -- 
> 2.25.1
> 


Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone Jack detection

2020-07-15 Thread Nicolin Chen
On Wed, Jul 15, 2020 at 12:14:01PM +0800, Shengjiu Wang wrote:
> On Wed, Jul 15, 2020 at 5:16 AM Nicolin Chen  wrote:
> >
> > Hi Shengjiu,
> >
> > The whole series looks good to me. Just a couple of small
> > questions inline:
> >
> > On Tue, Jul 14, 2020 at 05:05:36PM +0800, Shengjiu Wang wrote:
> > > Use asoc_simple_init_jack function from simple card to implement
> > > the Headphone and Microphone detection.
> > > Register notifier to disable Speaker when Headphone is plugged in
> > > and enable Speaker when Headphone is unplugged.
> > > Register notifier to disable Digital Microphone when Analog Microphone
> > > is plugged in and enable DMIC when Analog Microphone is unplugged.
> > >
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > >  sound/soc/fsl/Kconfig |  1 +
> > >  sound/soc/fsl/fsl-asoc-card.c | 69 ++-
> > >  2 files changed, 68 insertions(+), 2 deletions(-)
> >
> > >  static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
> > >  {
> > >   struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card);
> > > @@ -745,8 +789,29 @@ static int fsl_asoc_card_probe(struct 
> > > platform_device *pdev)
> > >   snd_soc_card_set_drvdata(>card, priv);
> > >
> > >   ret = devm_snd_soc_register_card(>dev, >card);
> > > - if (ret && ret != -EPROBE_DEFER)
> > > - dev_err(>dev, "snd_soc_register_card failed (%d)\n", 
> > > ret);
> > > + if (ret) {
> > > + if (ret != -EPROBE_DEFER)
> > > + dev_err(>dev, "snd_soc_register_card failed 
> > > (%d)\n", ret);
> >
> > I think we may move this EPROBE_DEFER to the asrc_fail label.
> 
> If we move this to asrc_fail label, then it will be hard to define the
> error message.
> There are many places that goto asrc_fail.

Oh...good point...

> > > + goto asrc_fail;
> > > + }
> > > +
> > > + if (of_property_read_bool(np, "hp-det-gpio")) {
> >
> > Could we move this check inside asoc_simple_init_jack? There's no
> > problem with doing it here though, yet I got a bit confused by it
> > as I thought it's a boolean type property, which would be against
> > the DT bindings until I saw asoc_simple_init_jack() uses the same
> > string to get the GPIO. Just it probably would be a bit tricky as
> > we need it to be optional here.
> >
> > Otherwise, I think we may add a line of comments to indicate that
> > the API would use the same string to get the GPIO.
> 
> In asoc_simple_init_jack, gpio_is_valid() will be invalid when there is
> no "hp-det-gpio" property, and asoc_simple_init_jack will return 0.
> 
> The reason why I add a check here is mostly for
> snd_soc_jack_notifier_register().
> when there is no jack created, there will be a kernel dump.
> 
> or I can use this code:
> 
> -   if (of_property_read_bool(np, "hp-det-gpio")) {
> -   ret = asoc_simple_init_jack(>card, >hp_jack,
> -   1, NULL, "Headphone Jack");
> -   if (ret)
> -   goto asrc_fail;
> +   ret = asoc_simple_init_jack(>card, >hp_jack,
> +   1, NULL, "Headphone Jack");
> +   if (ret)
> +   goto asrc_fail;
> 
> +   if (priv->hp_jack.jack.jack)
> snd_soc_jack_notifier_register(>hp_jack.jack,

It's pretty clean but not very obvious for the "optional" part.
So I think that it'd be slightly better to go for your previous
solution, but with a line of comments to show: these properties
are optional and asoc_simple_init_jack() uses the same strings.

Please add to all three changes once the comments being added:

Acked-by: Nicolin Chen 

Thanks


Re: [PATCH 3/3] ASoC: fsl-asoc-card: Support Headphone and Microphone Jack detection

2020-07-14 Thread Nicolin Chen
Hi Shengjiu,

The whole series looks good to me. Just a couple of small
questions inline:

On Tue, Jul 14, 2020 at 05:05:36PM +0800, Shengjiu Wang wrote:
> Use asoc_simple_init_jack function from simple card to implement
> the Headphone and Microphone detection.
> Register notifier to disable Speaker when Headphone is plugged in
> and enable Speaker when Headphone is unplugged.
> Register notifier to disable Digital Microphone when Analog Microphone
> is plugged in and enable DMIC when Analog Microphone is unplugged.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/Kconfig |  1 +
>  sound/soc/fsl/fsl-asoc-card.c | 69 ++-
>  2 files changed, 68 insertions(+), 2 deletions(-)

>  static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
>  {
>   struct fsl_asoc_card_priv *priv = snd_soc_card_get_drvdata(card);
> @@ -745,8 +789,29 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   snd_soc_card_set_drvdata(>card, priv);
>  
>   ret = devm_snd_soc_register_card(>dev, >card);
> - if (ret && ret != -EPROBE_DEFER)
> - dev_err(>dev, "snd_soc_register_card failed (%d)\n", ret);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(>dev, "snd_soc_register_card failed 
> (%d)\n", ret);

I think we may move this EPROBE_DEFER to the asrc_fail label.

> + goto asrc_fail;
> + }
> +
> + if (of_property_read_bool(np, "hp-det-gpio")) {

Could we move this check inside asoc_simple_init_jack? There's no
problem with doing it here though, yet I got a bit confused by it
as I thought it's a boolean type property, which would be against
the DT bindings until I saw asoc_simple_init_jack() uses the same
string to get the GPIO. Just it probably would be a bit tricky as
we need it to be optional here.

Otherwise, I think we may add a line of comments to indicate that
the API would use the same string to get the GPIO.

> + ret = asoc_simple_init_jack(>card, >hp_jack,
> + 1, NULL, "Headphone Jack");
> + if (ret)
> + goto asrc_fail;
> +
> + snd_soc_jack_notifier_register(>hp_jack.jack, 
> _jack_nb);
> + }


[PATCH v2] MAINTAINERS: Add Shengjiu to reviewer list of sound/soc/fsl

2020-07-06 Thread Nicolin Chen
Add Shengjiu who's actively working on the latest fsl/nxp audio drivers.

Signed-off-by: Nicolin Chen 
Acked-by: Shengjiu Wang 
Reviewed-by: Fabio Estevam 
---
Changelog
v1->v2:
 * Replaced Shengjiu's emaill address with his gmail one
 * Added Acked-by from Shengjiu and Reviewed-by from Fabio

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 496fd4eafb68..ff97b8cefaea 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6956,6 +6956,7 @@ M:Timur Tabi 
 M:     Nicolin Chen 
 M: Xiubo Li 
 R: Fabio Estevam 
+R: Shengjiu Wang 
 L: alsa-de...@alsa-project.org (moderated for non-subscribers)
 L: linuxppc-dev@lists.ozlabs.org
 S: Maintained
-- 
2.17.1



Re: [PATCH v2] ASoC: fsl_asrc: Add an option to select internal ratio mode

2020-07-03 Thread Nicolin Chen
On Fri, Jul 03, 2020 at 11:50:20PM +0100, Mark Brown wrote:
> On Fri, Jul 03, 2020 at 03:46:58PM -0700, Nicolin Chen wrote:
> 
> > > [1/1] ASoC: fsl_asrc: Add an option to select internal ratio mode
> > >   commit: d0250cf4f2abfbea64ed247230f08f5ae23979f0
> 
> > You already applied v3 of this change:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/169976.html
> 
> > And it's already in linux-next also. Not sure what's happening...
> 
> The script can't always tell the difference between versions - it looks
> like it's notified for v2 based on seeing v3 in git.

OK..as long as no revert nor re-applying happens, we can ignore :)

Thanks


Re: [PATCH v2] ASoC: fsl_asrc: Add an option to select internal ratio mode

2020-07-03 Thread Nicolin Chen
Hi Mark,

On Fri, Jul 03, 2020 at 06:03:43PM +0100, Mark Brown wrote:
> On Tue, 30 Jun 2020 16:47:56 +0800, Shengjiu Wang wrote:
> > The ASRC not only supports ideal ratio mode, but also supports
> > internal ratio mode.
> > 
> > For internal rato mode, the rate of clock source should be divided
> > with no remainder by sample rate, otherwise there is sound
> > distortion.
> > 
> > [...]
> 
> Applied to
> 
>https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
> 
> Thanks!
> 
> [1/1] ASoC: fsl_asrc: Add an option to select internal ratio mode
>   commit: d0250cf4f2abfbea64ed247230f08f5ae23979f0

You already applied v3 of this change:
https://mailman.alsa-project.org/pipermail/alsa-devel/2020-July/169976.html

And it's already in linux-next also. Not sure what's happening...


[PATCH] MAINTAINERS: Add Shengjiu to reviewer list of sound/soc/fsl

2020-07-02 Thread Nicolin Chen
Add Shengjiu who's actively working on the latest fsl/nxp audio drivers.

Signed-off-by: Nicolin Chen 
Cc: Shengjiu Wang 
---
To Shengjiu, please ack if you feel okay with this (your email address too).

 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 496fd4eafb68..54aab083bb88 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6956,6 +6956,7 @@ M:Timur Tabi 
 M: Nicolin Chen 
 M: Xiubo Li 
 R: Fabio Estevam 
+R: Shengjiu Wang 
 L: alsa-de...@alsa-project.org (moderated for non-subscribers)
 L: linuxppc-dev@lists.ozlabs.org
 S: Maintained
-- 
2.17.1



Re: [PATCH 6/6] ASoC: fsl: fsl_esai: fix kernel-doc

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:27PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. Fix kernel-doc syntax and add missing parameters.
> 
> Signed-off-by: Pierre-Louis Bossart 

> + * fsl_esai_set_dai_sysclk - This function mainly configures the clock 
> frequency of MCLK (HCKT/HCKR)

Can drop "This function mainly"

>  /**
> - * This function configures the related dividers according to the bclk rate
> + * fsl_esai_set_bclk - This function configures the related dividers 
> according to the bclk rate

Here too -- dropping "This function"

Otherwise,
Acked-by: Nicolin Chen 

Thanks!


Re: [PATCH 5/6] ASoC: fsl: fsl_asrc: fix kernel-doc

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:26PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. fix kernel doc and describe arguments.
> 
> Signed-off-by: Pierre-Louis Bossart 

Acked-by: Nicolin Chen 


Re: [PATCH 4/6] ASoC: fsl: fsl_spdif: fix kernel-doc

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:25PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. kernel-doc syntax was not followed and missing parameter
> 
> Signed-off-by: Pierre-Louis Bossart 
 
Acked-by: Nicolin Chen 


Re: [PATCH 3/6] ASoC: fsl: fsl-asoc-card: fix kernel-doc

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:24PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. Kernel-doc syntax was not properly used.
> 
> Signed-off-by: Pierre-Louis Bossart 
> ---
>  sound/soc/fsl/fsl-asoc-card.c | 21 +
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c

>  /**
> - * Freescale Generic ASOC card private data
> + * struct fsl_asoc_card_priv - struct Freescale Generic ASOC card private 
> data

Just a nit, can you drop the "struct" before "Freescale"?
Other parts of your changes don't really add this word.

Otherwise,
Acked-by: Nicolin Chen 


Re: [PATCH 2/6] ASoC: fsl: fsl_ssi: fix kernel-doc

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:23PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. The kernel-doc support is partial, add more
> descriptions and follow proper syntax
> 
> Signed-off-by: Pierre-Louis Bossart 

Acked-by: Nicolin Chen 


Re: [PATCH 1/6] ASoC: fsl: fsl_ssi_dbg: remove spurious kernel-doc comment start

2020-07-02 Thread Nicolin Chen
On Thu, Jul 02, 2020 at 12:22:22PM -0500, Pierre-Louis Bossart wrote:
> Fix W=1 warnings. There is no kernel-doc here.
> 
> Signed-off-by: Pierre-Louis Bossart 

Acked-by: Nicolin Chen 


Re: [PATCH 0/4] ASoC: fsl_asrc: allow selecting arbitrary clocks

2020-07-02 Thread Nicolin Chen
Hi Arnaud,

On Thu, Jul 02, 2020 at 04:22:31PM +0200, Arnaud Ferraris wrote:
> The current ASRC driver hardcodes the input and output clocks used for
> sample rate conversions. In order to allow greater flexibility and to
> cover more use cases, it would be preferable to select the clocks using
> device-tree properties.

We recent just merged a new change that auto-selecting internal
clocks based on sample rates as the first option -- ideal ratio
mode is the fallback mode now. Please refer to:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20200702=d0250cf4f2abfbea64ed247230f08f5ae23979f0

Having a quick review at your changes, I think the DT part may
not be necessary as it's more likely a software configuration.
I personally like the new auto-selecting solution more.

> This series also fix register configuration and clock assignment so
> conversion can be conducted effectively in both directions with a good
> quality.

If there's any further change that you feel you can improve on
the top of mentioned change after rebasing, I'd like to review.

Thanks
Nic


Re: [PATCH v3] ASoC: fsl_asrc: Add an option to select internal ratio mode

2020-07-01 Thread Nicolin Chen
On Tue, Jun 30, 2020 at 09:56:07PM +0800, Shengjiu Wang wrote:
> The ASRC not only supports ideal ratio mode, but also supports
> internal ratio mode.
> 
> For internal rato mode, the rate of clock source should be divided
> with no remainder by sample rate, otherwise there is sound
> distortion.
> 
> Add function fsl_asrc_select_clk() to find proper clock source for
> internal ratio mode, if the clock source is available then internal
> ratio mode will be selected.
> 
> With change, the ideal ratio mode is not the only option for user.
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_asrc: Add an option to select internal ratio mode

2020-06-29 Thread Nicolin Chen
On Mon, Jun 29, 2020 at 09:58:35PM +0800, Shengjiu Wang wrote:
> The ASRC not only supports ideal ratio mode, but also supports
> internal ratio mode.
> 
> For internal rato mode, the rate of clock source should be divided
> with no remainder by sample rate, otherwise there is sound
> distortion.
> 
> Add function fsl_asrc_select_clk() to find proper clock source for
> internal ratio mode, if the clock source is available then internal
> ratio mode will be selected.
> 
> With change, the ideal ratio mode is not the only option for user.
> 
> Signed-off-by: Shengjiu Wang 
> ---

> +static int fsl_asrc_select_clk(struct fsl_asrc_priv *asrc_priv,
> +struct fsl_asrc_pair *pair,
> +int in_rate,
> +int out_rate)
> +{
> + struct fsl_asrc_pair_priv *pair_priv = pair->private;
> + struct asrc_config *config = pair_priv->config;
> + int rate[2], select_clk[2]; /* Array size 2 means IN and OUT */
> + int clk_rate, clk_index;
> + int i = 0, j = 0;
> + bool clk_sel[2];
> +
> + rate[0] = in_rate;
> + rate[1] = out_rate;
> +
> + /* Select proper clock source for internal ratio mode */
> + for (j = 0; j < 2; j++) {
> + for (i = 0; i < ASRC_CLK_MAP_LEN; i++) {
> + clk_index = asrc_priv->clk_map[j][i];
> + clk_rate = 
> clk_get_rate(asrc_priv->asrck_clk[clk_index]);

+   /* Only match a perfect clock source with no remainder 
*/

> + if (clk_rate != 0 && (clk_rate / rate[j]) <= 1024 &&
> + (clk_rate % rate[j]) == 0)
> + break;
> + }
> +
> + if (i == ASRC_CLK_MAP_LEN) {
> + select_clk[j] = OUTCLK_ASRCK1_CLK;
> + clk_sel[j] = false;
> + } else {
> + select_clk[j] = i;
> + clk_sel[j] = true;
> + }
> + }
> +
> + /* Switch to ideal ratio mode if there is no proper clock source */
> + if (!clk_sel[IN] || !clk_sel[OUT])
> + select_clk[IN] = INCLK_NONE;

Could get rid of clk_set:

for (j) {
for (i) {
if (match)
break;
}

clk[j] = i;
}

if (clk[IN] == ASRC_CLK_MAP_LEN || clk[OUT] == ASRC_CLK_MAP_LEN)

And it only overrides clk[IN] setting but leaving clk[OUT] to
to the searching result. This means that clk[OUT] may be using
a clock source other than OUTCLK_ASRCK1_CLK if sel[IN] happens
to be false while sel[OUT] happens to be true. Not sure if it
is intended...but I feel it would probably be safer to use the
previous settings: INCLK_NONE + OUTCLK_ASRCK1_CLK?


Re: [PATCH] ASoC: fsl_sai: Refine regcache usage with pm runtime

2020-06-29 Thread Nicolin Chen
On Mon, Jun 29, 2020 at 02:42:33PM +0800, Shengjiu Wang wrote:
> When there is dedicated power domain bound with device, after probing
> the power will be disabled, then registers are not accessible in
> fsl_sai_dai_probe(), so regcache only need to be enabled in end of
> probe() and regcache_mark_dirty should be moved to pm runtime resume
> callback function.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH 2/2] ASoC: bindings: fsl-asoc-card: Add compatible string for wm8524

2020-06-23 Thread Nicolin Chen
On Tue, Jun 23, 2020 at 02:52:47PM +0800, Shengjiu Wang wrote:
> In order to support wm8524 codec with fsl-asoc-card machine
> driver, add compatible string "fsl,imx-audio-wm8524".
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH 1/2] ASoC: fsl-asoc-card: Add WM8524 support

2020-06-23 Thread Nicolin Chen
On Tue, Jun 23, 2020 at 02:52:46PM +0800, Shengjiu Wang wrote:
> WM8524 only supports playback mode, and only works at
> slave mode.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH v2 2/2] ASoC: fsl_mqs: Fix unchecked return value for clk_prepare_enable

2020-06-23 Thread Nicolin Chen
On Tue, Jun 23, 2020 at 02:01:12PM +0800, Shengjiu Wang wrote:
> Fix unchecked return value for clk_prepare_enable, add error
> handler in fsl_mqs_runtime_resume.
> 
> Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH v2 1/2] ASoC: fsl_mqs: Don't check clock is NULL before calling clk API

2020-06-23 Thread Nicolin Chen
On Tue, Jun 23, 2020 at 02:01:11PM +0800, Shengjiu Wang wrote:
> Because clk_prepare_enable and clk_disable_unprepare should
> check input clock parameter is NULL or not internally, then
> we don't need to check them before calling the function.
> 
> Fixes: 9e28f6532c61 ("ASoC: fsl_mqs: Add MQS component driver")
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_easrc: Fix uninitialized scalar variable in fsl_easrc_set_ctx_format

2020-06-22 Thread Nicolin Chen
On Mon, Jun 22, 2020 at 05:03:31PM +0800, Shengjiu Wang wrote:
> The "ret" in fsl_easrc_set_ctx_format is not initialized, then
> the unknown value maybe returned by this function.
> 
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_spdif: Add pm runtime function

2020-06-18 Thread Nicolin Chen
On Thu, Jun 18, 2020 at 07:55:34PM +0800, Shengjiu Wang wrote:
> Add pm runtime support and move clock handling there.
> Close the clocks at suspend to reduce the power consumption.
> 
> fsl_spdif_suspend is replaced by pm_runtime_force_suspend.
> fsl_spdif_resume is replaced by pm_runtime_force_resume.
> 
> Signed-off-by: Shengjiu Wang 

LGTM, yet some nits, please add my ack after fixing:

Acked-by: Nicolin Chen 

> @@ -495,25 +496,10 @@ static int fsl_spdif_startup(struct snd_pcm_substream 
> *substream,

>  
> -disable_txclk:
> - for (i--; i >= 0; i--)
> - clk_disable_unprepare(spdif_priv->txclk[i]);
>  err:
> - if (!IS_ERR(spdif_priv->spbaclk))
> - clk_disable_unprepare(spdif_priv->spbaclk);
> -err_spbaclk:
> - clk_disable_unprepare(spdif_priv->coreclk);
> -
>   return ret;

Only "return ret;" remains now. We could clean the goto away.

> -static int fsl_spdif_resume(struct device *dev)
> +static int fsl_spdif_runtime_resume(struct device *dev)

> +disable_rx_clk:
> + clk_disable_unprepare(spdif_priv->rxclk);
> +disable_tx_clk:
> +disable_spba_clk:

Why have two duplicated ones? Could probably drop the 2nd one.


Re: [PATCH v2 2/2] ASoC: fsl-asoc-card: Add MQS support

2020-06-17 Thread Nicolin Chen
On Wed, Jun 17, 2020 at 12:48:25PM +0800, Shengjiu Wang wrote:
> The MQS codec isn't an i2c device, so use of_find_device_by_node
> to get platform device pointer.
> 
> Because MQS only support playback, then add a new audio map.
> 
> And there maybe "model" property or no "audio-routing" property in
> devicetree, so add some enhancement for these two property.
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [PATCH v2 2/2] ASoC: fsl_spdif: Add support for imx6sx platform

2020-06-17 Thread Nicolin Chen
On Wed, Jun 17, 2020 at 12:30:17PM +0800, Shengjiu Wang wrote:
> The one difference on imx6sx platform is that the root clock
> is shared with ASRC module, so we add a new flags
> "shared_root_clock" which means the root clock is independent,

"shared" means "not independent", against "independent" ;)

> then we will not do the clk_set_rate and clk_round_rate to avoid
> impact ASRC module usage.
> 
> As add a new flags, we include the soc specific data struct.
> 
> Signed-off-by: Shengjiu Wang 

Can add this once fixing the remaining comments:

Reviewed-by: Nicolin Chen 

> +static inline bool fsl_spdif_can_set_clk_rate(struct fsl_spdif_priv *spdif,
> +   int clk)

Can actually merge into single line as kernel has 100-character
limit now, though 80-char is still preferable for a good coding
style. But I think this one wouldn't be too bad at all.

> @@ -421,7 +456,7 @@ static int spdif_set_sample_rate(struct snd_pcm_substream 
> *substream,
>   sysclk_df = spdif_priv->sysclk_df[rate];
>  
>   /* Don't mess up the clocks from other modules */

We can drop this comments now as it's out-of-date and the name of
the new helper function is straightforward enough.

> - if (clk != STC_TXCLK_SPDIF_ROOT)
> + if (!fsl_spdif_can_set_clk_rate(spdif_priv, clk))
>   goto clk_set_bypass;
>  
>   /* The S/PDIF block needs a clock of 64 * fs * txclk_df */


Re: [PATCH 2/2] ASoC: fsl-asoc-card: Add MQS support

2020-06-16 Thread Nicolin Chen
On Tue, Jun 16, 2020 at 03:30:37PM +0800, Shengjiu Wang wrote:
> The MQS codec isn't an i2c device, so add a new platform device for it.
> 
> MQS only support playback, so add a new audio map.
> 
> Add there maybe "model" property or no "audio-routing" property insertions

"Add" => "And"

> devicetree, so add some enhancement for these two property.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl-asoc-card.c | 70 ++-
>  1 file changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
> index 00be73900888..2ac8cb9ddd10 100644
> --- a/sound/soc/fsl/fsl-asoc-card.c
> +++ b/sound/soc/fsl/fsl-asoc-card.c

> @@ -482,6 +489,7 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>  {
>   struct device_node *cpu_np, *codec_np, *asrc_np;
>   struct device_node *np = pdev->dev.of_node;
> + struct platform_device *codec_pdev = NULL; /* used for non i2c device*/

Having both codec_pdev and codec_dev duplicates things. Actually
only a couple of places really need "codec_dev" -- most of them
need codec_dev->dev pointer instead. So we could have a cleanup:

-   struct i2c_client *codec_dev;
+   struct device *codec_dev = NULL;

> @@ -512,10 +520,13 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   }
>  
>   codec_np = of_parse_phandle(np, "audio-codec", 0);
> - if (codec_np)
> + if (codec_np) {
>   codec_dev = of_find_i2c_device_by_node(codec_np);
> - else
> + if (!codec_dev)
> + codec_pdev = of_find_device_by_node(codec_np);
> + } else {
>   codec_dev = NULL;
> + }

Here can have something like (feel free to simplify):

if (codec_np) {
struct platform_device *codec_pdev;
struct i2c_client *codec_i2c;

codec_i2c = of_find_i2c_device_by_node(codec_np);
if (codec_i2c)
codec_dev = _i2c->dev;

if (!codec_dev) {
codec_pdev = of_find_device_by_node(codec_np);
codec_dev = _pdev->dev;
}
}
>   asrc_np = of_parse_phandle(np, "audio-asrc", 0);
>   if (asrc_np)
> @@ -525,6 +536,13 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   if (codec_dev) {
>   struct clk *codec_clk = clk_get(_dev->dev, NULL);

Then here:

-   struct clk *codec_clk = clk_get(_dev->dev, NULL);
+   struct clk *codec_clk = clk_get(codec_dev, NULL);

> @@ -538,6 +556,11 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   /* Assign a default DAI format, and allow each card to overwrite it */
>   priv->dai_fmt = DAI_FMT_BASE;
>  
> + memcpy(priv->dai_link, fsl_asoc_card_dai,
> +sizeof(struct snd_soc_dai_link) * ARRAY_SIZE(priv->dai_link));

> @@ -573,13 +596,25 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   codec_dai_name = "ac97-hifi";
>   priv->card.set_bias_level = NULL;
>   priv->dai_fmt = SND_SOC_DAIFMT_AC97;
> + priv->card.dapm_routes = audio_map_ac97;
> + priv->card.num_dapm_routes = ARRAY_SIZE(audio_map_ac97);
> + } else if (of_device_is_compatible(np, "fsl,imx-audio-mqs")) {
> + codec_dai_name = "fsl-mqs-dai";
> + priv->card.set_bias_level = NULL;
> + priv->dai_fmt = SND_SOC_DAIFMT_LEFT_J |
> + SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF;
> + priv->dai_link[1].dpcm_playback = 1;
> + priv->dai_link[2].dpcm_playback = 1;

dpcm_playback = 1? That's the default value in fsl_asoc_card_dai.

> @@ -601,19 +636,18 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   priv->cpu_priv.sysclk_id[0] = FSL_SAI_CLK_MAST1;
>   }
>  
> - snprintf(priv->name, sizeof(priv->name), "%s-audio",
> -  fsl_asoc_card_is_ac97(priv) ? "ac97" :
> -  codec_dev->name);
> -
>   /* Initialize sound card */
>   priv->pdev = pdev;
>   priv->card.dev = >dev;
> - priv->card.name = priv->name;
> + ret = snd_soc_of_parse_card_name(>card, "model");
> + if (ret) {
> + snprintf(priv->name, sizeof(priv->name), "%s-audio",
> +  fsl_asoc_card_is_ac97(priv) ? "ac97" :
> +  (codec_dev ? codec_dev->name : codec_pdev->name));

We can just use dev_name() if codec_dev is struct device *
Or having a codec_dev_name to cache codec_pdev/i2c->name.

> - ret = snd_soc_of_parse_audio_routing(>card, "audio-routing");
> - if (ret) {
> - dev_err(>dev, "failed to parse audio-routing: %d\n", ret);
> - goto asrc_fail;
> + if (of_property_read_bool(np, "audio-routing")) {
> + ret = 

Re: [PATCH 2/2] ASoC: fsl_spdif: Add support for imx6sx platform

2020-06-16 Thread Nicolin Chen
On Tue, Jun 16, 2020 at 02:42:41PM +0800, Shengjiu Wang wrote:
> The one difference on imx6sx platform is that the root clock
> is shared with ASRC module, so we add a new flags "ind_root_clk"
> which means the root clock is independent, then we will not
> do the clk_set_rate and clk_round_rate to avoid impact ASRC
> module usage.
> 
> As add a new flags, we include the soc specific data struct.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_spdif.c | 43 +++
>  1 file changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
> index 1b2e516f9162..00e06803d32f 100644
> --- a/sound/soc/fsl/fsl_spdif.c
> +++ b/sound/soc/fsl/fsl_spdif.c
> @@ -42,6 +42,17 @@ static u8 srpc_dpll_locked[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 
> 0xa, 0xb };
>  
>  #define DEFAULT_RXCLK_SRC1
>  
> +/**
> + * struct fsl_spdif_soc_data: soc specific data
> + *
> + * @imx: for imx platform
> + * @ind_root_clk: flag for round clk rate
> + */
> +struct fsl_spdif_soc_data {
> + bool imx;
> + bool ind_root_clk;

"ind" doesn't look very straightforward; maybe "shared_root_clock"?

And for its comments:
* @shared_root_clock: flag of sharing a clock source with others;
* so the driver shouldn't set root clock rate

> +};
> +
>  /*
>   * SPDIF control structure
>   * Defines channel status, subcode and Q sub
> @@ -93,6 +104,7 @@ struct fsl_spdif_priv {
>   struct snd_soc_dai_driver cpu_dai_drv;
>   struct platform_device *pdev;
>   struct regmap *regmap;
> + const struct fsl_spdif_soc_data *soc;

Looks better if we move it to the top of the list :)

> @@ -421,7 +448,7 @@ static int spdif_set_sample_rate(struct snd_pcm_substream 
> *substream,
>   sysclk_df = spdif_priv->sysclk_df[rate];
>  
>   /* Don't mess up the clocks from other modules */
> - if (clk != STC_TXCLK_SPDIF_ROOT)
> + if (clk != STC_TXCLK_SPDIF_ROOT || !spdif_priv->soc->ind_root_clk)
>   goto clk_set_bypass;
>  
>   /* The S/PDIF block needs a clock of 64 * fs * txclk_df */
> @@ -1186,7 +1213,8 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv 
> *spdif_priv,
>   continue;
>  
>   ret = fsl_spdif_txclk_caldiv(spdif_priv, clk, savesub, index,
> -  i == STC_TXCLK_SPDIF_ROOT);
> +  i == STC_TXCLK_SPDIF_ROOT &&
> +  spdif_priv->soc->ind_root_clk);

Having more than one place that checks the condition, we can add:

/* Check if clk is a root clock that does not share clock source with others */
static inline bool fsl_spdif_can_set_clk_rate(struct fsl_spdif_priv *spdif, int 
clk)
{
return (clk == STC_TXCLK_SPDIF_ROOT) && !spdif->soc->shared_root_clock;
}


Re: [PATCH v3] ASoC: fsl_ssi: Fix bclk calculation for mono channel

2020-06-15 Thread Nicolin Chen
On Tue, Jun 16, 2020 at 10:53:48AM +0800, Shengjiu Wang wrote:
> For mono channel, SSI will switch to Normal mode.
> 
> In Normal mode and Network mode, the Word Length Control bits
> control the word length divider in clock generator, which is
> different with I2S Master mode (the word length is fixed to
> 32bit), it should be the value of params_width(hw_params).
> 
> The condition "slots == 2" is not good for I2S Master mode,
> because for Network mode and Normal mode, the slots can also
> be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> to check if it is I2S Master mode.
> 
> So we refine the formula for mono channel, otherwise there
> will be sound issue for S24_LE.
> 
> Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot 
> number and width")
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

2020-06-15 Thread Nicolin Chen
On Tue, Jun 16, 2020 at 09:48:39AM +0800, Shengjiu Wang wrote:
> On Tue, Jun 16, 2020 at 7:11 AM Nicolin Chen  wrote:
> >
> > On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> > > For mono channel, SSI will switch to Normal mode.
> > >
> > > In Normal mode and Network mode, the Word Length Control bits
> > > control the word length divider in clock generator, which is
> > > different with I2S Master mode (the word length is fixed to
> > > 32bit), it should be the value of params_width(hw_params).
> > >
> > > The condition "slots == 2" is not good for I2S Master mode,
> > > because for Network mode and Normal mode, the slots can also
> > > be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> > > to check if it is I2S Master mode.
> >
> > The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
> > though I agree that that line of comments sounds confusing now.
> 
> Actually I think fsl_ssi_is_i2s_master is not accurate, it just checks
> the Master mode,  but didn't check the I2S mode.
> 
> >
> > > So we refine the famula for mono channel, otherwise there
> >
> > famula => formula?
> >
> > > will be sound issue for S24_LE.
> > >
> > > Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot 
> > > number and width")
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > > changes in v2
> > > - refine patch for Network mode and Normal mode.
> > >
> > >  sound/soc/fsl/fsl_ssi.c | 15 +++
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> > > index bad89b0d129e..cbf67d132fda 100644
> > > --- a/sound/soc/fsl/fsl_ssi.c
> > > +++ b/sound/soc/fsl/fsl_ssi.c
> > > @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> > > *substream,
> > >   struct regmap *regs = ssi->regs;
> > >   u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> > >   unsigned long clkrate, baudrate, tmprate;
> > > - unsigned int slots = params_channels(hw_params);
> > > + unsigned int channels = params_channels(hw_params);
> > > + unsigned int slots;
> > >   unsigned int slot_width = 32;
> > >   u64 sub, savesub = 10;
> > >   unsigned int freq;
> > > @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> > > *substream,
> > >   /* Override slots and slot_width if being specifically set... */
> > >   if (ssi->slots)
> > >   slots = ssi->slots;
> > > - /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > > - if (ssi->slot_width && slots != 2)
> > > - slot_width = ssi->slot_width;
> > > + else
> > > + /* Apply two slots for mono channel, because DC = 2 */
> > > + slots = (channels == 1) ? 2 : channels;
> > > +
> > > + /* ...but keep 32 bits if I2S Master mode */
> > > + if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != 
> > > SSI_SCR_I2S_MODE_MASTER ||
> > > + channels == 1)
> > > + slot_width = ssi->slot_width ? ssi->slot_width :
> >
> > This looks very complicated...can you review and try mine?
> > (Basically, take 32-bit out of default but force it later)
> >
> > @@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> > *substream,
> > struct regmap *regs = ssi->regs;
> > u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
> > unsigned long clkrate, baudrate, tmprate;
> > -   unsigned int slots = params_channels(hw_params);
> > -   unsigned int slot_width = 32;
> > +   unsigned int channels = params_channels(hw_params);
> > +   unsigned int slot_width = params_width(hw_params);
> > +   unsigned int slots = 2;
> > u64 sub, savesub = 10;
> > unsigned int freq;
> > bool baudclk_is_used;
> > @@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> > *substream,
> > /* Override slots and slot_width if being specifically set... */
> > if (ssi->slots)
> > slots = ssi->slots;
> > -   /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> > -   if (ssi->slot_width && slot

Re: [PATCH v2] ASoC: fsl_ssi: Fix bclk calculation for mono channel

2020-06-15 Thread Nicolin Chen
On Mon, Jun 15, 2020 at 01:56:18PM +0800, Shengjiu Wang wrote:
> For mono channel, SSI will switch to Normal mode.
> 
> In Normal mode and Network mode, the Word Length Control bits
> control the word length divider in clock generator, which is
> different with I2S Master mode (the word length is fixed to
> 32bit), it should be the value of params_width(hw_params).
> 
> The condition "slots == 2" is not good for I2S Master mode,
> because for Network mode and Normal mode, the slots can also
> be 2. Then we need to use (ssi->i2s_net & SSI_SCR_I2S_MODE_MASK)
> to check if it is I2S Master mode.

The fsl_ssi_set_bclk is only called when fsl_ssi_is_i2s_master,
though I agree that that line of comments sounds confusing now.

> So we refine the famula for mono channel, otherwise there

famula => formula?

> will be sound issue for S24_LE.
> 
> Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot 
> number and width")
> Signed-off-by: Shengjiu Wang 
> ---
> changes in v2
> - refine patch for Network mode and Normal mode.
> 
>  sound/soc/fsl/fsl_ssi.c | 15 +++
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index bad89b0d129e..cbf67d132fda 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -678,7 +678,8 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   struct regmap *regs = ssi->regs;
>   u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
>   unsigned long clkrate, baudrate, tmprate;
> - unsigned int slots = params_channels(hw_params);
> + unsigned int channels = params_channels(hw_params);
> + unsigned int slots;
>   unsigned int slot_width = 32;
>   u64 sub, savesub = 10;
>   unsigned int freq;
> @@ -688,9 +689,15 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   /* Override slots and slot_width if being specifically set... */
>   if (ssi->slots)
>   slots = ssi->slots;
> - /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
> - if (ssi->slot_width && slots != 2)
> - slot_width = ssi->slot_width;
> + else
> + /* Apply two slots for mono channel, because DC = 2 */
> + slots = (channels == 1) ? 2 : channels;
> +
> + /* ...but keep 32 bits if I2S Master mode */
> + if ((ssi->i2s_net & SSI_SCR_I2S_MODE_MASK) != SSI_SCR_I2S_MODE_MASTER ||
> + channels == 1)
> + slot_width = ssi->slot_width ? ssi->slot_width :

This looks very complicated...can you review and try mine?
(Basically, take 32-bit out of default but force it later)

@@ -678,8 +678,9 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
*substream,
struct regmap *regs = ssi->regs;
u32 pm = 999, div2, psr, stccr, mask, afreq, factor, i;
unsigned long clkrate, baudrate, tmprate;
-   unsigned int slots = params_channels(hw_params);
-   unsigned int slot_width = 32;
+   unsigned int channels = params_channels(hw_params);
+   unsigned int slot_width = params_width(hw_params);
+   unsigned int slots = 2;
u64 sub, savesub = 10;
unsigned int freq;
bool baudclk_is_used;
@@ -688,10 +689,16 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
*substream,
/* Override slots and slot_width if being specifically set... */
if (ssi->slots)
slots = ssi->slots;
-   /* ...but keep 32 bits if slots is 2 -- I2S Master mode */
-   if (ssi->slot_width && slots != 2)
+   if (ssi->slot_width)
slot_width = ssi->slot_width;
 
+   /*
+* ...but force 32 bits for stereo audio. Note that mono audio is also
+* sent in 2 slots via NORMAL mode, so check both slots and channels.
+*/
+   if (slots == 2 && channels == 2)
+   slot_width = 32;
+
/* Generate bit clock based on the slot number and slot width */
freq = slots * slot_width * params_rate(hw_params);
 


Re: [PATCH] ASoC: fsl_ssi: Fix bclk calculation for mono channel

2020-06-12 Thread Nicolin Chen
On Tue, Jun 09, 2020 at 04:19:28PM +0800, Shengjiu Wang wrote:
> For mono channel, ssi will switch to normal mode. In normal
> mode, the Word Length Control bits control the word length
> divider in clock generator, which is different with I2S master
> mode, the word length is fixed to 32bit.
> 
> So we refine the famula for mono channel, otherwise there
> will be sound issue for S24_LE.
> 
> Fixes: b0a7043d5c2c ("ASoC: fsl_ssi: Caculate bit clock rate using slot 
> number and width")
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_ssi.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index bad89b0d129e..e347776590f7 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -695,6 +695,11 @@ static int fsl_ssi_set_bclk(struct snd_pcm_substream 
> *substream,
>   /* Generate bit clock based on the slot number and slot width */
>   freq = slots * slot_width * params_rate(hw_params);
>  
> + /* The slot_width is not fixed to 32 for normal mode */
> + if (params_channels(hw_params) == 1)

This function has a local variable that you can reuse here:
unsigned int slots = params_channels(hw_params);

> + freq = (slots <= 1 ? 2 : slots) * params_width(hw_params) *
> +params_rate(hw_params);

We have a small section of slots and slot_width calculation
at the top of this function where we can squash these in.


Re: [RFC PATCH v3 4/4] ASoC: fsl_asrc_dma: Fix data copying speed issue with EDMA

2020-06-12 Thread Nicolin Chen
On Fri, Jun 12, 2020 at 03:37:51PM +0800, Shengjiu Wang wrote:
> With EDMA, there is two dma channels can be used for dev_to_dev,
> one is from ASRC, one is from another peripheral (ESAI or SAI).
> 
> If we select the dma channel of ASRC, there is an issue for ideal
> ratio case, the speed of copy data is faster than sample
> frequency, because ASRC output data is very fast in ideal ratio
> mode.
> 
> So it is reasonable to use the dma channel of Back-End peripheral.
> then copying speed of DMA is controlled by data consumption
> speed in the peripheral FIFO,
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [RFC PATCH v3 3/4] ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End

2020-06-12 Thread Nicolin Chen
On Fri, Jun 12, 2020 at 03:37:50PM +0800, Shengjiu Wang wrote:
> The dma channel has been requested by Back-End cpu dai driver already.
> If fsl_asrc_dma requests dma chan with same dma:tx symlink, then
> there will be below warning with SDMA.
> 
> [   48.174236] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink
> 
> So if we can reuse the dma channel of Back-End, then the issue can be
> fixed.
> 
> In order to get the dma channel which is already requested in Back-End.
> we use the exported two functions (snd_soc_lookup_component_nolocked
> and soc_component_to_pcm). If we can get the dma channel, then reuse it,
> if can't, then request a new one.
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [RFC PATCH v3 2/4] ASoC: dmaengine_pcm: export soc_component_to_pcm

2020-06-12 Thread Nicolin Chen
On Fri, Jun 12, 2020 at 03:37:49PM +0800, Shengjiu Wang wrote:
> In DPCM case, Front-End needs to get the dma chan which has
> been requested by Back-End and reuse it.
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [RFC PATCH v3 1/4] ASoC: soc-card: export snd_soc_lookup_component_nolocked

2020-06-12 Thread Nicolin Chen
On Fri, Jun 12, 2020 at 03:37:48PM +0800, Shengjiu Wang wrote:
> snd_soc_lookup_component_nolocked can be used for the DPCM case
> that Front-End needs to get the unused platform component but
> added by Back-End cpu dai driver.
> 
> If the component is gotten, then we can get the dma chan created
> by Back-End component and reused it in Front-End.
> 
> Signed-off-by: Shengjiu Wang 

Reviewed-by: Nicolin Chen 


Re: [RFC PATCH v2 3/3] ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End

2020-06-11 Thread Nicolin Chen
On Fri, Jun 12, 2020 at 10:17:08AM +0800, Shengjiu Wang wrote:

> > > diff --git a/sound/soc/fsl/fsl_asrc_common.h 
> > > b/sound/soc/fsl/fsl_asrc_common.h

> > > + * @req_dma_chan_dev_to_dev: flag for release dev_to_dev chan
> >
> > Since we only have dma_request call for back-end only:
> > + * @req_dma_chan: flag to release back-end dma chan
> 
> I prefer to use the description "flag to release dev_to_dev chan"
> because we won't release the dma chan of the back-end. if the chan
> is from the back-end, it is owned by the back-end component.

TBH, it just looks too long. But I wouldn't have problem if you
insist so.

> > > @@ -273,19 +299,21 @@ static int fsl_asrc_dma_hw_params(struct 
> > > snd_soc_component *component,
> > >  static int fsl_asrc_dma_hw_free(struct snd_soc_component *component,
> > >   struct snd_pcm_substream *substream)
> > >  {
> > > + bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
> > >   struct snd_pcm_runtime *runtime = substream->runtime;
> > >   struct fsl_asrc_pair *pair = runtime->private_data;
> > > + u8 dir = tx ? OUT : IN;
> > >
> > >   snd_pcm_set_runtime_buffer(substream, NULL);
> > >
> > > - if (pair->dma_chan[IN])
> > > - dma_release_channel(pair->dma_chan[IN]);
> > > + if (pair->dma_chan[!dir])
> > > + dma_release_channel(pair->dma_chan[!dir]);
> > >
> > > - if (pair->dma_chan[OUT])
> > > - dma_release_channel(pair->dma_chan[OUT]);
> > > + if (pair->dma_chan[dir] && pair->req_dma_chan_dev_to_dev)
> > > + dma_release_channel(pair->dma_chan[dir]);
> >
> > Why we only apply this to one direction?
> 
> if the chan is from the back-end, it is owned by the back-end
> component, so it should be released by the back-end component,
> not here. That's why I added the flag "req_dma_chan".

Ah...I forgot the IN and OUT is for front-end and back-end. The
naming isn't very good indeed. Probably we should add a line of
comments somewhere as a reminder.

Thanks


Re: [RFC PATCH v2 3/3] ASoC: fsl_asrc_dma: Reuse the dma channel if available in Back-End

2020-06-11 Thread Nicolin Chen
On Wed, Jun 10, 2020 at 06:05:49PM +0800, Shengjiu Wang wrote:
> The dma channel has been requested by Back-End cpu dai driver already.
> If fsl_asrc_dma requests dma chan with same dma:tx symlink, then
> there will be below warning with SDMA.
> 
> [   48.174236] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink
> 
> or with EDMA the request operation will fail for EDMA channel
> can only be requested once.
> 
> So If we can reuse the dma channel of Back-End, then the issue can be
> fixed.
> 
> In order to get the dma channel which is already requested in Back-End.
> we use the exported two functions (snd_soc_lookup_component_nolocked
> and soc_component_to_pcm). If we can get the dma channel, then reuse it,
> if can't, then request a new one.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc_common.h |  2 ++
>  sound/soc/fsl/fsl_asrc_dma.c| 52 +
>  2 files changed, 42 insertions(+), 12 deletions(-)

> diff --git a/sound/soc/fsl/fsl_asrc_common.h b/sound/soc/fsl/fsl_asrc_common.h
> index 77665b15c8db..09512bc79b80 100644
> --- a/sound/soc/fsl/fsl_asrc_common.h
> +++ b/sound/soc/fsl/fsl_asrc_common.h
> @@ -32,6 +32,7 @@ enum asrc_pair_index {
>   * @dma_chan: inputer and output DMA channels
>   * @dma_data: private dma data
>   * @pos: hardware pointer position
> + * @req_dma_chan_dev_to_dev: flag for release dev_to_dev chan

Since we only have dma_request call for back-end only:
+ * @req_dma_chan: flag to release back-end dma chan

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index d6a3fc5f87e5..5ecb77d466d3 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -160,6 +161,9 @@ static int fsl_asrc_dma_hw_params(struct 
> snd_soc_component *component,
>   substream_be = snd_soc_dpcm_get_substream(be, stream);
>   dma_params_be = snd_soc_dai_get_dma_data(dai, substream_be);
>   dev_be = dai->dev;
> + component_be = snd_soc_lookup_component_nolocked(dev_be, 
> SND_DMAENGINE_PCM_DRV_NAME);
> + if (component_be)
> + tmp_chan = 
> soc_component_to_pcm(component_be)->chan[substream->stream];

Should we use substream_be->stream or just substream->stream?

And would be better to add these lines right before we really use
tmp_chan because there's still some distance till it reaches that
point. And would be better to have a line of comments too.

> @@ -205,10 +209,14 @@ static int fsl_asrc_dma_hw_params(struct 
> snd_soc_component *component,
>*/
>   if (!asrc->use_edma) {
>   /* Get DMA request of Back-End */
> - tmp_chan = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
> + if (!tmp_chan) {
> + tmp_chan_new = dma_request_slave_channel(dev_be, tx ? 
> "tx" : "rx");
> + tmp_chan = tmp_chan_new;

This is a bit confusing...though I finally got it :)
So probably better to have a line of comments.

> @@ -220,9 +228,26 @@ static int fsl_asrc_dma_hw_params(struct 
> snd_soc_component *component,
>  
>   pair->dma_chan[dir] =
>   dma_request_channel(mask, filter, >dma_data);
> + pair->req_dma_chan_dev_to_dev = true;
>   } else {
> - pair->dma_chan[dir] =
> - asrc->get_dma_channel(pair, dir);
> + /*
> +  * With EDMA, there is two dma channels can be used for p2p,
> +  * one is from ASRC, one is from another peripheral
> +  * (ESAI or SAI). Previously we select the dma channel of ASRC,
> +  * but find an issue for ideal ratio case, there is no control
> +  * for data copy speed, the speed is faster than sample
> +  * frequency.
> +  *
> +  * So we switch to use dma channel of peripheral (ESAI or SAI),
> +  * that copy speed of DMA is controlled by data consumption
> +  * speed in the peripheral FIFO.
> +  */

This sounds like a different issue and should be fixed separately?
If you prefer not to, better to move this one to commit log, other
than having a changelog here, in my opinion.

Since it no longer uses get_dma_channel() for EDMA case, we should
update the comments at the top as well.

> + pair->req_dma_chan_dev_to_dev = false;
> + pair->dma_chan[dir] = tmp_chan;
> + if (!pair->dma_chan[dir]) {
> + pair->dma_chan[dir] = dma_request_slave_channel(dev_be, 
> tx ? "tx" : "rx");
> + pair->req_dma_chan_dev_to_dev = true;
> + }
>   }

Now there are some duplicated lines between these if-else routines, so
combining my previous comments, we can do (sample change, not tested):

@@ -197,18 +199,29 @@ static int fsl_asrc_dma_hw_params(struct 
snd_soc_component *component,
dma_cap_set(DMA_SLAVE, mask);

Re: [PATCH 3/3] ASoC: fsl_easrc: Fix "Function parameter not described" warnings

2020-06-03 Thread Nicolin Chen
On Wed, Jun 03, 2020 at 11:39:41AM +0800, Shengjiu Wang wrote:
> Obtained with:
> $ make W=1
> 
> sound/soc/fsl/fsl_easrc.c:403: warning: Function parameter or member 'easrc' 
> not described in 'fsl_easrc_normalize_filter'
> sound/soc/fsl/fsl_easrc.c:403: warning: Function parameter or member 
> 'infilter' not described in 'fsl_easrc_normalize_filter'
> sound/soc/fsl/fsl_easrc.c:403: warning: Function parameter or member 
> 'outfilter' not described in 'fsl_easrc_normalize_filter'
> sound/soc/fsl/fsl_easrc.c:403: warning: Function parameter or member 'shift' 
> not described in 'fsl_easrc_normalize_filter'
> 
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Signed-off-by: Shengjiu Wang 
> Reported-by: kbuild test robot 

Acked-by: Nicolin Chen 


Re: [PATCH 2/3] ASoC: fsl_easrc: Fix -Wunused-but-set-variable

2020-06-03 Thread Nicolin Chen
On Wed, Jun 03, 2020 at 11:39:40AM +0800, Shengjiu Wang wrote:
> Obtained with:
> $ make W=1
> 
> sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_set_rs_ratio':
> sound/soc/fsl/fsl_easrc.c:182:15: warning: variable 'int_bits' set but not 
> used [-Wunused-but-set-variable]
>   unsigned int int_bits;
>^
> sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_set_ctx_organziation':
> sound/soc/fsl/fsl_easrc.c:1204:17: warning: variable 'dev' set but not used 
> [-Wunused-but-set-variable]
>   struct device *dev;
>  ^
> sound/soc/fsl/fsl_easrc.c: In function 'fsl_easrc_release_context':
> sound/soc/fsl/fsl_easrc.c:1294:17: warning: variable 'dev' set but not used 
> [-Wunused-but-set-variable]
>   struct device *dev;
>  ^
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Signed-off-by: Shengjiu Wang 
> Reported-by: kbuild test robot 

Acked-by: Nicolin Chen 


Re: [PATCH 1/3] ASoC: fsl_easrc: Fix -Wmissing-prototypes warning

2020-06-03 Thread Nicolin Chen
On Wed, Jun 03, 2020 at 11:39:39AM +0800, Shengjiu Wang wrote:
> Obtained with:
> $ make W=1
> 
> sound/soc/fsl/fsl_easrc.c:967:5: warning: no previous prototype for function 
> 'fsl_easrc_config_context' [-Wmissing-prototypes]
> int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
> ^
> sound/soc/fsl/fsl_easrc.c:967:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1128:5: warning: no previous prototype for function 
> 'fsl_easrc_set_ctx_format' [-Wmissing-prototypes]
> int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
> ^
> sound/soc/fsl/fsl_easrc.c:1128:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_set_ctx_format(struct fsl_asrc_pair *ctx,
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1201:5: warning: no previous prototype for function 
> 'fsl_easrc_set_ctx_organziation' [-Wmissing-prototypes]
> int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
> ^
> sound/soc/fsl/fsl_easrc.c:1201:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_set_ctx_organziation(struct fsl_asrc_pair *ctx)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1245:5: warning: no previous prototype for function 
> 'fsl_easrc_request_context' [-Wmissing-prototypes]
> int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
> ^
> sound/soc/fsl/fsl_easrc.c:1245:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_request_context(int channels, struct fsl_asrc_pair *ctx)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1290:6: warning: no previous prototype for function 
> 'fsl_easrc_release_context' [-Wmissing-prototypes]
> void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
>  ^
> sound/soc/fsl/fsl_easrc.c:1290:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1317:5: warning: no previous prototype for function 
> 'fsl_easrc_start_context' [-Wmissing-prototypes]
> int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
> ^
> sound/soc/fsl/fsl_easrc.c:1317:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_start_context(struct fsl_asrc_pair *ctx)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1335:5: warning: no previous prototype for function 
> 'fsl_easrc_stop_context' [-Wmissing-prototypes]
> int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
> ^
> sound/soc/fsl/fsl_easrc.c:1335:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> int fsl_easrc_stop_context(struct fsl_asrc_pair *ctx)
> ^
> static
> sound/soc/fsl/fsl_easrc.c:1382:18: warning: no previous prototype for 
> function 'fsl_easrc_get_dma_channel' [-Wmissing-prototypes]
> struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
>  ^
> sound/soc/fsl/fsl_easrc.c:1382:1: note: declare 'static' if the function is 
> not intended to be used outside of this translation unit
> struct dma_chan *fsl_easrc_get_dma_channel(struct fsl_asrc_pair *ctx,
> ^
> static
> 
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Signed-off-by: Shengjiu Wang 
> Reported-by: kbuild test robot 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_asrc: Merge suspend/resume function to runtime_suspend/resume

2020-05-25 Thread Nicolin Chen
On Mon, May 25, 2020 at 02:11:18PM +0800, Shengjiu Wang wrote:
> > > @@ -1135,6 +1137,24 @@ static int fsl_asrc_runtime_resume(struct device 
> > > *dev)
> > >   goto disable_asrck_clk;
> > >   }
> > >
> > > + /* Stop all pairs provisionally */
> > > + regmap_read(asrc->regmap, REG_ASRCTR, );
> > > + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> > > +ASRCTR_ASRCEi_ALL_MASK, 0);
> > > +
> > > + /* Restore all registers */
> > > + regcache_cache_only(asrc->regmap, false);
> > > + regcache_mark_dirty(asrc->regmap);
> >
> >
> > I see you doing regcache_mark_dirty() in the resume() now,
> > being different from previously doing in suspend()?

> Which is for probe -> runtime_resume case.
> After probe, the power may be disabled, so move mark_dirtry
> to runtime_resume, then regcache can re-write all the registers.

I see. Just noticed that you add a regcache_cache_only
in probe(). Acked already. Thanks.


Re: [PATCH] ASoC: fsl_asrc: Merge suspend/resume function to runtime_suspend/resume

2020-05-25 Thread Nicolin Chen
On Fri, May 22, 2020 at 05:57:24PM +0800, Shengjiu Wang wrote:
> With dedicated power domain for asrc, power can be disabled after
> probe and pm runtime suspend, then the value of all registers need to
> be restored in pm runtime resume. So we can merge suspend/resume function
> to runtime_suspend/resume function and enable regcache only in end of
> probe.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_asrc: Merge suspend/resume function to runtime_suspend/resume

2020-05-24 Thread Nicolin Chen
On Fri, May 22, 2020 at 05:57:24PM +0800, Shengjiu Wang wrote:
> With dedicated power domain for asrc, power can be disabled after
> probe and pm runtime suspend, then the value of all registers need to
> be restored in pm runtime resume. So we can merge suspend/resume function
> to runtime_suspend/resume function and enable regcache only in end of
> probe.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc.c | 70 
>  1 file changed, 27 insertions(+), 43 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 432936039de4..3ebbe15ac378 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -1100,6 +1100,7 @@ static int fsl_asrc_probe(struct platform_device *pdev)
>   platform_set_drvdata(pdev, asrc);
>   pm_runtime_enable(>dev);
>   spin_lock_init(>lock);
> + regcache_cache_only(asrc->regmap, true);
>  
>   ret = devm_snd_soc_register_component(>dev, _asrc_component,
> _asrc_dai, 1);
> @@ -1117,6 +1118,7 @@ static int fsl_asrc_runtime_resume(struct device *dev)
>   struct fsl_asrc *asrc = dev_get_drvdata(dev);
>   struct fsl_asrc_priv *asrc_priv = asrc->private;
>   int i, ret;
> + u32 asrctr;
>  
>   ret = clk_prepare_enable(asrc->mem_clk);
>   if (ret)
> @@ -1135,6 +1137,24 @@ static int fsl_asrc_runtime_resume(struct device *dev)
>   goto disable_asrck_clk;
>   }
>  
> + /* Stop all pairs provisionally */
> + regmap_read(asrc->regmap, REG_ASRCTR, );
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> +ASRCTR_ASRCEi_ALL_MASK, 0);
> +
> + /* Restore all registers */
> + regcache_cache_only(asrc->regmap, false);
> + regcache_mark_dirty(asrc->regmap);


I see you doing regcache_mark_dirty() in the resume() now,
being different from previously doing in suspend()?

Thanks
Nic


> + regcache_sync(asrc->regmap);
> +
> + regmap_update_bits(asrc->regmap, REG_ASRCFG,
> +ASRCFG_NDPRi_ALL_MASK | ASRCFG_POSTMODi_ALL_MASK |
> +ASRCFG_PREMODi_ALL_MASK, asrc_priv->regcache_cfg);
> +
> + /* Restart enabled pairs */
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> +ASRCTR_ASRCEi_ALL_MASK, asrctr);
> +
>   return 0;
>  
>  disable_asrck_clk:
> @@ -1155,6 +1175,11 @@ static int fsl_asrc_runtime_suspend(struct device *dev)
>   struct fsl_asrc_priv *asrc_priv = asrc->private;
>   int i;
>  
> + regmap_read(asrc->regmap, REG_ASRCFG,
> + _priv->regcache_cfg);
> +
> + regcache_cache_only(asrc->regmap, true);
> +
>   for (i = 0; i < ASRC_CLK_MAX_NUM; i++)
>   clk_disable_unprepare(asrc_priv->asrck_clk[i]);
>   if (!IS_ERR(asrc->spba_clk))
> @@ -1166,51 +1191,10 @@ static int fsl_asrc_runtime_suspend(struct device 
> *dev)
>  }
>  #endif /* CONFIG_PM */
>  
> -#ifdef CONFIG_PM_SLEEP
> -static int fsl_asrc_suspend(struct device *dev)
> -{
> - struct fsl_asrc *asrc = dev_get_drvdata(dev);
> - struct fsl_asrc_priv *asrc_priv = asrc->private;
> -
> - regmap_read(asrc->regmap, REG_ASRCFG,
> - _priv->regcache_cfg);
> -
> - regcache_cache_only(asrc->regmap, true);
> - regcache_mark_dirty(asrc->regmap);
> -
> - return 0;
> -}
> -
> -static int fsl_asrc_resume(struct device *dev)
> -{
> - struct fsl_asrc *asrc = dev_get_drvdata(dev);
> - struct fsl_asrc_priv *asrc_priv = asrc->private;
> - u32 asrctr;
> -
> - /* Stop all pairs provisionally */
> - regmap_read(asrc->regmap, REG_ASRCTR, );
> - regmap_update_bits(asrc->regmap, REG_ASRCTR,
> -ASRCTR_ASRCEi_ALL_MASK, 0);
> -
> - /* Restore all registers */
> - regcache_cache_only(asrc->regmap, false);
> - regcache_sync(asrc->regmap);
> -
> - regmap_update_bits(asrc->regmap, REG_ASRCFG,
> -ASRCFG_NDPRi_ALL_MASK | ASRCFG_POSTMODi_ALL_MASK |
> -ASRCFG_PREMODi_ALL_MASK, asrc_priv->regcache_cfg);
> -
> - /* Restart enabled pairs */
> - regmap_update_bits(asrc->regmap, REG_ASRCTR,
> -ASRCTR_ASRCEi_ALL_MASK, asrctr);
> -
> - return 0;
> -}
> -#endif /* CONFIG_PM_SLEEP */
> -
>  static const struct dev_pm_ops fsl_asrc_pm = {
>   SET_RUNTIME_PM_OPS(fsl_asrc_runtime_suspend, fsl_asrc_runtime_resume, 
> NULL)
> - SET_SYSTEM_SLEEP_PM_OPS(fsl_asrc_suspend, fsl_asrc_resume)
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
>  };
>  
>  static const struct fsl_asrc_soc_data fsl_asrc_imx35_data = {
> -- 
> 2.21.0
> 


Re: [PATCH 1/3] ASoC: fsl_esai: introduce SoC specific data

2020-05-04 Thread Nicolin Chen
On Fri, May 01, 2020 at 04:12:04PM +0800, Shengjiu Wang wrote:
> Introduce a SoC specific data structure which contains the
> differences between the different SoCs.
> This makes it easier to support more differences without having
> to introduce a new if/else each time.
> 
> Signed-off-by: Shengjiu Wang 

Though the 2nd patch is having comments to address, this one
looks fine to me and should be able to merge as long as Mark
is okay with this too:

Acked-by: Nicolin Chen 


Re: [PATCH v2] ASoC: fsl_esai: Disable exception interrupt before scheduling tasklet

2020-04-27 Thread Nicolin Chen
On Mon, Apr 27, 2020 at 02:23:21PM +0800, Shengjiu Wang wrote:
> Disable exception interrupt before scheduling tasklet, otherwise if
> the tasklet isn't handled immediately, there will be endless xrun
> interrupt.
> 
> Fixes: 7ccafa2b3879 ("ASoC: fsl_esai: recover the channel swap after xrun")
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH v2] ASoC: fsl_easrc: Check for null pointer before dereferencing "ctx" in fsl_easrc_hw_free()

2020-04-26 Thread Nicolin Chen
On Sat, Apr 25, 2020 at 03:19:29PM +0800, Shengjiu Wang wrote:
> The patch 955ac624058f: "ASoC: fsl_easrc: Add EASRC ASoC CPU DAI
> drivers" from Apr 16, 2020, leads to the following Smatch complaint:
> 
> sound/soc/fsl/fsl_easrc.c:1529 fsl_easrc_hw_free()
> warn: variable dereferenced before check 'ctx' (see line 1527)
> 
> sound/soc/fsl/fsl_easrc.c
>   1526  struct fsl_asrc_pair *ctx = runtime->private_data;
>   1527  struct fsl_easrc_ctx_priv *ctx_priv = ctx->private;
>   ^
> Dereference
> 
>   1528
>   1529  if (ctx && (ctx_priv->ctx_streams & BIT(substream->stream))) {
> ^^^
> This check is too late, to prevent a NULL dereference.
> 
>   1530  ctx_priv->ctx_streams &= ~BIT(substream->stream);
>   1531  fsl_easrc_release_context(ctx);
> 
> Fixes: 955ac624058f ("ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers")
> Reported-by: Dan Carpenter 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH] ASoC: fsl_esai: Remove the tasklet

2020-04-24 Thread Nicolin Chen
On Fri, Apr 24, 2020 at 02:54:06PM +0800, Shengjiu Wang wrote:
> Remove tasklet for it may cause the reset operation
> can't be handled immediately, then there will be
> endless xrun interrupt.

The reset routine is really long and expensive, so not sure
if it'd be good to do it completely inside HW ISR. Have you
tried to clear xEIE bits to disable xrun interrupts, before
scheduling the tasklet? If that does not solve the problem,
we may go for this change.


Re: [PATCH v7 5/7] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-04-13 Thread Nicolin Chen
On Tue, Apr 14, 2020 at 08:43:07AM +0800, Shengjiu Wang wrote:
> There is a new ASRC included in i.MX serial platform, there
> are some common definition can be shared with each other.
> So move the common definition to a separate header file.
> 
> And add fsl_asrc_pair_priv and fsl_asrc_priv for
> the variable specific for the module, which can be used
> internally.
> 
> Signed-off-by: Shengjiu Wang 

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> +static size_t fsl_asrc_get_pair_priv_size(void)
> +{
> + return sizeof(struct fsl_asrc_pair_priv);
> +}

Perhaps we haven't understood completely each other's point.

Yet, would the following change work?

> diff --git a/sound/soc/fsl/fsl_asrc_common.h b/sound/soc/fsl/fsl_asrc_common.h
> new file mode 100644
> index ..b15244e027d0
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_asrc_common.h
> +struct fsl_asrc {
> + size_t (*get_pair_priv_size)(void);

+   size_t pair_priv_size;

> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> @@ -992,25 +1012,32 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> + asrc->get_pair_priv_size = fsl_asrc_get_pair_priv_size;

+   asrc->pair_priv_size = sizeof(struct fsl_asrc_pair_priv);

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -311,11 +311,12 @@ static int fsl_asrc_dma_startup(struct 
> snd_soc_component *component,
>   return ret;
>   }
>  
> - pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
> + pair = kzalloc(sizeof(*pair) + asrc->get_pair_priv_size(), GFP_KERNEL);

+   pair = kzalloc(sizeof(*pair) + asrc->pair_priv_size, GFP_KERNEL);


Re: [PATCH v7 5/7] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-04-13 Thread Nicolin Chen
On Tue, Apr 14, 2020 at 10:21:29AM +0800, Shengjiu Wang wrote:
> On Tue, Apr 14, 2020 at 10:09 AM Nicolin Chen  wrote:
> >
> > On Tue, Apr 14, 2020 at 08:43:07AM +0800, Shengjiu Wang wrote:
> > > There is a new ASRC included in i.MX serial platform, there
> > > are some common definition can be shared with each other.
> > > So move the common definition to a separate header file.
> > >
> > > And add fsl_asrc_pair_priv and fsl_asrc_priv for
> > > the variable specific for the module, which can be used
> > > internally.
> > >
> > > Signed-off-by: Shengjiu Wang 
> >
> > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> > > +static size_t fsl_asrc_get_pair_priv_size(void)
> > > +{
> > > + return sizeof(struct fsl_asrc_pair_priv);
> > > +}
> >
> > Perhaps we haven't understood completely each other's point.
> >
> > Yet, would the following change work?
> 
> Should work, almost same

Function call involving branch instruction may fail CPU's branch
prediction and hurt performance, depending on the CPU arch. If a
variable simply does the job, we should avoid doing that.

> or do you want me to use variable to replace function pointer?

Yes. And please add my ack once you change it as the reset LGTM:

Acked-by: Nicolin Chen 


Re: [PATCH v7 4/7] ASoC: fsl_asrc: Support new property fsl,asrc-format

2020-04-13 Thread Nicolin Chen
On Tue, Apr 14, 2020 at 08:43:06AM +0800, Shengjiu Wang wrote:
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH v6 5/7] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-04-12 Thread Nicolin Chen
On Mon, Apr 13, 2020 at 11:16:31AM +0800, Shengjiu Wang wrote:
> On Sun, Apr 12, 2020 at 10:08 AM Nicolin Chen  wrote:
> >
> > On Sat, Apr 11, 2020 at 01:49:43PM +0800, Shengjiu Wang wrote:
> >
> > > > > diff --git a/sound/soc/fsl/fsl_asrc_dma.c 
> > > > > b/sound/soc/fsl/fsl_asrc_dma.c
> > > > > index b15946e03380..5cf0468ce6e3 100644
> > > > > --- a/sound/soc/fsl/fsl_asrc_dma.c
> > > > > +++ b/sound/soc/fsl/fsl_asrc_dma.c
> > > >
> > > > > @@ -311,11 +311,12 @@ static int fsl_asrc_dma_startup(struct 
> > > > > snd_soc_component *component,
> > > > >   return ret;
> > > > >   }
> > > > >
> > > > > - pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
> > > > > + pair = kzalloc(sizeof(struct fsl_asrc_pair) + PAIR_PRIVAT_SIZE, 
> > > > > GFP_KERNEL);
> > > >
> > > > If we only use the PAIR_PRIVATE_SIZE here, maybe we can put the
> > > > define in this file too, rather than in the header file.
> > > >
> > > > And could fit 80 characters:
> > > >
> > > > +   pair = kzalloc(sizeof(*pair) + PAIR_PRIVAT_SIZE, GFP_KERNEL);
> >
> > > I will use a function pointer
> > > int (*get_pair_priv_size)(void)
> >
> > Since it's the size of pair or cts structure, could be just a
> > size_t variable?
> 
> Yes, should be "size_t (*get_pair_priv_size)(void)"

Does it have to be a function? -- how about this:

struct pair {
...
size_t private_size;
void *private;
};

probe/or-somewhere() {
...
pair->private = pair_priv;
pair->private_size = sizeof(*pair_priv);
...
}


Re: [PATCH v6 5/7] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-04-11 Thread Nicolin Chen
On Sat, Apr 11, 2020 at 01:49:43PM +0800, Shengjiu Wang wrote:

> > > diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> > > index b15946e03380..5cf0468ce6e3 100644
> > > --- a/sound/soc/fsl/fsl_asrc_dma.c
> > > +++ b/sound/soc/fsl/fsl_asrc_dma.c
> >
> > > @@ -311,11 +311,12 @@ static int fsl_asrc_dma_startup(struct 
> > > snd_soc_component *component,
> > >   return ret;
> > >   }
> > >
> > > - pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
> > > + pair = kzalloc(sizeof(struct fsl_asrc_pair) + PAIR_PRIVAT_SIZE, 
> > > GFP_KERNEL);
> >
> > If we only use the PAIR_PRIVATE_SIZE here, maybe we can put the
> > define in this file too, rather than in the header file.
> >
> > And could fit 80 characters:
> >
> > +   pair = kzalloc(sizeof(*pair) + PAIR_PRIVAT_SIZE, GFP_KERNEL);

> I will use a function pointer
> int (*get_pair_priv_size)(void)

Since it's the size of pair or cts structure, could be just a
size_t variable?


Re: [PATCH v6 7/7] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers

2020-04-06 Thread Nicolin Chen
On Wed, Apr 01, 2020 at 04:45:40PM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new IP module
> found on i.MX8MN. It is different with old ASRC module.
> 
> The primary features for the EASRC are as follows:
> - 4 Contexts - groups of channels with an independent time base
> - Fully independent and concurrent context control
> - Simultaneous processing of up to 32 audio channels
> - Programmable filter charachteristics for each context
> - 32, 24, 20, and 16-bit fixed point audio sample support
> - 32-bit floating point audio sample support
> - 8kHz to 384kHz sample rate
> - 1/16 to 8x sample rate conversion ratio
> 
> Signed-off-by: Shengjiu Wang 
> Signed-off-by: Cosmin-Gabriel Samoila 

Overall, looks good to me.

Please add:
Acked-by: Nicolin Chen 

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> +static int fsl_easrc_normalize_filter(struct fsl_asrc *easrc,

> +  * If exponent is zero (value == 0), or 7ff (value == NaNs)
[...]
> + if (exp == 0 || exp == 0x7ff) {
[...]
> + if ((shift > 0 && exp >= 2047) ||
> + (shift < 0 && exp <= 0)) {

Could fit into one line, and would be probably nicer to re-use
"0x7ff" matching previous places, instead of "2047".


Re: [PATCH v6 5/7] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-04-06 Thread Nicolin Chen
On Wed, Apr 01, 2020 at 04:45:38PM +0800, Shengjiu Wang wrote:

>  static int fsl_asrc_probe(struct platform_device *pdev)
>  {
>   struct device_node *np = pdev->dev.of_node;
>   struct fsl_asrc *asrc;
> + struct fsl_asrc_priv *asrc_priv;

Could we move it before "struct fsl_asrc *asrc;"?

> diff --git a/sound/soc/fsl/fsl_asrc_common.h b/sound/soc/fsl/fsl_asrc_common.h
> new file mode 100644
> index ..5c93ccdfc30a
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_asrc_common.h

> +#define PAIR_CTX_NUM  0x4
> +#define PAIR_PRIVAT_SIZE 0x400

"PRIVAT_" => "PRIVATE_"

> +/**
> + * fsl_asrc_pair: ASRC common data

"fsl_asrc_pair" => "fsl_asrc"

> + */
> +struct fsl_asrc {

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index b15946e03380..5cf0468ce6e3 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c

> @@ -311,11 +311,12 @@ static int fsl_asrc_dma_startup(struct 
> snd_soc_component *component,
>   return ret;
>   }
>  
> - pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
> + pair = kzalloc(sizeof(struct fsl_asrc_pair) + PAIR_PRIVAT_SIZE, 
> GFP_KERNEL);

If we only use the PAIR_PRIVATE_SIZE here, maybe we can put the
define in this file too, rather than in the header file.

And could fit 80 characters:

+   pair = kzalloc(sizeof(*pair) + PAIR_PRIVAT_SIZE, GFP_KERNEL);


Re: [PATCH v6 4/7] ASoC: fsl_asrc: Support new property fsl,asrc-format

2020-04-06 Thread Nicolin Chen
Just some small comments.

On Wed, Apr 01, 2020 at 04:45:37PM +0800, Shengjiu Wang wrote:
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc.c | 40 ++--
>  sound/soc/fsl/fsl_asrc.h |  4 ++--
>  sound/soc/fsl/fsl_asrc_dma.c | 15 +++---
>  3 files changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 4d3e51bfa949..eea19e2b723b 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -1052,16 +1047,31 @@ static int fsl_asrc_probe(struct platform_device 
> *pdev)
>   return ret;
>   }
>  
> - ret = of_property_read_u32(np, "fsl,asrc-width",
> ->asrc_width);
> + ret = of_property_read_u32(np, "fsl,asrc-format", >asrc_format);
>   if (ret) {
> - dev_err(>dev, "failed to get output width\n");
> - return ret;
> + ret = of_property_read_u32(np, "fsl,asrc-width", );
> + if (ret) {
> + dev_err(>dev, "failed to get output width\n");

Similar to the comments against sound card driver:
"failed to decide output format"

> + return ret;
> + }
> +
> + switch (width) {
> + case 16:
> + asrc->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
> + break;
> + case 24:
> + asrc->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> + break;
> + default:
> + dev_warn(>dev, "unsupported width, switching to 
> 24bit\n");

Should match what the code does after the change:
+   dev_warn(>dev,
+"unsupported width, use default S24_LE\n");

> + asrc->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> + break;
> + }
>   }
>  
> - if (asrc->asrc_width != 16 && asrc->asrc_width != 24) {
> - dev_warn(>dev, "unsupported width, switching to 24bit\n");
> - asrc->asrc_width = 24;
> + if (!(FSL_ASRC_FORMATS & (1ULL << asrc->asrc_format))) {
> + dev_warn(>dev, "unsupported format, switching to 
> S24_LE\n");

Could fit 80 characters:
+   dev_warn(>dev, "unsupported width, use default S24_LE\n");

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index 5fe83aece25b..b15946e03380 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -230,10 +230,19 @@ static int fsl_asrc_dma_hw_params(struct 
> snd_soc_component *component,
>   return -EINVAL;
>   }
>  
> - if (asrc->asrc_width == 16)
> + bits = snd_pcm_format_physical_width(asrc->asrc_format);

Can we just use 'width' to match the function name?


Re: [PATCH v6 3/7] ASoC: fsl-asoc-card: Support new property fsl,asrc-format

2020-04-06 Thread Nicolin Chen
On Wed, Apr 01, 2020 at 04:45:36PM +0800, Shengjiu Wang wrote:
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 

> ---
>  sound/soc/fsl/fsl-asoc-card.c | 21 -
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
> index bb33601fab84..a0f5eb27d61a 100644
> --- a/sound/soc/fsl/fsl-asoc-card.c
> +++ b/sound/soc/fsl/fsl-asoc-card.c
> @@ -680,17 +680,20 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   goto asrc_fail;
>   }
>  
> - ret = of_property_read_u32(asrc_np, "fsl,asrc-width", );
> + ret = of_property_read_u32(asrc_np, "fsl,asrc-format", 
> >asrc_format);
>   if (ret) {
> - dev_err(>dev, "failed to get output rate\n");
> - ret = -EINVAL;
> - goto asrc_fail;
> - }
> + /* Fallback to old binding; translate to asrc_format */
> + ret = of_property_read_u32(asrc_np, "fsl,asrc-width", 
> );
> + if (ret) {
> + dev_err(>dev, "failed to get output 
> width\n");

Should warn 'format' over 'width', since it's preferable.

> + return ret;

Should goto asrc_fail as we did prior to the change.

And some of lines are over 80 characters.

Let's try this:
ret = of_property_read_u32(asrc_np, "fsl,asrc-format",
   >asrc_format);
if (ret) {
/* Fallback to old binding; translate to asrc_format */
ret = of_property_read_u32(asrc_np, "fsl,asrc-width",
   );
if (ret) {
dev_err(>dev,
"failed to decide output format\n");
goto asrc_fail;
}

if (width == 24)
priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
else
priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
}


Re: [PATCH v6 2/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format

2020-04-06 Thread Nicolin Chen
On Wed, Apr 01, 2020 at 04:45:35PM +0800, Shengjiu Wang wrote:
> In order to support new EASRC and simplify the code structure,
> We decide to share the common structure between them. This bring
> a problem that EASRC accept format directly from devicetree, but
> ASRC accept width from devicetree.
> 
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, then driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 

Once Rob has no objection:
Acked-by: Nicolin Chen 

> ---
>  Documentation/devicetree/bindings/sound/fsl,asrc.txt | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt 
> b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> index cb9a25165503..998b4c8a7f78 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> @@ -51,6 +51,10 @@ Optional properties:
> will be in use as default. Otherwise, the big endian
> mode will be in use for all the device registers.
>  
> +   - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> +   Ends, which can replace the fsl,asrc-width.
> +   The value is 2 (S16_LE), or 6 (S24_LE).
> +
>  Example:
>  
>  asrc: asrc@2034000 {
> -- 
> 2.21.0
> 


Re: [PATCH v6 1/7] ASoC: fsl_asrc: rename asrc_priv to asrc

2020-04-01 Thread Nicolin Chen
On Wed, Apr 01, 2020 at 04:45:34PM +0800, Shengjiu Wang wrote:
> In order to move common structure to fsl_asrc_common.h
> we change the name of asrc_priv to asrc, the asrc_priv
> will be used by new struct fsl_asrc_priv.
> 
> Signed-off-by: Shengjiu Wang 

Acked-by: Nicolin Chen 


Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format

2020-03-31 Thread Nicolin Chen
On Tue, Mar 31, 2020 at 10:28:25AM +0800, Shengjiu Wang wrote:
> Hi
> 
> On Tue, Mar 24, 2020 at 5:22 AM Nicolin Chen  wrote:
> >
> > On Fri, Mar 20, 2020 at 11:32:13AM -0600, Rob Herring wrote:
> > > On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > > > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > > > In order to support new EASRC and simplify the code structure,
> > > > > We decide to share the common structure between them. This bring
> > > > > a problem that EASRC accept format directly from devicetree, but
> > > > > ASRC accept width from devicetree.
> > > > >
> > > > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > > > can accept format from devicetree, don't need to convert it to
> > > > > format through width.
> > > > >
> > > > > Signed-off-by: Shengjiu Wang 
> > > > > ---
> > > > >  Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +
> > > > >  1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt 
> > > > > b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > > index cb9a25165503..780455cf7f71 100644
> > > > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > > > @@ -51,6 +51,11 @@ Optional properties:
> > > > > will be in use as default. Otherwise, the big 
> > > > > endian
> > > > > mode will be in use for all the device registers.
> > > > >
> > > > > +   - fsl,asrc-format : Defines a mutual sample format used by 
> > > > > DPCM Back
> > > > > +   Ends, which can replace the fsl,asrc-width.
> > > > > +   The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > > > +   SNDRV_PCM_FORMAT_S24_LE
> > > >
> > > > I am still holding the concern at the DT binding of this format,
> > > > as it uses values from ASoC header file instead of a dt-binding
> > > > header file -- not sure if we can do this. Let's wait for Rob's
> > > > comments.
> > >
> > > I assume those are an ABI as well, so it's okay to copy them unless we
> >
> > They are defined under include/uapi. So I think we can use them?
> >
> > > already have some format definitions for DT. But it does need to be copy
> > > in a header under include/dt-bindings/.
> >
> > Shengjiu is actually quoting those integral values, rather than
> > those macros, so actually no need copy to include/dt-bindings,
> > yet whoever adds this format property to a new DT would need to
> > look up the value in a header file under include/uapi. I's just
> > wondering if that's okay.
> >
> > Thanks
> Shall I keep this change or drop this change?

This version of patch defines the format using those two marcos.
So what Rob suggested is to copy those defines from uapi header
file to dt-bindings folder. But you don't intend to do that?

My follow-up mail is to find if using integral values is doable.
Yet, not seeing any reply further. I think you can make a choice
to send it -- We will see how Rob acks eventually, or not.


Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format

2020-03-23 Thread Nicolin Chen
On Fri, Mar 20, 2020 at 11:32:13AM -0600, Rob Herring wrote:
> On Mon, Mar 09, 2020 at 02:19:44PM -0700, Nicolin Chen wrote:
> > On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> > > In order to support new EASRC and simplify the code structure,
> > > We decide to share the common structure between them. This bring
> > > a problem that EASRC accept format directly from devicetree, but
> > > ASRC accept width from devicetree.
> > > 
> > > In order to align with new ESARC, we add new property fsl,asrc-format.
> > > The fsl,asrc-format can replace the fsl,asrc-width, then driver
> > > can accept format from devicetree, don't need to convert it to
> > > format through width.
> > > 
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > >  Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt 
> > > b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > index cb9a25165503..780455cf7f71 100644
> > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > @@ -51,6 +51,11 @@ Optional properties:
> > > will be in use as default. Otherwise, the big endian
> > > mode will be in use for all the device registers.
> > >  
> > > +   - fsl,asrc-format : Defines a mutual sample format used by DPCM 
> > > Back
> > > +   Ends, which can replace the fsl,asrc-width.
> > > +   The value is SNDRV_PCM_FORMAT_S16_LE, or
> > > +   SNDRV_PCM_FORMAT_S24_LE
> > 
> > I am still holding the concern at the DT binding of this format,
> > as it uses values from ASoC header file instead of a dt-binding
> > header file -- not sure if we can do this. Let's wait for Rob's
> > comments.
> 
> I assume those are an ABI as well, so it's okay to copy them unless we 

They are defined under include/uapi. So I think we can use them?

> already have some format definitions for DT. But it does need to be copy 
> in a header under include/dt-bindings/.

Shengjiu is actually quoting those integral values, rather than
those macros, so actually no need copy to include/dt-bindings,
yet whoever adds this format property to a new DT would need to
look up the value in a header file under include/uapi. I's just
wondering if that's okay.

Thanks


Re: [PATCH v5 7/7] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers

2020-03-09 Thread Nicolin Chen
A few small comments -- trying to improve readability.

On Mon, Mar 09, 2020 at 11:58:34AM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new IP module
> found on i.MX8MN. It is different with old ASRC module.
> 
> The primary features for the EASRC are as follows:
> - 4 Contexts - groups of channels with an independent time base
> - Fully independent and concurrent context control
> - Simultaneous processing of up to 32 audio channels
> - Programmable filter charachteristics for each context
> - 32, 24, 20, and 16-bit fixed point audio sample support
> - 32-bit floating point audio sample support
> - 8kHz to 384kHz sample rate
> - 1/16 to 8x sample rate conversion ratio
> 
> Signed-off-by: Shengjiu Wang 
> Signed-off-by: Cosmin-Gabriel Samoila 
> ---
>  sound/soc/fsl/Kconfig |   11 +
>  sound/soc/fsl/Makefile|2 +
>  sound/soc/fsl/fsl_easrc.c | 2111 +
>  sound/soc/fsl/fsl_easrc.h |  651 
>  4 files changed, 2775 insertions(+)
>  create mode 100644 sound/soc/fsl/fsl_easrc.c
>  create mode 100644 sound/soc/fsl/fsl_easrc.h

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c

> +static int fsl_easrc_resampler_config(struct fsl_asrc *easrc)
> +{
> + struct device *dev = >pdev->dev;
> + struct fsl_easrc_priv *easrc_priv = easrc->private;
> + struct asrc_firmware_hdr *hdr =  easrc_priv->firmware_hdr;
> + struct interp_params *interp = easrc_priv->interp;
> + struct interp_params *selected_interp = NULL;
> + unsigned int num_coeff;
> + unsigned int i;
> + u64 *arr;
> + u32 *r;
> + int ret;
> +
> + if (!hdr) {
> + dev_err(dev, "firmware not loaded!\n");
> + return -ENODEV;
> + }
> +
> + for (i = 0; i < hdr->interp_scen; i++) {
> + if ((interp[i].num_taps - 1) ==
> + bits_taps_to_val(easrc_priv->rs_num_taps)) {

Could fit everything under 80 characters:

+   if ((interp[i].num_taps - 1) !=
+   bits_taps_to_val(easrc_priv->rs_num_taps))
+   continue;
+
+   arr = interp[i].coeff;
+   selected_interp = [i];
+   dev_dbg(dev, "Selected interp_filter: %u taps - %u phases\n",
+   selected_interp->num_taps,
+   selected_interp->num_phases);
+   break;


> +static int fsl_easrc_normalize_filter(struct fsl_asrc *easrc,
> +   u64 *infilter,
> +   u64 *outfilter,
> +   int shift)
> +{
> + struct device *dev = >pdev->dev;
> + u64 coef = *infilter;

> + s64 exp  = (coef & 0x7ff0ll) >> 52;

Hmm...by masking 0x7ff0ll, MSB (sign bit) is gone.
Would the result still possibly be a negative value?

> + /*
> +  * If exponent is zero (value == 0), or 7ff (value == NaNs)
> +  * dont touch the content
> +  */
> + if (((coef & 0x7ff0ll) == 0) ||
> + ((coef & 0x7ff0ll) == ((u64)0x7ff << 52))) {

You have extracted "exp" already:
+   if (exp == 0 || (u64)exp & 0x7ff == 0x7ff)

> + *outfilter = coef;

Could simply a bit by returning here:
+   return 0;
+   }

Then:

+   /* coef * 2^shift == exp + shift */
+   exp += shift;
+
+   if ((shift > 0 && exp >= 2047) || (shift < 0 && exp <= 0)) {
+   dev_err(dev, "coef out of range\n");
+   return -EINVAL;
+   }
+
+   outcoef = (u64)(coef & 0x800Fll) + ((u64)exp << 52);
+   *outfilter = outcoef;


> +static int fsl_easrc_write_pf_coeff_mem(struct fsl_asrc *easrc, int ctx_id,
> + u64 *arr, int n_taps, int shift)
> +{
> + if (!arr) {
> + dev_err(dev, "NULL buffer\n");

Could it be slightly more specific?


> +static int fsl_easrc_prefilter_config(struct fsl_asrc *easrc,
> +   unsigned int ctx_id)
> +{
> + ctx_priv->in_filled_sample = bits_taps_to_val(easrc_priv->rs_num_taps) 
> / 2;
> + ctx_priv->out_missed_sample = ctx_priv->in_filled_sample *
> +   ctx_priv->out_params.sample_rate /
> +   ctx_priv->in_params.sample_rate;

There are quite a few references to sample_rate and sample_format
here, so we could use some local variables to cache them:

+   in_s_rate = ctx_priv->in_params.sample_rate;
+   out_s_rate = ctx_priv->out_params.sample_rate;
+   in_s_fmt = ctx_priv->in_params.sample_format;
+   out_s_fmt = ctx_priv->out_params.sample_format;


> +static int fsl_easrc_config_slot(struct fsl_asrc *easrc, unsigned int ctx_id)
> +{
> + struct fsl_easrc_priv *easrc_priv = easrc->private;
> + struct fsl_asrc_pair *ctx = easrc->pair[ctx_id];
> + int req_channels = ctx->channels;
> + 

Re: [PATCH v5 4/7] ASoC: fsl_asrc: rename asrc_priv to asrc

2020-03-09 Thread Nicolin Chen
On Mon, Mar 09, 2020 at 11:58:31AM +0800, Shengjiu Wang wrote:
> In order to move common structure to fsl_asrc_common.h
> we change the name of asrc_priv to asrc, the asrc_priv
> will be used by new struct fsl_asrc_priv.

This actually could be a cleanup patch which comes as the
first one in this series, so that we could ack it and get
merged without depending on others. Maybe next version?

Thanks

> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc.c | 296 +--
>  sound/soc/fsl/fsl_asrc.h |   4 +-
>  sound/soc/fsl/fsl_asrc_dma.c |  24 +--
>  3 files changed, 162 insertions(+), 162 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 11dfe3068b04..eea19e2b723b 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -21,10 +21,10 @@
>  #define IDEAL_RATIO_DECIMAL_DEPTH 26
>  
>  #define pair_err(fmt, ...) \
> - dev_err(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, 
> ##__VA_ARGS__)
> + dev_err(>pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
>  
>  #define pair_dbg(fmt, ...) \
> - dev_dbg(_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, 
> ##__VA_ARGS__)
> + dev_dbg(>pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
>  
>  /* Corresponding to process_option */
>  static unsigned int supported_asrc_rate[] = {
> @@ -157,15 +157,15 @@ static void fsl_asrc_sel_proc(int inrate, int outrate,
>  int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
>  {
>   enum asrc_pair_index index = ASRC_INVALID_PAIR;
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> - struct device *dev = _priv->pdev->dev;
> + struct fsl_asrc *asrc = pair->asrc;
> + struct device *dev = >pdev->dev;
>   unsigned long lock_flags;
>   int i, ret = 0;
>  
> - spin_lock_irqsave(_priv->lock, lock_flags);
> + spin_lock_irqsave(>lock, lock_flags);
>  
>   for (i = ASRC_PAIR_A; i < ASRC_PAIR_MAX_NUM; i++) {
> - if (asrc_priv->pair[i] != NULL)
> + if (asrc->pair[i] != NULL)
>   continue;
>  
>   index = i;
> @@ -177,17 +177,17 @@ int fsl_asrc_request_pair(int channels, struct 
> fsl_asrc_pair *pair)
>   if (index == ASRC_INVALID_PAIR) {
>   dev_err(dev, "all pairs are busy now\n");
>   ret = -EBUSY;
> - } else if (asrc_priv->channel_avail < channels) {
> + } else if (asrc->channel_avail < channels) {
>   dev_err(dev, "can't afford required channels: %d\n", channels);
>   ret = -EINVAL;
>   } else {
> - asrc_priv->channel_avail -= channels;
> - asrc_priv->pair[index] = pair;
> + asrc->channel_avail -= channels;
> + asrc->pair[index] = pair;
>   pair->channels = channels;
>   pair->index = index;
>   }
>  
> - spin_unlock_irqrestore(_priv->lock, lock_flags);
> + spin_unlock_irqrestore(>lock, lock_flags);
>  
>   return ret;
>  }
> @@ -195,25 +195,25 @@ int fsl_asrc_request_pair(int channels, struct 
> fsl_asrc_pair *pair)
>  /**
>   * Release ASRC pair
>   *
> - * It clears the resource from asrc_priv and releases the occupied channels.
> + * It clears the resource from asrc and releases the occupied channels.
>   */
>  void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
>  {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
>   enum asrc_pair_index index = pair->index;
>   unsigned long lock_flags;
>  
>   /* Make sure the pair is disabled */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
>  ASRCTR_ASRCEi_MASK(index), 0);
>  
> - spin_lock_irqsave(_priv->lock, lock_flags);
> + spin_lock_irqsave(>lock, lock_flags);
>  
> - asrc_priv->channel_avail += pair->channels;
> - asrc_priv->pair[index] = NULL;
> + asrc->channel_avail += pair->channels;
> + asrc->pair[index] = NULL;
>   pair->error = 0;
>  
> - spin_unlock_irqrestore(_priv->lock, lock_flags);
> + spin_unlock_irqrestore(>lock, lock_flags);
>  }
>  
>  /**
> @@ -221,10 +221,10 @@ void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
>   */
>  static void fsl_asrc_set_watermarks(struct fsl_asrc_pair *pair, u32 in, u32 
> out)
>  {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
>   enum asrc_pair_index index = pair->index;
>  
> - regmap_update_bits(asrc_priv->regmap, REG_ASRMCR(index),
> + regmap_update_bits(asrc->regmap, REG_ASRMCR(index),
>  ASRMCRi_EXTTHRSHi_MASK |
>  ASRMCRi_INFIFO_THRESHOLD_MASK |
>  ASRMCRi_OUTFIFO_THRESHOLD_MASK,
> @@ -257,7 +257,7 @@ static u32 fsl_asrc_cal_asrck_divisor(struct 
> fsl_asrc_pair *pair, u32 div)
>  static int fsl_asrc_set_ideal_ratio(struct 

Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format

2020-03-09 Thread Nicolin Chen
On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> In order to support new EASRC and simplify the code structure,
> We decide to share the common structure between them. This bring
> a problem that EASRC accept format directly from devicetree, but
> ASRC accept width from devicetree.
> 
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, then driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt 
> b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> index cb9a25165503..780455cf7f71 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> @@ -51,6 +51,11 @@ Optional properties:
> will be in use as default. Otherwise, the big endian
> mode will be in use for all the device registers.
>  
> +   - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> +   Ends, which can replace the fsl,asrc-width.
> +   The value is SNDRV_PCM_FORMAT_S16_LE, or
> +   SNDRV_PCM_FORMAT_S24_LE

I am still holding the concern at the DT binding of this format,
as it uses values from ASoC header file instead of a dt-binding
header file -- not sure if we can do this. Let's wait for Rob's
comments.


Re: [PATCH v5 2/7] ASoC: fsl-asoc-card: Support new property fsl,asrc-format

2020-03-09 Thread Nicolin Chen
On Mon, Mar 09, 2020 at 11:58:29AM +0800, Shengjiu Wang wrote:
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, driver
> can accept format from devicetree, don't need to convert it to
> format through width.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl-asoc-card.c | 20 +++-
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
> index 9ce55feaac22..32101b9a37b9 100644
> --- a/sound/soc/fsl/fsl-asoc-card.c
> +++ b/sound/soc/fsl/fsl-asoc-card.c
> @@ -680,17 +680,19 @@ static int fsl_asoc_card_probe(struct platform_device 
> *pdev)
>   goto asrc_fail;
>   }
>  
> - ret = of_property_read_u32(asrc_np, "fsl,asrc-width", );
> + ret = of_property_read_u32(asrc_np, "fsl,asrc-format", 
> >asrc_format);
>   if (ret) {
> - dev_err(>dev, "failed to get output rate\n");

Nice that your patch fixed my copy-n-paste typo here :)

> - ret = -EINVAL;
> - goto asrc_fail;
> - }

It'd be nicer to have a line of comments:
/* Fallback to old binding; translate to asrc_format */

> + ret = of_property_read_u32(asrc_np, "fsl,asrc-width", 
> );
> + if (ret) {
> + dev_err(>dev, "failed to get output 
> width\n");
> + return ret;
> + }
>  
> - if (width == 24)
> - priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> - else
> - priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
> + if (width == 24)
> + priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> + else
> + priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
> + }


Re: [PATCH v4 1/8] ASoC: dt-bindings: fsl_asrc: Change asrc-width to asrc-format

2020-03-02 Thread Nicolin Chen
On Tue, Mar 03, 2020 at 11:59:30AM +0800, Shengjiu Wang wrote:
> Hi
> 
> On Tue, Mar 3, 2020 at 9:43 AM Rob Herring  wrote:
> >
> > On Sun, Mar 01, 2020 at 01:24:12PM +0800, Shengjiu Wang wrote:
> > > asrc_format is more inteligent, which is align with the alsa
> > > definition snd_pcm_format_t, we don't need to convert it to
> > > format in driver, and it can distinguish S24_LE & S24_3LE.
> > >
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > >  Documentation/devicetree/bindings/sound/fsl,asrc.txt | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt 
> > > b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > index cb9a25165503..0cbb86c026d5 100644
> > > --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> > > @@ -38,7 +38,9 @@ Required properties:
> > >
> > > - fsl,asrc-rate   : Defines a mutual sample rate used by DPCM Back 
> > > Ends.
> > >
> > > -   - fsl,asrc-width  : Defines a mutual sample width used by DPCM Back 
> > > Ends.
> > > +   - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> > > +   Ends. The value is one of SNDRV_PCM_FORMAT_XX in
> > > +   "include/uapi/sound/asound.h"
> >
> > You can't just change properties. They are an ABI.
> 
> I have updated all the things related with this ABI in this patch series.
> What else should I do?

You probably should add one beside the old one. And all
the existing drivers would have to continue to support
"fsl,asrc-width", even if they start to support the new
"fsl,asrc-format". The ground rule here is that a newer
kernel should be able to work with an old DTB, IIRC.

One more concern here is about the format value. Though
I don't think those values, defined in asound.h, would
be changed, yet I am not sure if it's legit to align DT
bindings to a subsystem header file -- I only know that
usually we keep shared macros under include/dt-bindings
folder. I won't have any problem, if either Rob or Mark
has no objection.


Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format

2020-02-27 Thread Nicolin Chen
On Fri, Feb 28, 2020 at 10:54:02AM +0800, Shengjiu Wang wrote:
> Hi
> 
> On Fri, Feb 28, 2020 at 1:45 AM Nicolin Chen  wrote:
> >
> > On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote:
> > > On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen  
> > > wrote:
> > > >
> > > > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote:
> > > > > asrc_format is more inteligent variable, which is align
> > > > > with the alsa definition snd_pcm_format_t.
> > > > >
> > > > > Signed-off-by: Shengjiu Wang 
> > > > > ---
> > > > >  sound/soc/fsl/fsl_asrc.c | 23 +++
> > > > >  sound/soc/fsl/fsl_asrc.h |  4 ++--
> > > > >  sound/soc/fsl/fsl_asrc_dma.c |  2 +-
> > > > >  3 files changed, 14 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> > > > > index 0dcebc24c312..2b6a1643573c 100644
> > > > > --- a/sound/soc/fsl/fsl_asrc.c
> > > > > +++ b/sound/soc/fsl/fsl_asrc.c
> > > >
> > > > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct 
> > > > > snd_pcm_substream *substream,
> > > > >
> > > > >   pair->config = 
> > > > >
> > > > > - if (asrc_priv->asrc_width == 16)
> > > > > - format = SNDRV_PCM_FORMAT_S16_LE;
> > > > > - else
> > > > > - format = SNDRV_PCM_FORMAT_S24_LE;
> > > >
> > > > It feels better to me that we have format settings in hw_params().
> > > >
> > > > Why not let fsl_easrc align with this? Any reason that I'm missing?
> > >
> > > because the asrc_width is not formal,  in the future we can direct
> >
> > Hmm..that's our DT binding. And I don't feel it is a problem
> > to be ASoC irrelative.
> >
> > > input the format from the dts. format involve the info about width.
> >
> > Is there such any formal ASoC binding? I don't see those PCM
> > formats under include/dt-bindings/ folder. How are we going
> > to involve those formats in DT?
> 
> There is no formal binding of this case.
> 
> I think it is not good to convert width to format, because, for example

The thing is that fsl_easrc does the conversion too... It just
does in the probe instead of hw_params(), and then copies them
in the hw_params(). So I don't see obvious benefit by doing so.

> width = 24,  there is two option, we can select format S24_LE,  or
> format S24_3LE,  width is ambiguous for selecting.
> 
> In EASRC, it support other two 24bit format U24_LE, U24_3LE .

I understood the reason here, but am not seeing the necessity,
at this point.

> if we use the format in DT, then it is clear for usage in driver.

I think this is the thing that we should address first. If we
have such a binding being added with the new fsl_easrc driver,
I'd love to see the old driver aligning with the new one.

Otherwise, we can keep the old way, and change it when the new
binding is ready.


Re: [PATCH v3 2/4] ASoC: fsl_asrc: Move common definition to fsl_asrc_common

2020-02-27 Thread Nicolin Chen
On Thu, Feb 27, 2020 at 10:41:56AM +0800, Shengjiu Wang wrote:
> There is a new ASRC included in i.MX serial platform, there
> are some common definition can be shared with each other.
> So move the common definition to a separate header file.
> 
> And add fsl_asrc_pair_internal and fsl_asrc_internal for
> the variable specific for the module, which can be used
> internally.

I think we can just call it "priv", instead of "internal", since
it's passed by the "void *private" pointer.

And it'd be nicer to have an extra preparational patch to rename
existing "struct fsl_asrc *asrc_priv" to "struct fsl_asrc *asrc".

Something like:
struct fsl_asrc *asrc = ;
struct fsl_asrc_pair *pair = ;
struct fsl_asrc_priv *asrc_priv = asrc->private;
struct fsl_asrc_pair_priv *pair_priv = pair->private;

Thanks
--

> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc.c|  81 +++-
>  sound/soc/fsl/fsl_asrc.h|  74 ++
>  sound/soc/fsl/fsl_asrc_common.h | 105 
>  sound/soc/fsl/fsl_asrc_dma.c|  25 
>  4 files changed, 176 insertions(+), 109 deletions(-)
>  create mode 100644 sound/soc/fsl/fsl_asrc_common.h
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 2b6a1643573c..7f862d220a8e 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -308,8 +308,10 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair 
> *pair,
>   */
>  static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool 
> use_ideal_rate)
>  {
> - struct asrc_config *config = pair->config;
> + struct fsl_asrc_pair_internal *pair_internal = pair->private;
> + struct asrc_config *config = pair_internal->config;
>   struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc_internal *asrc_internal = asrc_priv->private;
>   enum asrc_pair_index index = pair->index;
>   enum asrc_word_width input_word_width;
>   enum asrc_word_width output_word_width;
> @@ -392,11 +394,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair 
> *pair, bool use_ideal_rate)
>   }
>  
>   /* Validate input and output clock sources */
> - clk_index[IN] = asrc_priv->clk_map[IN][config->inclk];
> - clk_index[OUT] = asrc_priv->clk_map[OUT][config->outclk];
> + clk_index[IN] = asrc_internal->clk_map[IN][config->inclk];
> + clk_index[OUT] = asrc_internal->clk_map[OUT][config->outclk];
>  
>   /* We only have output clock for ideal ratio mode */
> - clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> + clk = asrc_internal->asrck_clk[clk_index[ideal ? OUT : IN]];
>  
>   clk_rate = clk_get_rate(clk);
>   rem[IN] = do_div(clk_rate, inrate);
> @@ -417,7 +419,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair 
> *pair, bool use_ideal_rate)
>  
>   div[IN] = min_t(u32, 1024, div[IN]);
>  
> - clk = asrc_priv->asrck_clk[clk_index[OUT]];
> + clk = asrc_internal->asrck_clk[clk_index[OUT]];
>   clk_rate = clk_get_rate(clk);
>   if (ideal && use_ideal_rate)
>   rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> @@ -437,13 +439,13 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair 
> *pair, bool use_ideal_rate)
>   /* Set the channel number */
>   channels = config->channel_num;
>  
> - if (asrc_priv->soc->channel_bits < 4)
> + if (asrc_internal->soc->channel_bits < 4)
>   channels /= 2;
>  
>   /* Update channels for current pair */
>   regmap_update_bits(asrc_priv->regmap, REG_ASRCNCR,
> -ASRCNCR_ANCi_MASK(index, 
> asrc_priv->soc->channel_bits),
> -ASRCNCR_ANCi(index, channels, 
> asrc_priv->soc->channel_bits));
> +ASRCNCR_ANCi_MASK(index, 
> asrc_internal->soc->channel_bits),
> +ASRCNCR_ANCi(index, channels, 
> asrc_internal->soc->channel_bits));
>  
>   /* Default setting: Automatic selection for processing mode */
>   regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> @@ -568,9 +570,10 @@ static int fsl_asrc_dai_startup(struct snd_pcm_substream 
> *substream,
>   struct snd_soc_dai *dai)
>  {
>   struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> + struct fsl_asrc_internal *asrc_internal = asrc_priv->private;
>  
>   /* Odd channel number is not valid for older ASRC (channel_bits==3) */
> - if (asrc_priv->soc->channel_bits == 3)
> + if (asrc_internal->soc->channel_bits == 3)
>   snd_pcm_hw_constraint_step(substream->runtime, 0,
>  SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>  
> @@ -586,6 +589,7 @@ static int fsl_asrc_dai_hw_params(struct 
> snd_pcm_substream *substream,
>   struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
>   struct snd_pcm_runtime *runtime = substream->runtime;
>   struct fsl_asrc_pair *pair = 

Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format

2020-02-27 Thread Nicolin Chen
On Thu, Feb 27, 2020 at 01:10:19PM +0800, Shengjiu Wang wrote:
> On Thu, Feb 27, 2020 at 11:43 AM Nicolin Chen  wrote:
> >
> > On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote:
> > > asrc_format is more inteligent variable, which is align
> > > with the alsa definition snd_pcm_format_t.
> > >
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > >  sound/soc/fsl/fsl_asrc.c | 23 +++
> > >  sound/soc/fsl/fsl_asrc.h |  4 ++--
> > >  sound/soc/fsl/fsl_asrc_dma.c |  2 +-
> > >  3 files changed, 14 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> > > index 0dcebc24c312..2b6a1643573c 100644
> > > --- a/sound/soc/fsl/fsl_asrc.c
> > > +++ b/sound/soc/fsl/fsl_asrc.c
> >
> > > @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct 
> > > snd_pcm_substream *substream,
> > >
> > >   pair->config = 
> > >
> > > - if (asrc_priv->asrc_width == 16)
> > > - format = SNDRV_PCM_FORMAT_S16_LE;
> > > - else
> > > - format = SNDRV_PCM_FORMAT_S24_LE;
> >
> > It feels better to me that we have format settings in hw_params().
> >
> > Why not let fsl_easrc align with this? Any reason that I'm missing?
> 
> because the asrc_width is not formal,  in the future we can direct

Hmm..that's our DT binding. And I don't feel it is a problem
to be ASoC irrelative.

> input the format from the dts. format involve the info about width.

Is there such any formal ASoC binding? I don't see those PCM
formats under include/dt-bindings/ folder. How are we going
to involve those formats in DT?


Re: [PATCH v3 1/4] ASoC: fsl_asrc: Change asrc_width to asrc_format

2020-02-26 Thread Nicolin Chen
On Thu, Feb 27, 2020 at 10:41:55AM +0800, Shengjiu Wang wrote:
> asrc_format is more inteligent variable, which is align
> with the alsa definition snd_pcm_format_t.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/fsl_asrc.c | 23 +++
>  sound/soc/fsl/fsl_asrc.h |  4 ++--
>  sound/soc/fsl/fsl_asrc_dma.c |  2 +-
>  3 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 0dcebc24c312..2b6a1643573c 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c

> @@ -600,11 +599,6 @@ static int fsl_asrc_dai_hw_params(struct 
> snd_pcm_substream *substream,
>  
>   pair->config = 
>  
> - if (asrc_priv->asrc_width == 16)
> - format = SNDRV_PCM_FORMAT_S16_LE;
> - else
> - format = SNDRV_PCM_FORMAT_S24_LE;

It feels better to me that we have format settings in hw_params().

Why not let fsl_easrc align with this? Any reason that I'm missing?


Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers

2020-02-25 Thread Nicolin Chen
On Wed, Feb 26, 2020 at 09:51:39AM +0800, Shengjiu Wang wrote:
> > > > > +static const struct regmap_config fsl_easrc_regmap_config = {
> > > > > + .readable_reg = fsl_easrc_readable_reg,
> > > > > + .volatile_reg = fsl_easrc_volatile_reg,
> > > > > + .writeable_reg = fsl_easrc_writeable_reg,
> > > >
> > > > Can we use regmap_range and regmap_access_table?
> > > >
> > >
> > > Can the regmap_range support discontinuous registers?  The
> > > reg_stride = 4.
> >
> > I think it does. Giving an example here:
> > https://github.com/torvalds/linux/blob/master/drivers/mfd/da9063-i2c.c
> 
> The register in this i2c driver are continuous,  from 0x00, 0x01, 0x02...
> 
> But our case is 0x00, 0x04, 0x08, does it work?

Ah...I see your point now. I am not very sure -- have only used
in I2C drivers. You can ignore if it doesn't likely work for us.


Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers

2020-02-25 Thread Nicolin Chen
On Mon, Feb 24, 2020 at 08:53:25AM +, S.j. Wang wrote:
> Hi
> 
> > >
> > > Signed-off-by: Shengjiu Wang 
> > > ---
> > >  sound/soc/fsl/Kconfig   |   10 +
> > >  sound/soc/fsl/Makefile  |2 +
> > >  sound/soc/fsl/fsl_asrc_common.h |1 +
> > >  sound/soc/fsl/fsl_easrc.c   | 2265 +++
> > >  sound/soc/fsl/fsl_easrc.h   |  668 +
> > >  sound/soc/fsl/fsl_easrc_dma.c   |  440 ++
> > 
> > I see a 90% similarity between fsl_asrc_dma and fsl_easrc_dma files.
> > Would it be possible reuse the existing code? Could share structures from
> > my point of view, just like it reuses "enum asrc_pair_index", I know
> > differentiating "pair" and "context" is a big point here though.
> > 
> > A possible quick solution for that, off the top of my head, could be:
> > 
> > 1) in fsl_asrc_common.h
> > 
> > struct fsl_asrc {
> > 
> > };
> > 
> > struct fsl_asrc_pair {
> > 
> > };
> > 
> > 2) in fsl_easrc.h
> > 
> > /* Renaming shared structures */
> > #define fsl_easrc fsl_asrc
> > #define fsl_easrc_context fsl_asrc_pair
> > 
> > May be a good idea to see if others have some opinion too.
> > 
> 
> We need to modify the fsl_asrc and fsl_asrc_pair, let them
> To be used by both driver,  also we need to put the specific
> Definition for each module to same struct, right?

Yea. A merged structure if that doesn't look that bad. I see most
of the fields in struct fsl_asrc are being reused by in fsl_easrc.

> > 
> > > +static const struct regmap_config fsl_easrc_regmap_config = {
> > > + .readable_reg = fsl_easrc_readable_reg,
> > > + .volatile_reg = fsl_easrc_volatile_reg,
> > > + .writeable_reg = fsl_easrc_writeable_reg,
> > 
> > Can we use regmap_range and regmap_access_table?
> > 
> 
> Can the regmap_range support discontinuous registers?  The
> reg_stride = 4.

I think it does. Giving an example here:
https://github.com/torvalds/linux/blob/master/drivers/mfd/da9063-i2c.c


Re: [PATCH v2 3/3] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI and platform drivers

2020-02-18 Thread Nicolin Chen
On Tue, Feb 18, 2020 at 02:39:37PM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new IP module
> found on i.MX8MN. It is different with old ASRC module.
> 
> The primary features for the EASRC are as follows:
> - 4 Contexts - groups of channels with an independent time base
> - Fully independent and concurrent context control
> - Simultaneous processing of up to 32 audio channels
> - Programmable filter charachteristics for each context
> - 32, 24, 20, and 16-bit fixed point audio sample support
> - 32-bit floating point audio sample support
> - 8kHz to 384kHz sample rate
> - 1/16 to 8x sample rate conversion ratio
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  sound/soc/fsl/Kconfig   |   10 +
>  sound/soc/fsl/Makefile  |2 +
>  sound/soc/fsl/fsl_asrc_common.h |1 +
>  sound/soc/fsl/fsl_easrc.c   | 2265 +++
>  sound/soc/fsl/fsl_easrc.h   |  668 +
>  sound/soc/fsl/fsl_easrc_dma.c   |  440 ++

I see a 90% similarity between fsl_asrc_dma and fsl_easrc_dma files.
Would it be possible reuse the existing code? Could share structures
from my point of view, just like it reuses "enum asrc_pair_index", I
know differentiating "pair" and "context" is a big point here though.

A possible quick solution for that, off the top of my head, could be:

1) in fsl_asrc_common.h

struct fsl_asrc {

};

struct fsl_asrc_pair {

};

2) in fsl_easrc.h

/* Renaming shared structures */
#define fsl_easrc fsl_asrc
#define fsl_easrc_context fsl_asrc_pair

May be a good idea to see if others have some opinion too.

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> new file mode 100644
> index ..6fe2953317f2
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_easrc.c
> +
> +/* set_rs_ratio
> + *
> + * According to the resample taps, calculate the resample ratio
> + */
> +static int set_rs_ratio(struct fsl_easrc_context *ctx)

"fsl_easrc_" prefix? Would be nice to have a formula in the comments.

> +/* resets the pointer of the coeff memory pointers */
> +static int fsl_coeff_mem_ptr_reset(struct fsl_easrc *easrc,
> +unsigned int ctx_id, int mem_type)
> +{
> + /*  To reset the write pointer back to zero, the register field
> +  *  ASRC_CTX_CTRL_EXT1x[PF_COEFF_MEM_RST] can be toggled from
> +  *  0x0 to 0x1 to 0x0.
> +  */

Please use the style:
/*
 * xxx
 */

> +static int fsl_easrc_resampler_config(struct fsl_easrc *easrc)
> +{
> + for (i = 0; i < hdr->interp_scen; i++) {
> + if ((interp[i].num_taps - 1) ==
> + bits_taps_to_val(easrc->rs_num_taps)) {

Could do below to save some indentations from the rest of the routine:
+   if ((interp[i].num_taps - 1) !=
+   bits_taps_to_val(easrc->rs_num_taps))
+   continue;

> + arr = interp[i].coeff;
> + selected_interp = [i];
> + dev_dbg(dev, "Selected interp_filter: %u taps - %u 
> phases\n",
> + selected_interp->num_taps,
> + selected_interp->num_phases);
> + break;

> +/*
> + *  Scale filter coefficients (64 bits float)
> + *  For input float32 normalized range (1.0,-1.0) -> output int[16,24,32]:
> + *  scale it by multiplying filter coefficients by 2^31
> + *  For input int[16, 24, 32] -> output float32
> + *  scale it by multiplying filter coefficients by 2^-15, 2^-23, 2^-31
> + *  input:
> + *  easrc:  Structure pointer of fsl_easrc
> + *  infilter : Pointer to non-scaled input filter
> + *  shift:  The multiply factor
> + *  output:
> + *  outfilter: scaled filter
> + 
> */
> +static int NormalizedFilterForFloat32InIntOut(struct fsl_easrc *easrc,
> +   u64 *infilter,
> +   u64 *outfilter,
> +   int shift)

Coding style looks very different, at comments and function naming.

> +{
> + struct device *dev = >pdev->dev;
> + u64 coef = *infilter;
> + s64 exp  = (coef & 0x7ff0ll) >> 52;
> + u64 outcoef;
> +
> + /*
> +  * If exponent is zero (value == 0), or 7ff (value == NaNs)
> +  * dont touch the content
> +  */
> + if (((coef & 0x7ff0ll) == 0) ||
> + ((coef & 0x7ff0ll) == ((u64)0x7ff << 52))) {
> + *outfilter = coef;
> + } else {
> + if ((shift > 0 && (shift + exp) >= 2047) ||
> + (shift < 0 && (exp + shift) <= 0)) {
> + dev_err(dev, "coef 

Re: [PATCH v2 2/3] ASoC: dt-bindings: fsl_easrc: Add document for EASRC

2020-02-18 Thread Nicolin Chen
On Tue, Feb 18, 2020 at 02:39:36PM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new
> IP module found on i.MX8MN.
> 
> Signed-off-by: Shengjiu Wang 
> ---
>  .../devicetree/bindings/sound/fsl,easrc.txt   | 57 +++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/fsl,easrc.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/fsl,easrc.txt 
> b/Documentation/devicetree/bindings/sound/fsl,easrc.txt
> new file mode 100644
> index ..0e8153165e3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/fsl,easrc.txt
> @@ -0,0 +1,57 @@
> +NXP Asynchronous Sample Rate Converter (ASRC) Controller

Missing "Enhanced", I guess.

And "ASRC" => "EASRC"

> +The Asynchronous Sample Rate Converter (ASRC) converts the sampling rate of a

Ditto

> +signal associated with an input clock into a signal associated with a 
> different
> +output clock. The driver currently works as a Front End of DPCM with other 
> Back
> +Ends Audio controller such as ESAI, SSI and SAI. It has four context to 
> support

"context" => "contexts"

Btw, what's the definition of this "context"?

And, is SSI still available on imx8mn?


Re: [PATCH resend] ASoC: fsl_audmix: add missed pm_runtime_disable

2019-12-04 Thread Nicolin Chen
On Tue, Dec 03, 2019 at 07:13:03PM +0800, Chuhong Yuan wrote:
> The driver forgets to call pm_runtime_disable in probe failure
> and remove.
> Add the missed calls to fix it.
> 
> Signed-off-by: Chuhong Yuan 

Acked-by: Nicolin Chen 

Thanks

> ---
>  sound/soc/fsl/fsl_audmix.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl_audmix.c b/sound/soc/fsl/fsl_audmix.c
> index a1db1bce330f..5faecbeb5497 100644
> --- a/sound/soc/fsl/fsl_audmix.c
> +++ b/sound/soc/fsl/fsl_audmix.c
> @@ -505,15 +505,20 @@ static int fsl_audmix_probe(struct platform_device 
> *pdev)
> ARRAY_SIZE(fsl_audmix_dai));
>   if (ret) {
>   dev_err(dev, "failed to register ASoC DAI\n");
> - return ret;
> + goto err_disable_pm;
>   }
>  
>   priv->pdev = platform_device_register_data(dev, mdrv, 0, NULL, 0);
>   if (IS_ERR(priv->pdev)) {
>   ret = PTR_ERR(priv->pdev);
>   dev_err(dev, "failed to register platform %s: %d\n", mdrv, ret);
> + goto err_disable_pm;
>   }
>  
> + return 0;
> +
> +err_disable_pm:
> + pm_runtime_disable(dev);
>   return ret;
>  }
>  
> @@ -521,6 +526,8 @@ static int fsl_audmix_remove(struct platform_device *pdev)
>  {
>   struct fsl_audmix *priv = dev_get_drvdata(>dev);
>  
> + pm_runtime_disable(>dev);
> +
>   if (priv->pdev)
>   platform_device_unregister(priv->pdev);
>  
> -- 
> 2.24.0
> 


Re: [PATCH] ASoC: fsl_sai: add IRQF_SHARED

2019-12-04 Thread Nicolin Chen
On Thu, Nov 28, 2019 at 11:38:02PM +0100, Michael Walle wrote:
> The LS1028A SoC uses the same interrupt line for adjacent SAIs. Use
> IRQF_SHARED to be able to use these SAIs simultaneously.
> 
> Signed-off-by: Michael Walle 

Acked-by: Nicolin Chen 

Thanks

> ---
>  sound/soc/fsl/fsl_sai.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/fsl/fsl_sai.c b/sound/soc/fsl/fsl_sai.c
> index b517e4bc1b87..8c3ea7300972 100644
> --- a/sound/soc/fsl/fsl_sai.c
> +++ b/sound/soc/fsl/fsl_sai.c
> @@ -958,7 +958,8 @@ static int fsl_sai_probe(struct platform_device *pdev)
>   if (irq < 0)
>   return irq;
>  
> - ret = devm_request_irq(>dev, irq, fsl_sai_isr, 0, np->name, sai);
> + ret = devm_request_irq(>dev, irq, fsl_sai_isr, IRQF_SHARED,
> +np->name, sai);
>   if (ret) {
>   dev_err(>dev, "failed to claim irq %u\n", irq);
>   return ret;
> -- 
> 2.20.1
> 


Re: [PATCH V3 2/2] ASoC: fsl_asrc: Add support for imx8qm

2019-11-13 Thread Nicolin Chen
On Mon, Nov 11, 2019 at 05:18:23PM +0800, Shengjiu Wang wrote:
> There are two asrc module in imx8qm, each module has different
> clock configuration, and the DMA type is EDMA.
> 
> So in this patch, we define the new clocks, refine the clock map,
> and include struct fsl_asrc_soc_data for different soc usage.
> 
> The EDMA channel is fixed with each dma request, one dma request
> corresponding to one dma channel. So we need to request dma
> channel with dma request of asrc module.
> 
> Signed-off-by: Shengjiu Wang 

Two small comments inline. Once they are addressed,

Acked-by: Nicolin Chen 

> ---
> changes in v2
> - use !use_edma to wrap code in fsl_asrc_dma
> - add Acked-by: Nicolin Chen
> 
> changes in v3
> - remove the acked-by for commit is updated
> - read "fsl,asrc-clk-map" property, and update table
>   clk_map_imx8qm.
> 
>  sound/soc/fsl/fsl_asrc.c | 112 ---
>  sound/soc/fsl/fsl_asrc.h |  64 +++-
>  sound/soc/fsl/fsl_asrc_dma.c |  42 +
>  3 files changed, 183 insertions(+), 35 deletions(-)
> 
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index a3cfceea7d2f..03de33de8633 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -964,14 +1001,33 @@ static int fsl_asrc_probe(struct platform_device *pdev)

> + } else if (of_device_is_compatible(np, "fsl,imx8qm-asrc")) {

> + ret = of_property_read_u32(np, "fsl,asrc-clk-map",
> +_idx);

This seems to fit a single line?

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index d6146de9acd2..f871fdb9d1c6 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -197,21 +197,37 @@ static int fsl_asrc_dma_hw_params(struct 
> snd_soc_component *component,

> + /*
> +  * For EDMA DEV_TO_DEV channel, we don't need to configure
> +  * dma_request and dma_request2, we can get dma channel through
> +  * dma_request_slave_channel directly.
> +  * Compare with SDMA channel, EDMA channel is bound with dma
> +  * request event of each peripheral, and it is fixed. Not like SDMA,
> +  * the channel is allocated dynamically. So when DMA is EDMA, we
> +  * can only get EDMA channel through dma-names of Front-End device.
> +  */

Just trying to make it concise :)

+   /*
+* An EDMA DEV_TO_DEV channel is fixed and bound with DMA event of each
+* peripheral, unlike SDMA channel that is allocated dynamically. So no
+* need to configure dma_request and dma_request2, but get dma_chan via
+* dma_request_slave_channel directly with dma name of Front-End device
+*/


  1   2   3   4   5   6   7   8   >