diff mbox series

[v2] PCI: Add vendor ID for Quectel and Cinterion

Message ID 20221028023711.4196-1-slark_xiao@163.com
State New
Headers
Series [v2] PCI: Add vendor ID for Quectel and Cinterion |

Commit Message

Slark Xiao Oct. 28, 2022, 2:37 a.m. UTC 
  In MHI driver, there are some companies product still do not have their
 own PCI vendor macro. So we add it here to make the code neat. Ref ID
 could be found in link https://pcisig.com/membership/member-companies
 and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
their IOT modem card's trademark. So you will find 0x1269 belongs to
Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
 Thales.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
 include/linux/pci_ids.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Bagas Sanjaya Oct. 28, 2022, 3:51 a.m. UTC | #1 
On Fri, Oct 28, 2022 at 10:37:11AM +0800, Slark Xiao wrote:
> In MHI driver, there are some companies product still do not have their
>  own PCI vendor macro. So we add it here to make the code neat. Ref ID
>  could be found in link https://pcisig.com/membership/member-companies
>  and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
> their IOT modem card's trademark. So you will find 0x1269 belongs to
> Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
>  Thales.
> 

The patch description is confusing me.

What about below instead?

```
Add missing vendor ID for Cinterion (which is 0x1269).
```

> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>  include/linux/pci_ids.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index b362d90eb9b0..9e2b6286f53f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1765,6 +1765,8 @@
>  #define PCI_VENDOR_ID_SATSAGEM		0x1267
>  #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
>  
> +#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/
> +
>  #define PCI_VENDOR_ID_ENSONIQ		0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
> @@ -2585,6 +2587,8 @@
>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>  
> +#define PCI_VENDOR_ID_QUECTEL		0x1eac
> +

Why pourring in Quectel ID while this patch is about Cinterion?

Thanks.
Slark Xiao Oct. 28, 2022, 3:57 a.m. UTC | #2 
At 2022-10-28 11:51:23, "Bagas Sanjaya" <bagasdotme@gmail.com> wrote:
>On Fri, Oct 28, 2022 at 10:37:11AM +0800, Slark Xiao wrote:
>> In MHI driver, there are some companies product still do not have their
>>  own PCI vendor macro. So we add it here to make the code neat. Ref ID
>>  could be found in link https://pcisig.com/membership/member-companies
>>  and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
>> their IOT modem card's trademark. So you will find 0x1269 belongs to
>> Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
>>  Thales.
>> 
>
>The patch description is confusing me.
>
>What about below instead?
>
>```
>Add missing vendor ID for Cinterion (which is 0x1269).
>```
>
As you said 0x1269 belongs to Thales (not Cinterion), So I write it about the details.

>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>  include/linux/pci_ids.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index b362d90eb9b0..9e2b6286f53f 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -1765,6 +1765,8 @@
>>  #define PCI_VENDOR_ID_SATSAGEM		0x1267
>>  #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
>>  
>> +#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/
>> +
>>  #define PCI_VENDOR_ID_ENSONIQ		0x1274
>>  #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
>>  #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
>> @@ -2585,6 +2587,8 @@
>>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>>  
>> +#define PCI_VENDOR_ID_QUECTEL		0x1eac
>> +
>
>Why pourring in Quectel ID while this patch is about Cinterion?
>
>Thanks. 
>
Patch title is for 2 vendors,  Quectel and Cinterion. May I add 2 different VID at the 
same time?
>-- 
>An old man doll... just what I always wanted! - Clara
Bagas Sanjaya Oct. 28, 2022, 4 a.m. UTC | #3 
On 10/28/22 10:57, Slark Xiao wrote:
> At 2022-10-28 11:51:23, "Bagas Sanjaya" <bagasdotme@gmail.com> wrote:
>> On Fri, Oct 28, 2022 at 10:37:11AM +0800, Slark Xiao wrote:
>>> In MHI driver, there are some companies product still do not have their
>>>  own PCI vendor macro. So we add it here to make the code neat. Ref ID
>>>  could be found in link https://pcisig.com/membership/member-companies
>>>  and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
>>> their IOT modem card's trademark. So you will find 0x1269 belongs to
>>> Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
>>>  Thales.
>>>
>>
>> The patch description is confusing me.
>>
>> What about below instead?
>>
>> ```
>> Add missing vendor ID for Cinterion (which is 0x1269).
>> ```

