Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver - Jean-Baptiste Maneyrol

From: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: "jic23@kernel.org" <jic23@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"robh@kernel.org" <robh@kernel.org>,
	"mchehab+huawei@kernel.org" <mchehab+huawei@kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
Date: Mon, 18 May 2020 14:19:11 +0000	[thread overview]
Message-ID: <MN2PR12MB44228DAD29335621D4047260C4B80@MN2PR12MB4422.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20200508144406.00006b8c@Huawei.com>

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 <Jonathan.Cameron@Huawei.com>

Sent: Friday, May 8, 2020 15:44

To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>

Cc: jic23@kernel.org <jic23@kernel.org>; robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>;
 linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@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 <jmaneyrol@invensense.com> 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 <jmaneyrol@invensense.com>

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 000000000000..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 <linux/device.h>

> +#include <linux/module.h>

> +#include <linux/i2c.h>

> +#include <linux/regmap.h>

> +#include <linux/of_device.h>

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(&client->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, &inv_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 = &inv_icm42600_pm_ops,

> +     },

> +};

> +module_i2c_driver(inv_icm42600_driver);

> +

> +MODULE_AUTHOR("InvenSense, Inc.");

> +MODULE_DESCRIPTION("InvenSense ICM-426xx I2C driver");

> +MODULE_LICENSE("GPL");