Re: [PATCH 1/2] mailbox: Add support for bcm74110 mailbox

[Krzysztof Kozlowski]

On 27/03/2025 23:16, justin.chen@xxxxxxxxxxxx wrote:
> +static int bcm74110_mbox_init(struct bcm74110_mbox *mbox)
> +{
> +	int ret = 0;
> +
> +	/* Disable queues tx/rx */
> +	writel_relaxed(0x0, mbox->base_tx + BCM_MBOX_CTRL);
> +
> +	/* Clear status & restart tx/rx*/
> +	writel_relaxed(BCM_MBOX_CTRL_EN | BCM_MBOX_CTRL_CLR,
> +		       mbox->base_tx + BCM_MBOX_CTRL);
> +
> +	/* Unmask irq */
> +	writel_relaxed(BCM_MBOX_IRQ_NOT_EMPTY, mbox->base_rx_irq +
> +		       BCM_MBOX_IRQ_MASK_CLEAR);
> +
> +	ret = bcm74110_mbox_link_training(mbox);
> +	if (ret) {
> +		dev_err(&mbox->pdev->dev, "Training failed\n");
> +		return ret;
> +	}
> +
> +	return bcm74110_mbox_shmem_init(mbox);
> +}
> +
> +
> +

Just one blank line




> +static int bcm74110_mbox_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct bcm74110_mbox *mbox;
> +	int i, ret;
> +
> +	mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> +	if (!mbox)
> +		return -ENOMEM;
> +
> +	mbox->pdev = pdev;
> +	platform_set_drvdata(pdev, mbox);
> +
> +	mbox->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mbox->base))
> +		return dev_err_probe(dev, PTR_ERR(mbox->base), "Failed to iomap\n");
> +
> +	ret = of_property_read_u32(dev->of_node, "brcm,tx", &mbox->tx_chan);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to find tx channel\n");
> +
> +	ret = of_property_read_u32(dev->of_node, "brcm,rx", &mbox->rx_chan);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to find rx channel\n");
> +
> +	ret = of_property_read_u32_index(dev->of_node, "brcm,shmem", 0,
> +					 &mbox->shmem_offset);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get shmem offset\n");
> +
> +	ret = of_property_read_u32_index(dev->of_node, "brcm,shmem", 1,
> +					 &mbox->shmem_size);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get shmem size\n");
> +
> +	mbox->rx_irq = platform_get_irq(pdev, 0);
> +	if (mbox->rx_irq <= 0)


That's not correct check: look at help for this function.

> +		return dev_err_probe(dev, -EINVAL, "Failed to get irq\n");
> +
> +	mbox->base_tx = mbox->base + BCM_MBOX_BASE(mbox->tx_chan);
> +	mbox->base_rx = mbox->base + BCM_MBOX_BASE(mbox->rx_chan);
> +	mbox->base_rx_irq = mbox->base + BCM_MBOX_IRQ_BASE(mbox->rx_chan);

And all this suggests DT properties are not correct. Why would you
offset base? What is the memory layout of all your mbox controllers?

> +
> +	INIT_LIST_HEAD(&mbox->rx_svc_init_list);
> +	spin_lock_init(&mbox->rx_svc_list_lock);
> +	bcm74110_mbox_mask_and_clear(mbox);
> +
> +	ret = devm_request_irq(dev, mbox->rx_irq, bcm74110_mbox_isr,
> +			       IRQF_NO_SUSPEND, pdev->name, mbox);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +	mbox->controller.ops = &bcm74110_mbox_chan_ops;
> +	mbox->controller.dev = dev;
> +	mbox->controller.num_chans = BCM_MSG_SVC_MAX;
> +	mbox->controller.of_xlate = &bcm74110_mbox_of_xlate;
> +	mbox->controller.chans = devm_kcalloc(dev, BCM_MSG_SVC_MAX,
> +					      sizeof(*mbox->controller.chans),
> +					      GFP_KERNEL);
> +	if (!mbox->controller.chans)
> +		return -ENOMEM;
> +
> +	mbox->mbox_chan = devm_kcalloc(dev, BCM_MSG_SVC_MAX,
> +				       sizeof(*mbox->mbox_chan),
> +				       GFP_KERNEL);
> +	if (!mbox->mbox_chan)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < BCM_MSG_SVC_MAX; i++) {
> +		mbox->mbox_chan[i].mbox = mbox;
> +		mbox->controller.chans[i].con_priv = &mbox->mbox_chan[i];
> +	}
> +
> +	ret = devm_mbox_controller_register(dev, &mbox->controller);
> +	if (ret)
> +		return ret;
> +
> +	ret = bcm74110_mbox_init(mbox);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(dev, "Mailbox inited with %d channels\n", BCM_MSG_SVC_MAX);

Drop, drivers are supposed to be silent on success.



Best regards,
Krzysztof