| Message ID | cover.1550671256.git.hns@goldelico.com |
|---|---|
| Headers | show |
| Series | iio mount matrix - revitalize missing bindings documentation and provide code for bmc150, bmg160, bma180, itg3200, hmc584x | expand |
On Wed, Feb 20, 2019 at 03:00:50PM +0100, H. Nikolaus Schaller wrote: > This patch allows to read a mount-matrix device tree > property and report to user-space or in-kernel iio > clients. > +static const struct iio_mount_matrix * > +bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation; It's hard to read. Can you split such lines in your series to something like struct bmc150_accel_data *data = iio_priv(indio_dev); return &data->orientation; ? > +} > + > +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = { > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix), > + { }, Terminator lines better without comma. > +};
On Wed, Feb 20, 2019 at 03:00:52PM +0100, H. Nikolaus Schaller wrote: > This simplifies the code a little. > -err_buffer_cleanup: > - iio_triggered_buffer_cleanup(indio_dev); > -err_trigger_unregister: > - if (data->trig) > - iio_trigger_unregister(data->trig); > -err_trigger_free: > - iio_trigger_free(data->trig); > -err_chip_disable: > +err: Please, leave the original label name as it's more self-explanatory. > data->part_info->chip_disable(data); > > return ret;
On Wed, Feb 20, 2019 at 03:00:47PM +0100, H. Nikolaus Schaller wrote: > This patch series adds the mount-matrix to several iio sensor drivers > used in handheld devices. > > The mount-matrix translates the quite arbitrary orientation of the sensor > on some printed circuit board to user-tangible orientation in handheld > devices that relates to typical screen orientation. > > There was a bindings documentation by Linus Walleij but the patch > did not make it into mainline. Therefore I resend it here. > > Next I have added some clarifications (at least I hope it clarifies) > in a second patch. > > Finally, the patch set implements the hooks for the mount matrix > in several iio drivers: bmc150, bma180, bmg160, itg3200, hmc5843. > This includes also one patch for the bma180 to convert it to devm API. > > We use them in different variants of the omap3-gta04 so a separate > patch set will provide device tree additions for them. Thanks for the patch, overall good stuff there. Couple of things, though: - address my comments (consider them against entire series) - check my patch I sent today to support this from ACPI I wouldn't like to spread more drivers use specific of_foo_bar stuff where it's easily to support everything (note: device_property_* calls supports software fw nodes as well). Perhaps, you may incorporate my patch into your nice series. > > > H. Nikolaus Schaller (8): > iio: bindings: clarifications for mount-matrix bindings > iio: accel: bmc150: add mount matrix support > iio: accel: bma180: add mount matrix support > iio: accel: bma180: convert to devm > iio: gyro: bmg160: add mount matrix support > iio: gyro: itg3200: add mount matrix support > iio: magnetometer: bmc150: add mount matrix support > iio: magnetometer: hmc5843: add mount matrix support > > Linus Walleij (1): > iio: document bindings for mounting matrices > > .../devicetree/bindings/iio/mount-matrix.txt | 162 ++++++++++++++++++ > drivers/iio/accel/bma180.c | 70 ++++---- > drivers/iio/accel/bmc150-accel-core.c | 19 ++ > drivers/iio/gyro/bmg160_core.c | 19 ++ > drivers/iio/gyro/itg3200_core.c | 18 ++ > drivers/iio/magnetometer/bmc150_magn.c | 19 ++ > drivers/iio/magnetometer/hmc5843.h | 1 + > drivers/iio/magnetometer/hmc5843_core.c | 14 ++ > include/linux/iio/gyro/itg3200.h | 1 + > 9 files changed, 288 insertions(+), 35 deletions(-) > create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt > > -- > 2.19.1 >
> Am 20.02.2019 um 17:07 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Wed, Feb 20, 2019 at 03:00:50PM +0100, H. Nikolaus Schaller wrote: >> This patch allows to read a mount-matrix device tree >> property and report to user-space or in-kernel iio >> clients. > >> +static const struct iio_mount_matrix * >> +bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan) >> +{ >> + return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation; > > It's hard to read. > > Can you split such lines in your series to something like > > struct bmc150_accel_data *data = iio_priv(indio_dev); > > return &data->orientation; > > ? I think I did copy it verbatim from some other iio driver: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/iio/magnetometer/ak8975.c?h=next-20190220#n745 and checkpatch did not complain. But yes, it can and should be improved since it seems that I picked the only bad example as template... > >> +} >> + >> +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = { >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix), >> + { }, > > Terminator lines better without comma. Ok. > >> +}; > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, 20 Feb 2019 15:00:50 +0100 "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > This patch allows to read a mount-matrix device tree > property and report to user-space or in-kernel iio > clients. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> This will clash with Andy's current patch, but I'll fix that up if needed. Otherwise, one trivial suggestion inline. Jonathan > --- > drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > index 383c802eb5b8..9178846cfddc 100644 > --- a/drivers/iio/accel/bmc150-accel-core.c > +++ b/drivers/iio/accel/bmc150-accel-core.c > @@ -204,6 +204,7 @@ struct bmc150_accel_data { > int ev_enable_state; > int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */ > const struct bmc150_accel_chip_info *chip_info; > + struct iio_mount_matrix orientation; > }; > > static const struct { > @@ -796,6 +797,18 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev, > return sprintf(buf, "%d\n", state); > } > > +static const struct iio_mount_matrix * > +bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation; I would use a local variable as that casting is less than simple to parse to my eyes anyway! > +} > + > +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = { > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix), > + { }, > +}; > + > static IIO_CONST_ATTR(hwfifo_watermark_min, "1"); > static IIO_CONST_ATTR(hwfifo_watermark_max, > __stringify(BMC150_ACCEL_FIFO_LENGTH)); > @@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = { > .shift = 16 - (bits), \ > .endianness = IIO_LE, \ > }, \ > + .ext_info = bmc150_accel_ext_info, \ > .event_spec = &bmc150_accel_event, \ > .num_event_specs = 1 \ > } > @@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > > data->regmap = regmap; > > + ret = of_iio_read_mount_matrix(dev, "mount-matrix", > + &data->orientation); > + if (ret) > + return ret; > + > ret = bmc150_accel_chip_init(data); > if (ret < 0) > return ret;
> Am 20.02.2019 um 17:09 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Wed, Feb 20, 2019 at 03:00:52PM +0100, H. Nikolaus Schaller wrote: >> This simplifies the code a little. > >> -err_buffer_cleanup: >> - iio_triggered_buffer_cleanup(indio_dev); >> -err_trigger_unregister: >> - if (data->trig) >> - iio_trigger_unregister(data->trig); >> -err_trigger_free: >> - iio_trigger_free(data->trig); > >> -err_chip_disable: >> +err: > > Please, leave the original label name as it's more self-explanatory. Ok. > >> data->part_info->chip_disable(data); >> >> return ret; > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, 20 Feb 2019 15:00:52 +0100 "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > This simplifies the code a little. It does, but at the cost of introducing potential race conditions. Please don't do this. See below for why and a suggestion on how to resolve things if you want to make this change safely. Jonathan > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------ > 1 file changed, 21 insertions(+), 35 deletions(-) > > diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c > index 248be67e4f41..3f5ee2d495d3 100644 > --- a/drivers/iio/accel/bma180.c > +++ b/drivers/iio/accel/bma180.c > @@ -738,7 +738,7 @@ static int bma180_probe(struct i2c_client *client, > > ret = data->part_info->chip_config(data); > if (ret < 0) > - goto err_chip_disable; > + goto err; > > mutex_init(&data->mutex); > indio_dev->dev.parent = &client->dev; > @@ -748,12 +748,25 @@ static int bma180_probe(struct i2c_client *client, > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &bma180_info; > > + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, > + bma180_trigger_handler, NULL); > + if (ret < 0) { > + dev_err(&client->dev, "unable to setup iio triggered buffer\n"); > + goto err; > + } > + > + ret = devm_iio_device_register(&client->dev, indio_dev); > + if (ret < 0) { > + dev_err(&client->dev, "unable to register iio device\n"); > + goto err; > + } > + > if (client->irq > 0) { > - data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, > - indio_dev->id); > + data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", > + indio_dev->name, indio_dev->id); > if (!data->trig) { > ret = -ENOMEM; > - goto err_chip_disable; > + goto err; > } > > ret = devm_request_irq(&client->dev, client->irq, > @@ -761,7 +774,7 @@ static int bma180_probe(struct i2c_client *client, > "bma180_event", data->trig); > if (ret) { > dev_err(&client->dev, "unable to request IRQ\n"); > - goto err_trigger_free; > + goto err; > } > > data->trig->dev.parent = &client->dev; > @@ -769,34 +782,14 @@ static int bma180_probe(struct i2c_client *client, > iio_trigger_set_drvdata(data->trig, indio_dev); > indio_dev->trig = iio_trigger_get(data->trig); > > - ret = iio_trigger_register(data->trig); > + ret = devm_iio_trigger_register(&client->dev, data->trig); > if (ret) > - goto err_trigger_free; > - } > - > - ret = iio_triggered_buffer_setup(indio_dev, NULL, > - bma180_trigger_handler, NULL); > - if (ret < 0) { > - dev_err(&client->dev, "unable to setup iio triggered buffer\n"); > - goto err_trigger_unregister; > - } > - > - ret = iio_device_register(indio_dev); > - if (ret < 0) { > - dev_err(&client->dev, "unable to register iio device\n"); > - goto err_buffer_cleanup; > + goto err; > } > > return 0; > > -err_buffer_cleanup: > - iio_triggered_buffer_cleanup(indio_dev); > -err_trigger_unregister: > - if (data->trig) > - iio_trigger_unregister(data->trig); > -err_trigger_free: > - iio_trigger_free(data->trig); > -err_chip_disable: > +err: > data->part_info->chip_disable(data); > > return ret; > @@ -807,13 +800,6 @@ static int bma180_remove(struct i2c_client *client) > struct iio_dev *indio_dev = i2c_get_clientdata(client); > struct bma180_data *data = iio_priv(indio_dev); > > - iio_device_unregister(indio_dev); > - iio_triggered_buffer_cleanup(indio_dev); > - if (data->trig) { > - iio_trigger_unregister(data->trig); > - iio_trigger_free(data->trig); > - } > - Now we disable the device before removing it's userspace interface. Not a good idea. Generally I will insist on the remove order always being the precise opposite of probe simply because it is obviously correct. That includes cases which can be simply argued to be fine (not this one which isn't). The reason is readability trumps saving a few lines of code and it's a lot easier to check a probe and remove function against each other if the order is maintained. Of course, there are ways to do this by making all the unwind managed, but you haven't done this here. devm_add_action_or_reset is handy for this if you want to do it. > mutex_lock(&data->mutex); > data->part_info->chip_disable(data); > mutex_unlock(&data->mutex);
On Wed, 20 Feb 2019 15:00:56 +0100 "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > This patch allows to read a mount-matrix device tree > property and report to user-space or in-kernel iio > clients. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> The rest of these are all fine, though I suggest considering a local variable rather than the fiddly bit of casting. Jonathan > --- > drivers/iio/magnetometer/hmc5843.h | 1 + > drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h > index a75224cf99df..e3e22d2508d3 100644 > --- a/drivers/iio/magnetometer/hmc5843.h > +++ b/drivers/iio/magnetometer/hmc5843.h > @@ -43,6 +43,7 @@ struct hmc5843_data { > struct mutex lock; > struct regmap *regmap; > const struct hmc5843_chip_info *variant; > + struct iio_mount_matrix orientation; > __be16 buffer[8]; > }; > > diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c > index ada142fb7aa3..e6b6da70c152 100644 > --- a/drivers/iio/magnetometer/hmc5843_core.c > +++ b/drivers/iio/magnetometer/hmc5843_core.c > @@ -237,6 +237,13 @@ int hmc5843_set_measurement_configuration(struct iio_dev *indio_dev, > return hmc5843_set_meas_conf(data, meas_conf); > } > > +static const struct iio_mount_matrix * > +hmc5843_get_mount_matrix(const struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan) > +{ > + return &((struct hmc5843_data *)iio_priv(indio_dev))->orientation; > +} > + > static const struct iio_enum hmc5843_meas_conf_enum = { > .items = hmc5843_meas_conf_modes, > .num_items = ARRAY_SIZE(hmc5843_meas_conf_modes), > @@ -247,6 +254,7 @@ static const struct iio_enum hmc5843_meas_conf_enum = { > static const struct iio_chan_spec_ext_info hmc5843_ext_info[] = { > IIO_ENUM("meas_conf", true, &hmc5843_meas_conf_enum), > IIO_ENUM_AVAILABLE("meas_conf", &hmc5843_meas_conf_enum), > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix), > { }, > }; > > @@ -260,6 +268,7 @@ static const struct iio_enum hmc5983_meas_conf_enum = { > static const struct iio_chan_spec_ext_info hmc5983_ext_info[] = { > IIO_ENUM("meas_conf", true, &hmc5983_meas_conf_enum), > IIO_ENUM_AVAILABLE("meas_conf", &hmc5983_meas_conf_enum), > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix), > { }, > }; > > @@ -635,6 +644,11 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap, > data->variant = &hmc5843_chip_info_tbl[id]; > mutex_init(&data->lock); > > + ret = of_iio_read_mount_matrix(dev, "mount-matrix", > + &data->orientation); > + if (ret) > + return ret; > + > indio_dev->dev.parent = dev; > indio_dev->name = name; > indio_dev->info = &hmc5843_info;
> Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <jic23@kernel.org>: > > On Wed, 20 Feb 2019 15:00:50 +0100 > "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > >> This patch allows to read a mount-matrix device tree >> property and report to user-space or in-kernel iio >> clients. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > This will clash with Andy's current patch, but I'll fix that up if > needed. Otherwise, one trivial suggestion inline. Ok, thanks! > > Jonathan > >> --- >> drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c >> index 383c802eb5b8..9178846cfddc 100644 >> --- a/drivers/iio/accel/bmc150-accel-core.c >> +++ b/drivers/iio/accel/bmc150-accel-core.c >> @@ -204,6 +204,7 @@ struct bmc150_accel_data { >> int ev_enable_state; >> int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */ >> const struct bmc150_accel_chip_info *chip_info; >> + struct iio_mount_matrix orientation; >> }; >> >> static const struct { >> @@ -796,6 +797,18 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev, >> return sprintf(buf, "%d\n", state); >> } >> >> +static const struct iio_mount_matrix * >> +bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan) >> +{ >> + return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation; > > I would use a local variable as that casting is less than simple to parse > to my eyes anyway! Yes, Andy already suggested that. I just happend to pick one of the only two bad examples from all iio drivers as template... drivers/iio/imu/inv_mpu6050/inv_mpu_core.c drivers/iio/magnetometer/ak8975.c > >> +} >> + >> +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = { >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix), >> + { }, >> +}; >> + >> static IIO_CONST_ATTR(hwfifo_watermark_min, "1"); >> static IIO_CONST_ATTR(hwfifo_watermark_max, >> __stringify(BMC150_ACCEL_FIFO_LENGTH)); >> @@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = { >> .shift = 16 - (bits), \ >> .endianness = IIO_LE, \ >> }, \ >> + .ext_info = bmc150_accel_ext_info, \ >> .event_spec = &bmc150_accel_event, \ >> .num_event_specs = 1 \ >> } >> @@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, >> >> data->regmap = regmap; >> >> + ret = of_iio_read_mount_matrix(dev, "mount-matrix", >> + &data->orientation); >> + if (ret) >> + return ret; >> + >> ret = bmc150_accel_chip_init(data); >> if (ret < 0) >> return ret; > BR and thanks, Nikolaus
Hi, > Am 20.02.2019 um 17:18 schrieb Jonathan Cameron <jic23@kernel.org>: > > On Wed, 20 Feb 2019 15:00:52 +0100 > "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > >> This simplifies the code a little. > It does, but at the cost of introducing potential race conditions. > Please don't do this. See below for why and a suggestion on how > to resolve things if you want to make this change safely. Ok, I see. I just was working on that driver and thought that introducing devm is a good idea here. But since it does not improve any function (contrary to the mount-matrix patches), let's drop it for the moment. BR and thanks, Nikolaus > > Jonathan >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> >> --- >> drivers/iio/accel/bma180.c | 56 ++++++++++++++------------------------ >> 1 file changed, 21 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/iio/accel/bma180.c b/drivers/iio/accel/bma180.c >> index 248be67e4f41..3f5ee2d495d3 100644 >> --- a/drivers/iio/accel/bma180.c >> +++ b/drivers/iio/accel/bma180.c >> @@ -738,7 +738,7 @@ static int bma180_probe(struct i2c_client *client, >> >> ret = data->part_info->chip_config(data); >> if (ret < 0) >> - goto err_chip_disable; >> + goto err; >> >> mutex_init(&data->mutex); >> indio_dev->dev.parent = &client->dev; >> @@ -748,12 +748,25 @@ static int bma180_probe(struct i2c_client *client, >> indio_dev->modes = INDIO_DIRECT_MODE; >> indio_dev->info = &bma180_info; >> >> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL, >> + bma180_trigger_handler, NULL); >> + if (ret < 0) { >> + dev_err(&client->dev, "unable to setup iio triggered buffer\n"); >> + goto err; >> + } >> + >> + ret = devm_iio_device_register(&client->dev, indio_dev); >> + if (ret < 0) { >> + dev_err(&client->dev, "unable to register iio device\n"); >> + goto err; >> + } >> + >> if (client->irq > 0) { >> - data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, >> - indio_dev->id); >> + data->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d", >> + indio_dev->name, indio_dev->id); >> if (!data->trig) { >> ret = -ENOMEM; >> - goto err_chip_disable; >> + goto err; >> } >> >> ret = devm_request_irq(&client->dev, client->irq, >> @@ -761,7 +774,7 @@ static int bma180_probe(struct i2c_client *client, >> "bma180_event", data->trig); >> if (ret) { >> dev_err(&client->dev, "unable to request IRQ\n"); >> - goto err_trigger_free; >> + goto err; >> } >> >> data->trig->dev.parent = &client->dev; >> @@ -769,34 +782,14 @@ static int bma180_probe(struct i2c_client *client, >> iio_trigger_set_drvdata(data->trig, indio_dev); >> indio_dev->trig = iio_trigger_get(data->trig); >> >> - ret = iio_trigger_register(data->trig); >> + ret = devm_iio_trigger_register(&client->dev, data->trig); >> if (ret) >> - goto err_trigger_free; >> - } >> - >> - ret = iio_triggered_buffer_setup(indio_dev, NULL, >> - bma180_trigger_handler, NULL); >> - if (ret < 0) { >> - dev_err(&client->dev, "unable to setup iio triggered buffer\n"); >> - goto err_trigger_unregister; >> - } >> - >> - ret = iio_device_register(indio_dev); >> - if (ret < 0) { >> - dev_err(&client->dev, "unable to register iio device\n"); >> - goto err_buffer_cleanup; >> + goto err; >> } >> >> return 0; >> >> -err_buffer_cleanup: >> - iio_triggered_buffer_cleanup(indio_dev); >> -err_trigger_unregister: >> - if (data->trig) >> - iio_trigger_unregister(data->trig); >> -err_trigger_free: >> - iio_trigger_free(data->trig); >> -err_chip_disable: >> +err: >> data->part_info->chip_disable(data); >> >> return ret; >> @@ -807,13 +800,6 @@ static int bma180_remove(struct i2c_client *client) >> struct iio_dev *indio_dev = i2c_get_clientdata(client); >> struct bma180_data *data = iio_priv(indio_dev); >> >> - iio_device_unregister(indio_dev); >> - iio_triggered_buffer_cleanup(indio_dev); >> - if (data->trig) { >> - iio_trigger_unregister(data->trig); >> - iio_trigger_free(data->trig); >> - } >> - > Now we disable the device before removing it's userspace interface. > Not a good idea. > > Generally I will insist on the remove order always being the precise > opposite of probe simply because it is obviously correct. > That includes cases which can be simply argued to be fine (not > this one which isn't). The reason is readability trumps saving > a few lines of code and it's a lot easier to check a probe and > remove function against each other if the order is maintained. > > Of course, there are ways to do this by making all the unwind > managed, but you haven't done this here. > devm_add_action_or_reset is handy for this if you want to do it. > >> mutex_lock(&data->mutex); >> data->part_info->chip_disable(data); >> mutex_unlock(&data->mutex); >
> Am 20.02.2019 um 17:19 schrieb Jonathan Cameron <jic23@kernel.org>: > > On Wed, 20 Feb 2019 15:00:56 +0100 > "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > >> This patch allows to read a mount-matrix device tree >> property and report to user-space or in-kernel iio >> clients. >> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > The rest of these are all fine, though I suggest considering > a local variable rather than the fiddly bit of casting. Ok, fine. I will wait some days for further comments than then submit a v2 with all known improvements. BR and thanks, Nikolaus > > Jonathan >> --- >> drivers/iio/magnetometer/hmc5843.h | 1 + >> drivers/iio/magnetometer/hmc5843_core.c | 14 ++++++++++++++ >> 2 files changed, 15 insertions(+) >> >> diff --git a/drivers/iio/magnetometer/hmc5843.h b/drivers/iio/magnetometer/hmc5843.h >> index a75224cf99df..e3e22d2508d3 100644 >> --- a/drivers/iio/magnetometer/hmc5843.h >> +++ b/drivers/iio/magnetometer/hmc5843.h >> @@ -43,6 +43,7 @@ struct hmc5843_data { >> struct mutex lock; >> struct regmap *regmap; >> const struct hmc5843_chip_info *variant; >> + struct iio_mount_matrix orientation; >> __be16 buffer[8]; >> }; >> >> diff --git a/drivers/iio/magnetometer/hmc5843_core.c b/drivers/iio/magnetometer/hmc5843_core.c >> index ada142fb7aa3..e6b6da70c152 100644 >> --- a/drivers/iio/magnetometer/hmc5843_core.c >> +++ b/drivers/iio/magnetometer/hmc5843_core.c >> @@ -237,6 +237,13 @@ int hmc5843_set_measurement_configuration(struct iio_dev *indio_dev, >> return hmc5843_set_meas_conf(data, meas_conf); >> } >> >> +static const struct iio_mount_matrix * >> +hmc5843_get_mount_matrix(const struct iio_dev *indio_dev, >> + const struct iio_chan_spec *chan) >> +{ >> + return &((struct hmc5843_data *)iio_priv(indio_dev))->orientation; >> +} >> + >> static const struct iio_enum hmc5843_meas_conf_enum = { >> .items = hmc5843_meas_conf_modes, >> .num_items = ARRAY_SIZE(hmc5843_meas_conf_modes), >> @@ -247,6 +254,7 @@ static const struct iio_enum hmc5843_meas_conf_enum = { >> static const struct iio_chan_spec_ext_info hmc5843_ext_info[] = { >> IIO_ENUM("meas_conf", true, &hmc5843_meas_conf_enum), >> IIO_ENUM_AVAILABLE("meas_conf", &hmc5843_meas_conf_enum), >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix), >> { }, >> }; >> >> @@ -260,6 +268,7 @@ static const struct iio_enum hmc5983_meas_conf_enum = { >> static const struct iio_chan_spec_ext_info hmc5983_ext_info[] = { >> IIO_ENUM("meas_conf", true, &hmc5983_meas_conf_enum), >> IIO_ENUM_AVAILABLE("meas_conf", &hmc5983_meas_conf_enum), >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, hmc5843_get_mount_matrix), >> { }, >> }; >> >> @@ -635,6 +644,11 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap, >> data->variant = &hmc5843_chip_info_tbl[id]; >> mutex_init(&data->lock); >> >> + ret = of_iio_read_mount_matrix(dev, "mount-matrix", >> + &data->orientation); >> + if (ret) >> + return ret; >> + >> indio_dev->dev.parent = dev; >> indio_dev->name = name; >> indio_dev->info = &hmc5843_info; >
Hi Andy, > Am 20.02.2019 um 17:13 schrieb Andy Shevchenko <andriy.shevchenko@linux.intel.com>: > > On Wed, Feb 20, 2019 at 03:00:47PM +0100, H. Nikolaus Schaller wrote: >> This patch series adds the mount-matrix to several iio sensor drivers >> used in handheld devices. >> >> The mount-matrix translates the quite arbitrary orientation of the sensor >> on some printed circuit board to user-tangible orientation in handheld >> devices that relates to typical screen orientation. >> >> There was a bindings documentation by Linus Walleij but the patch >> did not make it into mainline. Therefore I resend it here. >> >> Next I have added some clarifications (at least I hope it clarifies) >> in a second patch. >> >> Finally, the patch set implements the hooks for the mount matrix >> in several iio drivers: bmc150, bma180, bmg160, itg3200, hmc5843. >> This includes also one patch for the bma180 to convert it to devm API. >> >> We use them in different variants of the omap3-gta04 so a separate >> patch set will provide device tree additions for them. > > Thanks for the patch, overall good stuff there. > Couple of things, though: > - address my comments (consider them against entire series) > - check my patch I sent today to support this from ACPI > > I wouldn't like to spread more drivers use specific of_foo_bar stuff where it's > easily to support everything (note: device_property_* calls supports software > fw nodes as well). > > Perhaps, you may incorporate my patch into your nice series. Ok, that looks like a good proposal which avoids compile issues right from beginning of v2. BR and thanks, Nikolaus > >> >> >> H. Nikolaus Schaller (8): >> iio: bindings: clarifications for mount-matrix bindings >> iio: accel: bmc150: add mount matrix support >> iio: accel: bma180: add mount matrix support >> iio: accel: bma180: convert to devm >> iio: gyro: bmg160: add mount matrix support >> iio: gyro: itg3200: add mount matrix support >> iio: magnetometer: bmc150: add mount matrix support >> iio: magnetometer: hmc5843: add mount matrix support >> >> Linus Walleij (1): >> iio: document bindings for mounting matrices >> >> .../devicetree/bindings/iio/mount-matrix.txt | 162 ++++++++++++++++++ >> drivers/iio/accel/bma180.c | 70 ++++---- >> drivers/iio/accel/bmc150-accel-core.c | 19 ++ >> drivers/iio/gyro/bmg160_core.c | 19 ++ >> drivers/iio/gyro/itg3200_core.c | 18 ++ >> drivers/iio/magnetometer/bmc150_magn.c | 19 ++ >> drivers/iio/magnetometer/hmc5843.h | 1 + >> drivers/iio/magnetometer/hmc5843_core.c | 14 ++ >> include/linux/iio/gyro/itg3200.h | 1 + >> 9 files changed, 288 insertions(+), 35 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/iio/mount-matrix.txt >> >> -- >> 2.19.1 >> > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, 20 Feb 2019 17:20:29 +0100 "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > > Am 20.02.2019 um 17:14 schrieb Jonathan Cameron <jic23@kernel.org>: > > > > On Wed, 20 Feb 2019 15:00:50 +0100 > > "H. Nikolaus Schaller" <hns@goldelico.com> wrote: > > > >> This patch allows to read a mount-matrix device tree > >> property and report to user-space or in-kernel iio > >> clients. > >> > >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > > This will clash with Andy's current patch, but I'll fix that up if > > needed. Otherwise, one trivial suggestion inline. > > Ok, thanks! > > > > > Jonathan > > > >> --- > >> drivers/iio/accel/bmc150-accel-core.c | 19 +++++++++++++++++++ > >> 1 file changed, 19 insertions(+) > >> > >> diff --git a/drivers/iio/accel/bmc150-accel-core.c b/drivers/iio/accel/bmc150-accel-core.c > >> index 383c802eb5b8..9178846cfddc 100644 > >> --- a/drivers/iio/accel/bmc150-accel-core.c > >> +++ b/drivers/iio/accel/bmc150-accel-core.c > >> @@ -204,6 +204,7 @@ struct bmc150_accel_data { > >> int ev_enable_state; > >> int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */ > >> const struct bmc150_accel_chip_info *chip_info; > >> + struct iio_mount_matrix orientation; > >> }; > >> > >> static const struct { > >> @@ -796,6 +797,18 @@ static ssize_t bmc150_accel_get_fifo_state(struct device *dev, > >> return sprintf(buf, "%d\n", state); > >> } > >> > >> +static const struct iio_mount_matrix * > >> +bmc150_accel_get_mount_matrix(const struct iio_dev *indio_dev, > >> + const struct iio_chan_spec *chan) > >> +{ > >> + return &((struct bmc150_accel_data *)iio_priv(indio_dev))->orientation; > > > > I would use a local variable as that casting is less than simple to parse > > to my eyes anyway! > > Yes, Andy already suggested that. I just happend to pick one of the only two bad examples > from all iio drivers as template... > > drivers/iio/imu/inv_mpu6050/inv_mpu_core.c > drivers/iio/magnetometer/ak8975.c If you like, feel free to send patches to tidy those up as well! Then no one else can be unlucky on the same thing in future. Jonathan > > > > >> +} > >> + > >> +static const struct iio_chan_spec_ext_info bmc150_accel_ext_info[] = { > >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, bmc150_accel_get_mount_matrix), > >> + { }, > >> +}; > >> + > >> static IIO_CONST_ATTR(hwfifo_watermark_min, "1"); > >> static IIO_CONST_ATTR(hwfifo_watermark_max, > >> __stringify(BMC150_ACCEL_FIFO_LENGTH)); > >> @@ -978,6 +991,7 @@ static const struct iio_event_spec bmc150_accel_event = { > >> .shift = 16 - (bits), \ > >> .endianness = IIO_LE, \ > >> }, \ > >> + .ext_info = bmc150_accel_ext_info, \ > >> .event_spec = &bmc150_accel_event, \ > >> .num_event_specs = 1 \ > >> } > >> @@ -1555,6 +1569,11 @@ int bmc150_accel_core_probe(struct device *dev, struct regmap *regmap, int irq, > >> > >> data->regmap = regmap; > >> > >> + ret = of_iio_read_mount_matrix(dev, "mount-matrix", > >> + &data->orientation); > >> + if (ret) > >> + return ret; > >> + > >> ret = bmc150_accel_chip_init(data); > >> if (ret < 0) > >> return ret; > > > > BR and thanks, > Nikolaus >