On Wed, Mar 13, 2024 at 06:11:50PM +0100, Alexandre Mergnat wrote:
> On 26/02/2024 17:09, Mark Brown wrote:
> > > index 000000000000..13e95c227114
> > > --- /dev/null
> > > +++ b/sound/soc/codecs/mt6357.c
> > > @@ -0,0 +1,1805 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * MT6357 ALSA SoC audio codec driver
> > Please use a C++ comment for the whole comment to make it clearer that
> > this is intentional.
> If I do that, the checkpatch raise a warning:
> WARNING: Improper SPDX comment style for
> 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead
> #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1:
> +/* SPDX-License-Identifier: GPL-2.0
That's not a C++ comment so checkpatch is correctly warning?
On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote:
> On 26/02/2024 17:09, Mark Brown wrote:
> > > + case MT6357_ZCD_CON2:
> > > + regmap_read(priv->regmap, MT6357_ZCD_CON2, ®);
> > > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] =
> > > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT;
> > > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] =
> > > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT;
> > > + break;
> > It would probably be less code and would definitely be clearer and
> > simpler to just read the values when we need them rather than constatly
> > keeping a cache separate to the register cache.
> Actually you must save the values because the gain selected by the user will
> be override to do a ramp => volume_ramp(.....):
> - When you switch on the HP, you start from gain=-40db to final_gain
> (selected by user).
> - When you switch off the HP, you start from final_gain (selected by user)
> to gain=-40db.
You can just read the value back when you need to do a ramp?
> Also, the microphone's gain change when it's enabled/disabled.
I don't understand what this means?
> > > + /* ul channel swap */
> > > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0),
> > On/off controls should end in Switch.
> Sorry, I don't understand your comment. Can you reword it please ?
See control-names.rst. Run mixer-test on a card with this driver and
fix all the issues it reports.
> > > +static int hslo_mux_map_value[] = {
> > > + 0x0, 0x1, 0x2, 0x3,
> > > +};
> > Why not just use a normal mux here, there's no missing values or
> > reordering? Similarly for other muxes.
> I've dug into some other codecs and it's done like that, but I've probably
> misunderstood something.
> The only bad thing I see is enum is missing currently:
>
> enum {
> PGA_MUX_OPEN = 0,
> PGA_MUX_DACR,
> PGA_MUX_PB,
> PGA_MUX_TM,
> PGA_MUX_MASK = 0x3,
> };
The whole thing with explicitly specfying the mapping is just completely
redundant, you may as well remove it.
Il 12/03/24 15:50, Alexandre Mergnat ha scritto:
>
>
> On 26/02/2024 16:25, AngeloGioacchino Del Regno wrote:
>>> + if (enable) {
>>> + /* set gpio mosi mode */
>>> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_CLR, GPIO_MODE2_CLEAR_ALL);
>>> + regmap_write(priv->regmap, MT6357_GPIO_MODE2_SET,
>>> GPIO8_MODE_SET_AUD_CLK_MOSI |
>>> + GPIO9_MODE_SET_AUD_DAT_MOSI0 |
>>> + GPIO10_MODE_SET_AUD_DAT_MOSI1 |
>>> + GPIO11_MODE_SET_AUD_SYNC_MOSI);
>>
>> Are you sure that you need to write to MODE2_SET *and* to MODE2?!
>
> This is downstream code and these registers aren't in my documentation.
> I've removed the MODE2_SET write and test the audio: it's still working.
>
> So I will keep the spurious write removed for v2. :)
>
Usually, MediaTek registers are laid out like "REG" being R/legacy-W and
"REG_SET/CLR" for setting and clearing bits in "REG" internally, and that
might account for internal latencies and such.
Can you please try to remove the MODE2 write instead of the MODE2_SET one
and check if that works?
You're already using the SETCLR way when manipulating registers in here,
so I would confidently expect that to work.
Cheers,
Angelo
Hi Jonathan,
Here's the final(tm) version of the IIO DMABUF patchset.
This v8 fixes the remaining few issues that Christian reported.
I also updated the documentation patch as there has been changes to
index.rst.
This was based on next-20240308.
Changelog:
- [3/6]:
- Fix swapped fence direction
- Simplify fence wait mechanism
- Remove "Buffer closed with active transfers" print, as it was dead
code
- Un-export iio_buffer_dmabuf_{get,put}. They are not used anywhere
else so they can even be static.
- Prevent attaching already-attached DMABUFs
- [6/6]:
Renamed dmabuf_api.rst -> iio_dmabuf_api.rst, and updated index.rst
whose format changed in iio/togreg.
Cheers,
-Paul
Paul Cercueil (6):
dmaengine: Add API function dmaengine_prep_peripheral_dma_vec()
dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec
iio: core: Add new DMABUF interface infrastructure
iio: buffer-dma: Enable support for DMABUFs
iio: buffer-dmaengine: Support new DMABUF based userspace API
Documentation: iio: Document high-speed DMABUF based API
Documentation/iio/iio_dmabuf_api.rst | 54 ++
Documentation/iio/index.rst | 1 +
drivers/dma/dma-axi-dmac.c | 40 ++
drivers/iio/buffer/industrialio-buffer-dma.c | 181 ++++++-
.../buffer/industrialio-buffer-dmaengine.c | 59 ++-
drivers/iio/industrialio-buffer.c | 462 ++++++++++++++++++
include/linux/dmaengine.h | 27 +
include/linux/iio/buffer-dma.h | 31 ++
include/linux/iio/buffer_impl.h | 30 ++
include/uapi/linux/iio/buffer.h | 22 +
10 files changed, 890 insertions(+), 17 deletions(-)
create mode 100644 Documentation/iio/iio_dmabuf_api.rst
--
2.43.0
On Tue, Mar 5, 2024 at 10:02 AM Ricardo B. Marliere
<ricardo(a)marliere.net> wrote:
>
> On 5 Mar 09:07, T.J. Mercier wrote:
> >
> > Reviewed-by: T.J. Mercier <tjmercier(a)google.com>
> >
> > Is this really a resend? I don't see anything on lore and I can't
> > recall seeing this patch in my inbox before.
>
> Hi T.J. thanks for reviewing!
>
> I'm sorry about that, I sent the series only to Greg before but I
> thought it had Cc'ed the lists as well. Then I realized it was sent
> publicly only once. Double mistake :(
>
> Best regards,
> - Ricardo.
Cheers, glad I don't have to try to rework my email filters. :)
On Thu, Jan 18, 2024 at 7:33 PM Tommy Chiang <ototot(a)chromium.org> wrote:
>
> This patch tries to improve the display of the code listing
> on The Linux Kernel documentation website for dma-buf [1] .
>
> Originally, it appears that it was attempting to escape
> the '*' character, but looks like it's not necessary (now),
> so we are seeing something like '\*' on the webite.
>
> This patch removes these unnecessary backslashes and adds syntax
> highlighting to improve the readability of the code listing.
>
> [1] https://docs.kernel.org/driver-api/dma-buf.html
>
> Signed-off-by: Tommy Chiang <ototot(a)chromium.org>
> ---
> drivers/dma-buf/dma-buf.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 8fe5aa67b167..e083a0ab06d7 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1282,10 +1282,12 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_move_notify, DMA_BUF);
> * vmap interface is introduced. Note that on very old 32-bit architectures
> * vmalloc space might be limited and result in vmap calls failing.
> *
> - * Interfaces::
> + * Interfaces:
> *
> - * void \*dma_buf_vmap(struct dma_buf \*dmabuf, struct iosys_map \*map)
> - * void dma_buf_vunmap(struct dma_buf \*dmabuf, struct iosys_map \*map)
> + * .. code-block:: c
> + *
> + * void *dma_buf_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
> + * void dma_buf_vunmap(struct dma_buf *dmabuf, struct iosys_map *map)
> *
> * The vmap call can fail if there is no vmap support in the exporter, or if
> * it runs out of vmalloc space. Note that the dma-buf layer keeps a reference
> @@ -1342,10 +1344,11 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_move_notify, DMA_BUF);
> * enough, since adding interfaces to intercept pagefaults and allow pte
> * shootdowns would increase the complexity quite a bit.
> *
> - * Interface::
> + * Interface:
> + *
> + * .. code-block:: c
> *
> - * int dma_buf_mmap(struct dma_buf \*, struct vm_area_struct \*,
> - * unsigned long);
> + * int dma_buf_mmap(struct dma_buf *, struct vm_area_struct *, unsigned long);
> *
> * If the importing subsystem simply provides a special-purpose mmap call to
> * set up a mapping in userspace, calling do_mmap with &dma_buf.file will
> --
> 2.43.0.381.gb435a96ce8-goog
Reviewed-by: T.J. Mercier <tjmercier(a)google.com>
The code block highlighting is nice.
> 3) From 2), am65_cpsw_alloc_skb() function removed and replaced by
> netdev_alloc_skb_ip_align(), as used by the driver before -> res = 506
> Conclusion: Here is where the loss comes from.
> IOW, My am65_cpsw_alloc_skb() function is not good.
>
> Initially, I mainly created this 'custom' am65_cpsw_alloc_skb() function
> because I thought that none of XDP memory models could be used along
> with netdev_alloc_skb_ip_align() function. Was I wrong ?
> By creating this custom am65_cpsw_alloc_skb(), I also wanted to handle
> the way headroom is reserved differently.
What is special about your device? Why would
netdev_alloc_skb_ip_align() not work?
Andrew
On Tue, Mar 05, 2024 at 11:46:00AM +0100, Julien Panis wrote:
> On 3/1/24 17:38, Andrew Lunn wrote:
> > On Fri, Mar 01, 2024 at 04:02:53PM +0100, Julien Panis wrote:
> > > This patch adds XDP (eXpress Data Path) support to TI AM65 CPSW
> > > Ethernet driver. The following features are implemented:
> > > - NETDEV_XDP_ACT_BASIC (XDP_PASS, XDP_TX, XDP_DROP, XDP_ABORTED)
> > > - NETDEV_XDP_ACT_REDIRECT (XDP_REDIRECT)
> > > - NETDEV_XDP_ACT_NDO_XMIT (ndo_xdp_xmit callback)
> > >
> > > The page pool memory model is used to get better performance.
> > Do you have any benchmark numbers? It should help with none XDP
> > traffic as well. So maybe iperf numbers before and after?
> >
> > Andrew
>
> Argh...Houston, we have a problem. I checked my v3, which is ready for
> submission, with iperf3:
> 1) Before = without page pool -> 500 MBits/sec
> 2) After = with page pool -> 442 MBits/sec
> -> ~ 10% worse with page pool here.
>
> Unless the difference is not due to page pool. Maybe there's something else
> which is not good in my patch. I'm going to send the v3 which uses page pool,
> hopefully someone will find out something suspicious. Meanwhile, I'll carry on
> investigating: I'll check the results with my patch, by removing only the using of
> page pool.
You can also go the other way. First add page pool support. For the
FEC, that improved its performance. Then add XDP, which i think
decreased the performance a little. It is extra processing in the hot
path, so a little loss is not unsurprising.
What tends to be expensive with ARM is cache invalidation and
flush. So make sure you have the lengths correct. You don't want to
operate on more memory than necessary. No point flushing the full MTU
for a 64 byte TCP ACK, etc.
Andrew