| Message ID | 1494192214-20082-2-git-send-email-jasmin@anw.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Sun, 7 May 2017 23:23:24 +0200 "Jasmin J." <jasmin@anw.at> escreveu: > From: Jasmin Jessich <jasmin@anw.at> > > Rename STATUSREG_?? -> STATREG_?? to reduce the line length. That sounds a bad idea. We use "stat" on the DVB subsystem as an alias for statistics. > > Signed-off-by: Jasmin Jessich <jasmin@anw.at> > --- > drivers/media/dvb-core/dvb_ca_en50221.c | 34 ++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c > index cc709c9..b978246 100644 > --- a/drivers/media/dvb-core/dvb_ca_en50221.c > +++ b/drivers/media/dvb-core/dvb_ca_en50221.c > @@ -72,11 +72,11 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug messages"); > #define CMDREG_DAIE 0x80 /* Enable DA interrupt */ > #define IRQEN (CMDREG_DAIE) > > -#define STATUSREG_RE 1 /* read error */ > -#define STATUSREG_WE 2 /* write error */ > -#define STATUSREG_FR 0x40 /* module free */ > -#define STATUSREG_DA 0x80 /* data available */ > -#define STATUSREG_TXERR (STATUSREG_RE|STATUSREG_WE) /* general transfer error */ > +#define STATREG_RE 1 /* read error */ > +#define STATREG_WE 2 /* write error */ > +#define STATREG_FR 0x40 /* module free */ > +#define STATREG_DA 0x80 /* data available */ > +#define STATREG_TXERR (STATREG_RE|STATREG_WE) /* general transfer error */ > > > #define DVB_CA_SLOTSTATE_NONE 0 > @@ -347,7 +347,7 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot) > /* read the buffer size from the CAM */ > if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN | CMDREG_SR)) != 0) > return ret; > - if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_DA, HZ)) != 0) > + if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATREG_DA, HZ)) != 0) > return ret; > if ((ret = dvb_ca_en50221_read_data(ca, slot, buf, 2)) != 2) > return -EIO; > @@ -366,7 +366,7 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot) > /* write the buffer size to the CAM */ > if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN | CMDREG_SW)) != 0) > return ret; > - if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_FR, HZ / 10)) != 0) > + if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATREG_FR, HZ / 10)) != 0) > return ret; > if ((ret = dvb_ca_en50221_write_data(ca, slot, buf, 2)) != 2) > return -EIO; > @@ -661,7 +661,7 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * eb > /* check if there is data available */ > if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) > goto exit; > - if (!(status & STATUSREG_DA)) { > + if (!(status & STATREG_DA)) { > /* no data */ > status = 0; > goto exit; > @@ -713,7 +713,7 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * eb > /* check for read error (RE should now be 0) */ > if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) > goto exit; > - if (status & STATUSREG_RE) { > + if (status & STATREG_RE) { > ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; > status = -EIO; > goto exit; > @@ -778,8 +778,8 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b > process the data if necessary. */ > if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) > goto exitnowrite; > - if (status & (STATUSREG_DA | STATUSREG_RE)) { > - if (status & STATUSREG_DA) > + if (status & (STATREG_DA | STATREG_RE)) { > + if (status & STATREG_DA) > dvb_ca_en50221_thread_wakeup(ca); > > status = -EAGAIN; > @@ -794,7 +794,7 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b > /* check if interface is still free */ > if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) > goto exit; > - if (!(status & STATUSREG_FR)) { > + if (!(status & STATREG_FR)) { > /* it wasn't free => try again later */ > status = -EAGAIN; > goto exit; > @@ -815,8 +815,8 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b > if (status < 0) > goto exit; > > - if (status & (STATUSREG_DA | STATUSREG_RE)) { > - if (status & STATUSREG_DA) > + if (status & (STATREG_DA | STATREG_RE)) { > + if (status & STATREG_DA) > dvb_ca_en50221_thread_wakeup(ca); > > status = -EAGAIN; > @@ -839,7 +839,7 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b > /* check for write error (WE should now be 0) */ > if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) > goto exit; > - if (status & STATUSREG_WE) { > + if (status & STATREG_WE) { > ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; > status = -EIO; > goto exit; > @@ -952,7 +952,7 @@ void dvb_ca_en50221_frda_irq(struct dvb_ca_en50221 *pubca, int slot) > switch (ca->slot_info[slot].slot_state) { > case DVB_CA_SLOTSTATE_LINKINIT: > flags = ca->pub->read_cam_control(pubca, slot, CTRLIF_STATUS); > - if (flags & STATUSREG_DA) { > + if (flags & STATREG_DA) { > dprintk("CAM supports DA IRQ\n"); > ca->slot_info[slot].da_irq_supported = 1; > } > @@ -1166,7 +1166,7 @@ static int dvb_ca_en50221_thread(void *data) > } > > flags = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS); > - if (flags & STATUSREG_FR) { > + if (flags & STATREG_FR) { > ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; > ca->wakeup = 1; > } Thanks, Mauro
Hello Mauro! >> Rename STATUSREG_?? -> STATREG_?? to reduce the line length. > That sounds a bad idea. We use "stat" on the DVB subsystem as an > alias for statistics. At the beginning of the style fixes, I thought it is a good idea to reduce as much as possible to get more characters, but at the end this patch doesn't save so much, so we can omit it. What is then the right procedure now? When I omit it in the first place, I can redo the whole work again and this were a lot of hours. Would it be acceptable to make a patch no. 12 at the end of the sequence, which renames it back? BR, Jasmin
Em Mon, 8 May 2017 19:28:48 +0200 "Jasmin J." <jasmin@anw.at> escreveu: > Hello Mauro! > > >> Rename STATUSREG_?? -> STATREG_?? to reduce the line length. > > That sounds a bad idea. We use "stat" on the DVB subsystem as an > > alias for statistics. > At the beginning of the style fixes, I thought it is a good idea to reduce > as much as possible to get more characters, but at the end this patch > doesn't save so much, so we can omit it. Renaming things is usually not a good idea, specially at core level, as it makes a way harder to apply patches from other sources. Also, if you're doing that just because of the 80cols warning, that's the wrong way of doing it ;) The hole idea when the 80cols warning was introduced is to point places at the code were there are potentially too much indentation or code complexity, possibly indicating complex functions that could otherwise be split. This is useful when new code gets added, but it usually doesn't make much sense to fix it on existing code, except when some function has to be re-implemented. So, I please drop patches 1 and 2 from this series. > What is then the right procedure now? > When I omit it in the first place, I can redo the whole work again and > this were a lot of hours. Would it be acceptable to make a patch no. 12 at > the end of the sequence, which renames it back? If you want it applied, this is needed anyway, as the patch doesn't apply cleanly: patching file drivers/media/dvb-core/dvb_ca_en50221.c Hunk #2 FAILED at 347. Hunk #4 succeeded at 649 (offset -12 lines). Hunk #5 succeeded at 702 (offset -11 lines). Hunk #6 succeeded at 763 (offset -15 lines). Hunk #7 succeeded at 779 (offset -15 lines). Hunk #8 succeeded at 800 (offset -15 lines). Hunk #9 succeeded at 824 (offset -15 lines). Hunk #10 succeeded at 937 (offset -15 lines). Hunk #11 succeeded at 1151 (offset -15 lines). 1 out of 11 hunks FAILED -- rejects in file drivers/media/dvb-core/dvb_ca_en50221.c Patch patches/lmml_41185_01_11_media_dvb_core_dvb_ca_en50221_c_rename_statusreg.patch does not apply (enforce with -f) Patch didn't apply. Aborting From this patch series, I was able to apply 2 patches. Btw, don't spend time fixing issues pointed by checkpatch on existing code, except if you're rewriting most of the code. We don't want to handle merge conflicts due to checkpatch-only changes. Thanks, Mauro
Hello Mauro! > If you want it applied, this is needed anyway, as the patch doesn't apply > cleanly: Because you didn't apply the first series! In the first series [PATCH 0/7] Add block read/write to en50221 CAM functions I wrote: There is another patch series coming soon "Fix coding style in en50221 CAM functions" which fixes nearly all the style issues in dvb-core/dvb_ca_en50221.c/.h, based on this patch series. So please be patient, if any of the dvb_ca_en50221.c/.h might be not 100% checkpatch.pl compliant. It was NOT intended to apply the second series with the code style changes before the first series! And now, that you accepted two out of this series the first series might not apply also and I need to rework it. Sorry for my feelings about this issue, but this is a bit frustrating! In the preamble of the style fix series I wrote: These patch series is a follow up to the series "Add block read/write to en50221 CAM functions". It fixed nearly all the style issues reported by checkpatch.pl in dvb-core/dvb_ca_en50221.c I can't do more as writing what is the right order! > Btw, don't spend time fixing issues pointed by checkpatch on existing > code, except if you're rewriting most of the code. We don't want to handle > merge conflicts due to checkpatch-only changes. I think you are talking about [PATCH 04/11] [media] dvb-core/dvb_ca_en50221.c: Refactored dvb_ca_en50221_thread This function is a mess and breaking it into smaller pieces helps for the 80cols limit and for the complexity. You just wrote: > The hole idea when the 80cols warning was introduced is to point places at > the code were there are potentially too much indentation or code complexity, > possibly indicating complex functions that could otherwise be split. But you wrote also: > This is useful when new code gets added, but it usually doesn't make > much sense to fix it on existing code, except when some function has to > be re-implemented. I would like to split the thread to make it more readable, but if you say you won't apply it it makes no sense to put effort on this subject. So what is your decision about this patch? BR, Jasmin
Em Wed, 7 Jun 2017 21:37:02 +0200 "Jasmin J." <jasmin@anw.at> escreveu: > Hello Mauro! > > > If you want it applied, this is needed anyway, as the patch doesn't apply > > cleanly: > Because you didn't apply the first series! > In the first series > [PATCH 0/7] Add block read/write to en50221 CAM functions > I wrote: > There is another patch series coming soon "Fix coding style in en50221 CAM > functions" which fixes nearly all the style issues in > dvb-core/dvb_ca_en50221.c/.h, based on this patch series. So please be > patient, if any of the dvb_ca_en50221.c/.h might be not 100% checkpatch.pl > compliant. > > It was NOT intended to apply the second series with the code style changes > before the first series! And now, that you accepted two out of this series > the first series might not apply also and I need to rework it. > Sorry for my feelings about this issue, but this is a bit frustrating! > > In the preamble of the style fix series I wrote: > These patch series is a follow up to the series "Add block read/write to > en50221 CAM functions". It fixed nearly all the style issues reported by > checkpatch.pl in dvb-core/dvb_ca_en50221.c > > I can't do more as writing what is the right order! Sorry, I missed it. Unfortunately, patchwork doesn't retrieve patch 00/xx. So, sometimes I end by not noticing that a patch series has a cover letter. > > Btw, don't spend time fixing issues pointed by checkpatch on existing > > code, except if you're rewriting most of the code. We don't want to handle > > merge conflicts due to checkpatch-only changes. > I think you are talking about > [PATCH 04/11] [media] dvb-core/dvb_ca_en50221.c: Refactored dvb_ca_en50221_thread > This function is a mess and breaking it into smaller pieces helps for the 80cols > limit and for the complexity. You just wrote: No, I'm actually talking about patches 1 and 2 of this series. Renaming macros just due to 80 cols is usually a bad idea, as it causes conflict with other stuff. The idea behind patch 04/11 makes sense to me. I'll review it carefully after having everything applied. Please re-send the first series, making sure that the authorship is preserved. Thanks, Mauro
Hello Mauro! > The idea behind patch 04/11 makes sense to me. I'll review it carefully > after having everything applied. Please don't do that now! The first series will then not be easily changed, because it changes the state machine. I will provide the first series and when it is applied the second series again, now that I know what you like. BR, Jasmin
diff --git a/drivers/media/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb-core/dvb_ca_en50221.c index cc709c9..b978246 100644 --- a/drivers/media/dvb-core/dvb_ca_en50221.c +++ b/drivers/media/dvb-core/dvb_ca_en50221.c @@ -72,11 +72,11 @@ MODULE_PARM_DESC(cam_debug, "enable verbose debug messages"); #define CMDREG_DAIE 0x80 /* Enable DA interrupt */ #define IRQEN (CMDREG_DAIE) -#define STATUSREG_RE 1 /* read error */ -#define STATUSREG_WE 2 /* write error */ -#define STATUSREG_FR 0x40 /* module free */ -#define STATUSREG_DA 0x80 /* data available */ -#define STATUSREG_TXERR (STATUSREG_RE|STATUSREG_WE) /* general transfer error */ +#define STATREG_RE 1 /* read error */ +#define STATREG_WE 2 /* write error */ +#define STATREG_FR 0x40 /* module free */ +#define STATREG_DA 0x80 /* data available */ +#define STATREG_TXERR (STATREG_RE|STATREG_WE) /* general transfer error */ #define DVB_CA_SLOTSTATE_NONE 0 @@ -347,7 +347,7 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot) /* read the buffer size from the CAM */ if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN | CMDREG_SR)) != 0) return ret; - if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_DA, HZ)) != 0) + if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATREG_DA, HZ)) != 0) return ret; if ((ret = dvb_ca_en50221_read_data(ca, slot, buf, 2)) != 2) return -EIO; @@ -366,7 +366,7 @@ static int dvb_ca_en50221_link_init(struct dvb_ca_private *ca, int slot) /* write the buffer size to the CAM */ if ((ret = ca->pub->write_cam_control(ca->pub, slot, CTRLIF_COMMAND, IRQEN | CMDREG_SW)) != 0) return ret; - if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATUSREG_FR, HZ / 10)) != 0) + if ((ret = dvb_ca_en50221_wait_if_status(ca, slot, STATREG_FR, HZ / 10)) != 0) return ret; if ((ret = dvb_ca_en50221_write_data(ca, slot, buf, 2)) != 2) return -EIO; @@ -661,7 +661,7 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * eb /* check if there is data available */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exit; - if (!(status & STATUSREG_DA)) { + if (!(status & STATREG_DA)) { /* no data */ status = 0; goto exit; @@ -713,7 +713,7 @@ static int dvb_ca_en50221_read_data(struct dvb_ca_private *ca, int slot, u8 * eb /* check for read error (RE should now be 0) */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exit; - if (status & STATUSREG_RE) { + if (status & STATREG_RE) { ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; status = -EIO; goto exit; @@ -778,8 +778,8 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b process the data if necessary. */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exitnowrite; - if (status & (STATUSREG_DA | STATUSREG_RE)) { - if (status & STATUSREG_DA) + if (status & (STATREG_DA | STATREG_RE)) { + if (status & STATREG_DA) dvb_ca_en50221_thread_wakeup(ca); status = -EAGAIN; @@ -794,7 +794,7 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b /* check if interface is still free */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exit; - if (!(status & STATUSREG_FR)) { + if (!(status & STATREG_FR)) { /* it wasn't free => try again later */ status = -EAGAIN; goto exit; @@ -815,8 +815,8 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b if (status < 0) goto exit; - if (status & (STATUSREG_DA | STATUSREG_RE)) { - if (status & STATUSREG_DA) + if (status & (STATREG_DA | STATREG_RE)) { + if (status & STATREG_DA) dvb_ca_en50221_thread_wakeup(ca); status = -EAGAIN; @@ -839,7 +839,7 @@ static int dvb_ca_en50221_write_data(struct dvb_ca_private *ca, int slot, u8 * b /* check for write error (WE should now be 0) */ if ((status = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS)) < 0) goto exit; - if (status & STATUSREG_WE) { + if (status & STATREG_WE) { ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; status = -EIO; goto exit; @@ -952,7 +952,7 @@ void dvb_ca_en50221_frda_irq(struct dvb_ca_en50221 *pubca, int slot) switch (ca->slot_info[slot].slot_state) { case DVB_CA_SLOTSTATE_LINKINIT: flags = ca->pub->read_cam_control(pubca, slot, CTRLIF_STATUS); - if (flags & STATUSREG_DA) { + if (flags & STATREG_DA) { dprintk("CAM supports DA IRQ\n"); ca->slot_info[slot].da_irq_supported = 1; } @@ -1166,7 +1166,7 @@ static int dvb_ca_en50221_thread(void *data) } flags = ca->pub->read_cam_control(ca->pub, slot, CTRLIF_STATUS); - if (flags & STATUSREG_FR) { + if (flags & STATREG_FR) { ca->slot_info[slot].slot_state = DVB_CA_SLOTSTATE_LINKINIT; ca->wakeup = 1; }