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

From: Manivannan Sadhasivam
Date: Sat Jun 29 2024 - 08:03:51 EST


On Sat, Jun 29, 2024 at 04:03:28PM +0800, Slark Xiao wrote:
>
> At 2024-06-28 22:38:57, "Jeffrey Hugo" <quic_jhugo@xxxxxxxxxxx> 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@xxxxxxx>
> >> ---
> >> 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

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