Re: Re: [PATCH v3 2/3] bus: mhi: host: Add name for mhi_controller

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Sat, Jun 29, 2024 at 04:03:28PM +0800, Slark Xiao wrote:
> 
> At 2024-06-28 22:38:57, "Jeffrey Hugo" <quic_jhugo@quicinc.com> wrote:
> >On 6/28/2024 1:36 AM, Slark Xiao wrote:
> >>   For SDX72 MBIM mode, 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)".
> >
> 
> >Weird indentation
> 
> My mistake. Will be corrected in next.
> 
> >
> >>   In oder to fix this issue, we decide to use the modem name
> >
> >"order"
> >
> >> to do a match in client driver side. Then client driver could
> >> set a corresponding mux_id value for this modem product.
> >> 
> >> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> >> ---
> >>   drivers/bus/mhi/host/pci_generic.c | 1 +
> >>   include/linux/mhi.h                | 2 ++
> >>   2 files changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> >> index 1fb1c2f2fe12..14a11880bcea 100644
> >> --- a/drivers/bus/mhi/host/pci_generic.c
> >> +++ b/drivers/bus/mhi/host/pci_generic.c
> >> @@ -1086,6 +1086,7 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>   	mhi_cntrl->runtime_get = mhi_pci_runtime_get;
> >>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
> >>   	mhi_cntrl->mru = info->mru_default;
> >> +	mhi_cntrl->name = info->name;
> >>   
> >>   	if (info->edl_trigger)
> >>   		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> >> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> >> index b573f15762f8..86aa4f52842c 100644
> >> --- a/include/linux/mhi.h
> >> +++ b/include/linux/mhi.h
> >> @@ -361,6 +361,7 @@ struct mhi_controller_config {
> >>    * @wake_set: Device wakeup set flag
> >>    * @irq_flags: irq flags passed to request_irq (optional)
> >>    * @mru: the default MRU for the MHI device
> >> + * @name: name of the modem
> >
> 
> >Why restrict this to modems?  There are plenty of other MHI devices
> 
> Actually all MHI devices could be called modems. I don't think this is
> a wrong name.
> 

No, not all MHI controllers are modems. This driver is a generic driver for MHI
controllers. So use below description:

	'Product or device name of the MHI controller'

> >
> >>    *
> >>    * Fields marked as (required) need to be populated by the controller driver
> >>    * before calling mhi_register_controller(). For the fields marked as (optional)
> >> @@ -445,6 +446,7 @@ struct mhi_controller {
> >>   	bool wake_set;
> >>   	unsigned long irq_flags;
> >>   	u32 mru;
> >> +	const char *name;
> >
> 
> >Please run pahole
> 
> Emm, just checked,  there are 3 holes:
>     u32                        M3;                   /*   312     4 */
>     /* XXX 4 bytes hole, try to pack */
> ...
>     bool                       wake_set;             /*   526     1 */
>     /* XXX 1 byte hole, try to pack */
> ...
>     u32                        mru;                  /*   536     4 */
>     /* XXX 4 bytes hole, try to pack */
> 
> I will put 'const char *name' above 'u32 mru' to avoid the last hole.
> Is this okay?
> 

Just put it at the top.

- Mani

-- 
மணிவண்ணன் சதாசிவம்