From: Jonathan Cameron <jic23@kernel.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Lorenzo Bianconi <lorenzo@kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>,
linux-iio@vger.kernel.org,
Dmitry Torokhov <dmitry.torokhov@gmail.com>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Andi Shyti <andi.shyti@kernel.org>,
Geert Uytterhoeven <geert+renesas@glider.be>,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH RFC/RFT] iio: imu: lsm6dsx: Use i2c_get_match_data()
Date: Mon, 28 Aug 2023 14:30:37 +0100 [thread overview]
Message-ID: <20230828143037.68e3e3bb@jic23-huawei> (raw)
In-Reply-To: <20230812083204.55346-1-biju.das.jz@bp.renesas.com>
On Sat, 12 Aug 2023 09:32:04 +0100
Biju Das <biju.das.jz@bp.renesas.com> wrote:
> Replace device_get_match_data() and id lookup for retrieving match data
> by i2c_get_match_data() by converting enum->pointer for data in the
> match table.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Hi Biju
So, there is an issue in this driver that I've been wanting fixed for a while
that is related to the fact we only pass through an enum and then map it
to the relevant structure internally. This driver doesn't handle fall back compatibles
and adds a look up that we shouldn't need.
Assuming we don't have incompatible parts with the same WhoAmI value
(it has happened in past *sigh)
What should be happening here is:
1) The match tables contain pointers to appropriate entries of st_lsm6dsx_sensor_settings[]
2) We then assume that that info isn't available and try matching whoami against all devices
possibly with a short cut for the value matching what is expected.
If it is the one we expect. Good.
If it is a different whoami but we know about it. Good - maybe print some info as that's
probably some outdated firmware..
If we have no idea print a message and muddle on with the data provided by the id table
as that's probably a fallback compatible case where the whoami is different but the chip
is fully compatible.
Key to this is that we should just point into st_lsm6dsx_sensor_settings but that needs some
mess probably via externs pointers with definitions in the header, but set in the core file,
to avoid having to make that huge structure directly visible.
Not a nice one to solve and I'm open to other suggestions on how to tidy this up.
Jonathan
> ---
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c | 126 ++++++++++++--------
> 1 file changed, 74 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> index 911444ec57c0..a2def435c9c2 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> @@ -16,6 +16,30 @@
>
> #include "st_lsm6dsx.h"
>
> +static const int lsm6ds3 = ST_LSM6DS3_ID;
> +static const int lsm6ds3h = ST_LSM6DS3H_ID;
> +static const int lsm6dsl = ST_LSM6DSL_ID;
> +static const int lsm6dsm = ST_LSM6DSM_ID;
> +static const int ism330dlc = ST_ISM330DLC_ID;
> +static const int lsm6dso = ST_LSM6DSO_ID;
> +static const int asm330lhh = ST_ASM330LHH_ID;
> +static const int lsm6dsox = ST_LSM6DSOX_ID;
> +static const int lsm6dsr = ST_LSM6DSR_ID;
> +static const int lsm6ds3tr_c = ST_LSM6DS3TRC_ID;
> +static const int ism330dhcx = ST_ISM330DHCX_ID;
> +static const int lsm9ds1_imu = ST_LSM9DS1_ID;
> +static const int lsm6ds0 = ST_LSM6DS0_ID;
> +static const int lsm6dsrx = ST_LSM6DSRX_ID;
> +static const int lsm6dst = ST_LSM6DST_ID;
> +static const int lsm6dsop = ST_LSM6DSOP_ID;
> +static const int asm330lhhx = ST_ASM330LHHX_ID;
> +static const int lsm6dstx = ST_LSM6DSTX_ID;
> +static const int lsm6dsv = ST_LSM6DSV_ID;
> +static const int lsm6dsv16x = ST_LSM6DSV16X_ID;
> +static const int lsm6dso16is = ST_LSM6DSO16IS_ID;
> +static const int ism330is = ST_ISM330IS_ID;
> +static const int asm330lhb = ST_ASM330LHB_ID;
> +
> static const struct regmap_config st_lsm6dsx_i2c_regmap_config = {
> .reg_bits = 8,
> .val_bits = 8,
> @@ -23,12 +47,10 @@ static const struct regmap_config st_lsm6dsx_i2c_regmap_config = {
>
> static int st_lsm6dsx_i2c_probe(struct i2c_client *client)
> {
> - int hw_id;
> + const int *hw_id;
> struct regmap *regmap;
>
> - hw_id = (kernel_ulong_t)device_get_match_data(&client->dev);
> - if (!hw_id)
> - hw_id = i2c_client_get_device_id(client)->driver_data;
> + hw_id = i2c_get_match_data(client);
> if (!hw_id)
> return -EINVAL;
>
> @@ -38,136 +60,136 @@ static int st_lsm6dsx_i2c_probe(struct i2c_client *client)
> return PTR_ERR(regmap);
> }
>
> - return st_lsm6dsx_probe(&client->dev, client->irq, hw_id, regmap);
> + return st_lsm6dsx_probe(&client->dev, client->irq, *hw_id, regmap);
> }
>
> static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
> {
> .compatible = "st,lsm6ds3",
> - .data = (void *)ST_LSM6DS3_ID,
> + .data = &lsm6ds3,
> },
> {
> .compatible = "st,lsm6ds3h",
> - .data = (void *)ST_LSM6DS3H_ID,
> + .data = &lsm6ds3h,
> },
> {
> .compatible = "st,lsm6dsl",
> - .data = (void *)ST_LSM6DSL_ID,
> + .data = &lsm6dsl,
> },
> {
> .compatible = "st,lsm6dsm",
> - .data = (void *)ST_LSM6DSM_ID,
> + .data = &lsm6dsm,
> },
> {
> .compatible = "st,ism330dlc",
> - .data = (void *)ST_ISM330DLC_ID,
> + .data = &ism330dlc,
> },
> {
> .compatible = "st,lsm6dso",
> - .data = (void *)ST_LSM6DSO_ID,
> + .data = &lsm6dso,
> },
> {
> .compatible = "st,asm330lhh",
> - .data = (void *)ST_ASM330LHH_ID,
> + .data = &asm330lhh,
> },
> {
> .compatible = "st,lsm6dsox",
> - .data = (void *)ST_LSM6DSOX_ID,
> + .data = &lsm6dsox,
> },
> {
> .compatible = "st,lsm6dsr",
> - .data = (void *)ST_LSM6DSR_ID,
> + .data = &lsm6dsr,
> },
> {
> .compatible = "st,lsm6ds3tr-c",
> - .data = (void *)ST_LSM6DS3TRC_ID,
> + .data = &lsm6ds3tr_c,
> },
> {
> .compatible = "st,ism330dhcx",
> - .data = (void *)ST_ISM330DHCX_ID,
> + .data = &ism330dhcx,
> },
> {
> .compatible = "st,lsm9ds1-imu",
> - .data = (void *)ST_LSM9DS1_ID,
> + .data = &lsm9ds1_imu,
> },
> {
> .compatible = "st,lsm6ds0",
> - .data = (void *)ST_LSM6DS0_ID,
> + .data = &lsm6ds0,
> },
> {
> .compatible = "st,lsm6dsrx",
> - .data = (void *)ST_LSM6DSRX_ID,
> + .data = &lsm6dsrx,
> },
> {
> .compatible = "st,lsm6dst",
> - .data = (void *)ST_LSM6DST_ID,
> + .data = &lsm6dst,
> },
> {
> .compatible = "st,lsm6dsop",
> - .data = (void *)ST_LSM6DSOP_ID,
> + .data = &lsm6dsop,
> },
> {
> .compatible = "st,asm330lhhx",
> - .data = (void *)ST_ASM330LHHX_ID,
> + .data = &asm330lhhx,
> },
> {
> .compatible = "st,lsm6dstx",
> - .data = (void *)ST_LSM6DSTX_ID,
> + .data = &lsm6dstx,
> },
> {
> .compatible = "st,lsm6dsv",
> - .data = (void *)ST_LSM6DSV_ID,
> + .data = &lsm6dsv,
> },
> {
> .compatible = "st,lsm6dsv16x",
> - .data = (void *)ST_LSM6DSV16X_ID,
> + .data = &lsm6dsv16x,
> },
> {
> .compatible = "st,lsm6dso16is",
> - .data = (void *)ST_LSM6DSO16IS_ID,
> + .data = &lsm6dso16is,
> },
> {
> .compatible = "st,ism330is",
> - .data = (void *)ST_ISM330IS_ID,
> + .data = &ism330is,
> },
> {
> .compatible = "st,asm330lhb",
> - .data = (void *)ST_ASM330LHB_ID,
> + .data = &asm330lhb,
> },
> {},
> };
> MODULE_DEVICE_TABLE(of, st_lsm6dsx_i2c_of_match);
>
> static const struct acpi_device_id st_lsm6dsx_i2c_acpi_match[] = {
> - { "SMO8B30", ST_LSM6DS3TRC_ID, },
> + { "SMO8B30", (kernel_ulong_t)&lsm6ds3tr_c, },
> {}
> };
> MODULE_DEVICE_TABLE(acpi, st_lsm6dsx_i2c_acpi_match);
>
> static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
> - { ST_LSM6DS3_DEV_NAME, ST_LSM6DS3_ID },
> - { ST_LSM6DS3H_DEV_NAME, ST_LSM6DS3H_ID },
> - { ST_LSM6DSL_DEV_NAME, ST_LSM6DSL_ID },
> - { ST_LSM6DSM_DEV_NAME, ST_LSM6DSM_ID },
> - { ST_ISM330DLC_DEV_NAME, ST_ISM330DLC_ID },
> - { ST_LSM6DSO_DEV_NAME, ST_LSM6DSO_ID },
> - { ST_ASM330LHH_DEV_NAME, ST_ASM330LHH_ID },
> - { ST_LSM6DSOX_DEV_NAME, ST_LSM6DSOX_ID },
> - { ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
> - { ST_LSM6DS3TRC_DEV_NAME, ST_LSM6DS3TRC_ID },
> - { ST_ISM330DHCX_DEV_NAME, ST_ISM330DHCX_ID },
> - { ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
> - { ST_LSM6DS0_DEV_NAME, ST_LSM6DS0_ID },
> - { ST_LSM6DSRX_DEV_NAME, ST_LSM6DSRX_ID },
> - { ST_LSM6DST_DEV_NAME, ST_LSM6DST_ID },
> - { ST_LSM6DSOP_DEV_NAME, ST_LSM6DSOP_ID },
> - { ST_ASM330LHHX_DEV_NAME, ST_ASM330LHHX_ID },
> - { ST_LSM6DSTX_DEV_NAME, ST_LSM6DSTX_ID },
> - { ST_LSM6DSV_DEV_NAME, ST_LSM6DSV_ID },
> - { ST_LSM6DSV16X_DEV_NAME, ST_LSM6DSV16X_ID },
> - { ST_LSM6DSO16IS_DEV_NAME, ST_LSM6DSO16IS_ID },
> - { ST_ISM330IS_DEV_NAME, ST_ISM330IS_ID },
> - { ST_ASM330LHB_DEV_NAME, ST_ASM330LHB_ID },
> + { ST_LSM6DS3_DEV_NAME, (kernel_ulong_t)&lsm6ds3 },
> + { ST_LSM6DS3H_DEV_NAME, (kernel_ulong_t)&lsm6ds3h },
> + { ST_LSM6DSL_DEV_NAME, (kernel_ulong_t)&lsm6dsl },
> + { ST_LSM6DSM_DEV_NAME, (kernel_ulong_t)&lsm6dsm },
> + { ST_ISM330DLC_DEV_NAME, (kernel_ulong_t)&ism330dlc },
> + { ST_LSM6DSO_DEV_NAME, (kernel_ulong_t)&lsm6dso },
> + { ST_ASM330LHH_DEV_NAME, (kernel_ulong_t)&asm330lhh },
> + { ST_LSM6DSOX_DEV_NAME, (kernel_ulong_t)&lsm6dsox },
> + { ST_LSM6DSR_DEV_NAME, (kernel_ulong_t)&lsm6dsr },
> + { ST_LSM6DS3TRC_DEV_NAME, (kernel_ulong_t)&lsm6ds3tr_c },
> + { ST_ISM330DHCX_DEV_NAME, (kernel_ulong_t)&ism330dhcx },
> + { ST_LSM9DS1_DEV_NAME, (kernel_ulong_t)&lsm9ds1_imu },
> + { ST_LSM6DS0_DEV_NAME, (kernel_ulong_t)&lsm6ds0 },
> + { ST_LSM6DSRX_DEV_NAME, (kernel_ulong_t)&lsm6dsrx },
> + { ST_LSM6DST_DEV_NAME, (kernel_ulong_t)&lsm6dst },
> + { ST_LSM6DSOP_DEV_NAME, (kernel_ulong_t)&lsm6dsop },
> + { ST_ASM330LHHX_DEV_NAME, (kernel_ulong_t)&asm330lhhx },
> + { ST_LSM6DSTX_DEV_NAME, (kernel_ulong_t)&lsm6dstx },
> + { ST_LSM6DSV_DEV_NAME, (kernel_ulong_t)&lsm6dsv },
> + { ST_LSM6DSV16X_DEV_NAME, (kernel_ulong_t)&lsm6dsv16x },
> + { ST_LSM6DSO16IS_DEV_NAME, (kernel_ulong_t)&lsm6dso16is },
> + { ST_ISM330IS_DEV_NAME, (kernel_ulong_t)&ism330is },
> + { ST_ASM330LHB_DEV_NAME, (kernel_ulong_t)&asm330lhb },
> {},
> };
> MODULE_DEVICE_TABLE(i2c, st_lsm6dsx_i2c_id_table);
prev parent reply other threads:[~2023-08-28 13:31 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-12 8:32 [PATCH RFC/RFT] iio: imu: lsm6dsx: Use i2c_get_match_data() Biju Das
2023-08-14 12:31 ` Geert Uytterhoeven
2023-08-14 12:59 ` Biju Das
2023-08-15 7:16 ` Andy Shevchenko
2023-08-28 13:30 ` Jonathan Cameron [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230828143037.68e3e3bb@jic23-huawei \
--to=jic23@kernel.org \
--cc=andi.shyti@kernel.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=biju.das.jz@bp.renesas.com \
--cc=dmitry.torokhov@gmail.com \
--cc=geert+renesas@glider.be \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=lorenzo@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox