On Mon, 15 Oct 2018 09:32:24 -0700
Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> On 10/15/18 9:03 AM, Jonathan Cameron wrote:
> > On Sun, 14 Oct 2018 17:27:05 -0700
> > Martin Kelly <martin@xxxxxxxxxxxxxxxx> wrote:
> >
> >> From: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
> >>
> >> Miscellaneous cleanup to fix minor consistency, grammar, and spelling
> >> issues.
> >>
> >> Signed-off-by: Martin Kelly <martin@xxxxxxxxxxxxxxxx>
> > Hi Martin,
> >
> > Always nice to tidy up the corners.
> >
> > A few comments inline...
> >
> >> ---
> >> drivers/iio/common/st_sensors/st_sensors_core.c | 4 ++--
> >> drivers/iio/common/st_sensors/st_sensors_trigger.c | 4 ++--
> >> drivers/iio/magnetometer/st_magn_core.c | 6 +++---
> >> 3 files changed, 7 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> >> index 26fbd1bd9413..82882bc505f5 100644
> >> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> >> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> >> @@ -133,7 +133,7 @@ static int st_sensors_match_fs(struct st_sensor_settings *sensor_settings,
> >>
> >> for (i = 0; i < ST_SENSORS_FULLSCALE_AVL_MAX; i++) {
> >> if (sensor_settings->fs.fs_avl[i].num == 0)
> >> - goto st_sensors_match_odr_error;
> >> + goto st_sensors_match_fs_error;
> >>
> >> if (sensor_settings->fs.fs_avl[i].num == fs) {
> >> *index_fs_avl = i;
> >> @@ -142,7 +142,7 @@ static int st_sensors_match_fs(struct st_sensor_settings *sensor_settings,
> >> }
> >> }
> >>
> >> -st_sensors_match_odr_error:
> >> +st_sensors_match_fs_error:
> >> return ret;
> > This goto seems a little pointless in general. Let's just directly return instead of coming here
> > in the first place.
> >
>
> I think the goto is in place in case later logic is added that needs
> explicit cleanup. However, it's fine to just add that logic later, so
> it's a judgment call. If you prefer, I'll change it to an explicit return.
Please do.
>
> >> }
> >>
> >> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> >> index fdcc5a891958..224596b0e189 100644
> >> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> >> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> >> @@ -104,7 +104,7 @@ static irqreturn_t st_sensors_irq_thread(int irq, void *p)
> >> return IRQ_HANDLED;
> >>
> >> /*
> >> - * If we are using egde IRQs, new samples arrived while processing
> >> + * If we are using edge IRQs, new samples arrived while processing
> >> * the IRQ and those may be missed unless we pick them here, so poll
> >> * again. If the sensor delivery frequency is very high, this thread
> >> * turns into a polled loop handler.
> >> @@ -148,7 +148,7 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> >> if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
> >> dev_err(&indio_dev->dev,
> >> "falling/low specified for IRQ "
> >> - "but hardware only support rising/high: "
> >> + "but hardware supports only rising/high: "
> > Either of those two is fine to my mind so please leave this one alone.
> >
> > Good thing we never adopted an English language guide for the kernel ;)
> >
>
> This is a case in which I wouldn't issue a standalone patch just for it,
> but since I'm fixing other things, I might as well fix this too. There
> are two grammar issues here, one more important than the other:
>
> (1) "hardware support" --> "hardware supports". This is the main one I
> wanted to fix.
>
> (2) While I'm at it, technically the hardware "only supports rising/high
> interrupts" means the *only* thing it supports is rising/high
> interrupts, while we actually want to say that the only type of
> *interrupt* it supports is rising/high. "supports only" does that.
Yeah, maybe, but that's very much a specific context thing. If we were
writing a specification then that level of clarity would be needed -
actually you'd go further and probably word it something like.
"but the only interrupt types that the hardware supports are rising/high."
I don't really feel that strongly on it. If you want to go with it,
then fair enough - it's arguably more correct as you say though
it's not really open to being misinterpreted as it stands..
>
> Again, these things are minor, so if you'd like to fix (1), (2), or
> neither, it's all fine with me. Just let me know how to proceed.
>
> >> "will request rising/high\n");
> >> if (irq_trig == IRQF_TRIGGER_FALLING)
> >> irq_trig = IRQF_TRIGGER_RISING;
> >> diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
> >> index 72f6d1335a04..880c11c7f1cb 100644
> >> --- a/drivers/iio/magnetometer/st_magn_core.c
> >> +++ b/drivers/iio/magnetometer/st_magn_core.c
> >> @@ -29,9 +29,9 @@
> >> #define ST_MAGN_NUMBER_DATA_CHANNELS 3
> >>
> >> /* DEFAULT VALUE FOR SENSORS */
> >> -#define ST_MAGN_DEFAULT_OUT_X_H_ADDR 0X03
> >> -#define ST_MAGN_DEFAULT_OUT_Y_H_ADDR 0X07
> >> -#define ST_MAGN_DEFAULT_OUT_Z_H_ADDR 0X05
> >> +#define ST_MAGN_DEFAULT_OUT_X_H_ADDR 0x03
> >> +#define ST_MAGN_DEFAULT_OUT_Y_H_ADDR 0x07
> >> +#define ST_MAGN_DEFAULT_OUT_Z_H_ADDR 0x05
> >>
> >> /* FULLSCALE */
> >> #define ST_MAGN_FS_AVL_1300MG 1300
> >
> >