Oops, I mean also for Quectel (0x1eac).

>>
> As you said 0x1269 belongs to Thales (not Cinterion), So I write it about the details.
> 
>>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>>> ---
>>>  include/linux/pci_ids.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>>> index b362d90eb9b0..9e2b6286f53f 100644
>>> --- a/include/linux/pci_ids.h
>>> +++ b/include/linux/pci_ids.h
>>> @@ -1765,6 +1765,8 @@
>>>  #define PCI_VENDOR_ID_SATSAGEM		0x1267
>>>  #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
>>>  
>>> +#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/
>>> +
>>>  #define PCI_VENDOR_ID_ENSONIQ		0x1274
>>>  #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
>>>  #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
>>> @@ -2585,6 +2587,8 @@
>>>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>>>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>>>  
>>> +#define PCI_VENDOR_ID_QUECTEL		0x1eac
>>> +
>>
>> Why pourring in Quectel ID while this patch is about Cinterion?
>>
>> Thanks. 
>>
> Patch title is for 2 vendors,  Quectel and Cinterion. May I add 2 different VID at the 
> same time?

Ah! I don't see the patch subject.

Next time, please just send plain-text email, not HTML.

Thanks.
Bjorn Helgaas Oct. 28, 2022, 4:06 p.m. UTC | #4 
On Fri, Oct 28, 2022 at 10:37:11AM +0800, Slark Xiao wrote:
> In MHI driver, there are some companies product still do not have their
>  own PCI vendor macro. So we add it here to make the code neat. Ref ID
>  could be found in link https://pcisig.com/membership/member-companies
>  and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
> their IOT modem card's trademark. So you will find 0x1269 belongs to
> Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
>  Thales.

There should not be spaces at the beginning of these lines.

Don't bother with the sourceforge URL; I don't think that's really
useful here.

> Signed-off-by: Slark Xiao <slark_xiao@163.com>
> ---
>  include/linux/pci_ids.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index b362d90eb9b0..9e2b6286f53f 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -1765,6 +1765,8 @@
>  #define PCI_VENDOR_ID_SATSAGEM		0x1267
>  #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
>  
> +#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/

This should identify the *vendor*, not a trademark for a specific
product line.  And it should correspond somehow with the PCI-SIG
registration.  So I think PCI_VENDOR_ID_THALES would be more
appropriate here.

I think the best thing here would be two patches.  One patch would add
PCI_VENDOR_ID_THALES to pci_ids.h and also add a use of it in the MHI
driver.  The second patch would do the same for PCI_VENDOR_ID_QUECTEL.

Then each one is logically self-contained.

>  #define PCI_VENDOR_ID_ENSONIQ		0x1274
>  #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
>  #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
> @@ -2585,6 +2587,8 @@
>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>  
> +#define PCI_VENDOR_ID_QUECTEL		0x1eac
> +
>  #define PCI_VENDOR_ID_TEHUTI		0x1fc9
>  #define PCI_DEVICE_ID_TEHUTI_3009	0x3009
>  #define PCI_DEVICE_ID_TEHUTI_3010	0x3010
> -- 
> 2.34.1
>
Slark Xiao Oct. 31, 2022, 9:08 a.m. UTC | #5 
At 2022-10-29 00:06:21, "Bjorn Helgaas" <helgaas@kernel.org> wrote:
>On Fri, Oct 28, 2022 at 10:37:11AM +0800, Slark Xiao wrote:
>> In MHI driver, there are some companies product still do not have their
>>  own PCI vendor macro. So we add it here to make the code neat. Ref ID
>>  could be found in link https://pcisig.com/membership/member-companies
>>  and https://pciids.sourceforge.net/pci.ids . Thales use Cinterion as
>> their IOT modem card's trademark. So you will find 0x1269 belongs to
>> Thales. Actually, Cinterion belongs to Gemalto, and Gemalto belongs to
>>  Thales.
>
>There should not be spaces at the beginning of these lines.
>
>Don't bother with the sourceforge URL; I don't think that's really
>useful here.
 The space issue will be fixed in next patch.
