Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
Hi Christian,
On Thu, Sep 19, 2019 at 4:05 AM Koenig, Christian
wrote:
>
> > +out4:
> > + kfree(i2s_pdata);
> > +out3:
> > + kfree(adev->acp.acp_res);
> > +out2:
> > + kfree(adev->acp.acp_cell);
> > +out1:
> > + kfree(adev->acp.acp_genpd);
>
> kfree on a NULL pointer is harmless, so a single error label should be
> sufficient.
That is true, but I notice that the adev structure comes from outside this
driver:
static int acp_hw_init(void *handle)
{
...
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
...
}
As far as I can tell, the init() does not explicitly set these to NULL:
adev->acp.acp_res
adev->acp.acp_cell
adev->acp.acp_genpd
I am assuming that core code sets these to NULL, before calling
acp_hw_init(). But is that guaranteed or simply a happy accident?
Ie. if they are NULL today, are they likely to remain NULL tomorrow?
Because calling kfree() on a stale pointer would be, well
not good. Especially not on an error path, which typically
does not receive much testing, if any.
My apologies if I have misinterpreted this, I am not familiar with
this code base.
Sven
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks
On Wed, Sep 18, 2019 at 3:09 PM Navid Emamdoost
wrote:
>
> i2s_pdata = kcalloc(3, sizeof(struct i2s_platform_data), GFP_KERNEL);
> if (i2s_pdata == NULL) {
> - kfree(adev->acp.acp_res);
> - kfree(adev->acp.acp_cell);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto out3;
> }
I don't see a corresponding kfree() for i2s_pdata in acp_hw_fini().
Could this be a memory leak?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap
On Fri, Jun 21, 2019 at 11:15 AM Russell King - ARM Linux admin wrote: > > Another con is the need to keep the functions that detail the register > properties up to date, which if they're wrong may result in unexpected > behaviour. > > I subscribe to the "keep it simple" approach, and regmap, although > useful, seems like a giant sledgehammer for this. > Thank you for the review ! I added this back when I was debugging audio artifacts related to this chip. The regmap's debugfs binding was extremely useful. So I dressed it up a bit in the hope that it would have some general use. But if the cons outweigh the pros, then this is as far as this patch will go...
[PATCH] drm/etnaviv: optionally set gpu linear window to cma area
On Vivante GPUs with an iommu-v1, the gpu linear window's
position must be determined at probe time, and cannot change
during the driver's lifetime.
The existing code attempts to guess a suitable window position
using the dma mask, but this does not always work [1].
According to a recent thread [2], Lucas Stach mentions that
on MC2.0 GPUs, one may simply point the window to the start
of the global cma area.
This patch provides an optional method to set the window
to the cma area, by adding a phandle to the etnaviv driver's
devicetree node, pointing to the global cma area.
Note that on iommu-v2 GPUs, this setting has no effect:
they do not depend on a fixed window position.
/ {
reserved-memory {
#address-cells = <1>;
#size-cells = <1>;
ranges;
cma_area: linux,cma {
compatible = "shared-dma-pool";
reusable;
size = <0x1000>;
linux,cma-default;
};
};
};
/* Tell the Vivante GPUS where the CMA area is */
_3d {
default-cma-region = <_area>;
};
_2d {
default-cma-region = <_area>;
};
[1] iMX6Q 2G ram, 256M cma, v5.2-rc4 kernel:
cma area @ 0x3000 (auto-assigned by kernel)
dma mask is 0x
driver sets gpu window to 0x8000
result: gpu iommu-v1 cannot access cma area
[2] https://lkml.org/lkml/2017/11/15/599
To: Lucas Stach
Cc: Russell King
Cc: Christian Gmeiner
Cc: David Airlie
Cc: Daniel Vetter
Cc: etna...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: Alexey Brodkin
Cc: Gaël PORTAY
Signed-off-by: Sven Van Asbroeck
---
devicetree documentation patch to follow, depending on how this patch
is received.
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 48 +--
drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 2 ++
2 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index 72d01e873160..7ed8705744d0 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
@@ -687,6 +688,12 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
prefetch);
}
+static bool gpu_is_3d_mc10(const struct etnaviv_chip_identity *id)
+{
+ return (id->features & chipFeatures_PIPE_3D) &&
+ !(id->minor_features0 & chipMinorFeatures0_MC20);
+}
+
int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
{
int ret, i;
@@ -713,23 +720,33 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
goto fail;
}
- /*
-* Set the GPU linear window to be at the end of the DMA window, where
-* the CMA area is likely to reside. This ensures that we are able to
-* map the command buffers while having the linear window overlap as
-* much RAM as possible, so we can optimize mappings for other buffers.
-*
-* For 3D cores only do this if MC2.0 is present, as with MC1.0 it leads
-* to different views of the memory on the individual engines.
-*/
- if (!(gpu->identity.features & chipFeatures_PIPE_3D) ||
- (gpu->identity.minor_features0 & chipMinorFeatures0_MC20)) {
+ if (!gpu_is_3d_mc10(>identity) && gpu->cma_base) {
+ /*
+* If we know the location of the CMA area, point the
+* GPU linear window to it. This ensures that we are able to
+* map the command buffers while having the linear window
+* overlap as much RAM as possible, so we can optimize mappings
+* for other buffers.
+*/
+ gpu->memory_base = (u32)gpu->cma_base;
+ } else if (!gpu_is_3d_mc10(>identity)) {
+ /*
+* Set the GPU linear window to be at the end of the DMA window,
+* where the CMA area is likely to reside. In most cases, this
+* ensures that we are able to map the command buffers while
+* having the linear window overlap as much RAM as possible, so
+* we can optimize mappings for other buffers.
+*/
u32 dma_mask = (u32)dma_get_required_mask(gpu->dev);
if (dma_mask < PHYS_OFFSET + SZ_2G)
gpu->memory_base = PHYS_OFFSET;
else
gpu->memory_base = dma_mask - SZ_2G + 1;
} else if (PHYS_OFFSET >= SZ_2G) {
+ /*
+* For MC1.0 3D cores, moving the linear window leads to
+* different views of the memory on the individual eng
Re: [PATCH v2 00/13] tda998x updates
On Thu, Jun 13, 2019 at 10:29 AM Russell King - ARM Linux admin wrote: > > This series represents development work collected over the last six > months to improve the TDA998x driver, particularly for the audio > side. These patches can be found in my "drm-tda998x-devel" branch > at git://git.armlinux.org.uk/~rmk/linux-arm.git For the whole patchset, after 'hacking' the bclk_ratio to correspond with my imx6q ssi's wire format: Tested-by: Sven Van Asbroeck ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 01/13] drm/i2c: tda998x: introduce tda998x_audio_settings
Nit: checkpatch.pl appears unhappy with this patch: WARNING: line over 80 characters #222: FILE: drivers/gpu/drm/i2c/tda998x_drv.c:1011: + audio.params.config = priv->audio_port[i].config; WARNING: line over 80 characters #230: FILE: drivers/gpu/drm/i2c/tda998x_drv.c:1017: + audio.params.config = priv->audio_port[i].config; total: 0 errors, 2 warnings, 178 lines checked ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/13] drm/i2c: tda998x: add bridge timing information
On Tue, Jun 11, 2019 at 7:04 AM Russell King wrote:
>
> Add bridge timing information so that bridge users can figure out the
> timing parameters that are necessary for TDA998x.
>
> Signed-off-by: Russell King
> ---
+static const struct drm_bridge_timings tda9989_timings = {
+ .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .setup_time_ps = 1500,
+ .hold_time_ps = 1000,
+};
+
+static const struct drm_bridge_timings tda19988_timings = {
+ .sampling_edge = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+ .setup_time_ps = 1600,
+ .hold_time_ps = 1200,
+};
Need to port these to 5.1 kernel: sampling_edge -> input_bus_flags ?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/13] drm/i2c: tda998x: derive CTS_N value from aclk sample rate ratio
On Wed, Jun 12, 2019 at 12:42 PM Russell King - ARM Linux admin wrote: > > I think you're confusing tda998x_derive_cts_n() and tda998x_get_adiv(). > tda998x_derive_cts_n() only returns 0 or -EINVAL. > True. Apologies for the confusion. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 03/13] drm/i2c: tda998x: improve programming of audio divisor
On Tue, Jun 11, 2019 at 7:02 AM Russell King wrote:
>
> Improve the selection of the audio clock divisor so that more modes
> and sample rates work.
>
> Signed-off-by: Russell King
> ---
+static u8 tda998x_get_adiv(struct tda998x_priv *priv, unsigned int fs)
+{
+ unsigned long min_audio_clk = fs * 128;
+ unsigned long ser_clk = priv->tmds_clock * 1000;
+ u8 adiv;
+
+ for (adiv = AUDIO_DIV_SERCLK_32; adiv != AUDIO_DIV_SERCLK_1; adiv--)
+ if (ser_clk > min_audio_clk << adiv)
+ break;
+
+ dev_dbg(>hdmi->dev,
+ "ser_clk=%luHz fs=%uHz min_aclk=%luHz adiv=%d\n",
+ ser_clk, fs, min_audio_clk, adiv);
+
+ return adiv;
Doesn't this function need an error return in case min_audio_clk > ser_clk ?
Or is that a situation that can never arise?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/13] tda998x updates
On Tue, Jun 11, 2019 at 7:01 AM Russell King - ARM Linux admin wrote: > > This series represents development work collected over the last six > months to improve the TDA998x driver, particularly for the audio > side. These patches can be found in my "drm-tda998x-devel" branch > at git://git.armlinux.org.uk/~rmk/linux-arm.git Thank you Russell !! I tested this patch set on my platform: imx6q ssi codec driving a tda19988. No issues as far as I can tell. Note that I still have to 'hack' the bclk_ratio in the tda driver to correspond with the wire format that the imx6q ssi is generating. But that's a known issue. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/13] drm/i2c: tda998x: move audio routing configuration
On Tue, Jun 11, 2019 at 7:02 AM Russell King wrote:
>
> Move the mux and clocking selection out of tda998x_configure_audio()
> into the parent functions, so we can validate this when parameters
> are set outside of the audio mutex.
>
> Signed-off-by: Russell King
> ---
+/* Configure the TDA998x audio data and clock routing. */
+static int tda998x_derive_routing(struct tda998x_priv *priv,
+ struct tda998x_audio_settings *s,
+ unsigned int route)
+{
+ s->route = _audio_route[route];
+ s->ena_ap = priv->audio_port_enable[route];
+ if (s->ena_ap == 0) {
+ dev_err(>hdmi->dev, "no audio configuration found\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
Nit: priv is nearly unused in this function.
Maybe delegate the error log to the caller, in that case we could just pass
route and const audio_port_enable to the function. Instead of passing in the
'kitchen sink' priv ?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/13] drm/i2c: tda998x: clean up tda998x_configure_audio()
On Tue, Jun 11, 2019 at 7:02 AM Russell King wrote:
>
> tda998x_configure_audio() is called via some paths where an error
> return is meaningless, and as a result of moving the audio routing
> code, this function no longer returns any errors, so let's make it
> void. We can also make tda998x_write_aif() return void as well.
>
> tda998x_configure_audio() also only ever needs to write the current
> audio settings, so simplify the code in tda998x_audio_hw_params()
> so that can happen.
>
> Signed-off-by: Russell King
> ---
Nit:
+static void tda998x_configure_audio(struct tda998x_priv *priv)
{
+ struct tda998x_audio_settings *settings = >audio;
settings could be const ?
-static int tda998x_write_aif(struct tda998x_priv *priv,
-struct hdmi_audio_infoframe *cea)
+static void tda998x_write_aif(struct tda998x_priv *priv,
+ struct hdmi_audio_infoframe *cea)
cea could be const ?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/13] drm/i2c: tda998x: introduce tda998x_audio_settings
On Tue, Jun 11, 2019 at 7:01 AM Russell King wrote:
>
> Introduce a structure to hold the register values to be programmed while
> programming the TDA998x audio settings. This is currently a stub
> structure, which will be populated in subsequent commits.
>
> When we initialise this from the platform data, only do so if there is a
> valid audio format specification.
>
> Signed-off-by: Russell King
> ---
Nit:
Fix compile errors? Note that these errors disappear if patch 2/13
is applied, but maybe we want to make sure that every patch compiles
so git bisect does not break?
drivers/gpu/drm/i2c/tda998x_drv.c: In function ‘tda998x_audio_hw_params’:
drivers/gpu/drm/i2c/tda998x_drv.c:1011:10: error: ‘struct
tda998x_audio_settings’ has no member named ‘config’
audio.config = priv->audio_port[i].config;
^
drivers/gpu/drm/i2c/tda998x_drv.c:1012:8: error: ‘struct
tda998x_audio_settings’ has no member named ‘format’
audio.format = AFMT_I2S;
^
drivers/gpu/drm/i2c/tda998x_drv.c:1017:10: error: ‘struct
tda998x_audio_settings’ has no member named ‘config’
audio.config = priv->audio_port[i].config;
^
drivers/gpu/drm/i2c/tda998x_drv.c:1018:8: error: ‘struct
tda998x_audio_settings’ has no member named ‘format’
audio.format = AFMT_SPDIF;
^
drivers/gpu/drm/i2c/tda998x_drv.c:1025:11: error: ‘struct
tda998x_audio_settings’ has no member named ‘config’
if (audio.config == 0) {
^
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/13] drm/i2c: tda998x: derive CTS_N value from aclk sample rate ratio
On Wed, Jun 12, 2019 at 12:28 PM Russell King - ARM Linux admin wrote: > > The platform data path has never supported the HDMI codec way of doing > things, so that really isn't a concern here. The platform data way > was sufficient to allow SPDIF streams to work with a static setup of > the TDA998x, and has never been intended to support anything beyond > that. Thank you, I am not using the platform data path, so I had no idea. Wouldn't the current code always fail on the platform data path though? create() calls tda998x_set_config() in the platform data case. If the audio_params.format is used (i.e. the platform data configures audio), the function then returns the return value of tda998x_derive_cts_n(), which is a positive divider (can be 0 if /1). Then in create(): if (ret) goto fail; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 04/13] drm/i2c: tda998x: derive CTS_N value from aclk sample rate ratio
On Tue, Jun 11, 2019 at 7:02 AM Russell King wrote:
>
> The TDA998x derives the CTS value using the supplied I2S bit clock
> (ACLK, in TDA998x parlence) rather than 128·fs. TDA998x uses two
> constants named m and k in the CTS generator such that we have this
> relationship between the I2S source ACLK and the sink fs:
>
> 128·fs_sink = ACLK·m / k
>
> Where ACLK = aclk_ratio·fs_source.
>
> When audio support was originally added, we supported a fixed ratio
> of 64·fs, intending to support the Kirkwood I2S on Dove. However,
> when hdmi-codec support was added, this was changed to scale the
> ratio with the sample width, which would've broken its use with
> Kirkwood I2S.
>
> We are now starting to see other users whose I2S blocks send at 64·fs
> for 16-bit samples, so we need to reinstate the support for the fixed
> ratio I2S bit clock.
>
> This commit takes a step towards supporting these configurations by
> selecting the CTS_N register m and k values based on the bit clock
> ratio. However, as the driver is not given the bit clock ratio from
> ALSA, continue deriving this from the sample width. This will be
> addressed in a later commit.
>
> Signed-off-by: Russell King
> ---
@@ -1657,9 +1701,17 @@ static void tda998x_set_config(struct tda998x_priv *priv,
(p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
if (p->audio_params.format != AFMT_UNUSED) {
+ unsigned int ratio;
+ bool spdif = p->audio_params.format == AFMT_SPDIF;
+
priv->audio.params = p->audio_params;
priv->audio.i2s_format = I2S_FORMAT_PHILIPS;
+
+ ratio = spdif ? 64 : p->audio_params.sample_width * 2;
+ return tda998x_derive_cts_n(priv, >audio, ratio);
Won't this make the platform_data path fail all the time?
Also, the platform_data path doesn't appear to instantiate the HDMI_CODEC,
so tda audio support would be completely missing in this case?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v1 1/2] drm/i2c: tda998x: access chip registers via a regmap
The tda988x i2c chip registers are accessed through a
paged register scheme. The kernel regmap abstraction
supports paged accesses. Replace this driver's
dedicated i2c access functions with a standard i2c
regmap.
Pros:
- dedicated i2c access code disappears: accesses now
go through well-maintained regmap code
- page atomicity requirements now handled by regmap
- ro/wo/rw access modes are now explicitly defined:
any inappropriate register accesses (e.g. write to a
read-only register) will now be explicitly rejected
by the regmap core
- all tda988x registers are now visible via debugfs
Cons:
- this will probably require extensive testing
Tested on a TDA19988 using a Freescale/NXP imx6q.
Signed-off-by: Sven Van Asbroeck
---
I originally hacked this together while debugging an incompatibility between
the tda988x's audio input and the audio codec I was driving it with.
That required me to have debug access to the chip's register values.
A regmap did the trick, it has good debugfs support.
drivers/gpu/drm/i2c/tda998x_drv.c | 350 --
1 file changed, 234 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7f34601bb515..8153e2e19e18 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -21,6 +21,7 @@
#include
#include
#include
+#include
#include
#include
@@ -43,10 +44,9 @@ struct tda998x_audio_port {
struct tda998x_priv {
struct i2c_client *cec;
struct i2c_client *hdmi;
- struct mutex mutex;
+ struct regmap *regmap;
u16 rev;
u8 cec_addr;
- u8 current_page;
bool is_on;
bool supports_infoframes;
bool sink_has_audio;
@@ -88,12 +88,10 @@ struct tda998x_priv {
/* The TDA9988 series of devices use a paged register scheme.. to simplify
* things we encode the page # in upper bits of the register #. To read/
* write a given register, we need to make sure CURPAGE register is set
- * appropriately. Which implies reads/writes are not atomic. Fun!
+ * appropriately.
*/
#define REG(page, addr) (((page) << 8) | (addr))
-#define REG2ADDR(reg) ((reg) & 0xff)
-#define REG2PAGE(reg) (((reg) >> 8) & 0xff)
#define REG_CURPAGE 0xff/* write */
@@ -285,8 +283,9 @@ struct tda998x_priv {
/* Page 09h: EDID Control */
+/* EDID_DATA consists of 128 successive registers read */
#define REG_EDID_DATA_0 REG(0x09, 0x00) /* read */
-/* next 127 successive registers are the EDID block */
+#define REG_EDID_DATA_127 REG(0x09, 0x7f) /* read */
#define REG_EDID_CTRL REG(0x09, 0xfa) /* read/write */
#define REG_DDC_ADDR REG(0x09, 0xfb) /* read/write */
#define REG_DDC_OFFS REG(0x09, 0xfc) /* read/write */
@@ -295,11 +294,17 @@ struct tda998x_priv {
/* Page 10h: information frames and packets */
+/* REG_IF1_HB consists of 32 successive registers read/write */
#define REG_IF1_HB0 REG(0x10, 0x20) /* read/write */
+/* REG_IF2_HB consists of 32 successive registers read/write */
#define REG_IF2_HB0 REG(0x10, 0x40) /* read/write */
+/* REG_IF3_HB consists of 32 successive registers read/write */
#define REG_IF3_HB0 REG(0x10, 0x60) /* read/write */
+/* REG_IF4_HB consists of 32 successive registers read/write */
#define REG_IF4_HB0 REG(0x10, 0x80) /* read/write */
+/* REG_IF5_HB consists of 32 successive registers read/write */
#define REG_IF5_HB0 REG(0x10, 0xa0) /* read/write */
+#define REG_IF5_HB31 REG(0x10, 0xbf) /* read/write */
/* Page 11h: audio settings and content info packets */
@@ -542,93 +547,29 @@ static void tda998x_cec_hook_release(void *data)
cec_enamods(priv, CEC_ENAMODS_EN_CEC_CLK | CEC_ENAMODS_EN_CEC, false);
}
-static int
-set_page(struct tda998x_priv *priv, u16 reg)
-{
- if (REG2PAGE(reg) != priv->current_page) {
- struct i2c_client *client = priv->hdmi;
- u8 buf[] = {
- REG_CURPAGE, REG2PAGE(reg)
- };
- int ret = i2c_master_send(client, buf, sizeof(buf));
- if (ret < 0) {
- dev_err(>dev, "%s %04x err %d\n", __func__,
- reg, ret);
- return ret;
- }
-
- priv->current_page = REG2PAGE(reg);
- }
- return 0;
-}
-
static int
reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
{
- struct i2c_client *client = priv->hdmi;
- u8 addr = REG2ADDR(reg);
int ret;
- mutex_lock(>mutex);
- ret = set_page(priv, reg);
- if (ret < 0)
- goto out;
-
- ret
[PATCH v1 2/2] drm/i2c: tda998x: remove indirect reg_read/_write() calls
Remove indirect reg_read/_write() calls, and replace them
with direct calls to regmap functions.
For the sake of readability, keep the following indirect
register access calls:
- reg_set()
- reg_clear()
- reg_write16()
Signed-off-by: Sven Van Asbroeck
---
drivers/gpu/drm/i2c/tda998x_drv.c | 333 ++
1 file changed, 157 insertions(+), 176 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 8153e2e19e18..1bddd2cf92ea 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -548,89 +548,59 @@ static void tda998x_cec_hook_release(void *data)
}
static int
-reg_read_range(struct tda998x_priv *priv, u16 reg, char *buf, int cnt)
-{
- int ret;
-
- ret = regmap_bulk_read(priv->regmap, reg, buf, cnt);
- if (ret < 0)
- return ret;
- return cnt;
-}
-
-static void
-reg_write_range(struct tda998x_priv *priv, u16 reg, u8 *p, int cnt)
-{
- regmap_bulk_write(priv->regmap, reg, p, cnt);
-}
-
-static int
-reg_read(struct tda998x_priv *priv, u16 reg)
-{
- int ret, val;
-
- ret = regmap_read(priv->regmap, reg, );
- if (ret < 0)
- return ret;
- return val;
-}
-
-static void
-reg_write(struct tda998x_priv *priv, u16 reg, u8 val)
-{
- regmap_write(priv->regmap, reg, val);
-}
-
-static void
-reg_write16(struct tda998x_priv *priv, u16 reg, u16 val)
+reg_write16(struct regmap *regmap, u16 reg, u16 val)
{
u8 buf[] = {val >> 8, val};
- regmap_bulk_write(priv->regmap, reg, buf, ARRAY_SIZE(buf));
+ return regmap_bulk_write(regmap, reg, buf, ARRAY_SIZE(buf));
}
-static void
-reg_set(struct tda998x_priv *priv, u16 reg, u8 val)
+static int
+reg_set(struct regmap *regmap, u16 reg, u8 val)
{
- regmap_update_bits(priv->regmap, reg, val, val);
+ return regmap_update_bits(regmap, reg, val, val);
}
-static void
-reg_clear(struct tda998x_priv *priv, u16 reg, u8 val)
+static int
+reg_clear(struct regmap *regmap, u16 reg, u8 val)
{
- regmap_update_bits(priv->regmap, reg, val, 0);
+ return regmap_update_bits(regmap, reg, val, 0);
}
static void
tda998x_reset(struct tda998x_priv *priv)
{
+ struct regmap *regmap = priv->regmap;
+
/* reset audio and i2c master: */
- reg_write(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
+ regmap_write(regmap, REG_SOFTRESET,
+ SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER);
msleep(50);
- reg_write(priv, REG_SOFTRESET, 0);
+ regmap_write(regmap, REG_SOFTRESET, 0);
msleep(50);
/* reset transmitter: */
- reg_set(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
- reg_clear(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+ reg_set(regmap, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
+ reg_clear(regmap, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
/* PLL registers common configuration */
- reg_write(priv, REG_PLL_SERIAL_1, 0x00);
- reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
- reg_write(priv, REG_PLL_SERIAL_3, 0x00);
- reg_write(priv, REG_SERIALIZER, 0x00);
- reg_write(priv, REG_BUFFER_OUT, 0x00);
- reg_write(priv, REG_PLL_SCG1, 0x00);
- reg_write(priv, REG_AUDIO_DIV,AUDIO_DIV_SERCLK_8);
- reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_CLK1 |
SEL_CLK_ENA_SC_CLK);
- reg_write(priv, REG_PLL_SCGN1,0xfa);
- reg_write(priv, REG_PLL_SCGN2,0x00);
- reg_write(priv, REG_PLL_SCGR1,0x5b);
- reg_write(priv, REG_PLL_SCGR2,0x00);
- reg_write(priv, REG_PLL_SCG2, 0x10);
+ regmap_write(regmap, REG_PLL_SERIAL_1, 0x00);
+ regmap_write(regmap, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1));
+ regmap_write(regmap, REG_PLL_SERIAL_3, 0x00);
+ regmap_write(regmap, REG_SERIALIZER, 0x00);
+ regmap_write(regmap, REG_BUFFER_OUT, 0x00);
+ regmap_write(regmap, REG_PLL_SCG1, 0x00);
+ regmap_write(regmap, REG_AUDIO_DIV,AUDIO_DIV_SERCLK_8);
+ regmap_write(regmap, REG_SEL_CLK,
+ SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
+ regmap_write(regmap, REG_PLL_SCGN1,0xfa);
+ regmap_write(regmap, REG_PLL_SCGN2,0x00);
+ regmap_write(regmap, REG_PLL_SCGR1,0x5b);
+ regmap_write(regmap, REG_PLL_SCGR2,0x00);
+ regmap_write(regmap, REG_PLL_SCG2, 0x10);
/* Write the default value MUX register */
- reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
+ regmap_write(regmap, REG_MUX_VP_VIP_OUT, 0x24);
}
/*
@@ -685,16 +655,18 @@ static void tda998x_detect_work(struct work_struct *work)
static irqreturn_t tda998x_irq_thread(int irq, void *data)
{
struct tda998x_priv *priv = data;
- u8 sta, cec, lvl, flag0, flag1, flag2;
+ struct regmap *regmap = priv->regmap;
+ u8 sta, cec, lvl;
+
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 4:11 AM Russell King - ARM Linux admin wrote: > > Given that it is normal to talk about I2S being clocked at "64fs", > "32fs" etc, wouldn't it just be much neater to specify _this_ value > in DT, rather than half that value? > Would we be able to reach consensus on Russell's suggestion ? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 10:45 AM Russell King - ARM Linux admin
wrote:
>
>
> I can't see how you'd extend a single I2S setup to support multi-
> channel audio without either adding more I2S data lines or adding
> additional WS signals (so making it e.g., a binary number).
>
That's a very good point too. In light of this, I struggle to understand how
the ssl_ssi can specify this:
static struct snd_soc_dai_driver fsl_ssi_dai_template = {
.playback = {
.stream_name = "CPU-Playback",
.channels_min = 1,
.channels_max = 32,
},
There is talk in the manual about "network mode", which could work by changing
the LRCLK only at the first slot - thereby allowing clients to receive all
slots just by counting, as long as they know the slot size?
LRCLK _/-\_/-
DATASLOT1|SLOT2|SLOT3|SLOT4|SLOT1
Well idk, I'm way out of my depth here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
On Tue, Feb 26, 2019 at 4:11 AM Russell King - ARM Linux admin wrote: > > This code actually requires a lot more thought - while it may solve > Sven's issue, it isn't generic. Wholeheartedly agree. My patches are marked RFC in the hope that they will be picked apart by folks much more in touch with the "big audio picture" than I am. It's great to learn from this process. > So far, I have this table put > together from various sources of information: > > bclk_ratio > sample widthcurrent mcasp fslssi kirkwood > 16 32 32 64 64 > 24 48 48 64 64 > 32 64 64 64 64 > > Let's also consider whether it should depend on the number of channels. > I2S uses a WS/LRCLK signal to differentiate each channel and to demark > where the MSB bit is. > > If userspace negotiates one channel, what happens - if WS becomes > static, then there is no signal indicating where the MSB bit is in > the stream, so there is no way for a receiver to synchronise. So, > it is highly unlikely that bclk_ratio = channels * sample_width. > > If the signal continues toggling, what does it do for the inactive > channel - is that just one BCLK period long or does it assume there > is still the second channel? If the former, it means we could end > up with bclk_ratios of 17, 25, 33 which imho is unlikely, and > would mess up TDA998x's CTS measurement. Good point. i2s with a single channel makes no sense. > > What about setups where we have multiple I2S data signals (to support > multi-channel audio) - if we have four channels transmitted on two > data lines (as would be required by the TDA998x), that doesn't mean > BCLK becomes sample_width * 4. > > So, I don't think the number of channels comes into the bclk_ratio > calculation at all. Given the above code, it effectively means we'd > always specify channels = 2 to snd_soc_calc_frame_size(). I notice that hdmi-codec.c supports up to 8 channels in hdmi multi-channel playback mode. If we had a _theoretical_ hdmi xmitter with 8chan support, would the bclk_ratio not be 8 x slot_size - or 8 x 32 if using an fsl_ssi in master mode? This will of course never happen with the tda998x, because .max_i2s_channels = 2, but we are thinking about a generic solution here. > > Given that it is normal to talk about I2S being clocked at "64fs", > "32fs" etc, wouldn't it just be much neater to specify _this_ value > in DT, rather than half that value? > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up > According to speedtest.net: 11.9Mbps down 500kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH 2/2] ASoC: core: standardize snd_soc_dai_set_bclk_ratio() behaviour
snd_soc_dai_set_bclk_ratio() should behave like other
snd_soc_dai_XXX functions, and return -ENOTSUPP if the
callback in driver->ops is NULL.
Signed-off-by: Sven Van Asbroeck
---
sound/soc/soc-core.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 50617db05c46..5611caf25ea3 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2550,10 +2550,11 @@ EXPORT_SYMBOL_GPL(snd_soc_component_set_pll);
*/
int snd_soc_dai_set_bclk_ratio(struct snd_soc_dai *dai, unsigned int ratio)
{
- if (dai->driver->ops->set_bclk_ratio)
- return dai->driver->ops->set_bclk_ratio(dai, ratio);
- else
+ if (dai->driver == NULL)
return -EINVAL;
+ if (dai->driver->ops->set_bclk_ratio == NULL)
+ return -ENOTSUPP;
+ return dai->driver->ops->set_bclk_ratio(dai, ratio);
}
EXPORT_SYMBOL_GPL(snd_soc_dai_set_bclk_ratio);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH 1/2] ASoC: simple-card: add support for bclk_ratio
In some situations, codec configuration will depend on the
bclk_ratio generated by the cpu dai. The tda998x hdmi transmitter
is an example of this.
Allow simple-card to set the bclk_ratio via the 'bclk-slot-ratio'
devicetree property, which describes the bclk to slot rate ratio.
This value is converted to the bclk to sample ratio before being
passed into set_bclk_ratio().
Signed-off-by: Sven Van Asbroeck
---
sound/soc/generic/simple-card.c | 33 -
1 file changed, 32 insertions(+), 1 deletion(-)
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 3fe34417ec89..61e9ba4e9b58 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -25,6 +25,7 @@ struct simple_card_data {
struct asoc_simple_card_data adata;
struct snd_soc_codec_conf *codec_conf;
unsigned int mclk_fs;
+ unsigned int bclk_slot_ratio;
} *dai_props;
struct asoc_simple_jack hp_jack;
struct asoc_simple_jack mic_jack;
@@ -97,7 +98,7 @@ static int asoc_simple_card_hw_params(struct
snd_pcm_substream *substream,
struct simple_card_data *priv = snd_soc_card_get_drvdata(rtd->card);
struct simple_dai_props *dai_props =
simple_priv_to_props(priv, rtd->num);
- unsigned int mclk, mclk_fs = 0;
+ unsigned int mclk, bclk_ratio, mclk_fs = 0, bclk_slot_ratio = 0;
int ret = 0;
if (dai_props->mclk_fs)
@@ -124,6 +125,23 @@ static int asoc_simple_card_hw_params(struct
snd_pcm_substream *substream,
if (ret && ret != -ENOTSUPP)
goto err;
}
+
+ if (dai_props->bclk_slot_ratio)
+ bclk_slot_ratio = dai_props->bclk_slot_ratio;
+
+ if (bclk_slot_ratio) {
+ /* FIXME do we need to care about TDM slots here ? */
+ bclk_ratio = snd_soc_calc_frame_size(bclk_slot_ratio,
+ params_channels(params), 1);
+
+ ret = snd_soc_dai_set_bclk_ratio(codec_dai, bclk_ratio);
+ if (ret && ret != -ENOTSUPP)
+ goto err;
+
+ ret = snd_soc_dai_set_bclk_ratio(cpu_dai, bclk_ratio);
+ if (ret && ret != -ENOTSUPP)
+ goto err;
+ }
return 0;
err:
return ret;
@@ -277,6 +295,12 @@ static int asoc_simple_card_dai_link_of_dpcm(struct
device_node *top,
of_property_read_u32(node, prop, _props->mclk_fs);
of_property_read_u32(np, prop, _props->mclk_fs);
+ snprintf(prop, sizeof(prop), "%sbclk-slot-ratio", prefix);
+ of_property_read_u32(top, PREFIX "bclk-slot-ratio",
+ _props->bclk_slot_ratio);
+ of_property_read_u32(node, prop, _props->bclk_slot_ratio);
+ of_property_read_u32(np, prop, _props->bclk_slot_ratio);
+
ret = asoc_simple_card_parse_daifmt(dev, node, codec,
prefix, _link->dai_fmt);
if (ret < 0)
@@ -349,6 +373,13 @@ static int asoc_simple_card_dai_link_of(struct device_node
*top,
of_property_read_u32(cpu, prop, _props->mclk_fs);
of_property_read_u32(codec, prop, _props->mclk_fs);
+ snprintf(prop, sizeof(prop), "%sbclk-slot-ratio", prefix);
+ of_property_read_u32(top, PREFIX "bclk-slot-ratio",
+ _props->bclk_slot_ratio);
+ of_property_read_u32(node, prop, _props->bclk_slot_ratio);
+ of_property_read_u32(cpu, prop, _props->bclk_slot_ratio);
+ of_property_read_u32(codec, prop, _props->bclk_slot_ratio);
+
ret = asoc_simple_card_parse_cpu(cpu, dai_link,
DAI, CELL, _cpu);
if (ret < 0)
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 1/3] drm/i2c: tda998x: implement different I2S flavours
On Fri, Feb 22, 2019 at 4:27 PM Russell King wrote: > > Add support for the left and right justified I2S formats as well as the > more tranditional "Philips" I2S format. > > Signed-off-by: Russell King My imx6q ssi has i2s and left-justified in common with the tda998x. Left justified does not appear to work, but I believe this is because of missing support for left-justified master mode in fsl_ssi ? So on an imx6q ssi, using i2s philips format only: Tested-by: Sven Van Asbroeck ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 3/3] drm/i2c: tda998x: add support for bclk_ratio
On Fri, Feb 22, 2019 at 4:27 PM Russell King wrote: > > It appears that TDA998x derives the CTS value using the supplied I2S > bit clock (BCLK) rather than 128·fs. TDA998x uses two constants named > m and k in the CTS generator such that we have this relationship > between the source BCLK and the sink fs: > > 128·fs_sink = BCLK·m / k > > Where BCLK = bclk_ratio·fs_source. > > We have been lucky up to now that all users have scaled their bclk_ratio > to match the sample width - for example, on Beagle Bone Black, with a > 16-bit sample width, BCLK = 32·fs, which increases to 64·fs for 32-bit > samples. 24-bit samples are sent as 32-bit. > > We are now starting to see users whose I2S blocks send at 64·fs for > 16-bit samples, which means TDA998x now breaks. > > ASoC has a snd_soc_dai_set_bclk_ratio() call available which sets the > ratio of BCLK to the sample rate. Implement support for this. > > Signed-off-by: Russell King Works with an imx6q ssi, but only if the card driver calls set_bclk_ratio() with the correct value, which is 32/channel for the fsl_ssi in master mode. I will post an RFC patch shortly which adds bclk-ratio support to simple-card. Tested-by: Sven Van Asbroeck ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
On Fri, Feb 22, 2019 at 8:21 AM Russell King - ARM Linux admin
wrote:
>
> On Thu, Feb 21, 2019 at 01:18:13PM -0500, Sven Van Asbroeck wrote:
>
> > [SNDRV_PCM_FORMAT_S24_LE] = {
> > .width = 24, .phys = 32, .le = 1, .signd = 1,
> > .silence = {},
> > },
>
> The above table describes the memory format, not the wire format.
> Look further down for SNDRV_PCM_FORMAT_S24_3LE, which is 24-bit
> packed into three bytes (see include/uapi/sound/asound.h for
> the comment specifying that.)
>
> ASoC uses DAIFMT to specify the on-wire format in connection with
> the above.
>
Interesting ! So you're saying that currently, nobody strictly defines the
layout of the on-wire format, correct? I'm not sure how this works in practice,
because codec and cpu dai should agree on the on-wire format? Except if the
formats used have enough flexibility so you don't have to care.
If so, we don't seem to have this luxury here :(
>
> This doesn't really help in terms of working out what the correct
> settings should be, and other information I have laying around does not
> provide any further enlightenment.
I have access to the NXP software library shipped with the tda19988.
The library's release notes have the following entry:
. "I2S audio does not work, CTS value is not good"
Check the audio I2S format
CTS is automatically computed by the TDA accordingly to the audio input
so accordingly to the upstream settings (like an OMAP ;)
For example, I2S 16 bits or 32 bits do not produce the same CTS value
The config structure which you need to fill in to init the audio has a
"i2s qualifier" field, where you have the choice between 16 and 32 bits.
This then maps to a "Clock Time Stamp factor x" called CTSX, which maps to
the following CTS_N register settings:
CTSX -> CTS_N (m,k)
---
16 -> (3,0)
32 -> (3,1) (i2s qualifier = 16 bits)
48 -> (3,2)
64 -> (3,3) (i2s qualifier = 32 bits)
128 -> (0,0)
Does this information bring us any closer to our assumption that CTS_N needs
to be calculated off the bclk to sample rate ratio ?
>
> I think what I'd like to see is passing of the Fs value into the driver
> from hdmi-codec, but I suspect that requires a bit of work in multiple
> drivers.
>
I'd love to take a shot at this, but first I'd like to understand what you're
suggesting :)
Currently there is set_bclk_ratio() support, but no-one is actually using it.
If hdmi-codec is to retrieve the ratio, wouldn't we need to add .GET_blk_ratio
to snd_soc_dai_ops ?
I could add this to fsl_ssi in master mode, but what if somebody connects the
tda to a cpu dai for which no-one implemented .GET_bclk_ratio ? Do we guess?
Or just error out?
Also, what would a proposed snd_soc_dai_GET_bclk_ratio() return e.g. on
fsl_ssi in slave mode, where the value arguably doesn't exist because the ssi
will accept pretty much anything you throw at it?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
On Fri, Feb 22, 2019 at 4:36 PM Russell King - ARM Linux admin wrote: > There's actually two threads of conversation going, and I recently had > a reply from the maintainer of hdmi-codec suggesting a way forward - so > I've coded that up as the three RFC patches you should have just > received. Thank you, that's awesome ! > It probably would be better to try and find some generic way to deal > with this. > > After all, the I2S source probably knows which ratios it supports. > Given that many sinks support a limited set of values as well, if > ASoC core knew the supported set at each end of an I2S DAI format > link, it could probably select a working bclk ratio automatically. Agree, possibly the same way the ASoC core auto-matches both sides when they are connected with a dai_link? Pardon my ignorance. Of course the auto-matching should only happen when both sides provide a bclk ratio range - to avoid having to retro-fit every single dai. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
Russell, thank you so much for your patience, help and explanation, I really
appreciate it !
On Fri, Feb 22, 2019 at 3:16 PM Russell King - ARM Linux admin
wrote:
>
> A possible solution to that would be for hdmi-codec to default that to
> zero unless it's been definitively provided by the ASoC "card", which
> would allow the old behaviour of selecting the CTS_N M/K values
> depending on the sample width, which we know works for some people.
>
Yes, that would keep the current users in business without them having to
change anything.
Of course, poor souls like myself would have to patch, say, simple-card so we
could provide a bclk ratio in the devicetree. Which would then propagate down
to the tda998x via hdmi-codec. Which is fine by me.
Combining your two previous suggestions, I get the following. Now sample_width
can be removed from tda998x_audio_params, as it's no longer used.
Would this be a good start?
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..a306994ecc79 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -893,19 +893,25 @@ tda998x_configure_audio(struct tda998x_priv *priv,
reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
clksel_aip = AIP_CLKSEL_AIP_I2S;
clksel_fs = AIP_CLKSEL_FS_ACLK;
- switch (params->sample_width) {
+ switch (params->bclk_ratio) {
case 16:
+ cts_n = CTS_N_M(3) | CTS_N_K(0);
+ break;
+ case 32:
cts_n = CTS_N_M(3) | CTS_N_K(1);
break;
- case 18:
- case 20:
- case 24:
+ case 48:
cts_n = CTS_N_M(3) | CTS_N_K(2);
break;
- default:
- case 32:
+ case 64:
cts_n = CTS_N_M(3) | CTS_N_K(3);
break;
+ case 128:
+ cts_n = CTS_N_M(0) | CTS_N_K(0);
+ break;
+ default:
+ dev_err(>hdmi->dev, "unsupported I2S bclk
ratio\n");
+ return -EINVAL;
}
break;
@@ -982,7 +988,7 @@ static int tda998x_audio_hw_params(struct device *dev, void
*data,
struct tda998x_priv *priv = dev_get_drvdata(dev);
int i, ret;
struct tda998x_audio_params audio = {
- .sample_width = params->sample_width,
+ .bclk_ratio = daifmt->bclk_ratio,
.sample_rate = params->sample_rate,
.cea = params->cea,
};
@@ -1004,6 +1010,23 @@ static int tda998x_audio_hw_params(struct device *dev,
void *data,
if (priv->audio_port[i].format == AFMT_I2S)
audio.config = priv->audio_port[i].config;
audio.format = AFMT_I2S;
+ if (!audio.bclk_ratio) {
+ /* compatibility */
+ switch (params->sample_width) {
+ case 16:
+ audio.bclk_ratio = 32;
+ break;
+ case 18:
+ case 20:
+ case 24:
+ audio.bclk_ratio = 48;
+ break;
+ default:
+ case 32:
+ audio.bclk_ratio = 64;
+ break;
+ }
+ }
break;
case HDMI_SPDIF:
for (i = 0; i < ARRAY_SIZE(priv->audio_port); i++)
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3cb25ccbe5e6..039e1d9af2e0 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -14,7 +14,7 @@ enum {
struct tda998x_audio_params {
u8 config;
u8 format;
- unsigned sample_width;
+ u8 bclk_ratio;
unsigned sample_rate;
struct hdmi_audio_infoframe cea;
u8 status[5];
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 9483c55f871b..0fca69880dc3 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -42,6 +42,7 @@ struct hdmi_codec_daifmt {
unsigned int frame_clk_inv:1;
unsigned int bit_clk_master:1;
unsigned int frame_clk_master:1;
+ unsigned int bclk_ratio;
};
/*
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index d00734d31e04..d82f26854c90 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -524,6 +524,17 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream
*substream,
>daifmt[dai->id], );
}
+static int hdmi_codec_set_bclk_ratio(struct
[PATCH 2/2] sound: hdmi-codec: propagate physical_width
Infrastructure patch which allows the tda998x patch to
build.
This patch will be submitted for review if/when the
tda998x patch gets accepted.
Signed-off-by: Sven Van Asbroeck
---
include/sound/hdmi-codec.h| 1 +
sound/soc/codecs/hdmi-codec.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h
index 9483c55f871b..5ba3db37e349 100644
--- a/include/sound/hdmi-codec.h
+++ b/include/sound/hdmi-codec.h
@@ -52,6 +52,7 @@ struct hdmi_codec_params {
struct snd_aes_iec958 iec;
int sample_rate;
int sample_width;
+ int physical_width;
int channels;
};
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index e5b6769b9797..007f20f2c5ac 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -518,6 +518,7 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream
*substream,
hp.sample_width = params_width(params);
hp.sample_rate = params_rate(params);
+ hp.physical_width = params_physical_width(params);
hp.channels = params_channels(params);
return hcp->hcd.ops->hw_params(dai->dev->parent, hcp->hcd.data,
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/i2c: tda998x: adjust CTS_N audio pre-divider calculation
My cpu dai driving the tda998x in master mode outputs
SNDRV_PCM_FORMAT_S24_LE, i2s left justified, two channels:
[SNDRV_PCM_FORMAT_S24_LE] = {
.width = 24, .phys = 32, .le = 1, .signd = 1,
.silence = {},
},
This format has a sample width of 24 bits, but a physical
size of 32 bits per channel. The redundant bits are padded
with zeros. This gives a total frame width of 64 bits.
According to the tda19988 datasheet, this format is supported:
"Audio samples with a precision better than 24-bit are
truncated to 24-bit. [...] If the input clock has a
frequency of 64fs and is left justified or Philips,
the audio word is truncated to 24-bit format and other
bits padded with zeros."
(tda19988 product datasheet Rev. 3 - 21 July 2011)
However, supplying this format to the chip results in distorted
audio. I noticed that the audio problem disappears when the
CTS_N pre-divider is calculated from the physical width, _not_
the sample width.
This patch adjusts the CTS_N calculation so that the audio
distortion goes away.
Caveats:
- I am only capable of generating audio with a frame width of
64fs. So I cannot test if 32fs or 48fs audio formats still
work. Such as S16_LE or S20_LE.
- I have no access to a datasheet which accurately describes
the CTS_N register. So I cannot check if my assumption is
correct.
Tested with an imx6 ssi.
Cc: Peter Rosin
Signed-off-by: Sven Van Asbroeck
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 ++--
include/drm/i2c/tda998x.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index a7c39f39793f..ba9f55176e1b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -893,7 +893,7 @@ tda998x_configure_audio(struct tda998x_priv *priv,
reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
clksel_aip = AIP_CLKSEL_AIP_I2S;
clksel_fs = AIP_CLKSEL_FS_ACLK;
- switch (params->sample_width) {
+ switch (params->physical_width) {
case 16:
cts_n = CTS_N_M(3) | CTS_N_K(1);
break;
@@ -982,7 +982,7 @@ static int tda998x_audio_hw_params(struct device *dev, void
*data,
struct tda998x_priv *priv = dev_get_drvdata(dev);
int i, ret;
struct tda998x_audio_params audio = {
- .sample_width = params->sample_width,
+ .physical_width = params->physical_width,
.sample_rate = params->sample_rate,
.cea = params->cea,
};
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3cb25ccbe5e6..7e9fddcc4ddd 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -14,7 +14,7 @@ enum {
struct tda998x_audio_params {
u8 config;
u8 format;
- unsigned sample_width;
+ unsigned int physical_width;
unsigned sample_rate;
struct hdmi_audio_infoframe cea;
u8 status[5];
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
