Hi Tzung-Bi,
Thanks for your suggestions.
On Wed, 2021-11-24 at 18:25 +0800, Tzung-Bi Shih wrote:
> On Wed, Nov 24, 2021 at 04:45:14PM +0800, allen-kh.cheng wrote:
> > drivers/firmware/Kconfig | 1 +
> > drivers/firmware/Makefile | 1 +
> > drivers/firmware/mediatek/Kconfig | 10 ++
> > drivers/firmware/mediatek/Makefile | 2 +
> > drivers/firmware/mediatek/mtk-adsp-ipc.c | 130
> > ++++++++++++++++++
> > .../linux/firmware/mediatek/mtk-adsp-ipc.h | 72 ++++++++++
> > 6 files changed, 216 insertions(+)
> > create mode 100644 drivers/firmware/mediatek/Kconfig
> > create mode 100644 drivers/firmware/mediatek/Makefile
> > create mode 100644 drivers/firmware/mediatek/mtk-adsp-ipc.c
> > create mode 100644 include/linux/firmware/mediatek/mtk-adsp-ipc.h
>
> The patch should move before the 2nd patch in the series as the 2nd
> patch uses mtk-adsp-ipc.h.
>
> > diff --git a/drivers/firmware/mediatek/mtk-adsp-ipc.c
> > b/drivers/firmware/mediatek/mtk-adsp-ipc.c
>
> [...]
> > +int adsp_ipc_send(struct mtk_adsp_ipc *ipc, unsigned int idx,
> > uint32_t op)
> > +{
> > + struct mtk_adsp_chan *dsp_chan = &ipc->chans[idx];
> > + struct adsp_mbox_ch_info *ch_info = dsp_chan->ch->con_priv;
> > + int ret;
> > +
> > + if (idx >= MTK_ADSP_MBOX_NUM)
> > + return -EINVAL;
>
> If idx >= MTK_ADSP_MBOX_NUM, the invalid memory access has occurred
> at beginning of the function.
>
> > +static int mtk_adsp_ipc_probe(struct platform_device *pdev)
> > +{
>
> [...]
> > + device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
>
> Why does it need to call device_set_of_node_from_dev()?
The original design regards mt8195 sof of_node as a parent deivce of
mtk-adsp-ipc.
device_set_of_node_from_dev will set of_node_reuse flag to prevent
driver from attempting to claim any mbox ipc resources already claimed
by the sof dsp device.
>
> > + for (i = 0; i < MTK_ADSP_MBOX_NUM; i++) {
> > + chan_name = kasprintf(GFP_KERNEL, "mbox%d", i);
> > + if (!chan_name)
> > + return -ENOMEM;
> > +
> > + dsp_chan = &dsp_ipc->chans[i];
> > + cl = &dsp_chan->cl;
> > + cl->dev = dev->parent;
> > + cl->tx_block = false;
> > + cl->knows_txdone = false;
> > + cl->tx_prepare = NULL;
> > + cl->rx_callback = adsp_ipc_recv;
> > +
> > + dsp_chan->ipc = dsp_ipc;
> > + dsp_chan->idx = i;
> > + dsp_chan->ch = mbox_request_channel_byname(cl,
> > chan_name);
> > + if (IS_ERR(dsp_chan->ch)) {
> > + ret = PTR_ERR(dsp_chan->ch);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to request mbox
> > chan %d ret %d\n",
> > + i, ret);
>
> If ret == -EPROBE_DEFER, wouldn't it need to return
> -EPROBE_DEFER? It doesn't retry later if -EPROBE_DEFER.
If ret != -EPROBE_DEFER, it will show a error message then goto out.
If ret == -EPROBE_DEFER, probe funcation also will goto out.
Both of them will return ret. will not go to next round.
Thanks,
Allen
