diff mbox series

[v2] bus: mhi: host: pci_generic: Add macro for some VIDs

Message ID 20221107084826.8888-1-slark_xiao@163.com
State New
Headers
Series [v2] bus: mhi: host: pci_generic: Add macro for some VIDs |

Commit Message

Slark Xiao Nov. 7, 2022, 8:48 a.m. UTC 
  To make code neat and for convenience purpose, use macro for
some VIDs. These macros are supposed to be added to pci_ids.h.
But until the macros are used in multiple places, it is not
recommended. So adding it locally for now.

Signed-off-by: Slark Xiao <slark_xiao@163.com>
---
v2: Update description and format issue
---
 drivers/bus/mhi/host/pci_generic.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Greg KH Nov. 7, 2022, 9:02 a.m. UTC | #1 
On Mon, Nov 07, 2022 at 04:48:26PM +0800, Slark Xiao wrote:
> To make code neat and for convenience purpose, use macro for
> some VIDs. These macros are supposed to be added to pci_ids.h.

No, they are not supposed to be added there at all.

And they are not a "macro", it is a "#define".

> But until the macros are used in multiple places, it is not
> recommended. So adding it locally for now.

Again, these are not macros

thanks,

greg k-h
Slark Xiao Nov. 7, 2022, 9:30 a.m. UTC | #2 
At 2022-11-07 17:02:29, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Mon, Nov 07, 2022 at 04:48:26PM +0800, Slark Xiao wrote:
>> To make code neat and for convenience purpose, use macro for
>> some VIDs. These macros are supposed to be added to pci_ids.h.
>
>No, they are not supposed to be added there at all.
>
>And they are not a "macro", it is a "#define".
>
>> But until the macros are used in multiple places, it is not
>> recommended. So adding it locally for now.
>
>Again, these are not macros
>
>thanks,
>
>greg k-h

Hi Greg,
Thanks for your comment.
In my opinion, MACRO almost same as a '#define'.  May I know how do 
you call such definition?

And could you give your comments in previous patch, not the 'final' one?
In another pci_ids patch, you break it in v3 and break it here again in v2.
Honestly,  it's positive for whole project. But it's negative  for contributor 
and maintainer. 

Finally, it's welcome to point out the error.
Greg KH Nov. 7, 2022, 9:53 a.m. UTC | #3 
On Mon, Nov 07, 2022 at 05:30:56PM +0800, Slark Xiao wrote:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> At 2022-11-07 17:02:29, "Greg KH" <gregkh@linuxfoundation.org> wrote:
> >On Mon, Nov 07, 2022 at 04:48:26PM +0800, Slark Xiao wrote:
> >> To make code neat and for convenience purpose, use macro for
> >> some VIDs. These macros are supposed to be added to pci_ids.h.
> >
> >No, they are not supposed to be added there at all.
> >
> >And they are not a "macro", it is a "#define".
> >
> >> But until the macros are used in multiple places, it is not
> >> recommended. So adding it locally for now.
> >
> >Again, these are not macros
> >
> >thanks,
> >
> >greg k-h
> 
> Hi Greg,
> Thanks for your comment.
> In my opinion, MACRO almost same as a '#define'.  May I know how do 
> you call such definition?

As I said, this is just a define, not a macro at all.

> And could you give your comments in previous patch, not the 'final' one?

I do not understand, what previous patrch?  What "final" one?  What is
the "latest" patch?

> In another pci_ids patch, you break it in v3 and break it here again in v2.

I broke what?

> Honestly,  it's positive for whole project. But it's negative  for contributor 
> and maintainer. 

I am totally confused here and do not understand what you are referring
to, sorry.

