| Message ID | 20221107084826.8888-1-slark_xiao@163.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] bus: mhi: host: pci_generic: Add macro for some VIDs | expand |
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
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.
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
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
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
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 --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 }, { } };
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(-)