>
>> Signed-off-by: Slark Xiao <slark_xiao@163.com>
>> ---
>>  include/linux/pci_ids.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index b362d90eb9b0..9e2b6286f53f 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -1765,6 +1765,8 @@
>>  #define PCI_VENDOR_ID_SATSAGEM		0x1267
>>  #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
>>  
>> +#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/
>
>This should identify the *vendor*, not a trademark for a specific
>product line.  And it should correspond somehow with the PCI-SIG
>registration.  So I think PCI_VENDOR_ID_THALES would be more
>appropriate here.
>
Yeah, currently it's used by IOT modules which support PCIE. But we
can't know if they will use this VID for other non-IOT modules product.
Thales would be better.

>I think the best thing here would be two patches.  One patch would add
>PCI_VENDOR_ID_THALES to pci_ids.h and also add a use of it in the MHI
>driver.  The second patch would do the same for PCI_VENDOR_ID_QUECTEL.
>
>Then each one is logically self-contained.
I must make sure these ids are applied, then I could commit the changes in MHI
driver side. Otherwise it will cause build error.
So I combine QUECTEL with THALES as a single patch. Shall I separate it from
each other? 1 for PCI IDs, and another for MHI change. It would be better, I think.
>
>>  #define PCI_VENDOR_ID_ENSONIQ		0x1274
>>  #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
>>  #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
>> @@ -2585,6 +2587,8 @@
>>  #define PCI_VENDOR_ID_TEKRAM		0x1de1
>>  #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
>>  
>> +#define PCI_VENDOR_ID_QUECTEL		0x1eac
>> +
>>  #define PCI_VENDOR_ID_TEHUTI		0x1fc9
>>  #define PCI_DEVICE_ID_TEHUTI_3009	0x3009
>>  #define PCI_DEVICE_ID_TEHUTI_3010	0x3010
>> -- 
>> 2.34.1
>>
Bjorn Helgaas Oct. 31, 2022, 2:58 p.m. UTC | #6 
On Mon, Oct 31, 2022 at 05:08:18PM +0800, Slark Xiao wrote:
> At 2022-10-29 00:06:21, "Bjorn Helgaas" <helgaas@kernel.org> wrote:

> >I think the best thing here would be two patches.  One patch would add
> >PCI_VENDOR_ID_THALES to pci_ids.h and also add a use of it in the MHI
> >driver.  The second patch would do the same for PCI_VENDOR_ID_QUECTEL.
> >
> >Then each one is logically self-contained.
>
> I must make sure these ids are applied, then I could commit the
> changes in MHI driver side. Otherwise it will cause build error.
>
> So I combine QUECTEL with THALES as a single patch. Shall I separate
> it from each other? 1 for PCI IDs, and another for MHI change. It
> would be better, I think.

Make two patches:

  - A single patch that adds PCI_VENDOR_ID_THALES to pci_ids.h and
    also adds a use of it in the MHI driver.

  - A single patch that adds PCI_VENDOR_ID_QUECTEL to pci_ids.h and
    also adds a use of it in the MHI driver.

That way there's no possibility of build error and no need to
coordinate patches.

Similar previous work:

  https://git.kernel.org/linus/fae74feacd2d ("hisi_acc_qm: Move VF PCI device IDs to common header")
  https://git.kernel.org/linus/d3826a95222c ("nfp: add support for NFP3800/NFP3803 PCIe devices")
  https://git.kernel.org/linus/4fb0abfee424 ("x86/amd_nb: Add AMD Family 19h Models (10h-1Fh) and (A0h-AFh) PCI IDs")
diff mbox series

Patch

diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index b362d90eb9b0..9e2b6286f53f 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1765,6 +1765,8 @@ 
 #define PCI_VENDOR_ID_SATSAGEM		0x1267
 #define PCI_DEVICE_ID_SATSAGEM_NICCY	0x1016
 
+#define PCI_VENDOR_ID_CINTERION		0x1269	/* Celluar Modules*/
+
 #define PCI_VENDOR_ID_ENSONIQ		0x1274
 #define PCI_DEVICE_ID_ENSONIQ_CT5880	0x5880
 #define PCI_DEVICE_ID_ENSONIQ_ES1370	0x5000
@@ -2585,6 +2587,8 @@ 
 #define PCI_VENDOR_ID_TEKRAM		0x1de1
 #define PCI_DEVICE_ID_TEKRAM_DC290	0xdc29
 
+#define PCI_VENDOR_ID_QUECTEL		0x1eac
+
 #define PCI_VENDOR_ID_TEHUTI		0x1fc9
 #define PCI_DEVICE_ID_TEHUTI_3009	0x3009
 #define PCI_DEVICE_ID_TEHUTI_3010	0x3010