Re: [PATCH v2 2/2] net: wwan: mhi: make default data link id configurable

[Sergey Ryazanov <ryazanov.s.a@gmail.com>]

Hello Slark, Manivannan,

On 12.06.2024 12:39, Slark Xiao wrote:
> For SDX72 MBIM device, it starts data mux id from 112 instead of 0.
> This would lead to device can't ping outside successfully.
> Also MBIM side would report "bad packet session (112)".
> So we add a link id default value for these SDX72 products which
> works in MBIM mode.

The patch itself looks good to me except a tiny nitpick (see below). 
Meanwhile, I can not understand when we should merge it. During the V1 
discussion, It was mentioned that we need this change specifically for 
Foxconn SDX72 modem. Without any actual users the configurable default 
data link id is a dead code.

According to the ARM MSM patchwork [1], the main Foxconn SDX72 
introducing patch is (a) not yet merged, (b) no more applicable. So, as 
far as I understand, it should be resend. In this context, a best way to 
merge the modem support is to prepend the modem introduction patch with 
these changes forming a series:
1/3: bus: mhi: host: Import mux_id item
2/3: net: wwan: mhi: make default data link id configurable
3/3: bus: mhi: host: Add Foxconn SDX72 related support

And merge the series as whole, when everything will be ready. This will 
help us to avoid partially merged work and will keep the modem support 
introduction clear.

Manivannan, could you share the main [1] Foxconn SDX72 introduction 
patch status, and your thoughts regarding the merging process?


1. 
https://patchwork.kernel.org/project/linux-arm-msm/patch/20240520070633.308913-1-slark_xiao@163.com/

> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>   drivers/net/wwan/mhi_wwan_mbim.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wwan/mhi_wwan_mbim.c b/drivers/net/wwan/mhi_wwan_mbim.c
> index 3f72ae943b29..c731fe20814f 100644
> --- a/drivers/net/wwan/mhi_wwan_mbim.c
> +++ b/drivers/net/wwan/mhi_wwan_mbim.c
> @@ -618,7 +618,8 @@ static int mhi_mbim_probe(struct mhi_device *mhi_dev, const struct mhi_device_id
>   	mbim->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>   
>   	/* Register wwan link ops with MHI controller representing WWAN instance */
> -	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim, 0);
> +	return wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_mbim_wwan_ops, mbim,
> +		mhi_dev->mhi_cntrl->link_id);

Just a nitpick. The second line had better be aligned with the opening 
bracket:

return wwan_register_ops(&cntrl->...
                          mhi_dev->...

>   }
>   
>   static void mhi_mbim_remove(struct mhi_device *mhi_dev)

--
Sergey