Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
On Mon, 18 May 2020 14:19:11 +
Jean-Baptiste Maneyrol wrote:
> Hi Jonathan,
>
> I am using generic device_get_match_data because I was thinking it was now
> the way to go. But since only of is supported with the driver, I can switch
> to using of_device_get_match_data instead.
>
> Tell me what do you think is better.
Leave it as you have it. We can look at if we can tidy up
what I was rambling about as a separate exercise.
J
>
> I could definitely use the new probe interface indeed, good idea.
>
> Thanks,
> JB
>
>
>
> From: Jonathan Cameron
>
> Sent: Friday, May 8, 2020 15:44
>
> To: Jean-Baptiste Maneyrol
>
> Cc: ji...@kernel.org ; robh...@kernel.org
> ; r...@kernel.org ;
> mchehab+hua...@kernel.org ; da...@davemloft.net
> ; gre...@linuxfoundation.org
> ;
> linux-...@vger.kernel.org ;
> devicet...@vger.kernel.org ;
> linux-kernel@vger.kernel.org
>
> Subject: Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for
> inv_icm42600 driver
>
>
>
>
> CAUTION: This email originated from outside of the organization. Please make
> sure the sender is who they say they are and do not click links or open
> attachments unless you recognize the sender and know the content is safe.
>
>
>
> On Thu, 7 May 2020 16:42:12 +0200
>
> Jean-Baptiste Maneyrol wrote:
>
>
>
> > Add I2C driver for InvenSense ICM-426xxx devices.
>
> >
>
> > Configure bus signal slew rates as indicated in the datasheet.
>
> >
>
> > Signed-off-by: Jean-Baptiste Maneyrol
>
> Some incoherent rambling inline. + a few comments
>
>
>
> Jonathan
>
>
>
> > ---
>
> > .../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 117 ++
>
> > 1 file changed, 117 insertions(+)
>
> > create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> >
>
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> > new file mode 100644
>
> > index ..b61f993beacf
>
> > --- /dev/null
>
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> > @@ -0,0 +1,117 @@
>
> > +// SPDX-License-Identifier: GPL-2.0-or-later
>
> > +/*
>
> > + * Copyright (C) 2020 InvenSense, Inc.
>
> > + */
>
> > +
>
> > +#include
>
> > +#include
>
> > +#include
>
> > +#include
>
> > +#include
>
>
>
> Why? Looks like you need the table and the device property stuff neither
>
> of which are in that file.
>
>
>
> linux/mod_devicetable.h
>
> linux/property.h
>
>
>
>
>
> > +
>
> > +#include "inv_icm42600.h"
>
> > +
>
> > +static int inv_icm42600_i2c_bus_setup(struct inv_icm42600_state *st)
>
> > +{
>
> > + unsigned int mask, val;
>
> > + int ret;
>
> > +
>
> > + /* setup interface registers */
>
> > + mask = INV_ICM42600_INTF_CONFIG6_MASK;
>
> > + val = INV_ICM42600_INTF_CONFIG6_I3C_EN;
>
> > + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG6,
>
> > + mask, val);
>
>
>
> I'd put the values inline where they are simple like these rather than
>
> using local variables.
>
>
>
> > + if (ret)
>
> > + return ret;
>
> > +
>
> > + mask = INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY;
>
> > + val = 0;
>
> > + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG4,
>
> > + mask, val);
>
> > + if (ret)
>
> > + return ret;
>
> > +
>
> > + /* set slew rates for I2C and SPI */
>
> > + mask = INV_ICM42600_DRIVE_CONFIG_I2C_MASK |
>
> > + INV_ICM42600_DRIVE_CONFIG_SPI_MASK;
>
> > + val = INV_ICM42600_DRIVE_CONFIG_I2C(INV_ICM42600_SLEW_RATE_12_36NS) |
> >
>
> > + INV_ICM42600_DRIVE_CONFIG_SPI(INV_ICM42600_SLEW_RATE_12_36NS);
>
> > + ret = regmap_update_bits(st->map, INV_ICM42600_REG_DRIVE_CONFIG,
>
> > + mask, val);
>
> > + if (ret)
>
> > + return ret;
>
> > +
>
> > + /* disable SPI bus */
>
> > + mask = INV_ICM42600_INTF_CONFI
Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
Hi Jonathan,
I am using generic device_get_match_data because I was thinking it was now the
way to go. But since only of is supported with the driver, I can switch to
using of_device_get_match_data instead.
Tell me what do you think is better.
I could definitely use the new probe interface indeed, good idea.
Thanks,
JB
From: Jonathan Cameron
Sent: Friday, May 8, 2020 15:44
To: Jean-Baptiste Maneyrol
Cc: ji...@kernel.org ; robh...@kernel.org
; r...@kernel.org ;
mchehab+hua...@kernel.org ; da...@davemloft.net
; gre...@linuxfoundation.org ;
linux-...@vger.kernel.org ;
devicet...@vger.kernel.org ;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for
inv_icm42600 driver
CAUTION: This email originated from outside of the organization. Please make
sure the sender is who they say they are and do not click links or open
attachments unless you recognize the sender and know the content is safe.
On Thu, 7 May 2020 16:42:12 +0200
Jean-Baptiste Maneyrol wrote:
> Add I2C driver for InvenSense ICM-426xxx devices.
>
> Configure bus signal slew rates as indicated in the datasheet.
>
> Signed-off-by: Jean-Baptiste Maneyrol
Some incoherent rambling inline. + a few comments
Jonathan
> ---
> .../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 117 ++
> 1 file changed, 117 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> new file mode 100644
> index ..b61f993beacf
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 InvenSense, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
Why? Looks like you need the table and the device property stuff neither
of which are in that file.
linux/mod_devicetable.h
linux/property.h
> +
> +#include "inv_icm42600.h"
> +
> +static int inv_icm42600_i2c_bus_setup(struct inv_icm42600_state *st)
> +{
> + unsigned int mask, val;
> + int ret;
> +
> + /* setup interface registers */
> + mask = INV_ICM42600_INTF_CONFIG6_MASK;
> + val = INV_ICM42600_INTF_CONFIG6_I3C_EN;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG6,
> + mask, val);
I'd put the values inline where they are simple like these rather than
using local variables.
> + if (ret)
> + return ret;
> +
> + mask = INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY;
> + val = 0;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG4,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + /* set slew rates for I2C and SPI */
> + mask = INV_ICM42600_DRIVE_CONFIG_I2C_MASK |
> + INV_ICM42600_DRIVE_CONFIG_SPI_MASK;
> + val = INV_ICM42600_DRIVE_CONFIG_I2C(INV_ICM42600_SLEW_RATE_12_36NS) |
> + INV_ICM42600_DRIVE_CONFIG_SPI(INV_ICM42600_SLEW_RATE_12_36NS);
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_DRIVE_CONFIG,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + /* disable SPI bus */
> + mask = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK;
> + val = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS;
> + return regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> + mask, val);
> +}
> +
> +static int inv_icm42600_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const void *match;
> + enum inv_icm42600_chip chip;
> + struct regmap *regmap;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return -ENOTSUPP;
> +
> + match = device_get_match_data(>dev);
Hmm. Annoyingly if one were to call the of specific option
of i2c_of_match_device it would handle the old style i2c match just fine without
needing special handling. However, it would fail to handle PRP0001 ACPI.
Rather feels like there should be something similar for
device_get_match_data so we could use the probe_new version of i2c device
probing.
Oh well. I guess thats a separate question for another day ;)
Mind you can we actually probe this driver via the sysfs route?
If not why do we need to handle the non firmware case at all?
> + if (match)
> + chip = (enum inv_icm42600_chip)match;
> +
Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
On Thu, 7 May 2020 16:42:12 +0200
Jean-Baptiste Maneyrol wrote:
> Add I2C driver for InvenSense ICM-426xxx devices.
>
> Configure bus signal slew rates as indicated in the datasheet.
>
> Signed-off-by: Jean-Baptiste Maneyrol
Some incoherent rambling inline. + a few comments
Jonathan
> ---
> .../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 117 ++
> 1 file changed, 117 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> new file mode 100644
> index ..b61f993beacf
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 InvenSense, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
Why? Looks like you need the table and the device property stuff neither
of which are in that file.
linux/mod_devicetable.h
linux/property.h
> +
> +#include "inv_icm42600.h"
> +
> +static int inv_icm42600_i2c_bus_setup(struct inv_icm42600_state *st)
> +{
> + unsigned int mask, val;
> + int ret;
> +
> + /* setup interface registers */
> + mask = INV_ICM42600_INTF_CONFIG6_MASK;
> + val = INV_ICM42600_INTF_CONFIG6_I3C_EN;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG6,
> + mask, val);
I'd put the values inline where they are simple like these rather than
using local variables.
> + if (ret)
> + return ret;
> +
> + mask = INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY;
> + val = 0;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG4,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + /* set slew rates for I2C and SPI */
> + mask = INV_ICM42600_DRIVE_CONFIG_I2C_MASK |
> +INV_ICM42600_DRIVE_CONFIG_SPI_MASK;
> + val = INV_ICM42600_DRIVE_CONFIG_I2C(INV_ICM42600_SLEW_RATE_12_36NS) |
> + INV_ICM42600_DRIVE_CONFIG_SPI(INV_ICM42600_SLEW_RATE_12_36NS);
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_DRIVE_CONFIG,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + /* disable SPI bus */
> + mask = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK;
> + val = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS;
> + return regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> + mask, val);
> +}
> +
> +static int inv_icm42600_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const void *match;
> + enum inv_icm42600_chip chip;
> + struct regmap *regmap;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return -ENOTSUPP;
> +
> + match = device_get_match_data(>dev);
Hmm. Annoyingly if one were to call the of specific option
of i2c_of_match_device it would handle the old style i2c match just fine without
needing special handling. However, it would fail to handle PRP0001 ACPI.
Rather feels like there should be something similar for
device_get_match_data so we could use the probe_new version of i2c device
probing.
Oh well. I guess thats a separate question for another day ;)
Mind you can we actually probe this driver via the sysfs route?
If not why do we need to handle the non firmware case at all?
> + if (match)
> + chip = (enum inv_icm42600_chip)match;
> + else if (id)
> + chip = (enum inv_icm42600_chip)id->driver_data;
> + else
> + return -EINVAL;
> +
> + regmap = devm_regmap_init_i2c(client, _icm42600_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return inv_icm42600_core_probe(regmap, chip,
> +inv_icm42600_i2c_bus_setup);
> +}
> +
> +static const struct of_device_id inv_icm42600_of_matches[] = {
> + {
> + .compatible = "invensense,icm42600",
> + .data = (void *)INV_CHIP_ICM42600,
> + }, {
> + .compatible = "invensense,icm42602",
> + .data = (void *)INV_CHIP_ICM42602,
> + }, {
> + .compatible = "invensense,icm42605",
> + .data = (void *)INV_CHIP_ICM42605,
> + }, {
> + .compatible = "invensense,icm42622",
> + .data = (void *)INV_CHIP_ICM42622,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, inv_icm42600_of_matches);
> +
> +static const struct i2c_device_id inv_icm42600_ids[] = {
> + {"icm42600", INV_CHIP_ICM42600},
> + {"icm42602", INV_CHIP_ICM42602},
> + {"icm42605", INV_CHIP_ICM42605},
> + {"icm42622", INV_CHIP_ICM42622},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, inv_icm42600_ids);
> +
> +static struct i2c_driver inv_icm42600_driver = {
>
[PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
Add I2C driver for InvenSense ICM-426xxx devices.
Configure bus signal slew rates as indicated in the datasheet.
Signed-off-by: Jean-Baptiste Maneyrol
---
.../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 117 ++
1 file changed, 117 insertions(+)
create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
new file mode 100644
index ..b61f993beacf
--- /dev/null
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 InvenSense, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include "inv_icm42600.h"
+
+static int inv_icm42600_i2c_bus_setup(struct inv_icm42600_state *st)
+{
+ unsigned int mask, val;
+ int ret;
+
+ /* setup interface registers */
+ mask = INV_ICM42600_INTF_CONFIG6_MASK;
+ val = INV_ICM42600_INTF_CONFIG6_I3C_EN;
+ ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG6,
+mask, val);
+ if (ret)
+ return ret;
+
+ mask = INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY;
+ val = 0;
+ ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG4,
+mask, val);
+ if (ret)
+ return ret;
+
+ /* set slew rates for I2C and SPI */
+ mask = INV_ICM42600_DRIVE_CONFIG_I2C_MASK |
+ INV_ICM42600_DRIVE_CONFIG_SPI_MASK;
+ val = INV_ICM42600_DRIVE_CONFIG_I2C(INV_ICM42600_SLEW_RATE_12_36NS) |
+ INV_ICM42600_DRIVE_CONFIG_SPI(INV_ICM42600_SLEW_RATE_12_36NS);
+ ret = regmap_update_bits(st->map, INV_ICM42600_REG_DRIVE_CONFIG,
+mask, val);
+ if (ret)
+ return ret;
+
+ /* disable SPI bus */
+ mask = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK;
+ val = INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS;
+ return regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
+ mask, val);
+}
+
+static int inv_icm42600_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ const void *match;
+ enum inv_icm42600_chip chip;
+ struct regmap *regmap;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
+ return -ENOTSUPP;
+
+ match = device_get_match_data(>dev);
+ if (match)
+ chip = (enum inv_icm42600_chip)match;
+ else if (id)
+ chip = (enum inv_icm42600_chip)id->driver_data;
+ else
+ return -EINVAL;
+
+ regmap = devm_regmap_init_i2c(client, _icm42600_regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return inv_icm42600_core_probe(regmap, chip,
+ inv_icm42600_i2c_bus_setup);
+}
+
+static const struct of_device_id inv_icm42600_of_matches[] = {
+ {
+ .compatible = "invensense,icm42600",
+ .data = (void *)INV_CHIP_ICM42600,
+ }, {
+ .compatible = "invensense,icm42602",
+ .data = (void *)INV_CHIP_ICM42602,
+ }, {
+ .compatible = "invensense,icm42605",
+ .data = (void *)INV_CHIP_ICM42605,
+ }, {
+ .compatible = "invensense,icm42622",
+ .data = (void *)INV_CHIP_ICM42622,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, inv_icm42600_of_matches);
+
+static const struct i2c_device_id inv_icm42600_ids[] = {
+ {"icm42600", INV_CHIP_ICM42600},
+ {"icm42602", INV_CHIP_ICM42602},
+ {"icm42605", INV_CHIP_ICM42605},
+ {"icm42622", INV_CHIP_ICM42622},
+ {}
+};
+MODULE_DEVICE_TABLE(i2c, inv_icm42600_ids);
+
+static struct i2c_driver inv_icm42600_driver = {
+ .probe = inv_icm42600_probe,
+ .id_table = inv_icm42600_ids,
+ .driver = {
+ .of_match_table = inv_icm42600_of_matches,
+ .name = "inv-icm42600-i2c",
+ .pm = _icm42600_pm_ops,
+ },
+};
+module_i2c_driver(inv_icm42600_driver);
+
+MODULE_AUTHOR("InvenSense, Inc.");
+MODULE_DESCRIPTION("InvenSense ICM-426xx I2C driver");
+MODULE_LICENSE("GPL");
--
2.17.1
