| Message ID | 20221109161713.31723-1-davide.tronchin.94@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/3] USB: serial: option: remove old LARA-R6 PID | expand |
On Wed, Nov 09, 2022 at 05:17:11PM +0100, Davide Tronchin wrote: > The LARA-R6 module old PID (defined as: UBLOX_PRODUCT_R6XX > 0x90fa) has been removed since is no longer used by the > current u-blox LARA-R6 product. However, PID 0x90fa has been > kept to maintain the support to other products that use the > same VID/PID. First, please wrap your commit messages at 72 column or so (not 60). We've asked you repeatedly whether you for u-blox or not, but you keep ignoring this question. Knowing this would allow us to better evaluate the reasoning and motivation behind this this change. The above commit message still does not explain why you want to remove it and whether it would be safe to do so. Why was added in the first place? What u-blox products used the old PID? By just removing the define this is less of an issue, but you should not make the life of reviewers harder by ignoring request to properly motivate your changes and explain why they are safe to apply. > Signed-off-by: Davide Tronchin <davide.tronchin.94@gmail.com> > --- > > V4 -> V5: kept PID 0x90fa to maintain the support for other products > which use VID:PID 0x05c6:0x90fa. Remove interface 4 from blacklist > for LARA-L6 default mode since it is not needed. You can either put a shared changelog for the whole series in a cover letter, or you describe changes to each individual patch. But mixing the two styles as you do in this entry is just confusing. > V3 -> V4: as requested, the patch has been split to 3 sub-patches. > Fix comment format. > > V2 -> V3: added this section to tracking changes with previous versions. > Added some explanations about the RSVD(4) in the description session. > Added reservation to port 4 of VID:PID 0x05C6:0x908B to meet other > companies QMI net interface implementation. > > V1 -> V2: define UBLOX_PRODUCT_LARA_R6 0x908b has been deleted together > with the previosly provided definition of USB_DEVICE since the PID > is used by another vendor. > The LARA-L6 patch part is the same of the previosly provided one. Johan
My apologies, this is my first attempt to submit a patch to the kernel community. > We've asked you repeatedly whether you for u-blox or not, but you keep > ignoring this question. Knowing this would allow us to better evaluate > the reasoning and motivation behind this this change. Yes, i am a u-blox employee and i've been asked to integrate LARA-L6 in the linux kernel and update the current code for LARA-R6 00B (updating the PID from 0x90fa to 0x908b). > The above commit message still does not explain why you want to remove > it and whether it would be safe to do so. Why was added in the first > place? What u-blox products used the old PID? The first prototype of LARA-R6 00B had 0x90fa PID but, just before the product finalization, it has been decided to adopt a new USB composition and consequently a change of PID was necessary. The 0x90fa PID has been used only for some internal prototypes, hence no u-blox products with that PID have been shipped to customers. As pointed out in the discussion, the 0x90fa PID is used by other module vendors which sell Qualcomm based modems, hence i proposed to remove the association between u-blox (thedefine UBLOX_PRODUCT_R6XX) and 0x90fa, moving it directly in the option_ids array. > By just removing the define this is less of an issue, but you should > not make the life of reviewers harder by ignoring request to properly > motivate your changes and explain why they are safe to apply. My apologies again, thanks for the patience. > You can either put a shared changelog for the whole series in a cover > letter, or you describe changes to each individual patch. But mixing the > two styles as you do in this entry is just confusing. Thanks for the suggestions. In order to simplify the submission process, i propose to split the submission for the LARA-L6 patches and the update for LARA-R6 00B. Do you think could it be feasible? Davide
On Tue, Nov 15, 2022 at 05:46:54PM +0100, Davide Tronchin wrote: > My apologies, this is my first attempt to submit a patch to the kernel > community. > > > We've asked you repeatedly whether you for u-blox or not, but you keep > > ignoring this question. Knowing this would allow us to better evaluate > > the reasoning and motivation behind this this change. > > Yes, i am a u-blox employee and i've been asked to integrate LARA-L6 in > the linux kernel and update the current code for LARA-R6 00B (updating > the PID from 0x90fa to 0x908b). Thanks. Was it it also a colleague of yours that submitted the initial PID then? > > The above commit message still does not explain why you want to remove > > it and whether it would be safe to do so. Why was added in the first > > place? What u-blox products used the old PID? > > The first prototype of LARA-R6 00B had 0x90fa PID but, just before the > product finalization, it has been decided to adopt a new USB composition > and consequently a change of PID was necessary. > The 0x90fa PID has been used only for some internal prototypes, hence > no u-blox products with that PID have been shipped to customers. > As pointed out in the discussion, the 0x90fa PID is used by other module > vendors which sell Qualcomm based modems, hence i proposed to remove the > association between u-blox (thedefine UBLOX_PRODUCT_R6XX) and 0x90fa, > moving it directly in the option_ids array. Thanks, this is the kind of details we've been asking for. Please put some of this in the commit in some form so that it is obvious from just reading the commit message that the patch is correct and safe to apply. Make sure to mention that this Qualcomm PID is used by other products and that's why you're leaving it in. Perhaps a Link tag with a reference to Lars message pointing this out is in place. For example: Link: https://lore.kernel.org/all/6572c4e6-d8bc-b8d3-4396-d879e4e76338@gmail.com/ > > By just removing the define this is less of an issue, but you should > > not make the life of reviewers harder by ignoring request to properly > > motivate your changes and explain why they are safe to apply. > > My apologies again, thanks for the patience. > > > You can either put a shared changelog for the whole series in a cover > > letter, or you describe changes to each individual patch. But mixing the > > two styles as you do in this entry is just confusing. > > Thanks for the suggestions. In order to simplify the submission process, > i propose to split the submission for the LARA-L6 patches and the update > for LARA-R6 00B. > Do you think could it be feasible? I don't think that's necessary now that you've provided some more details. Just respin the series and address the review comments given so far (either by rejecting a suggestion and explaining why, or by incorporating it in your next submission). It seems you only need to tweak some of the commit messages in a v6. Johan
> > Yes, i am a u-blox employee and i've been asked to integrate LARA-L6 in > > the linux kernel and update the current code for LARA-R6 00B (updating > > the PID from 0x90fa to 0x908b). > > Thanks. Was it it also a colleague of yours that submitted the initial > PID then? Correct, the initial PID has been submitted by another u-blox employee. > > The first prototype of LARA-R6 00B had 0x90fa PID but, just before the > > product finalization, it has been decided to adopt a new USB composition > > and consequently a change of PID was necessary. > > The 0x90fa PID has been used only for some internal prototypes, hence > > no u-blox products with that PID have been shipped to customers. > > As pointed out in the discussion, the 0x90fa PID is used by other module > > vendors which sell Qualcomm based modems, hence i proposed to remove the > > association between u-blox (thedefine UBLOX_PRODUCT_R6XX) and 0x90fa, > > moving it directly in the option_ids array. > > Thanks, this is the kind of details we've been asking for. Please put > some of this in the commit in some form so that it is obvious from just > reading the commit message that the patch is correct and safe to apply. > > Make sure to mention that this Qualcomm PID is used by other products > and that's why you're leaving it in. Perhaps a Link tag with a > reference to Lars message pointing this out is in place. For example: Ok thanks for the suggestions. I will add all the details to the v6 1/3 patch. > > Thanks for the suggestions. In order to simplify the submission process, > > i propose to split the submission for the LARA-L6 patches and the update > > for LARA-R6 00B. > > Do you think could it be feasible? > > I don't think that's necessary now that you've provided some more > details. Just respin the series and address the review comments given > so far (either by rejecting a suggestion and explaining why, or by > incorporating it in your next submission). > > It seems you only need to tweak some of the commit messages in a v6. I will submit a v6 version of the patch with the provided suggestions. I have just one doubt, in patch v6, should i edit also the previous sent changelog by describing changes to each indiviual sub-patch? Thank you for the patience. Davide
On Wed, Nov 16, 2022 at 10:50:42AM +0100, Davide Tronchin wrote: > > > Thanks for the suggestions. In order to simplify the submission process, > > > i propose to split the submission for the LARA-L6 patches and the update > > > for LARA-R6 00B. > > > Do you think could it be feasible? > > > > I don't think that's necessary now that you've provided some more > > details. Just respin the series and address the review comments given > > so far (either by rejecting a suggestion and explaining why, or by > > incorporating it in your next submission). > > > > It seems you only need to tweak some of the commit messages in a v6. > > I will submit a v6 version of the patch with the provided suggestions. > > I have just one doubt, in patch v6, should i edit also the previous > sent changelog by describing changes to each indiviual sub-patch? No, you can leave them as they are, and just use the "per-patch" commit log style for v6. Johan
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c index 697683e3fbff..b93285b5175b 100644 --- a/drivers/usb/serial/option.c +++ b/drivers/usb/serial/option.c @@ -240,7 +240,6 @@ static void option_instat_callback(struct urb *urb); #define QUECTEL_PRODUCT_UC15 0x9090 /* These u-blox products use Qualcomm's vendor ID */ #define UBLOX_PRODUCT_R410M 0x90b2 -#define UBLOX_PRODUCT_R6XX 0x90fa /* These Yuga products use Qualcomm's vendor ID */ #define YUGA_PRODUCT_CLM920_NC5 0x9625 @@ -1124,7 +1123,7 @@ static const struct usb_device_id option_ids[] = { /* u-blox products using Qualcomm vendor ID */ { USB_DEVICE(QUALCOMM_VENDOR_ID, UBLOX_PRODUCT_R410M), .driver_info = RSVD(1) | RSVD(3) }, - { USB_DEVICE(QUALCOMM_VENDOR_ID, UBLOX_PRODUCT_R6XX), + { USB_DEVICE(QUALCOMM_VENDOR_ID, 0x90fa), .driver_info = RSVD(3) }, /* Quectel products using Quectel vendor ID */ { USB_DEVICE_AND_INTERFACE_INFO(QUECTEL_VENDOR_ID, QUECTEL_PRODUCT_EC21, 0xff, 0xff, 0xff),
The LARA-R6 module old PID (defined as: UBLOX_PRODUCT_R6XX 0x90fa) has been removed since is no longer used by the current u-blox LARA-R6 product. However, PID 0x90fa has been kept to maintain the support to other products that use the same VID/PID. Signed-off-by: Davide Tronchin <davide.tronchin.94@gmail.com> --- V4 -> V5: kept PID 0x90fa to maintain the support for other products which use VID:PID 0x05c6:0x90fa. Remove interface 4 from blacklist for LARA-L6 default mode since it is not needed. V3 -> V4: as requested, the patch has been split to 3 sub-patches. Fix comment format. V2 -> V3: added this section to tracking changes with previous versions. Added some explanations about the RSVD(4) in the description session. Added reservation to port 4 of VID:PID 0x05C6:0x908B to meet other companies QMI net interface implementation. V1 -> V2: define UBLOX_PRODUCT_LARA_R6 0x908b has been deleted together with the previosly provided definition of USB_DEVICE since the PID is used by another vendor. The LARA-L6 patch part is the same of the previosly provided one. drivers/usb/serial/option.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)