greg k-h
Slark Xiao Nov. 7, 2022, 10:26 a.m. UTC | #4 
At 2022-11-07 17:53:57, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Mon, Nov 07, 2022 at 05:30:56PM +0800, Slark Xiao wrote:
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> At 2022-11-07 17:02:29, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>> >On Mon, Nov 07, 2022 at 04:48:26PM +0800, Slark Xiao wrote:
>> >> To make code neat and for convenience purpose, use macro for
>> >> some VIDs. These macros are supposed to be added to pci_ids.h.
>> >
>> >No, they are not supposed to be added there at all.
>> >
>> >And they are not a "macro", it is a "#define".
>> >
>> >> But until the macros are used in multiple places, it is not
>> >> recommended. So adding it locally for now.
>> >
>> >Again, these are not macros
>> >
>> >thanks,
>> >
>> >greg k-h
>> 
>> Hi Greg,
>> Thanks for your comment.
>> In my opinion, MACRO almost same as a '#define'.  May I know how do 
>> you call such definition?
>
>As I said, this is just a define, not a macro at all.
>
>> And could you give your comments in previous patch, not the 'final' one?
>
>I do not understand, what previous patrch?  What "final" one?  What is
>the "latest" patch?
previous patch:
https://lore.kernel.org/lkml/20221027115123.5326-1-slark_xiao@163.com/
https://lore.kernel.org/lkml/20221028023711.4196-1-slark_xiao@163.com/
https://lore.kernel.org/lkml/20221102024437.15248-1-slark_xiao@163.com/

'final' patch:
https://lore.kernel.org/lkml/20221107084826.8888-1-slark_xiao@163.com/
https://lore.kernel.org/lkml/20221101015858.6777-1-slark_xiao@163.com/

The 'final' patch was committed according to the advice of the feature
maintainer. 
>
>> In another pci_ids patch, you break it in v3 and break it here again in v2.
>
>I broke what?
You could have  voiced out such comment in V1, V2 before the 'final'. 
>
>> Honestly,  it's positive for whole project. But it's negative  for contributor 
>> and maintainer. 
>
>I am totally confused here and do not understand what you are referring
>to, sorry.
>
>greg k-h
Greg KH Nov. 7, 2022, 12:07 p.m. UTC | #5 
On Mon, Nov 07, 2022 at 06:26:16PM +0800, Slark Xiao wrote:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 

Why all the blank lines?

> At 2022-11-07 17:53:57, "Greg KH" <gregkh@linuxfoundation.org> wrote:
> >On Mon, Nov 07, 2022 at 05:30:56PM +0800, Slark Xiao wrote:
> >As I said, this is just a define, not a macro at all.
> >
> >> And could you give your comments in previous patch, not the 'final' one?
> >
> >I do not understand, what previous patrch?  What "final" one?  What is
> >the "latest" patch?
> previous patch:
> https://lore.kernel.org/lkml/20221027115123.5326-1-slark_xiao@163.com/
> https://lore.kernel.org/lkml/20221028023711.4196-1-slark_xiao@163.com/
> https://lore.kernel.org/lkml/20221102024437.15248-1-slark_xiao@163.com/
> 
> 'final' patch:
> https://lore.kernel.org/lkml/20221107084826.8888-1-slark_xiao@163.com/
> https://lore.kernel.org/lkml/20221101015858.6777-1-slark_xiao@163.com/

That's 2 different versions, with a total of 3.

> 
> The 'final' patch was committed according to the advice of the feature
> maintainer. 
> >
> >> In another pci_ids patch, you break it in v3 and break it here again in v2.
> >
> >I broke what?
> You could have  voiced out such comment in V1, V2 before the 'final'. 

We all review patches when we can.  There is no rule that people can not
comment on newer patches, or older ones.

In fact, it would be wonderful if you could take some time and review
patches from others.  It would help your understanding of the code and
how the kernel development process works.

thanks,

greg k-h
Slark Xiao Nov. 10, 2022, 11:14 a.m. UTC | #6 
At 2022-11-07 20:07:52, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>On Mon, Nov 07, 2022 at 06:26:16PM +0800, Slark Xiao wrote:
>Why all the blank lines?

163 mail automatically add it , and I forget remove it.

