On Mon, 18 May 2020 14:19:11 +0000 Jean-Baptiste Maneyrol <[email protected]> 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 <[email protected]> > > Sent: Friday, May 8, 2020 15:44 > > To: Jean-Baptiste Maneyrol <[email protected]> > > Cc: [email protected] <[email protected]>; [email protected] > <[email protected]>; [email protected] <[email protected]>; > [email protected] <[email protected]>; [email protected] > <[email protected]>; [email protected] > <[email protected]>; > [email protected] <[email protected]>; > [email protected] <[email protected]>; > [email protected] <[email protected]> > > 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 <[email protected]> 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 <[email protected]> > > 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"); > > > > >
