* [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation
@ 2020-03-14 19:47 Deepak R Varma
2020-03-14 19:57 ` [Outreachy kernel] " Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: Deepak R Varma @ 2020-03-14 19:47 UTC (permalink / raw)
To: outreachy-kernel
Cc: gregkh, daniel.baluta, kieran.bingham, abbotti, hsweeten,
mh12gx2825
Resolve general code indentation problems as detected by checkpatch script.
Implement code reformat and re-indentation as per coding style to
improve readability.
Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
---
.../staging/comedi/drivers/ni_mio_common.c | 62 ++++++++++---------
1 file changed, 32 insertions(+), 30 deletions(-)
diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
index b72a40a79930..48e20de3ebcf 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -2198,10 +2198,10 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
NISTC_AI_TRIG_START1_SEL(0);
break;
case TRIG_EXT:
- ai_trig |= NISTC_AI_TRIG_START1_SEL(
- ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
- NI_AI_StartTrigger,
- &devpriv->routing_tables, 1));
+ ai_trig |= NISTC_AI_TRIG_START1_SEL(ni_get_reg_value_roffs(
+ CR_CHAN(cmd->start_arg),
+ NI_AI_StartTrigger,
+ &devpriv->routing_tables, 1));
if (cmd->start_arg & CR_INVERT)
ai_trig |= NISTC_AI_TRIG_START1_POLARITY;
@@ -2311,10 +2311,11 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
(cmd->scan_begin_arg & ~CR_EDGE) !=
(cmd->convert_arg & ~CR_EDGE))
start_stop_select |= NISTC_AI_START_SYNC;
- start_stop_select |= NISTC_AI_START_SEL(
- ni_get_reg_value_roffs(CR_CHAN(cmd->scan_begin_arg),
- NI_AI_SampleClock,
- &devpriv->routing_tables, 1));
+
+ start_stop_select |= NISTC_AI_START_SEL(ni_get_reg_value_roffs(
+ CR_CHAN(cmd->scan_begin_arg),
+ NI_AI_SampleClock,
+ &devpriv->routing_tables, 1));
ni_stc_writew(dev, start_stop_select, NISTC_AI_START_STOP_REG);
break;
}
@@ -2342,10 +2343,10 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
ni_stc_writew(dev, mode2, NISTC_AI_MODE2_REG);
break;
case TRIG_EXT:
- mode1 |= NISTC_AI_MODE1_CONVERT_SRC(
- ni_get_reg_value_roffs(CR_CHAN(cmd->convert_arg),
- NI_AI_ConvertClock,
- &devpriv->routing_tables, 1));
+ mode1 |= NISTC_AI_MODE1_CONVERT_SRC(ni_get_reg_value_roffs(
+ CR_CHAN(cmd->convert_arg),
+ NI_AI_ConvertClock,
+ &devpriv->routing_tables, 1));
if ((cmd->convert_arg & CR_INVERT) == 0)
mode1 |= NISTC_AI_MODE1_CONVERT_POLARITY;
ni_stc_writew(dev, mode1, NISTC_AI_MODE1_REG);
@@ -2969,10 +2970,11 @@ static void ni_ao_cmd_set_trigger(struct comedi_device *dev,
trigsel = NISTC_AO_TRIG_START1_EDGE |
NISTC_AO_TRIG_START1_SYNC;
} else { /* TRIG_EXT */
- trigsel = NISTC_AO_TRIG_START1_SEL(
- ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
- NI_AO_StartTrigger,
- &devpriv->routing_tables, 1));
+ trigsel = NISTC_AO_TRIG_START1_SEL(ni_get_reg_value_roffs(
+ CR_CHAN(cmd->start_arg),
+ NI_AO_StartTrigger,
+ &devpriv->routing_tables, 1));
+
/* 0=active high, 1=active low. see daq-stc 3-24 (p186) */
if (cmd->start_arg & CR_INVERT)
trigsel |= NISTC_AO_TRIG_START1_POLARITY;
@@ -3079,12 +3081,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
* zero out these bit fields to be set below. Does an ao-reset do this
* automatically?
*/
- devpriv->ao_mode1 &= ~(
- NISTC_AO_MODE1_UI_SRC_MASK |
- NISTC_AO_MODE1_UI_SRC_POLARITY |
- NISTC_AO_MODE1_UPDATE_SRC_MASK |
- NISTC_AO_MODE1_UPDATE_SRC_POLARITY
- );
+ devpriv->ao_mode1 &= ~(NISTC_AO_MODE1_UI_SRC_MASK |
+ NISTC_AO_MODE1_UI_SRC_POLARITY |
+ NISTC_AO_MODE1_UPDATE_SRC_MASK |
+ NISTC_AO_MODE1_UPDATE_SRC_POLARITY);
if (cmd->scan_begin_src == TRIG_TIMER) {
unsigned int trigvar;
@@ -3133,10 +3133,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
} else { /* TRIG_EXT */
/* FIXME: assert scan_begin_arg != 0, ret failure otherwise */
devpriv->ao_cmd2 |= NISTC_AO_CMD2_BC_GATE_ENA;
- devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(
- ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
- NI_AO_SampleClock,
- &devpriv->routing_tables));
+ devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(ni_get_reg_value(
+ CR_CHAN(cmd->scan_begin_arg),
+ NI_AO_SampleClock,
+ &devpriv->routing_tables));
if (cmd->scan_begin_arg & CR_INVERT)
devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC_POLARITY;
}
@@ -3672,13 +3672,15 @@ static int ni_cdio_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
*/
cdo_mode_bits = NI_M_CDO_MODE_FIFO_MODE |
NI_M_CDO_MODE_HALT_ON_ERROR |
- NI_M_CDO_MODE_SAMPLE_SRC(
- ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
- NI_DO_SampleClock,
- &devpriv->routing_tables));
+ NI_M_CDO_MODE_SAMPLE_SRC(ni_get_reg_value(
+ CR_CHAN(cmd->scan_begin_arg),
+ NI_DO_SampleClock,
+ &devpriv->routing_tables));
if (cmd->scan_begin_arg & CR_INVERT)
cdo_mode_bits |= NI_M_CDO_MODE_POLARITY;
+
ni_writel(dev, cdo_mode_bits, NI_M_CDO_MODE_REG);
+
if (s->io_bits) {
ni_writel(dev, s->state, NI_M_CDO_FIFO_DATA_REG);
ni_writel(dev, NI_M_CDO_CMD_SW_UPDATE, NI_M_CDIO_CMD_REG);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation
2020-03-14 19:47 [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation Deepak R Varma
@ 2020-03-14 19:57 ` Julia Lawall
2020-03-14 20:13 ` Deepak Varma
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2020-03-14 19:57 UTC (permalink / raw)
To: Deepak R Varma
Cc: outreachy-kernel, gregkh, daniel.baluta, kieran.bingham, abbotti,
hsweeten
On Sun, 15 Mar 2020, Deepak R Varma wrote:
> Resolve general code indentation problems as detected by checkpatch script.
> Implement code reformat and re-indentation as per coding style to
> improve readability.
>
> Signed-off-by: Deepak R Varma <mh12gx2825@gmail.com>
> ---
> .../staging/comedi/drivers/ni_mio_common.c | 62 ++++++++++---------
> 1 file changed, 32 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index b72a40a79930..48e20de3ebcf 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -2198,10 +2198,10 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> NISTC_AI_TRIG_START1_SEL(0);
> break;
> case TRIG_EXT:
> - ai_trig |= NISTC_AI_TRIG_START1_SEL(
> - ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
> - NI_AI_StartTrigger,
> - &devpriv->routing_tables, 1));
> + ai_trig |= NISTC_AI_TRIG_START1_SEL(ni_get_reg_value_roffs(
> + CR_CHAN(cmd->start_arg),
> + NI_AI_StartTrigger,
> + &devpriv->routing_tables, 1));
These may have been better as they were. At least the two function calls
are more visible in the original.
[...]
> @@ -3079,12 +3081,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
> * zero out these bit fields to be set below. Does an ao-reset do this
> * automatically?
> */
> - devpriv->ao_mode1 &= ~(
> - NISTC_AO_MODE1_UI_SRC_MASK |
> - NISTC_AO_MODE1_UI_SRC_POLARITY |
> - NISTC_AO_MODE1_UPDATE_SRC_MASK |
> - NISTC_AO_MODE1_UPDATE_SRC_POLARITY
> - );
> + devpriv->ao_mode1 &= ~(NISTC_AO_MODE1_UI_SRC_MASK |
> + NISTC_AO_MODE1_UI_SRC_POLARITY |
> + NISTC_AO_MODE1_UPDATE_SRC_MASK |
> + NISTC_AO_MODE1_UPDATE_SRC_POLARITY);
This looks like an improveemnt. Are the subsequent NISTCs lines up with
the right side of the (? It doesn't look like it, but the +s can change
the appearance of the indentation.
> if (cmd->scan_begin_src == TRIG_TIMER) {
> unsigned int trigvar;
> @@ -3133,10 +3133,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
> } else { /* TRIG_EXT */
> /* FIXME: assert scan_begin_arg != 0, ret failure otherwise */
> devpriv->ao_cmd2 |= NISTC_AO_CMD2_BC_GATE_ENA;
> - devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(
> - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> - NI_AO_SampleClock,
> - &devpriv->routing_tables));
> + devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(ni_get_reg_value(
> + CR_CHAN(cmd->scan_begin_arg),
> + NI_AO_SampleClock,
> + &devpriv->routing_tables));
> if (cmd->scan_begin_arg & CR_INVERT)
> devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC_POLARITY;
> }
> @@ -3672,13 +3672,15 @@ static int ni_cdio_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> */
> cdo_mode_bits = NI_M_CDO_MODE_FIFO_MODE |
> NI_M_CDO_MODE_HALT_ON_ERROR |
> - NI_M_CDO_MODE_SAMPLE_SRC(
> - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> - NI_DO_SampleClock,
> - &devpriv->routing_tables));
> + NI_M_CDO_MODE_SAMPLE_SRC(ni_get_reg_value(
> + CR_CHAN(cmd->scan_begin_arg),
> + NI_DO_SampleClock,
> + &devpriv->routing_tables));
These like the earlier ones are probably better left as is.
> if (cmd->scan_begin_arg & CR_INVERT)
> cdo_mode_bits |= NI_M_CDO_MODE_POLARITY;
> +
> ni_writel(dev, cdo_mode_bits, NI_M_CDO_MODE_REG);
> +
I'm not sure what motivated adding these blank lines, but it doesn't seem
to be the same issue as what is done in the rest of the patch.
julia
> if (s->io_bits) {
> ni_writel(dev, s->state, NI_M_CDO_FIFO_DATA_REG);
> ni_writel(dev, NI_M_CDO_CMD_SW_UPDATE, NI_M_CDIO_CMD_REG);
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200314194724.GA2711%40deeUbuntu.
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation
2020-03-14 19:57 ` [Outreachy kernel] " Julia Lawall
@ 2020-03-14 20:13 ` Deepak Varma
2020-03-14 20:32 ` Julia Lawall
0 siblings, 1 reply; 5+ messages in thread
From: Deepak Varma @ 2020-03-14 20:13 UTC (permalink / raw)
To: outreachy-kernel
[-- Attachment #1.1: Type: text/plain, Size: 6604 bytes --]
On Sunday, 15 March 2020 01:27:27 UTC+5:30, Julia Lawall wrote:
>
>
>
> On Sun, 15 Mar 2020, Deepak R Varma wrote:
>
> > Resolve general code indentation problems as detected by checkpatch
> script.
> > Implement code reformat and re-indentation as per coding style to
> > improve readability.
> >
> > Signed-off-by: Deepak R Varma <mh12g...@gmail.com <javascript:>>
> > ---
> > .../staging/comedi/drivers/ni_mio_common.c | 62 ++++++++++---------
> > 1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index b72a40a79930..48e20de3ebcf 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > @@ -2198,10 +2198,10 @@ static int ni_ai_cmd(struct comedi_device *dev,
> struct comedi_subdevice *s)
> > NISTC_AI_TRIG_START1_SEL(0);
> > break;
> > case TRIG_EXT:
> > - ai_trig |= NISTC_AI_TRIG_START1_SEL(
> > - ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
> > - NI_AI_StartTrigger,
> > -
> &devpriv->routing_tables, 1));
> > + ai_trig |=
> NISTC_AI_TRIG_START1_SEL(ni_get_reg_value_roffs(
> > + CR_CHAN(cmd->start_arg),
> > + NI_AI_StartTrigger,
> > + &devpriv->routing_tables, 1));
>
> These may have been better as they were. At least the two function calls
> are more visible in the original.
>
I think the reformatting makes it look simpler and more readable. Did you
review it in mutt or here on this page? Please confirm if you want me to
revert these back.
>
> [...]
>
> > @@ -3079,12 +3081,10 @@ static void ni_ao_cmd_set_update(struct
> comedi_device *dev,
> > * zero out these bit fields to be set below. Does an ao-reset
> do this
> > * automatically?
> > */
> > - devpriv->ao_mode1 &= ~(
> > - NISTC_AO_MODE1_UI_SRC_MASK |
> > - NISTC_AO_MODE1_UI_SRC_POLARITY |
> > - NISTC_AO_MODE1_UPDATE_SRC_MASK |
> > - NISTC_AO_MODE1_UPDATE_SRC_POLARITY
> > - );
> > + devpriv->ao_mode1 &= ~(NISTC_AO_MODE1_UI_SRC_MASK |
> > + NISTC_AO_MODE1_UI_SRC_POLARITY |
> > + NISTC_AO_MODE1_UPDATE_SRC_MASK |
> > + NISTC_AO_MODE1_UPDATE_SRC_POLARITY);
>
> This looks like an improveemnt. Are the subsequent NISTCs lines up with
> the right side of the (? It doesn't look like it, but the +s can change
> the appearance of the indentation.
>
sorry, I did not follow this fully. Could you please explain what do you
mean by improvement? There is no code change. I only realigned the NISTC*
lines so that the "&= ~(" part is clearly visible. It definitely looks
different here on the web page that the vi editor.
>
>
> > if (cmd->scan_begin_src == TRIG_TIMER) {
> > unsigned int trigvar;
> > @@ -3133,10 +3133,10 @@ static void ni_ao_cmd_set_update(struct
> comedi_device *dev,
> > } else { /* TRIG_EXT */
> > /* FIXME: assert scan_begin_arg != 0, ret failure
> otherwise */
> > devpriv->ao_cmd2 |= NISTC_AO_CMD2_BC_GATE_ENA;
> > - devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(
> > - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> > - NI_AO_SampleClock,
> > - &devpriv->routing_tables));
> > + devpriv->ao_mode1 |=
> NISTC_AO_MODE1_UPDATE_SRC(ni_get_reg_value(
> >
> + CR_CHAN(cmd->scan_begin_arg),
>
> > + NI_AO_SampleClock,
> >
> + &devpriv->routing_tables));
>
> > if (cmd->scan_begin_arg & CR_INVERT)
> > devpriv->ao_mode1 |=
> NISTC_AO_MODE1_UPDATE_SRC_POLARITY;
> > }
> > @@ -3672,13 +3672,15 @@ static int ni_cdio_cmd(struct comedi_device
> *dev, struct comedi_subdevice *s)
> > */
> > cdo_mode_bits = NI_M_CDO_MODE_FIFO_MODE |
> > NI_M_CDO_MODE_HALT_ON_ERROR |
> > - NI_M_CDO_MODE_SAMPLE_SRC(
> >
> - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
>
> > - NI_DO_SampleClock,
> > -
> &devpriv->routing_tables));
> > + NI_M_CDO_MODE_SAMPLE_SRC(ni_get_reg_value(
> >
> + CR_CHAN(cmd->scan_begin_arg),
>
> > + NI_DO_SampleClock,
> >
> + &devpriv->routing_tables));
>
>
> These like the earlier ones are probably better left as is.
>
Same as above. Please confirm if you want me to revert these.
> > if (cmd->scan_begin_arg & CR_INVERT)
> > cdo_mode_bits |= NI_M_CDO_MODE_POLARITY;
> > +
> > ni_writel(dev, cdo_mode_bits, NI_M_CDO_MODE_REG);
> > +
>
> I'm not sure what motivated adding these blank lines, but it doesn't seem
> to be the same issue as what is done in the rest of the patch.
>
I think with the new line in place, it makes it easier to spot the ni_write
function call easily. Otherwise it may get overlooked during a quick glace
of this area. Let me know if you still think the new line is unnecessary
and I will revert it. No problem.
>
> julia
>
> > if (s->io_bits) {
> > ni_writel(dev, s->state, NI_M_CDO_FIFO_DATA_REG);
> > ni_writel(dev, NI_M_CDO_CMD_SW_UPDATE,
> NI_M_CDIO_CMD_REG);
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreach...@googlegroups.com <javascript:>.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20200314194724.GA2711%40deeUbuntu.
>
> >
>
[-- Attachment #1.2: Type: text/html, Size: 10391 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation
2020-03-14 20:13 ` Deepak Varma
@ 2020-03-14 20:32 ` Julia Lawall
2020-03-14 20:47 ` Deepak Varma
0 siblings, 1 reply; 5+ messages in thread
From: Julia Lawall @ 2020-03-14 20:32 UTC (permalink / raw)
To: Deepak Varma; +Cc: outreachy-kernel
[-- Attachment #1: Type: text/plain, Size: 9384 bytes --]
On Sat, 14 Mar 2020, Deepak Varma wrote:
>
>
> On Sunday, 15 March 2020 01:27:27 UTC+5:30, Julia Lawall wrote:
>
>
> On Sun, 15 Mar 2020, Deepak R Varma wrote:
>
> > Resolve general code indentation problems as detected by checkpatch script.
> > Implement code reformat and re-indentation as per coding style to
> > improve readability.
> >
> > Signed-off-by: Deepak R Varma <mh12g...@gmail.com>
> > ---
> > .../staging/comedi/drivers/ni_mio_common.c | 62 ++++++++++---------
> > 1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> > index b72a40a79930..48e20de3ebcf 100644
> > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > @@ -2198,10 +2198,10 @@ static int ni_ai_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> > NISTC_AI_TRIG_START1_SEL(0);
> > break;
> > case TRIG_EXT:
> > - ai_trig |= NISTC_AI_TRIG_START1_SEL(
> > - ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
> > - NI_AI_StartTrigger,
> > - &devpriv->routing_tables, 1));
> > + ai_trig |= NISTC_AI_TRIG_START1_SEL(ni_get_reg_value_roffs(
> > + CR_CHAN(cmd->start_arg),
> > + NI_AI_StartTrigger,
> > + &devpriv->routing_tables, 1));
>
> These may have been better as they were. At least the two function calls
> are more visible in the original.
>
>
> I think the reformatting makes it look simpler and more readable. Did you review it in mutt or here on this page? Please confirm if you want me to revert these back.
It looks strange to have the call to ni_get_reg_value_roffs to the right
of its arguments. It looks like the arguments are the arguments of
NISTC_AI_TRIG_START1_SEL.
>
> [...]
>
> > @@ -3079,12 +3081,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
> > * zero out these bit fields to be set below. Does an ao-reset do this
> > * automatically?
> > */
> > - devpriv->ao_mode1 &= ~(
> > - NISTC_AO_MODE1_UI_SRC_MASK |
> > - NISTC_AO_MODE1_UI_SRC_POLARITY |
> > - NISTC_AO_MODE1_UPDATE_SRC_MASK |
> > - NISTC_AO_MODE1_UPDATE_SRC_POLARITY
> > - );
> > + devpriv->ao_mode1 &= ~(NISTC_AO_MODE1_UI_SRC_MASK |
> > + NISTC_AO_MODE1_UI_SRC_POLARITY |
> > + NISTC_AO_MODE1_UPDATE_SRC_MASK |
> > + NISTC_AO_MODE1_UPDATE_SRC_POLARITY);
>
> This looks like an improveemnt. Are the subsequent NISTCs lines up with
> the right side of the (? It doesn't look like it, but the +s can change
> the appearance of the indentation.
>
>
> sorry, I did not follow this fully. Could you please explain what do you mean by improvement? There is no code change. I only realigned the NISTC* lines so that the "&= ~(" part is clearly visible. It definitely looks different here on the web page
> that the vi editor.
I would expect
(NISTC_AO_MODE1...
NISTC_AO_MODE1...
NISTC_AO_MODE1...
NISTC_AO_MODE1...
As far as I can see, you have
(NISTC_AO_MODE1...
NISTC_AO_MODE1...
NISTC_AO_MODE1...
NISTC_AO_MODE1...
>
>
> > if (cmd->scan_begin_src == TRIG_TIMER) {
> > unsigned int trigvar;
> > @@ -3133,10 +3133,10 @@ static void ni_ao_cmd_set_update(struct comedi_device *dev,
> > } else { /* TRIG_EXT */
> > /* FIXME: assert scan_begin_arg != 0, ret failure otherwise */
> > devpriv->ao_cmd2 |= NISTC_AO_CMD2_BC_GATE_ENA;
> > - devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(
> > - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> > - NI_AO_SampleClock,
> > - &devpriv->routing_tables));
> > + devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(ni_get_reg_value(
> > + CR_CHAN(cmd->scan_begin_arg),
> > + NI_AO_SampleClock,
> > + &devpriv->routing_tables));
> > if (cmd->scan_begin_arg & CR_INVERT)
> > devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC_POLARITY;
> > }
> > @@ -3672,13 +3672,15 @@ static int ni_cdio_cmd(struct comedi_device *dev, struct comedi_subdevice *s)
> > */
> > cdo_mode_bits = NI_M_CDO_MODE_FIFO_MODE |
> > NI_M_CDO_MODE_HALT_ON_ERROR |
> > - NI_M_CDO_MODE_SAMPLE_SRC(
> > - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> > - NI_DO_SampleClock,
> > - &devpriv->routing_tables));
> > + NI_M_CDO_MODE_SAMPLE_SRC(ni_get_reg_value(
> > + CR_CHAN(cmd->scan_begin_arg),
> > + NI_DO_SampleClock,
> > + &devpriv->routing_tables));
>
> These like the earlier ones are probably better left as is.
>
>
> Same as above. Please confirm if you want me to revert these.
>
>
> > if (cmd->scan_begin_arg & CR_INVERT)
> > cdo_mode_bits |= NI_M_CDO_MODE_POLARITY;
> > +
> > ni_writel(dev, cdo_mode_bits, NI_M_CDO_MODE_REG);
> > +
>
> I'm not sure what motivated adding these blank lines, but it doesn't seem
> to be the same issue as what is done in the rest of the patch.
>
>
> I think with the new line in place, it makes it easier to spot the ni_write function call easily. Otherwise it may get overlooked during a quick glace of this area. Let me know if you still think the new line is unnecessary and I will revert it. No
> problem.
It's not so much of whether it is necessary or not. The point is just
that none of the other changes involve adding new blank lines. So these
look like a different kind of change than the rest.
julia
> julia
>
> > if (s->io_bits) {
> > ni_writel(dev, s->state, NI_M_CDO_FIFO_DATA_REG);
> > ni_writel(dev, NI_M_CDO_CMD_SW_UPDATE, NI_M_CDIO_CMD_REG);
> > --
> > 2.17.1
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreach...@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200314194724.GA2711%40deeUbuntu.
> >
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/facae09d-0a64-4007-af28-d9da637a95c8%40googlegroups.com.
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation
2020-03-14 20:32 ` Julia Lawall
@ 2020-03-14 20:47 ` Deepak Varma
0 siblings, 0 replies; 5+ messages in thread
From: Deepak Varma @ 2020-03-14 20:47 UTC (permalink / raw)
To: outreachy-kernel
[-- Attachment #1.1: Type: text/plain, Size: 8916 bytes --]
On Sunday, 15 March 2020 02:02:07 UTC+5:30, Julia Lawall wrote:
>
>
>
> On Sat, 14 Mar 2020, Deepak Varma wrote:
>
> >
> >
> > On Sunday, 15 March 2020 01:27:27 UTC+5:30, Julia Lawall wrote:
> >
> >
> > On Sun, 15 Mar 2020, Deepak R Varma wrote:
> >
> > > Resolve general code indentation problems as detected by
> checkpatch script.
> > > Implement code reformat and re-indentation as per coding style
> to
> > > improve readability.
> > >
> > > Signed-off-by: Deepak R Varma <mh12g...@gmail.com>
> > > ---
> > > .../staging/comedi/drivers/ni_mio_common.c | 62
> ++++++++++---------
> > > 1 file changed, 32 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c
> b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > index b72a40a79930..48e20de3ebcf 100644
> > > --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> > > +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> > > @@ -2198,10 +2198,10 @@ static int ni_ai_cmd(struct
> comedi_device *dev, struct comedi_subdevice *s)
> > > NISTC_AI_TRIG_START1_SEL(0);
> > > break;
> > > case TRIG_EXT:
> > > - ai_trig |= NISTC_AI_TRIG_START1_SEL(
> > >
> - ni_get_reg_value_roffs(CR_CHAN(cmd->start_arg),
> > > -
> NI_AI_StartTrigger,
> > > -
> &devpriv->routing_tables, 1));
> > > + ai_trig |=
> NISTC_AI_TRIG_START1_SEL(ni_get_reg_value_roffs(
> > >
> + CR_CHAN(cmd->start_arg),
> > > + NI_AI_StartTrigger,
> > >
> + &devpriv->routing_tables, 1));
> >
> > These may have been better as they were. At least the two
> function calls
> > are more visible in the original.
> >
> >
> > I think the reformatting makes it look simpler and more readable. Did
> you review it in mutt or here on this page? Please confirm if you want me
> to revert these back.
>
> It looks strange to have the call to ni_get_reg_value_roffs to the right
> of its arguments. It looks like the arguments are the arguments of
> NISTC_AI_TRIG_START1_SEL.
>
I understand now. Makes sense. Let me try refining it and resend in v2.
>
> >
> > [...]
> >
> > > @@ -3079,12 +3081,10 @@ static void ni_ao_cmd_set_update(struct
> comedi_device *dev,
> > > * zero out these bit fields to be set below. Does an
> ao-reset do this
> > > * automatically?
> > > */
> > > - devpriv->ao_mode1 &= ~(
> > > - NISTC_AO_MODE1_UI_SRC_MASK |
> > > - NISTC_AO_MODE1_UI_SRC_POLARITY |
> > > - NISTC_AO_MODE1_UPDATE_SRC_MASK |
> > > - NISTC_AO_MODE1_UPDATE_SRC_POLARITY
> > > - );
> > > + devpriv->ao_mode1 &= ~(NISTC_AO_MODE1_UI_SRC_MASK
> |
> > > + NISTC_AO_MODE1_UI_SRC_POLARITY
> |
> > > + NISTC_AO_MODE1_UPDATE_SRC_MASK
> |
> > >
> + NISTC_AO_MODE1_UPDATE_SRC_POLARITY);
> >
> > This looks like an improveemnt. Are the subsequent NISTCs lines
> up with
> > the right side of the (? It doesn't look like it, but the +s can
> change
> > the appearance of the indentation.
> >
> >
> > sorry, I did not follow this fully. Could you please explain what do you
> mean by improvement? There is no code change. I only realigned the NISTC*
> lines so that the "&= ~(" part is clearly visible. It definitely looks
> different here on the web page
> > that the vi editor.
>
> I would expect
>
> (NISTC_AO_MODE1...
> NISTC_AO_MODE1...
> NISTC_AO_MODE1...
> NISTC_AO_MODE1...
>
> As far as I can see, you have
>
> (NISTC_AO_MODE1...
> NISTC_AO_MODE1...
> NISTC_AO_MODE1...
> NISTC_AO_MODE1...
>
> Yeah. I should correct that. Will include in v2. Thank you!
>
> >
> >
> > > if (cmd->scan_begin_src == TRIG_TIMER) {
> > > unsigned int trigvar;
> > > @@ -3133,10 +3133,10 @@ static void ni_ao_cmd_set_update(struct
> comedi_device *dev,
> > > } else { /* TRIG_EXT */
> > > /* FIXME: assert scan_begin_arg != 0, ret
> failure otherwise */
> > > devpriv->ao_cmd2 |= NISTC_AO_CMD2_BC_GATE_ENA;
> > > - devpriv->ao_mode1 |= NISTC_AO_MODE1_UPDATE_SRC(
> > >
> - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
> > > - NI_AO_SampleClock,
> > > -
> &devpriv->routing_tables));
> > > + devpriv->ao_mode1 |=
> NISTC_AO_MODE1_UPDATE_SRC(ni_get_reg_value(
> > >
> + CR_CHAN(cmd->scan_begin_arg),
>
> > >
> + NI_AO_SampleClock,
> > >
> + &devpriv->routing_tables));
>
> > > if (cmd->scan_begin_arg & CR_INVERT)
> > > devpriv->ao_mode1 |=
> NISTC_AO_MODE1_UPDATE_SRC_POLARITY;
> > > }
> > > @@ -3672,13 +3672,15 @@ static int ni_cdio_cmd(struct
> comedi_device *dev, struct comedi_subdevice *s)
> > > */
> > > cdo_mode_bits = NI_M_CDO_MODE_FIFO_MODE |
> > > NI_M_CDO_MODE_HALT_ON_ERROR |
> > > - NI_M_CDO_MODE_SAMPLE_SRC(
> > >
> - ni_get_reg_value(CR_CHAN(cmd->scan_begin_arg),
>
> > > -
> NI_DO_SampleClock,
> > > -
> &devpriv->routing_tables));
> > >
> + NI_M_CDO_MODE_SAMPLE_SRC(ni_get_reg_value(
> > >
> + CR_CHAN(cmd->scan_begin_arg),
>
> > >
> + NI_DO_SampleClock,
> > >
> + &devpriv->routing_tables));
>
> >
> > These like the earlier ones are probably better left as is.
> >
> >
> > Same as above. Please confirm if you want me to revert these.
> >
> >
> > > if (cmd->scan_begin_arg & CR_INVERT)
> > > cdo_mode_bits |= NI_M_CDO_MODE_POLARITY;
> > > +
> > > ni_writel(dev, cdo_mode_bits, NI_M_CDO_MODE_REG);
> > > +
> >
> > I'm not sure what motivated adding these blank lines, but it
> doesn't seem
> > to be the same issue as what is done in the rest of the patch.
> >
> >
> > I think with the new line in place, it makes it easier to spot the
> ni_write function call easily. Otherwise it may get overlooked during a
> quick glace of this area. Let me know if you still think the new line is
> unnecessary and I will revert it. No
> > problem.
>
> It's not so much of whether it is necessary or not. The point is just
> that none of the other changes involve adding new blank lines. So these
> look like a different kind of change than the rest.
>
Okay. I will revert it in v2. Thank you.
>
> julia
>
> > julia
> >
> > > if (s->io_bits) {
> > > ni_writel(dev, s->state,
> NI_M_CDO_FIFO_DATA_REG);
> > > ni_writel(dev, NI_M_CDO_CMD_SW_UPDATE,
> NI_M_CDIO_CMD_REG);
> > > --
> > > 2.17.1
> > >
> > > --
> > > You received this message because you are subscribed to the
> Google Groups "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from
> it, send an email to outreach...@googlegroups.com.
> > > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20200314194724.GA2711%40deeUbuntu.
>
> > >
> >
> > --
> > You received this message because you are subscribed to the Google
> Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to outreach...@googlegroups.com <javascript:>.
> > To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/facae09d-0a64-4007-af28-d9da637a95c8%40googlegroups.com.
>
> >
> >
[-- Attachment #1.2: Type: text/html, Size: 13557 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-14 20:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-03-14 19:47 [PATCH] staging: comedi: ni_mio_common: Code reformat and re-indentation Deepak R Varma
2020-03-14 19:57 ` [Outreachy kernel] " Julia Lawall
2020-03-14 20:13 ` Deepak Varma
2020-03-14 20:32 ` Julia Lawall
2020-03-14 20:47 ` Deepak Varma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.