>
>> At 2022-11-07 17:53:57, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>> >On Mon, Nov 07, 2022 at 05:30:56PM +0800, Slark Xiao wrote:
>> >As I said, this is just a define, not a macro at all.
>> >
>> >> And could you give your comments in previous patch, not the 'final' one?
>> >
>> >I do not understand, what previous patrch?  What "final" one?  What is
>> >the "latest" patch?
>> previous patch:
>> https://lore.kernel.org/lkml/20221027115123.5326-1-slark_xiao@163.com/
>> https://lore.kernel.org/lkml/20221028023711.4196-1-slark_xiao@163.com/
>> https://lore.kernel.org/lkml/20221102024437.15248-1-slark_xiao@163.com/
>> 
>> 'final' patch:
>> https://lore.kernel.org/lkml/20221107084826.8888-1-slark_xiao@163.com/
>> https://lore.kernel.org/lkml/20221101015858.6777-1-slark_xiao@163.com/
>
>That's 2 different versions, with a total of 3.
>
>> 
>> The 'final' patch was committed according to the advice of the feature
>> maintainer. 
>> >
>> >> In another pci_ids patch, you break it in v3 and break it here again in v2.
>> >
>> >I broke what?
>> You could have  voiced out such comment in V1, V2 before the 'final'. 
>
>We all review patches when we can.  There is no rule that people can not
>comment on newer patches, or older ones.
>
>In fact, it would be wonderful if you could take some time and review
>patches from others.  It would help your understanding of the code and
>how the kernel development process works.
>
>thanks,
>
>greg k-h
Agree with this.  But you know, 5 attempt   just for a little update  were rejected
It's so frustrating. Anyway, I re-submit another v3 patch for this change.
Please give your comment if you are free.

Thanks
diff mbox series

Patch

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index c4259cb2d289..3a789bb2f631 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -24,6 +24,10 @@ 
 
 #define HEALTH_CHECK_PERIOD (HZ * 2)
 
+/* PCI VID definitions */
+#define PCI_VENDOR_ID_THALES	0x1269
+#define PCI_VENDOR_ID_QUECTEL	0x1eac
+
 /**
  * struct mhi_pci_dev_info - MHI PCI device specific information
  * @config: MHI controller configuration
@@ -557,11 +561,11 @@  static const struct pci_device_id mhi_pci_id_table[] = {
 		.driver_data = (kernel_ulong_t) &mhi_telit_fn990_info },
 	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x0308),
 		.driver_data = (kernel_ulong_t) &mhi_qcom_sdx65_info },
-	{ PCI_DEVICE(0x1eac, 0x1001), /* EM120R-GL (sdx24) */
+	{ PCI_DEVICE(PCI_VENDOR_ID_QUECTEL, 0x1001), /* EM120R-GL (sdx24) */
 		.driver_data = (kernel_ulong_t) &mhi_quectel_em1xx_info },
-	{ PCI_DEVICE(0x1eac, 0x1002), /* EM160R-GL (sdx24) */
+	{ PCI_DEVICE(PCI_VENDOR_ID_QUECTEL, 0x1002), /* EM160R-GL (sdx24) */
 		.driver_data = (kernel_ulong_t) &mhi_quectel_em1xx_info },
-	{ PCI_DEVICE(0x1eac, 0x2001), /* EM120R-GL for FCCL (sdx24) */
+	{ PCI_DEVICE(PCI_VENDOR_ID_QUECTEL, 0x2001), /* EM120R-GL for FCCL (sdx24) */
 		.driver_data = (kernel_ulong_t) &mhi_quectel_em1xx_info },
 	/* T99W175 (sdx55), Both for eSIM and Non-eSIM */
 	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xe0ab),
@@ -585,16 +589,16 @@  static const struct pci_device_id mhi_pci_id_table[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_FOXCONN, 0xe0d9),
 		.driver_data = (kernel_ulong_t) &mhi_foxconn_sdx65_info },
 	/* MV31-W (Cinterion) */
-	{ PCI_DEVICE(0x1269, 0x00b3),
+	{ PCI_DEVICE(PCI_VENDOR_ID_THALES, 0x00b3),
 		.driver_data = (kernel_ulong_t) &mhi_mv31_info },
 	/* MV31-W (Cinterion), based on new baseline */
-	{ PCI_DEVICE(0x1269, 0x00b4),
+	{ PCI_DEVICE(PCI_VENDOR_ID_THALES, 0x00b4),
 		.driver_data = (kernel_ulong_t) &mhi_mv31_info },
 	/* MV32-WA (Cinterion) */
-	{ PCI_DEVICE(0x1269, 0x00ba),
+	{ PCI_DEVICE(PCI_VENDOR_ID_THALES, 0x00ba),
 		.driver_data = (kernel_ulong_t) &mhi_mv32_info },
 	/* MV32-WB (Cinterion) */
-	{ PCI_DEVICE(0x1269, 0x00bb),
+	{ PCI_DEVICE(PCI_VENDOR_ID_THALES, 0x00bb),
 		.driver_data = (kernel_ulong_t) &mhi_mv32_info },
 	{  }
 };