From: Song Qiang <songqiang1304521@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net,
robh+dt@kernel.org, mark.rutland@arm.com,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer
Date: Mon, 24 Sep 2018 22:37:14 +0800 [thread overview]
Message-ID: <20180924143714.GA16236@Eros> (raw)
In-Reply-To: <20180922111409.597126fd@archlinux>
On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 21:13:40 +0800
> Song Qiang <songqiang1304521@gmail.com> wrote:
>
> > PNI RM3100 magnetometer is a high resolution, large signal immunity
> > magnetometer, composed of 3 single sensors and a processing chip.
> > PNI is currently not in the vendors list, so this is also adding it.
> >
> > Following functions are available:
> > - Single-shot measurement from
> > /sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> > - Triggerd buffer measurement.
> > - Both i2c and spi interface are supported.
> > - Both interrupt and polling measurement is supported, depands on if
> > the 'interrupts' in DT is declared.
> >
> > Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> Hi Song,
...
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* 3sec more wait time. */
> > + ret = regmap_read(data->regmap, RM_REG_TMRC, &tmp);
> > + data->conversion_time = rm3100_samp_rates[tmp-RM_TMRC_OFFSET][2] + 3000;
> > +
> > + /* Starting all channels' conversion. */
> > + ret = regmap_write(regmap, RM_REG_CMM,
> > + RM_CMM_PMX | RM_CMM_PMY | RM_CMM_PMZ | RM_CMM_START);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> Nope. Can't do this without having a race condition. You need
> to ensure the userspace and in kernel interfaces are removed 'before'.
> you do that RM_REG_CMM write in remove.
>
> One option is to use devm_add_action to add a custom unwind function
> to the automatic handling. The other is to not use devm for everything
> after the write above and do the device_unregister manually.
>
Hi Jonathan,
I considered the both options you mentioned, and I was going to use the
manual way. But then something came to my mind, what if there is another
devm_* operation needs care, what if more. I checked devm_add_action in
source code, and it says that this function only adds unwinding handlers.
I guess this method was designed for this situation, and if we have
already used devm_ in our code, it's better to use devm_add_action for
cleanup, is that right?
yours,
Song Qiang
> > +}
> > +EXPORT_SYMBOL(rm3100_common_probe);
> > +
> > +int rm3100_common_remove(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct rm3100_data *data = iio_priv(indio_dev);
> > + struct regmap *regmap = data->regmap;
> > +
> > + regmap_write(regmap, RM_REG_CMM, 0x00);
> > +
> > + return 0;
> No real point in returning int if you are always going to put 0 in
> in it. Should probably check the regmap_write though and output
> a log message if it fails (as no other way of telling).
>
> > +}
> > +EXPORT_SYMBOL(rm3100_common_remove);
> > +
> > +MODULE_AUTHOR("Song Qiang <songqiang1304521@gmail.com>");
> > +MODULE_DESCRIPTION("PNI RM3100 9-axis magnetometer i2c driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/magnetometer/rm3100-i2c.c b/drivers/iio/magnetometer/rm3100-i2c.c
> > new file mode 100644
> > index 000000000000..b50dc5b1b30b
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/rm3100-i2c.c
> > @@ -0,0 +1,66 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Support for PNI RM3100 9-axis geomagnetic sensor a i2c bus.
> > + *
> > + * Copyright (C) 2018 Song Qiang <songqiang1304521@gmail.com>
> > + *
> > + * User Manual available at
> > + * <https://www.pnicorp.com/download/rm3100-user-manual/>
> > + *
> > + * i2c slave address 0x20 + SA1 << 1 + SA0.
> > + */
> > +
> > +#include <linux/i2c.h>
> > +
> > +#include "rm3100.h"
> > +
> > +static const struct regmap_config rm3100_regmap_config = {
> > + .reg_bits = 8,
> This indenting seems an extra tab on conventional choice...
> > + .val_bits = 8,
> > +
> > + .rd_table = &rm3100_readable_table,
> > + .wr_table = &rm3100_writable_table,
> > + .volatile_table = &rm3100_volatile_table,
> > +
> > + .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +static int rm3100_probe(struct i2c_client *client)
> > +{
> > + struct regmap *regmap;
> > +
> > + if (!i2c_check_functionality(client->adapter,
> > + I2C_FUNC_SMBUS_READ_I2C_BLOCK | I2C_FUNC_SMBUS_BYTE_DATA))
> > + return -EOPNOTSUPP;
> > +
> > + regmap = devm_regmap_init_i2c(client, &rm3100_regmap_config);
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
> > +
> > + return rm3100_common_probe(&client->dev, regmap, client->irq);
> > +}
> > +
> > +static int rm3100_remove(struct i2c_client *client)
> > +{
> > + return rm3100_common_remove(&client->dev);
> > +}
> > +
> > +static const struct of_device_id rm3100_dt_match[] = {
> > + { .compatible = "pni,rm3100-i2c", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rm3100_dt_match);
> > +
> > +static struct i2c_driver rm3100_driver = {
> > + .driver = {
> > + .name = "rm3100-i2c",
> > + .of_match_table = rm3100_dt_match,
> > + },
> > + .probe_new = rm3100_probe,
> > + .remove = rm3100_remove,
> > +};
> > +module_i2c_driver(rm3100_driver);
> > +
> > +MODULE_AUTHOR("Song Qiang <songqiang1304521@gmail.com>");
> > +MODULE_DESCRIPTION("PNI RM3100 9-axis magnetometer i2c driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/magnetometer/rm3100-spi.c b/drivers/iio/magnetometer/rm3100-spi.c
> > new file mode 100644
> > index 000000000000..2c7dd9e3a1a2
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/rm3100-spi.c
> > @@ -0,0 +1,72 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Support for PNI RM3100 9-axis geomagnetic sensor a spi bus.
> > + *
> > + * Copyright (C) 2018 Song Qiang <songqiang1304521@gmail.com>
> > + *
> > + * User Manual available at
> > + * <https://www.pnicorp.com/download/rm3100-user-manual/>
>
> Probably only worth mentioning the manual in the main file.
>
> > + */
> > +
> > +#include <linux/spi/spi.h>
> > +
> > +#include "rm3100.h"
> > +
> > +static const struct regmap_config rm3100_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +
> > + .rd_table = &rm3100_readable_table,
> > + .wr_table = &rm3100_writable_table,
> > + .volatile_table = &rm3100_volatile_table,
> > +
> > + .read_flag_mask = 0x80,
> > +
> > + .cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +static int rm3100_probe(struct spi_device *spi)
> > +{
> > + struct regmap *regmap;
> > + int ret;
> > +
> > + /* Actually this device supports both mode 0 and mode 3. */
> > + spi->mode = SPI_MODE_0;
> > + /* data rates cannot exceeds 1Mbits. */
> > + spi->max_speed_hz = 1000000;
> > + spi->bits_per_word = 8;
> > + ret = spi_setup(spi);
> > + if (ret)
> > + return ret;
> > +
> > + regmap = devm_regmap_init_spi(spi, &rm3100_regmap_config);
> > + if (IS_ERR(regmap))
> > + return PTR_ERR(regmap);
> > +
> > + return rm3100_common_probe(&spi->dev, regmap, spi->irq);
> > +}
> > +
> > +static int rm3100_remove(struct spi_device *spi)
> > +{
> > + return rm3100_common_remove(&spi->dev);
> > +}
> > +
> > +static const struct of_device_id rm3100_dt_match[] = {
> > + { .compatible = "pni,rm3100-spi", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rm3100_dt_match);
> > +
> > +static struct spi_driver rm3100_driver = {
> > + .driver = {
> > + .name = "rm3100-spi",
> > + .of_match_table = rm3100_dt_match,
>
> Indenting looks oddly deep...
>
> > + },
> > + .probe = rm3100_probe,
> > + .remove = rm3100_remove,
> > +};
> > +module_spi_driver(rm3100_driver);
> > +
> > +MODULE_AUTHOR("Song Qiang <songqiang1304521@gmail.com>");
> > +MODULE_DESCRIPTION("PNI RM3100 9-axis magnetometer spi driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/iio/magnetometer/rm3100.h b/drivers/iio/magnetometer/rm3100.h
> > new file mode 100644
> > index 000000000000..5e30bc0f5149
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/rm3100.h
> > @@ -0,0 +1,90 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Header file for PNI RM3100 driver
> > + *
> > + * Copyright (C) 2018 Song Qiang <songqiang1304521@gmail.com>
> > + */
> > +
> > +#ifndef RM3100_CORE_H
> > +#define RM3100_CORE_H
> > +
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
>
> What in here needs module.h? Push that down into the
> individual c files. It's repetition but it si generally
> good practice to include headers at as low a level as possible.
>
> > +
> > +#define RM_REG_REV_ID 0x36
> I would prefer RM3100 as the prefix.
> > +
> > +/* Cycle Count Registers MSBs and LSBs. */
> Could probably give these slightly more human readable names?
> RM3100_REG_CC_X_MSB
>
> Actually I doubt you use both of them in the driver anyway...
> (just searched, nope you don't so drop the ones you don't
> use and then the MSB, LSB definition probably not necessary).
>
> > +#define RM_REG_CCXM 0x04
> > +#define RM_REG_CCXL 0x05
> > +#define RM_REG_CCYM 0x06
> > +#define RM_REG_CCYL 0x07
> > +#define RM_REG_CCZM 0x08
> > +#define RM_REG_CCZL 0x09
> > +
> > +/* Single Measurement Mode register. */
> > +#define RM_REG_POLL 0x00
> > +#define RM_POLL_PMX BIT(4)
> > +#define RM_POLL_PMY BIT(5)
> > +#define RM_POLL_PMZ BIT(6)
> > +
> > +/* Continues Measurement Mode register. */
> > +#define RM_REG_CMM 0x01
> > +#define RM_CMM_START BIT(0)
> > +#define RM_CMM_DRDM BIT(2)
> > +#define RM_CMM_PMX BIT(4)
> > +#define RM_CMM_PMY BIT(5)
> > +#define RM_CMM_PMZ BIT(6)
> > +
> > +/* TiMe Rate Configuration register. */
> > +#define RM_REG_TMRC 0x0B
> > +#define RM_TMRC_OFFSET 0x92
> > +
> > +/* Result Status register. */
> > +#define RM_REG_STATUS 0x34
> > +#define RM_STATUS_DRDY BIT(7)
> > +
> > +/* Measurement result registers. */
> > +#define RM_REG_MX2 0x24
>
> You only use some of these. Not sure having defines for the
> others is really helpful.
>
> > +#define RM_REG_MX1 0x25
> > +#define RM_REG_MX0 0x26
> > +#define RM_REG_MY2 0x27
> > +#define RM_REG_MY1 0x28
> > +#define RM_REG_MY0 0x29
> > +#define RM_REG_MZ2 0x2a
> > +#define RM_REG_MZ1 0x2b
> > +#define RM_REG_MZ0 0x2c
> > +
> > +#define RM_REG_HSHAKE 0x35
> > +
> > +#define RM_W_REG_START RM_REG_POLL
> > +#define RM_W_REG_END RM_REG_REV_ID
> > +#define RM_R_REG_START RM_REG_POLL
> > +#define RM_R_REG_END RM_REG_HSHAKE
> > +#define RM_V_REG_START RM_REG_MX2
> > +#define RM_V_REG_END RM_REG_HSHAKE
> > +
> > +/* Built-In Self Test reigister. */
> > +#define RM_REG_BIST 0x33
> > +
> > +struct rm3100_data {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct completion measuring_done;
> > + bool use_interrupt;
> > +
> > + int conversion_time;
> > +
> > + /* To protect consistency of every measurement and sampling
>
> /*
> * To protect
> */
> (common format to most of the kernel other than those 'crazy' :)
> people in net and a few other corners.
>
> > + * frequency change operations.
> > + */
> > + struct mutex lock;
> > +};
> > +
> > +extern const struct regmap_access_table rm3100_readable_table;
> > +extern const struct regmap_access_table rm3100_writable_table;
> > +extern const struct regmap_access_table rm3100_volatile_table;
> > +
> > +int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq);
> > +int rm3100_common_remove(struct device *dev);
> > +
> > +#endif /* RM3100_CORE_H */
>
next prev parent reply other threads:[~2018-09-24 14:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-20 13:13 [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer Song Qiang
2018-09-20 13:46 ` Peter Meerwald-Stadler
2018-09-20 18:05 ` Song Qiang
2018-09-22 9:42 ` Jonathan Cameron
2018-09-22 10:08 ` Jonathan Cameron
2018-09-21 5:07 ` Phil Reid
2018-09-21 11:29 ` Song Qiang
2018-09-21 12:20 ` Jonathan Cameron
2018-09-21 12:20 ` Jonathan Cameron
2018-09-22 9:18 ` Song Qiang
2018-09-21 2:05 ` Phil Reid
2018-09-21 9:13 ` Song Qiang
2018-09-22 10:14 ` Jonathan Cameron
2018-09-23 15:17 ` Song Qiang
2018-09-24 20:04 ` Jonathan Cameron
2018-09-24 14:37 ` Song Qiang [this message]
2018-09-29 12:44 ` Jonathan Cameron
2018-09-23 11:01 ` Dan Carpenter
2018-09-23 11:01 ` Dan Carpenter
2018-09-24 22:23 ` Rob Herring
2018-09-26 0:34 ` Song Qiang
2018-09-29 11:22 ` Jonathan Cameron
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=20180924143714.GA16236@Eros \
--to=songqiang1304521@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.