diff for duplicates of <20181103104132.7140c9ed@archlinux> diff --git a/a/1.txt b/N1/1.txt index 0720a19..d59e79d 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -4,87 +4,77 @@ Matheus Tavares <matheus.bernardino@usp.br> wrote: > On 10/28/18 1:40 PM, Jonathan Cameron wrote: > > On Fri, 26 Oct 2018 23:00:00 -0300 > > Matheus Tavares <matheus.bernardino@usp.br> wrote: -> > =20 -> >> Previously, when spi_read returned an error code inside ad2s90_read_ra= -w, +> > +> >> Previously, when spi_read returned an error code inside ad2s90_read_raw, > >> the code was ignored and IIO_VAL_INT was returned. This patch makes the > >> function return the error code returned by spi_read when it fails. > >> -> >> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> =20 +> >> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> > > Hi Matheus, > > -> > One quick process note is that it takes people a while to get around to= - reviewing -> > a series, so whilst it's tempting to very quickly send out a fix the mo= -ment -> > someone points out something that needs fixing, it is perhaps better to= - wait -> > at least a few days to see if you can pick up a few more reviews before= - you +> > One quick process note is that it takes people a while to get around to reviewing +> > a series, so whilst it's tempting to very quickly send out a fix the moment +> > someone points out something that needs fixing, it is perhaps better to wait +> > at least a few days to see if you can pick up a few more reviews before you > > do a V2. > > > > A few comments on this one inline. I think it can be done 'slightly' -> > (and I mean only slightly) nicer than the version you have. Result is = -the +> > (and I mean only slightly) nicer than the version you have. Result is the > > same though. > > > > Thanks, > > > > Jonathan -> > =20 +> > > >> --- > >> drivers/staging/iio/resolver/ad2s90.c | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> -> >> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/i= -io/resolver/ad2s90.c +> >> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c > >> index 59586947a936..11fac9f90148 100644 > >> --- a/drivers/staging/iio/resolver/ad2s90.c > >> +++ b/drivers/staging/iio/resolver/ad2s90.c -> >> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_d= -ev, -> >> struct ad2s90_state *st =3D iio_priv(indio_dev); -> >> =20 +> >> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev, +> >> struct ad2s90_state *st = iio_priv(indio_dev); +> >> > >> mutex_lock(&st->lock); -> >> + =20 +> >> + > > Unconnected change. I'm not against the change in principle but please > > group white space tidying up in it's own patch. -> > =20 -> >> ret =3D spi_read(st->sdev, st->rx, 2); +> > +> >> ret = spi_read(st->sdev, st->rx, 2); > >> - if (ret) > >> - goto error_ret; > >> + if (ret < 0) { > >> + mutex_unlock(&st->lock); -> >> + return ret; =20 +> >> + return ret; > > I'd actually prefer to keep the return path the same as before as then > > it is easy (if the function gets more complex in future) to be sure -> > that all paths unlock the mutex. =20 ->=20 ->=20 -> Ok, got it! But then, in patch 5, when we add the switch for=20 -> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and=20 +> > that all paths unlock the mutex. +> +> +> Ok, got it! But then, in patch 5, when we add the switch for +> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and > label inside the switch case? I mean, should it be something like this: ->=20 ->=20 -> =C2=A0=C2=A0=C2=A0 switch (m) { -> =C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_SCALE: -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ... // Does not use mutex -> =C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_RAW: -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 mutex_lock(&st->lock); -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ret =3D spi_read(st->sdev, st->rx,= - 2); -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (ret) -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 goto error_ret; -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 *val =3D (((u16)(st->rx[0])) << 4)= - | ((st->rx[1] & 0xF0) >> 4); ->=20 +> +> +> switch (m) { +> case IIO_CHAN_INFO_SCALE: +> ... // Does not use mutex +> case IIO_CHAN_INFO_RAW: +> mutex_lock(&st->lock); +> ret = spi_read(st->sdev, st->rx, 2); +> if (ret) +> goto error_ret; +> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4); +> > error_ret: -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 mutex_unlock(&st->lock); ->=20 -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 return ret ? ret : IIO_VAL_INT; -> =C2=A0=C2=A0=C2=A0 default: -> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 break; -> =C2=A0=C2=A0=C2=A0 } +> mutex_unlock(&st->lock); +> +> return ret ? ret : IIO_VAL_INT; +> default: +> break; +> } This is was of those ugly signs that perhaps a given block of code should be factored out as a separate function. It does feel like overkill here though given how short the ugly bit will be ;) @@ -100,20 +90,20 @@ statement is there then we go back to the simple exit path. Thanks, Jonathan ->=20 ->=20 +> +> > Matheus ->=20 ->=20 +> +> > >> + } > >> + -> >> *val =3D (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4); -> >> =20 +> >> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4); +> >> > >> -error_ret: > >> mutex_unlock(&st->lock); -> >> =20 -> >> return IIO_VAL_INT; =20 +> >> +> >> return IIO_VAL_INT; > > The 'standard' if slightly nasty way of doing this is: > > > > return ret ? ret : IIO_VAL_INT; -> > =20 +> > diff --git a/a/content_digest b/N1/content_digest index 309099d..b262d60 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -23,87 +23,77 @@ "> On 10/28/18 1:40 PM, Jonathan Cameron wrote:\n" "> > On Fri, 26 Oct 2018 23:00:00 -0300\n" "> > Matheus Tavares <matheus.bernardino@usp.br> wrote:\n" - "> > =20\n" - "> >> Previously, when spi_read returned an error code inside ad2s90_read_ra=\n" - "w,\n" + "> > \n" + "> >> Previously, when spi_read returned an error code inside ad2s90_read_raw,\n" "> >> the code was ignored and IIO_VAL_INT was returned. This patch makes the\n" "> >> function return the error code returned by spi_read when it fails.\n" "> >>\n" - "> >> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> =20\n" + "> >> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> \n" "> > Hi Matheus,\n" "> >\n" - "> > One quick process note is that it takes people a while to get around to=\n" - " reviewing\n" - "> > a series, so whilst it's tempting to very quickly send out a fix the mo=\n" - "ment\n" - "> > someone points out something that needs fixing, it is perhaps better to=\n" - " wait\n" - "> > at least a few days to see if you can pick up a few more reviews before=\n" - " you\n" + "> > One quick process note is that it takes people a while to get around to reviewing\n" + "> > a series, so whilst it's tempting to very quickly send out a fix the moment\n" + "> > someone points out something that needs fixing, it is perhaps better to wait\n" + "> > at least a few days to see if you can pick up a few more reviews before you\n" "> > do a V2.\n" "> >\n" "> > A few comments on this one inline. I think it can be done 'slightly'\n" - "> > (and I mean only slightly) nicer than the version you have. Result is =\n" - "the\n" + "> > (and I mean only slightly) nicer than the version you have. Result is the\n" "> > same though.\n" "> >\n" "> > Thanks,\n" "> >\n" "> > Jonathan\n" - "> > =20\n" + "> > \n" "> >> ---\n" "> >> drivers/staging/iio/resolver/ad2s90.c | 9 ++++++---\n" "> >> 1 file changed, 6 insertions(+), 3 deletions(-)\n" "> >>\n" - "> >> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/i=\n" - "io/resolver/ad2s90.c\n" + "> >> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c\n" "> >> index 59586947a936..11fac9f90148 100644\n" "> >> --- a/drivers/staging/iio/resolver/ad2s90.c\n" "> >> +++ b/drivers/staging/iio/resolver/ad2s90.c\n" - "> >> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_d=\n" - "ev,\n" - "> >> \tstruct ad2s90_state *st =3D iio_priv(indio_dev);\n" - "> >> =20\n" + "> >> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,\n" + "> >> \tstruct ad2s90_state *st = iio_priv(indio_dev);\n" + "> >> \n" "> >> \tmutex_lock(&st->lock);\n" - "> >> + =20\n" + "> >> + \n" "> > Unconnected change. I'm not against the change in principle but please\n" "> > group white space tidying up in it's own patch.\n" - "> > =20\n" - "> >> \tret =3D spi_read(st->sdev, st->rx, 2);\n" + "> > \n" + "> >> \tret = spi_read(st->sdev, st->rx, 2);\n" "> >> -\tif (ret)\n" "> >> -\t\tgoto error_ret;\n" "> >> +\tif (ret < 0) {\n" "> >> +\t\tmutex_unlock(&st->lock);\n" - "> >> +\t\treturn ret; =20\n" + "> >> +\t\treturn ret; \n" "> > I'd actually prefer to keep the return path the same as before as then\n" "> > it is easy (if the function gets more complex in future) to be sure\n" - "> > that all paths unlock the mutex. =20\n" - ">=20\n" - ">=20\n" - "> Ok, got it! But then, in patch 5, when we add the switch for=20\n" - "> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and=20\n" + "> > that all paths unlock the mutex. \n" + "> \n" + "> \n" + "> Ok, got it! But then, in patch 5, when we add the switch for \n" + "> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and \n" "> label inside the switch case? I mean, should it be something like this:\n" - ">=20\n" - ">=20\n" - "> =C2=A0=C2=A0=C2=A0 switch (m) {\n" - "> =C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_SCALE:\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ... // Does not use mutex\n" - "> =C2=A0=C2=A0=C2=A0 case IIO_CHAN_INFO_RAW:\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 mutex_lock(&st->lock);\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 ret =3D spi_read(st->sdev, st->rx,=\n" - " 2);\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 if (ret)\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 goto error_ret;\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 *val =3D (((u16)(st->rx[0])) << 4)=\n" - " | ((st->rx[1] & 0xF0) >> 4);\n" - ">=20\n" + "> \n" + "> \n" + "> \302\240\302\240\302\240 switch (m) {\n" + "> \302\240\302\240\302\240 case IIO_CHAN_INFO_SCALE:\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 ... // Does not use mutex\n" + "> \302\240\302\240\302\240 case IIO_CHAN_INFO_RAW:\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 mutex_lock(&st->lock);\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 ret = spi_read(st->sdev, st->rx, 2);\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 if (ret)\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 \302\240\302\240\302\240 goto error_ret;\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);\n" + "> \n" "> error_ret:\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 mutex_unlock(&st->lock);\n" - ">=20\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 return ret ? ret : IIO_VAL_INT;\n" - "> =C2=A0=C2=A0=C2=A0 default:\n" - "> =C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 break;\n" - "> =C2=A0=C2=A0=C2=A0 }\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 mutex_unlock(&st->lock);\n" + "> \n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 return ret ? ret : IIO_VAL_INT;\n" + "> \302\240\302\240\302\240 default:\n" + "> \302\240\302\240\302\240 \302\240\302\240\302\240 break;\n" + "> \302\240\302\240\302\240 }\n" "This is was of those ugly signs that perhaps a given block of code\n" "should be factored out as a separate function. It does feel like\n" "overkill here though given how short the ugly bit will be ;)\n" @@ -119,22 +109,22 @@ "Thanks,\n" "\n" "Jonathan\n" - ">=20\n" - ">=20\n" + "> \n" + "> \n" "> Matheus\n" - ">=20\n" - ">=20\n" + "> \n" + "> \n" "> >> +\t}\n" "> >> +\n" - "> >> \t*val =3D (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);\n" - "> >> =20\n" + "> >> \t*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);\n" + "> >> \n" "> >> -error_ret:\n" "> >> \tmutex_unlock(&st->lock);\n" - "> >> =20\n" - "> >> \treturn IIO_VAL_INT; =20\n" + "> >> \n" + "> >> \treturn IIO_VAL_INT; \n" "> > The 'standard' if slightly nasty way of doing this is:\n" "> >\n" "> > \treturn ret ? ret : IIO_VAL_INT;\n" - > > =20 + > > -872261f4b853db8b392533c56b2e78ea5e2e7e4674536264865c8a10b8ed5024 +c8df58ca6a22d4d76487502632d08da7ef11a0705ebdb6fe138e292d2d21c9ab
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.