Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Mon, 24 Sep 2018 22:37:14 +0800
Song Qiang  wrote:

> On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> > On Thu, 20 Sep 2018 21:13:40 +0800
> > Song Qiang  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   
> > Hi Song,  
> 
> ...
> 
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* 3sec more wait time. */
> > > + ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > > + 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?

It can provide a very clean solution.  Be careful though. You do need
to make sure to handle a failure of this function itself.

Jonathan

> 
> 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 ");
> > > +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 ..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 
> > > + *
> > > + * User Manual available at
> > > + * 
> > > + *
> > > + * i2c slave address 0x20 + SA1 << 1 + SA0.
> > > + */
> > > +
> > > +#include 
> > > +
> > > +#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 = _readable_table,
> > > + .wr_table = _writable_table,
> > > + .volatile_table = _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, _regmap_config);
> > > + if (IS_ERR(regmap))
> > > + return PTR_ERR(regmap);
> > > +
> > > + return rm3100_common_probe(>dev, regmap, client->irq);
> > > +}
> > > +
> > > +static int rm3100_remove(struct i2c_client *client)
> > > +{
> > > + return rm3100_common_remove(>dev);
> > > +}
> > > +
> > > +static const struct of_device_id rm3100_dt_match[] = {

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Mon, 24 Sep 2018 22:37:14 +0800
Song Qiang  wrote:

> On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> > On Thu, 20 Sep 2018 21:13:40 +0800
> > Song Qiang  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   
> > Hi Song,  
> 
> ...
> 
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* 3sec more wait time. */
> > > + ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > > + 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?

It can provide a very clean solution.  Be careful though. You do need
to make sure to handle a failure of this function itself.

Jonathan

> 
> 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 ");
> > > +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 ..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 
> > > + *
> > > + * User Manual available at
> > > + * 
> > > + *
> > > + * i2c slave address 0x20 + SA1 << 1 + SA0.
> > > + */
> > > +
> > > +#include 
> > > +
> > > +#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 = _readable_table,
> > > + .wr_table = _writable_table,
> > > + .volatile_table = _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, _regmap_config);
> > > + if (IS_ERR(regmap))
> > > + return PTR_ERR(regmap);
> > > +
> > > + return rm3100_common_probe(>dev, regmap, client->irq);
> > > +}
> > > +
> > > +static int rm3100_remove(struct i2c_client *client)
> > > +{
> > > + return rm3100_common_remove(>dev);
> > > +}
> > > +
> > > +static const struct of_device_id rm3100_dt_match[] = {

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Wed, 26 Sep 2018 08:34:02 +0800
Song Qiang  wrote:

> On Mon, Sep 24, 2018 at 03:23:52PM -0700, Rob Herring wrote:
> > On Thu, Sep 20, 2018 at 09:13:40PM +0800, Song Qiang 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.
> > >   
> 
> ...
> 
> > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > index 41f0b97eb933..5bf3395fe9ae 100644
> > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > @@ -288,6 +288,7 @@ pine64Pine64
> > >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> > >  plathome Plat'Home Co., Ltd.
> > >  plda PLDA
> > > +pni PNI  
> > 
> > PNI doesn't stand for something?
> >   
> 
> Hi Rob,
> 
> PNI should be 'PNI Sensor Corporation'. I saw that PLDA above mine and
> thought I should write down its abbreviation, which apparently is wrong.

To answer more directly.  If PNI itself stands for anything the company
is deliberately making it non obvious!  I had a good search to try and
find out and got nowhere...

> 
> yours,
> Song Qiang
> 
> > >  portwell Portwell Inc.
> > >  poslab   Poslab Technology Co., Ltd.
> > >  powervr  PowerVR (deprecated, use img)  
> >   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Wed, 26 Sep 2018 08:34:02 +0800
Song Qiang  wrote:

> On Mon, Sep 24, 2018 at 03:23:52PM -0700, Rob Herring wrote:
> > On Thu, Sep 20, 2018 at 09:13:40PM +0800, Song Qiang 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.
> > >   
> 
> ...
> 
> > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > index 41f0b97eb933..5bf3395fe9ae 100644
> > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > @@ -288,6 +288,7 @@ pine64Pine64
> > >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> > >  plathome Plat'Home Co., Ltd.
> > >  plda PLDA
> > > +pni PNI  
> > 
> > PNI doesn't stand for something?
> >   
> 
> Hi Rob,
> 
> PNI should be 'PNI Sensor Corporation'. I saw that PLDA above mine and
> thought I should write down its abbreviation, which apparently is wrong.

To answer more directly.  If PNI itself stands for anything the company
is deliberately making it non obvious!  I had a good search to try and
find out and got nowhere...

> 
> yours,
> Song Qiang
> 
> > >  portwell Portwell Inc.
> > >  poslab   Poslab Technology Co., Ltd.
> > >  powervr  PowerVR (deprecated, use img)  
> >   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Mon, Sep 24, 2018 at 03:23:52PM -0700, Rob Herring wrote:
> On Thu, Sep 20, 2018 at 09:13:40PM +0800, Song Qiang 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.
> > 

...

> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 41f0b97eb933..5bf3395fe9ae 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -288,6 +288,7 @@ pine64  Pine64
> >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> >  plathome   Plat'Home Co., Ltd.
> >  plda   PLDA
> > +pni PNI
> 
> PNI doesn't stand for something?
> 

Hi Rob,

PNI should be 'PNI Sensor Corporation'. I saw that PLDA above mine and
thought I should write down its abbreviation, which apparently is wrong.

yours,
Song Qiang

> >  portwell   Portwell Inc.
> >  poslab Poslab Technology Co., Ltd.
> >  powervrPowerVR (deprecated, use img)
> 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Mon, Sep 24, 2018 at 03:23:52PM -0700, Rob Herring wrote:
> On Thu, Sep 20, 2018 at 09:13:40PM +0800, Song Qiang 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.
> > 

...

> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 41f0b97eb933..5bf3395fe9ae 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -288,6 +288,7 @@ pine64  Pine64
> >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> >  plathome   Plat'Home Co., Ltd.
> >  plda   PLDA
> > +pni PNI
> 
> PNI doesn't stand for something?
> 

Hi Rob,

PNI should be 'PNI Sensor Corporation'. I saw that PLDA above mine and
thought I should write down its abbreviation, which apparently is wrong.

yours,
Song Qiang

> >  portwell   Portwell Inc.
> >  poslab Poslab Technology Co., Ltd.
> >  powervrPowerVR (deprecated, use img)
> 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Sun, 23 Sep 2018 23:17:22 +0800
Song Qiang  wrote:

> On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> > On Thu, 20 Sep 2018 21:13:40 +0800
> > Song Qiang  wrote:
> >   
> 
> ...
> 
> > > +const struct regmap_access_table rm3100_volatile_table = {
> > > + .yes_ranges = rm3100_volatile_ranges,
> > > + .n_yes_ranges = ARRAY_SIZE(rm3100_volatile_ranges),
> > > +};
> > > +
> > > +static irqreturn_t rm3100_measurement_irq_handler(int irq, void *d)  
> > 
> > Silly question: Does the chip have two interrupt lines?  (if so they
> > should be in the binding). If not, then this is the irq handler
> > for everything so why have the measurement in it's name?
> >   
> 
> Hi Jonathan
Hi Song.

> 
> Ah, always some other things need to care, I didn't put enough focus on
> this naming and thought it looks like ok. So I should throw these
> unnecessary information in names away!
> 
> > > +{
> > > + struct rm3100_data *data = d;
> > > +
> > > + complete(>measuring_done);
> > > +
> > > + return IRQ_HANDLED;
> > > +}  
> 
> ... 
> 
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* 3sec more wait time. */
> > > + ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > > + 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.
> >   
> 
> I've already handled some of those problems, and most of them are not a 
> big deal, except this one and the locking problems, about how should I 
> deal with locks properly. I'm already reading the lockdep conventions and
> some articles about it.
> Autobuilder are complaining about my locks, seems like a mess it is!

I suspect it's mostly about error paths with no unlocks in them.
At least to my eye there isn't any complex locking needed in here.

> 
> > > +}
> > > +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);
> > > +  
> 
> ...
> 
> > > +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.
> >   
> 
> Actually, I've been wondering why the perl scripts didn't find this out,
> and not only this one, many other problems like too many indents, 
> parameters in open brackets are not aligned can be detected.
> I don't know perl, but this has drawn my attention. Is there any
> particular reason these problems still can not be detected? or I think
> we can work some patch out! Make reviewing code like mine easier!

Unfortunately there are enough corners of the kernel with different
formats, and legacy code that predates there being any conventions at
all, that checkpatch tends to be 'relaxed' on this stuff these days.

> 
> yours,
> Song Qiang
> 
> > > +  * 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 */  
> >   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Sun, 23 Sep 2018 23:17:22 +0800
Song Qiang  wrote:

> On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> > On Thu, 20 Sep 2018 21:13:40 +0800
> > Song Qiang  wrote:
> >   
> 
> ...
> 
> > > +const struct regmap_access_table rm3100_volatile_table = {
> > > + .yes_ranges = rm3100_volatile_ranges,
> > > + .n_yes_ranges = ARRAY_SIZE(rm3100_volatile_ranges),
> > > +};
> > > +
> > > +static irqreturn_t rm3100_measurement_irq_handler(int irq, void *d)  
> > 
> > Silly question: Does the chip have two interrupt lines?  (if so they
> > should be in the binding). If not, then this is the irq handler
> > for everything so why have the measurement in it's name?
> >   
> 
> Hi Jonathan
Hi Song.

> 
> Ah, always some other things need to care, I didn't put enough focus on
> this naming and thought it looks like ok. So I should throw these
> unnecessary information in names away!
> 
> > > +{
> > > + struct rm3100_data *data = d;
> > > +
> > > + complete(>measuring_done);
> > > +
> > > + return IRQ_HANDLED;
> > > +}  
> 
> ... 
> 
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + /* 3sec more wait time. */
> > > + ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > > + 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.
> >   
> 
> I've already handled some of those problems, and most of them are not a 
> big deal, except this one and the locking problems, about how should I 
> deal with locks properly. I'm already reading the lockdep conventions and
> some articles about it.
> Autobuilder are complaining about my locks, seems like a mess it is!

I suspect it's mostly about error paths with no unlocks in them.
At least to my eye there isn't any complex locking needed in here.

> 
> > > +}
> > > +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);
> > > +  
> 
> ...
> 
> > > +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.
> >   
> 
> Actually, I've been wondering why the perl scripts didn't find this out,
> and not only this one, many other problems like too many indents, 
> parameters in open brackets are not aligned can be detected.
> I don't know perl, but this has drawn my attention. Is there any
> particular reason these problems still can not be detected? or I think
> we can work some patch out! Make reviewing code like mine easier!

Unfortunately there are enough corners of the kernel with different
formats, and legacy code that predates there being any conventions at
all, that checkpatch tends to be 'relaxed' on this stuff these days.

> 
> yours,
> Song Qiang
> 
> > > +  * 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 */  
> >   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 21:13:40 +0800
> Song Qiang  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 
> Hi Song,

...

> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* 3sec more wait time. */
> > +   ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > +   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 ");
> > +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 ..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 
> > + *
> > + * User Manual available at
> > + * 
> > + *
> > + * i2c slave address 0x20 + SA1 << 1 + SA0.
> > + */
> > +
> > +#include 
> > +
> > +#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 = _readable_table,
> > +   .wr_table = _writable_table,
> > +   .volatile_table = _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, _regmap_config);
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   return rm3100_common_probe(>dev, regmap, client->irq);
> > +}
> > +
> > +static int rm3100_remove(struct i2c_client *client)
> > +{
> > +   return rm3100_common_remove(>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);
> > +
> > 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 21:13:40 +0800
> Song Qiang  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 
> Hi Song,

...

> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* 3sec more wait time. */
> > +   ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > +   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 ");
> > +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 ..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 
> > + *
> > + * User Manual available at
> > + * 
> > + *
> > + * i2c slave address 0x20 + SA1 << 1 + SA0.
> > + */
> > +
> > +#include 
> > +
> > +#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 = _readable_table,
> > +   .wr_table = _writable_table,
> > +   .volatile_table = _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, _regmap_config);
> > +   if (IS_ERR(regmap))
> > +   return PTR_ERR(regmap);
> > +
> > +   return rm3100_common_probe(>dev, regmap, client->irq);
> > +}
> > +
> > +static int rm3100_remove(struct i2c_client *client)
> > +{
> > +   return rm3100_common_remove(>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);
> > +
> > 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 21:13:40 +0800
> Song Qiang  wrote:
> 

...

> > +const struct regmap_access_table rm3100_volatile_table = {
> > +   .yes_ranges = rm3100_volatile_ranges,
> > +   .n_yes_ranges = ARRAY_SIZE(rm3100_volatile_ranges),
> > +};
> > +
> > +static irqreturn_t rm3100_measurement_irq_handler(int irq, void *d)
> 
> Silly question: Does the chip have two interrupt lines?  (if so they
> should be in the binding). If not, then this is the irq handler
> for everything so why have the measurement in it's name?
> 

Hi Jonathan

Ah, always some other things need to care, I didn't put enough focus on
this naming and thought it looks like ok. So I should throw these
unnecessary information in names away!

> > +{
> > +   struct rm3100_data *data = d;
> > +
> > +   complete(>measuring_done);
> > +
> > +   return IRQ_HANDLED;
> > +}

... 

> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* 3sec more wait time. */
> > +   ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > +   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.
> 

I've already handled some of those problems, and most of them are not a 
big deal, except this one and the locking problems, about how should I 
deal with locks properly. I'm already reading the lockdep conventions and
some articles about it.
Autobuilder are complaining about my locks, seems like a mess it is!

> > +}
> > +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);
> > +

...

> > +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.
> 

Actually, I've been wondering why the perl scripts didn't find this out,
and not only this one, many other problems like too many indents, 
parameters in open brackets are not aligned can be detected.
I don't know perl, but this has drawn my attention. Is there any
particular reason these problems still can not be detected? or I think
we can work some patch out! Make reviewing code like mine easier!

yours,
Song Qiang

> > +* 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 */
> 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Sat, Sep 22, 2018 at 11:14:09AM +0100, Jonathan Cameron wrote:
> On Thu, 20 Sep 2018 21:13:40 +0800
> Song Qiang  wrote:
> 

...

> > +const struct regmap_access_table rm3100_volatile_table = {
> > +   .yes_ranges = rm3100_volatile_ranges,
> > +   .n_yes_ranges = ARRAY_SIZE(rm3100_volatile_ranges),
> > +};
> > +
> > +static irqreturn_t rm3100_measurement_irq_handler(int irq, void *d)
> 
> Silly question: Does the chip have two interrupt lines?  (if so they
> should be in the binding). If not, then this is the irq handler
> for everything so why have the measurement in it's name?
> 

Hi Jonathan

Ah, always some other things need to care, I didn't put enough focus on
this naming and thought it looks like ok. So I should throw these
unnecessary information in names away!

> > +{
> > +   struct rm3100_data *data = d;
> > +
> > +   complete(>measuring_done);
> > +
> > +   return IRQ_HANDLED;
> > +}

... 

> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* 3sec more wait time. */
> > +   ret = regmap_read(data->regmap, RM_REG_TMRC, );
> > +   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.
> 

I've already handled some of those problems, and most of them are not a 
big deal, except this one and the locking problems, about how should I 
deal with locks properly. I'm already reading the lockdep conventions and
some articles about it.
Autobuilder are complaining about my locks, seems like a mess it is!

> > +}
> > +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);
> > +

...

> > +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.
> 

Actually, I've been wondering why the perl scripts didn't find this out,
and not only this one, many other problems like too many indents, 
parameters in open brackets are not aligned can be detected.
I don't know perl, but this has drawn my attention. Is there any
particular reason these problems still can not be detected? or I think
we can work some patch out! Make reviewing code like mine easier!

yours,
Song Qiang

> > +* 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 */
> 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Dan Carpenter]

Hi Song,

I love your patch! Perhaps something to improve:

url:
https://github.com/0day-ci/linux/commits/Song-Qiang/iio-magnetometer-Add-support-for-PNI-RM3100-9-axis-magnetometer/20180920-215124
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg

smatch warnings:
drivers/iio/magnetometer/rm3100-core.c:235 rm3100_set_samp_freq() warn: 
inconsistent returns 'mutex:>lock'.
  Locked on:   line 206
  Unlocked on: line 194
drivers/iio/magnetometer/rm3100-core.c:319 rm3100_trigger_handler() warn: 
inconsistent returns 'mutex:>lock'.
  Locked on:   line 310
  Unlocked on: line 319

# 
https://github.com/0day-ci/linux/commit/0345472c15bab336397b25a25eb76a9f8586faf0
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0345472c15bab336397b25a25eb76a9f8586faf0
vim +235 drivers/iio/magnetometer/rm3100-core.c

0345472c Song Qiang 2018-09-20  183  
0345472c Song Qiang 2018-09-20  184  static int rm3100_set_samp_freq(struct 
rm3100_data *data, int val, int val2)
0345472c Song Qiang 2018-09-20  185  {
0345472c Song Qiang 2018-09-20  186 struct regmap *regmap = data->regmap;
0345472c Song Qiang 2018-09-20  187 int cycle_count;
0345472c Song Qiang 2018-09-20  188 int ret;
0345472c Song Qiang 2018-09-20  189 int i;
0345472c Song Qiang 2018-09-20  190  
0345472c Song Qiang 2018-09-20  191 /* All cycle count registers use the 
same value. */
0345472c Song Qiang 2018-09-20  192 ret = regmap_read(regmap, RM_REG_CCXL, 
_count);
0345472c Song Qiang 2018-09-20  193 if (cycle_count < 0)
0345472c Song Qiang 2018-09-20  194 return cycle_count;
0345472c Song Qiang 2018-09-20  195  
0345472c Song Qiang 2018-09-20  196 for (i = 0; i < RM_SAMP_NUM; i++) {
0345472c Song Qiang 2018-09-20  197 if (val == 
rm3100_samp_rates[i][0] &&
0345472c Song Qiang 2018-09-20  198 val2 == 
rm3100_samp_rates[i][1])
0345472c Song Qiang 2018-09-20  199 break;
0345472c Song Qiang 2018-09-20  200 }
0345472c Song Qiang 2018-09-20  201  
0345472c Song Qiang 2018-09-20  202 if (i != RM_SAMP_NUM) {
0345472c Song Qiang 2018-09-20  203 mutex_lock(>lock);
^^^
0345472c Song Qiang 2018-09-20  204 ret = regmap_write(regmap, 
RM_REG_TMRC, i + RM_TMRC_OFFSET);
0345472c Song Qiang 2018-09-20  205 if (ret < 0)
0345472c Song Qiang 2018-09-20  206 return ret;
^^
goto unlock;  there are a bunch of these.

0345472c Song Qiang 2018-09-20  207  
0345472c Song Qiang 2018-09-20  208 /* Checking if cycle count 
registers need changing. */
0345472c Song Qiang 2018-09-20  209 if (val == 600 && cycle_count 
== 200) {
0345472c Song Qiang 2018-09-20  210 for (i = 0; i < 3; i++) 
{
0345472c Song Qiang 2018-09-20  211 
regmap_write(regmap, RM_REG_CCXL + 2 * i, 100);
0345472c Song Qiang 2018-09-20  212 if (ret < 0)
0345472c Song Qiang 2018-09-20  213 return 
ret;
0345472c Song Qiang 2018-09-20  214 }
0345472c Song Qiang 2018-09-20  215 } else if (val != 600 && 
cycle_count == 100) {
0345472c Song Qiang 2018-09-20  216 for (i = 0; i < 3; i++) 
{
0345472c Song Qiang 2018-09-20  217 
regmap_write(regmap, RM_REG_CCXL + 2 * i, 200);
0345472c Song Qiang 2018-09-20  218 if (ret < 0)
0345472c Song Qiang 2018-09-20  219 return 
ret;
0345472c Song Qiang 2018-09-20  220 }
0345472c Song Qiang 2018-09-20  221 }
0345472c Song Qiang 2018-09-20  222 /* Writing TMRC registers 
requires CMM reset. */
0345472c Song Qiang 2018-09-20  223 ret = regmap_write(regmap, 
RM_REG_CMM, 0);
0345472c Song Qiang 2018-09-20  224 if (ret < 0)
0345472c Song Qiang 2018-09-20  225 return ret;
0345472c Song Qiang 2018-09-20  226 ret = regmap_write(regmap, 
RM_REG_CMM, RM_CMM_PMX |
0345472c Song Qiang 2018-09-20  227 RM_CMM_PMY | RM_CMM_PMZ 
| RM_CMM_START);
0345472c Song Qiang 2018-09-20  228 if (ret < 0)
0345472c Song Qiang 2018-09-20  229 return ret;
0345472c Song Qiang 2018-09-20  230 mutex_unlock(>lock);
0345472c Song Qiang 2018-09-20  231  
0345472c Song Qiang 2018-09-20  232 data->conversion_time = 
rm3100_samp_rates[i][2] + 3000;
0345472c Song Qiang 2018-09-20  233 return 0;
0345472c Song Qiang 2018-09-20  234 }
0345472c Song Qiang 2018-09-20 @235 return -EINVAL;
0345472c Song Qiang 2018-09-20  236  }
0345472c Song Qiang 2018-09-20  237  
0345472c Song Qiang 2018-09-20  238  static int 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Dan Carpenter]

Hi Song,

I love your patch! Perhaps something to improve:

url:
https://github.com/0day-ci/linux/commits/Song-Qiang/iio-magnetometer-Add-support-for-PNI-RM3100-9-axis-magnetometer/20180920-215124
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg

smatch warnings:
drivers/iio/magnetometer/rm3100-core.c:235 rm3100_set_samp_freq() warn: 
inconsistent returns 'mutex:>lock'.
  Locked on:   line 206
  Unlocked on: line 194
drivers/iio/magnetometer/rm3100-core.c:319 rm3100_trigger_handler() warn: 
inconsistent returns 'mutex:>lock'.
  Locked on:   line 310
  Unlocked on: line 319

# 
https://github.com/0day-ci/linux/commit/0345472c15bab336397b25a25eb76a9f8586faf0
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 0345472c15bab336397b25a25eb76a9f8586faf0
vim +235 drivers/iio/magnetometer/rm3100-core.c

0345472c Song Qiang 2018-09-20  183  
0345472c Song Qiang 2018-09-20  184  static int rm3100_set_samp_freq(struct 
rm3100_data *data, int val, int val2)
0345472c Song Qiang 2018-09-20  185  {
0345472c Song Qiang 2018-09-20  186 struct regmap *regmap = data->regmap;
0345472c Song Qiang 2018-09-20  187 int cycle_count;
0345472c Song Qiang 2018-09-20  188 int ret;
0345472c Song Qiang 2018-09-20  189 int i;
0345472c Song Qiang 2018-09-20  190  
0345472c Song Qiang 2018-09-20  191 /* All cycle count registers use the 
same value. */
0345472c Song Qiang 2018-09-20  192 ret = regmap_read(regmap, RM_REG_CCXL, 
_count);
0345472c Song Qiang 2018-09-20  193 if (cycle_count < 0)
0345472c Song Qiang 2018-09-20  194 return cycle_count;
0345472c Song Qiang 2018-09-20  195  
0345472c Song Qiang 2018-09-20  196 for (i = 0; i < RM_SAMP_NUM; i++) {
0345472c Song Qiang 2018-09-20  197 if (val == 
rm3100_samp_rates[i][0] &&
0345472c Song Qiang 2018-09-20  198 val2 == 
rm3100_samp_rates[i][1])
0345472c Song Qiang 2018-09-20  199 break;
0345472c Song Qiang 2018-09-20  200 }
0345472c Song Qiang 2018-09-20  201  
0345472c Song Qiang 2018-09-20  202 if (i != RM_SAMP_NUM) {
0345472c Song Qiang 2018-09-20  203 mutex_lock(>lock);
^^^
0345472c Song Qiang 2018-09-20  204 ret = regmap_write(regmap, 
RM_REG_TMRC, i + RM_TMRC_OFFSET);
0345472c Song Qiang 2018-09-20  205 if (ret < 0)
0345472c Song Qiang 2018-09-20  206 return ret;
^^
goto unlock;  there are a bunch of these.

0345472c Song Qiang 2018-09-20  207  
0345472c Song Qiang 2018-09-20  208 /* Checking if cycle count 
registers need changing. */
0345472c Song Qiang 2018-09-20  209 if (val == 600 && cycle_count 
== 200) {
0345472c Song Qiang 2018-09-20  210 for (i = 0; i < 3; i++) 
{
0345472c Song Qiang 2018-09-20  211 
regmap_write(regmap, RM_REG_CCXL + 2 * i, 100);
0345472c Song Qiang 2018-09-20  212 if (ret < 0)
0345472c Song Qiang 2018-09-20  213 return 
ret;
0345472c Song Qiang 2018-09-20  214 }
0345472c Song Qiang 2018-09-20  215 } else if (val != 600 && 
cycle_count == 100) {
0345472c Song Qiang 2018-09-20  216 for (i = 0; i < 3; i++) 
{
0345472c Song Qiang 2018-09-20  217 
regmap_write(regmap, RM_REG_CCXL + 2 * i, 200);
0345472c Song Qiang 2018-09-20  218 if (ret < 0)
0345472c Song Qiang 2018-09-20  219 return 
ret;
0345472c Song Qiang 2018-09-20  220 }
0345472c Song Qiang 2018-09-20  221 }
0345472c Song Qiang 2018-09-20  222 /* Writing TMRC registers 
requires CMM reset. */
0345472c Song Qiang 2018-09-20  223 ret = regmap_write(regmap, 
RM_REG_CMM, 0);
0345472c Song Qiang 2018-09-20  224 if (ret < 0)
0345472c Song Qiang 2018-09-20  225 return ret;
0345472c Song Qiang 2018-09-20  226 ret = regmap_write(regmap, 
RM_REG_CMM, RM_CMM_PMX |
0345472c Song Qiang 2018-09-20  227 RM_CMM_PMY | RM_CMM_PMZ 
| RM_CMM_START);
0345472c Song Qiang 2018-09-20  228 if (ret < 0)
0345472c Song Qiang 2018-09-20  229 return ret;
0345472c Song Qiang 2018-09-20  230 mutex_unlock(>lock);
0345472c Song Qiang 2018-09-20  231  
0345472c Song Qiang 2018-09-20  232 data->conversion_time = 
rm3100_samp_rates[i][2] + 3000;
0345472c Song Qiang 2018-09-20  233 return 0;
0345472c Song Qiang 2018-09-20  234 }
0345472c Song Qiang 2018-09-20 @235 return -EINVAL;
0345472c Song Qiang 2018-09-20  236  }
0345472c Song Qiang 2018-09-20  237  
0345472c Song Qiang 2018-09-20  238  static int 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Thu, 20 Sep 2018 21:13:40 +0800
Song Qiang  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 
Hi Song,

Great to have a driver for a pni device.  I hadn't come across them
before!

I'll try and avoid repeating Peter and Phil's comments on the basis
you probably have those covered by now.  I would have left this
for V2 before reviewing but not sure if I'll get to IIO stuff
during the coming week so better to get you some comments from me today.

Thanks,

Jonathan

> ---
>  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
>  MAINTAINERS   |  10 +
>  drivers/iio/magnetometer/Kconfig  |  29 ++
>  drivers/iio/magnetometer/Makefile |   4 +
>  drivers/iio/magnetometer/rm3100-core.c| 399 ++
>  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
>  drivers/iio/magnetometer/rm3100-spi.c |  72 
>  drivers/iio/magnetometer/rm3100.h |  90 
>  9 files changed, 728 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
>  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
>  create mode 100644 drivers/iio/magnetometer/rm3100.h
> 
> diff --git 
> a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> new file mode 100644
> index ..d0d2063e943f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> @@ -0,0 +1,57 @@
> +* PNI RM3100 9-axis magnetometer sensor
> +
> +I2C Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-i2c"
> +- reg : the I2C address of the magnetometer
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line.

I'm not a DT expert by any means, but I think the convention is
to not mention pinctrl in a given driver because otherwise
in theory we may need to mention it everywhere...

> +
> +Example:
> +
> +rm3100: rm3100@20 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> +
> + compatible = "pni,rm3100-i2c";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> +
> +SPI Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-spi"
> +- reg : address of sensor, usually 0 or 1.
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> +
> +Example:
> +
> +rm3100: rm3100@0{
> + compatible = "pni,rm3100-spi";
> + reg = <0>;
> +
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 41f0b97eb933..5bf3395fe9ae 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -288,6 +288,7 @@ pine64Pine64
>  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
>  plathome Plat'Home Co., Ltd.
>  plda PLDA
> +pni PNI

Please break the whole binding out as a separate patch.  Ideally do this
vendor-prefixes.txt as a separate patch again.  This cuts down
on the amount of searching that the DT people have to do to find
the bits that they want to look at.

>  portwell Portwell Inc.
>  poslab   Poslab Technology Co., Ltd.
>  powervr  PowerVR (deprecated, use img)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..30ee8cf98312 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
>  S:   Maintained
>  F:   drivers/pnp/
>  
> +PNI RM3100 IIO DRIVER
> +M:   Song Qiang 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/iio/magnetometer/rm3100-core.c

rm3110* perhaps?

> +F:   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Thu, 20 Sep 2018 21:13:40 +0800
Song Qiang  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 
Hi Song,

Great to have a driver for a pni device.  I hadn't come across them
before!

I'll try and avoid repeating Peter and Phil's comments on the basis
you probably have those covered by now.  I would have left this
for V2 before reviewing but not sure if I'll get to IIO stuff
during the coming week so better to get you some comments from me today.

Thanks,

Jonathan

> ---
>  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
>  MAINTAINERS   |  10 +
>  drivers/iio/magnetometer/Kconfig  |  29 ++
>  drivers/iio/magnetometer/Makefile |   4 +
>  drivers/iio/magnetometer/rm3100-core.c| 399 ++
>  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
>  drivers/iio/magnetometer/rm3100-spi.c |  72 
>  drivers/iio/magnetometer/rm3100.h |  90 
>  9 files changed, 728 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
>  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
>  create mode 100644 drivers/iio/magnetometer/rm3100.h
> 
> diff --git 
> a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> new file mode 100644
> index ..d0d2063e943f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> @@ -0,0 +1,57 @@
> +* PNI RM3100 9-axis magnetometer sensor
> +
> +I2C Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-i2c"
> +- reg : the I2C address of the magnetometer
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line.

I'm not a DT expert by any means, but I think the convention is
to not mention pinctrl in a given driver because otherwise
in theory we may need to mention it everywhere...

> +
> +Example:
> +
> +rm3100: rm3100@20 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> +
> + compatible = "pni,rm3100-i2c";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> +
> +SPI Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-spi"
> +- reg : address of sensor, usually 0 or 1.
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> +
> +Example:
> +
> +rm3100: rm3100@0{
> + compatible = "pni,rm3100-spi";
> + reg = <0>;
> +
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 41f0b97eb933..5bf3395fe9ae 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -288,6 +288,7 @@ pine64Pine64
>  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
>  plathome Plat'Home Co., Ltd.
>  plda PLDA
> +pni PNI

Please break the whole binding out as a separate patch.  Ideally do this
vendor-prefixes.txt as a separate patch again.  This cuts down
on the amount of searching that the DT people have to do to find
the bits that they want to look at.

>  portwell Portwell Inc.
>  poslab   Poslab Technology Co., Ltd.
>  powervr  PowerVR (deprecated, use img)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..30ee8cf98312 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
>  S:   Maintained
>  F:   drivers/pnp/
>  
> +PNI RM3100 IIO DRIVER
> +M:   Song Qiang 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/iio/magnetometer/rm3100-core.c

rm3110* perhaps?

> +F:   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Sat, 22 Sep 2018 10:42:44 +0100
Jonathan Cameron  wrote:

> A few follow ups to the discussion here from me..
> 
> Note it's helpful to crop and email and no one really minds if you
> just act on their review without acknowledging it (so cut the
> bits you fully agree with out too - saves on scrolling / reading time ;)
> 
> A catch all, "Agree with everything else and will fix for v2" covers
> everything you don't want to discuss further.
> (not that I'm great at doing this either :(
> ...
> > > > diff --git a/drivers/iio/magnetometer/rm3100-core.c 
> > > > b/drivers/iio/magnetometer/rm3100-core.c
> > > > new file mode 100644
> > > > index ..55d515e0fe67
> > > > --- /dev/null
> > > > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > > > @@ -0,0 +1,399 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * PNI RM3100 9-axis geomagnetic sensor driver core.
> > > > + *
> > > > + * Copyright (C) 2018 Song Qiang 
> > > > + *
> > > > + * User Manual available at
> > > > + * 
> > > > + *
> > > > + * TODO: Scale channel, event generaton, pm.
> > > 
> > > at least read support for _SCALE is mandatory, IMHO
> > > 
> > 
> > Okay, I'll add it in next version.
> >   
> Just for the record, it's only mandatory in cases where
> it is known (you'd be amazed how often we only know the value is monotonic)
> 
> Now as you put it in the todo list, it's presumably known here
> hence Peter's comment :)
> 
> (just avoiding setting precedence)
> 
> 
> 
> ...
> > > > +static const struct regmap_range rm3100_readable_ranges[] = {
> > > > +   regmap_reg_range(RM_W_REG_START, RM_W_REG_END),
> > > > +};
> > > > +
> > > > +const struct regmap_access_table rm3100_readable_table = {
> > > 
> > > static
> > > 
> > 
> > I was planning to let rm3100-i2c.c and rm3100-spi.c to share these 6
> > structures, because the only different configuration of regmap between
> > these two files lies in 'struct regmap_config'. To achieve this, I have
> > to expose these 3 structures to be referenced in rm3100-i2c.c and
> > rm3100-spi.c
> > Since *_common_probe() and *_common_remove() are exposed, I thought it
> > was fine to expose these structures to reduce redundant code, is this
> > prohibited?  
> Nope, but are you doing it in this patch? + you need to export the
> symbols if you are going to do that otherwise modular builds
> will fail to probe (and possibly build - I can't recall and am too
> lazy to check this morning - not enough coffee yet!)
> 
> ..
> > > > +   *val = le32_to_cpu((buffer[0] << 16) + (buffer[1] << 8) + 
> > > > buffer[2]);
> > > 
> > > no need for le32_to_cpu() 
> > > 
> > 
> > I think I didn't fully understand this, I'll look into it.
> >   
> 
> To point you in the right direction here.   When you explicitly pull out
> individual bytes like you are doing here, you are getting them in a particular
> endian order.   Shifts and adding (though normally convention is | when doing
> this) are done in machine endianness, hence the *val is already in the
> endian type of your cpu.
> 
> > > > +   *val = sign_extend32(*val, 23);
> > > > +
> > > > +   return IIO_VAL_INT;
> > > > +}
> > > > +
> > > > +#define RM_CHANNEL(axis, idx)  \
> > > 
> > > use RM3100_ prefix please
> > > 
> > 
> > In the last driver I wrote, name of registers are so long that I have to
> > suppress them as possible as I can, it seems like this one doesn't need
> > to. :)
> >   
> > > > +   {   
> > > > \
> > > > +   .type = IIO_MAGN,   
> > > > \
> > > > +   .modified = 1,  
> > > > \
> > > > +   .channel2 = IIO_MOD_##axis, 
> > > > \
> > > > +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   
> > > > \
> > > > +   .info_mask_shared_by_type = 
> > > > BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> > > > +   .scan_index = idx,  
> > > > \
> > > > +   .scan_type = {  
> > > > \
> > > > +   .sign = 's',
> > > > \
> > > > +   .realbits = 24, 
> > > > \
> > > > +   .storagebits = 32,  
> > > > \
> > > > +   .shift = 8, 
> > > > \
> > > > +   .endianness = IIO_LE,   
> > > > \
> > > > +   },  
> > > > \
> > > > +   }
> > > > +
> > > > +static const struct iio_chan_spec rm3100_channels[] = {
> > > > +   RM_CHANNEL(X, 0),
> > > > +   RM_CHANNEL(Y, 1),
> > > > 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

On Sat, 22 Sep 2018 10:42:44 +0100
Jonathan Cameron  wrote:

> A few follow ups to the discussion here from me..
> 
> Note it's helpful to crop and email and no one really minds if you
> just act on their review without acknowledging it (so cut the
> bits you fully agree with out too - saves on scrolling / reading time ;)
> 
> A catch all, "Agree with everything else and will fix for v2" covers
> everything you don't want to discuss further.
> (not that I'm great at doing this either :(
> ...
> > > > diff --git a/drivers/iio/magnetometer/rm3100-core.c 
> > > > b/drivers/iio/magnetometer/rm3100-core.c
> > > > new file mode 100644
> > > > index ..55d515e0fe67
> > > > --- /dev/null
> > > > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > > > @@ -0,0 +1,399 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * PNI RM3100 9-axis geomagnetic sensor driver core.
> > > > + *
> > > > + * Copyright (C) 2018 Song Qiang 
> > > > + *
> > > > + * User Manual available at
> > > > + * 
> > > > + *
> > > > + * TODO: Scale channel, event generaton, pm.
> > > 
> > > at least read support for _SCALE is mandatory, IMHO
> > > 
> > 
> > Okay, I'll add it in next version.
> >   
> Just for the record, it's only mandatory in cases where
> it is known (you'd be amazed how often we only know the value is monotonic)
> 
> Now as you put it in the todo list, it's presumably known here
> hence Peter's comment :)
> 
> (just avoiding setting precedence)
> 
> 
> 
> ...
> > > > +static const struct regmap_range rm3100_readable_ranges[] = {
> > > > +   regmap_reg_range(RM_W_REG_START, RM_W_REG_END),
> > > > +};
> > > > +
> > > > +const struct regmap_access_table rm3100_readable_table = {
> > > 
> > > static
> > > 
> > 
> > I was planning to let rm3100-i2c.c and rm3100-spi.c to share these 6
> > structures, because the only different configuration of regmap between
> > these two files lies in 'struct regmap_config'. To achieve this, I have
> > to expose these 3 structures to be referenced in rm3100-i2c.c and
> > rm3100-spi.c
> > Since *_common_probe() and *_common_remove() are exposed, I thought it
> > was fine to expose these structures to reduce redundant code, is this
> > prohibited?  
> Nope, but are you doing it in this patch? + you need to export the
> symbols if you are going to do that otherwise modular builds
> will fail to probe (and possibly build - I can't recall and am too
> lazy to check this morning - not enough coffee yet!)
> 
> ..
> > > > +   *val = le32_to_cpu((buffer[0] << 16) + (buffer[1] << 8) + 
> > > > buffer[2]);
> > > 
> > > no need for le32_to_cpu() 
> > > 
> > 
> > I think I didn't fully understand this, I'll look into it.
> >   
> 
> To point you in the right direction here.   When you explicitly pull out
> individual bytes like you are doing here, you are getting them in a particular
> endian order.   Shifts and adding (though normally convention is | when doing
> this) are done in machine endianness, hence the *val is already in the
> endian type of your cpu.
> 
> > > > +   *val = sign_extend32(*val, 23);
> > > > +
> > > > +   return IIO_VAL_INT;
> > > > +}
> > > > +
> > > > +#define RM_CHANNEL(axis, idx)  \
> > > 
> > > use RM3100_ prefix please
> > > 
> > 
> > In the last driver I wrote, name of registers are so long that I have to
> > suppress them as possible as I can, it seems like this one doesn't need
> > to. :)
> >   
> > > > +   {   
> > > > \
> > > > +   .type = IIO_MAGN,   
> > > > \
> > > > +   .modified = 1,  
> > > > \
> > > > +   .channel2 = IIO_MOD_##axis, 
> > > > \
> > > > +   .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   
> > > > \
> > > > +   .info_mask_shared_by_type = 
> > > > BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> > > > +   .scan_index = idx,  
> > > > \
> > > > +   .scan_type = {  
> > > > \
> > > > +   .sign = 's',
> > > > \
> > > > +   .realbits = 24, 
> > > > \
> > > > +   .storagebits = 32,  
> > > > \
> > > > +   .shift = 8, 
> > > > \
> > > > +   .endianness = IIO_LE,   
> > > > \
> > > > +   },  
> > > > \
> > > > +   }
> > > > +
> > > > +static const struct iio_chan_spec rm3100_channels[] = {
> > > > +   RM_CHANNEL(X, 0),
> > > > +   RM_CHANNEL(Y, 1),
> > > > 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

A few follow ups to the discussion here from me..

Note it's helpful to crop and email and no one really minds if you
just act on their review without acknowledging it (so cut the
bits you fully agree with out too - saves on scrolling / reading time ;)

A catch all, "Agree with everything else and will fix for v2" covers
everything you don't want to discuss further.
(not that I'm great at doing this either :(
...
> > > diff --git a/drivers/iio/magnetometer/rm3100-core.c 
> > > b/drivers/iio/magnetometer/rm3100-core.c
> > > new file mode 100644
> > > index ..55d515e0fe67
> > > --- /dev/null
> > > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > > @@ -0,0 +1,399 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * PNI RM3100 9-axis geomagnetic sensor driver core.
> > > + *
> > > + * Copyright (C) 2018 Song Qiang 
> > > + *
> > > + * User Manual available at
> > > + * 
> > > + *
> > > + * TODO: Scale channel, event generaton, pm.  
> > 
> > at least read support for _SCALE is mandatory, IMHO
> >   
> 
> Okay, I'll add it in next version.
> 
Just for the record, it's only mandatory in cases where
it is known (you'd be amazed how often we only know the value is monotonic)

Now as you put it in the todo list, it's presumably known here
hence Peter's comment :)

(just avoiding setting precedence)



...
> > > +static const struct regmap_range rm3100_readable_ranges[] = {
> > > + regmap_reg_range(RM_W_REG_START, RM_W_REG_END),
> > > +};
> > > +
> > > +const struct regmap_access_table rm3100_readable_table = {  
> > 
> > static
> >   
> 
> I was planning to let rm3100-i2c.c and rm3100-spi.c to share these 6
> structures, because the only different configuration of regmap between
> these two files lies in 'struct regmap_config'. To achieve this, I have
> to expose these 3 structures to be referenced in rm3100-i2c.c and
> rm3100-spi.c
> Since *_common_probe() and *_common_remove() are exposed, I thought it
> was fine to expose these structures to reduce redundant code, is this
> prohibited?
Nope, but are you doing it in this patch? + you need to export the
symbols if you are going to do that otherwise modular builds
will fail to probe (and possibly build - I can't recall and am too
lazy to check this morning - not enough coffee yet!)

..
> > > + *val = le32_to_cpu((buffer[0] << 16) + (buffer[1] << 8) + buffer[2]);  
> > 
> > no need for le32_to_cpu() 
> >   
> 
> I think I didn't fully understand this, I'll look into it.
> 

To point you in the right direction here.   When you explicitly pull out
individual bytes like you are doing here, you are getting them in a particular
endian order.   Shifts and adding (though normally convention is | when doing
this) are done in machine endianness, hence the *val is already in the
endian type of your cpu.

> > > + *val = sign_extend32(*val, 23);
> > > +
> > > + return IIO_VAL_INT;
> > > +}
> > > +
> > > +#define RM_CHANNEL(axis, idx)\  
> > 
> > use RM3100_ prefix please
> >   
> 
> In the last driver I wrote, name of registers are so long that I have to
> suppress them as possible as I can, it seems like this one doesn't need
> to. :)
> 
> > > + {   \
> > > + .type = IIO_MAGN,   \
> > > + .modified = 1,  \
> > > + .channel2 = IIO_MOD_##axis, \
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
> > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> > > + .scan_index = idx,  \
> > > + .scan_type = {  \
> > > + .sign = 's',\
> > > + .realbits = 24, \
> > > + .storagebits = 32,  \
> > > + .shift = 8, \
> > > + .endianness = IIO_LE,   \
> > > + },  \
> > > + }
> > > +
> > > +static const struct iio_chan_spec rm3100_channels[] = {
> > > + RM_CHANNEL(X, 0),
> > > + RM_CHANNEL(Y, 1),
> > > + RM_CHANNEL(Z, 2),
> > > + IIO_CHAN_SOFT_TIMESTAMP(3),
> > > +};
> > > +
> > > +static const unsigned long rm3100_scan_masks[] = {GENMASK(2, 0), 0};
> > > +
> > > +#define RM_SAMP_NUM  14  
> > 
> > prefix
> >   
> > > +
> > > +/* Frequency : rm3100_samp_rates[][0].rm3100_samp_rates[][1]Hz.
> > > + * Time between reading: rm3100_sam_rates[][2]ms (The first on is 
> > > actially 1.7).  
> > 
> > one
> > actually
> > 1.7 what unit?
> >   
> 
> It's in milliseconds. These time values are used for lookup so I do not
> need to compute time between conversion from 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

A few follow ups to the discussion here from me..

Note it's helpful to crop and email and no one really minds if you
just act on their review without acknowledging it (so cut the
bits you fully agree with out too - saves on scrolling / reading time ;)

A catch all, "Agree with everything else and will fix for v2" covers
everything you don't want to discuss further.
(not that I'm great at doing this either :(
...
> > > diff --git a/drivers/iio/magnetometer/rm3100-core.c 
> > > b/drivers/iio/magnetometer/rm3100-core.c
> > > new file mode 100644
> > > index ..55d515e0fe67
> > > --- /dev/null
> > > +++ b/drivers/iio/magnetometer/rm3100-core.c
> > > @@ -0,0 +1,399 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * PNI RM3100 9-axis geomagnetic sensor driver core.
> > > + *
> > > + * Copyright (C) 2018 Song Qiang 
> > > + *
> > > + * User Manual available at
> > > + * 
> > > + *
> > > + * TODO: Scale channel, event generaton, pm.  
> > 
> > at least read support for _SCALE is mandatory, IMHO
> >   
> 
> Okay, I'll add it in next version.
> 
Just for the record, it's only mandatory in cases where
it is known (you'd be amazed how often we only know the value is monotonic)

Now as you put it in the todo list, it's presumably known here
hence Peter's comment :)

(just avoiding setting precedence)



...
> > > +static const struct regmap_range rm3100_readable_ranges[] = {
> > > + regmap_reg_range(RM_W_REG_START, RM_W_REG_END),
> > > +};
> > > +
> > > +const struct regmap_access_table rm3100_readable_table = {  
> > 
> > static
> >   
> 
> I was planning to let rm3100-i2c.c and rm3100-spi.c to share these 6
> structures, because the only different configuration of regmap between
> these two files lies in 'struct regmap_config'. To achieve this, I have
> to expose these 3 structures to be referenced in rm3100-i2c.c and
> rm3100-spi.c
> Since *_common_probe() and *_common_remove() are exposed, I thought it
> was fine to expose these structures to reduce redundant code, is this
> prohibited?
Nope, but are you doing it in this patch? + you need to export the
symbols if you are going to do that otherwise modular builds
will fail to probe (and possibly build - I can't recall and am too
lazy to check this morning - not enough coffee yet!)

..
> > > + *val = le32_to_cpu((buffer[0] << 16) + (buffer[1] << 8) + buffer[2]);  
> > 
> > no need for le32_to_cpu() 
> >   
> 
> I think I didn't fully understand this, I'll look into it.
> 

To point you in the right direction here.   When you explicitly pull out
individual bytes like you are doing here, you are getting them in a particular
endian order.   Shifts and adding (though normally convention is | when doing
this) are done in machine endianness, hence the *val is already in the
endian type of your cpu.

> > > + *val = sign_extend32(*val, 23);
> > > +
> > > + return IIO_VAL_INT;
> > > +}
> > > +
> > > +#define RM_CHANNEL(axis, idx)\  
> > 
> > use RM3100_ prefix please
> >   
> 
> In the last driver I wrote, name of registers are so long that I have to
> suppress them as possible as I can, it seems like this one doesn't need
> to. :)
> 
> > > + {   \
> > > + .type = IIO_MAGN,   \
> > > + .modified = 1,  \
> > > + .channel2 = IIO_MOD_##axis, \
> > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),   \
> > > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> > > + .scan_index = idx,  \
> > > + .scan_type = {  \
> > > + .sign = 's',\
> > > + .realbits = 24, \
> > > + .storagebits = 32,  \
> > > + .shift = 8, \
> > > + .endianness = IIO_LE,   \
> > > + },  \
> > > + }
> > > +
> > > +static const struct iio_chan_spec rm3100_channels[] = {
> > > + RM_CHANNEL(X, 0),
> > > + RM_CHANNEL(Y, 1),
> > > + RM_CHANNEL(Z, 2),
> > > + IIO_CHAN_SOFT_TIMESTAMP(3),
> > > +};
> > > +
> > > +static const unsigned long rm3100_scan_masks[] = {GENMASK(2, 0), 0};
> > > +
> > > +#define RM_SAMP_NUM  14  
> > 
> > prefix
> >   
> > > +
> > > +/* Frequency : rm3100_samp_rates[][0].rm3100_samp_rates[][1]Hz.
> > > + * Time between reading: rm3100_sam_rates[][2]ms (The first on is 
> > > actially 1.7).  
> > 
> > one
> > actually
> > 1.7 what unit?
> >   
> 
> It's in milliseconds. These time values are used for lookup so I do not
> need to compute time between conversion from 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 01:20:12PM +0100, Jonathan Cameron wrote:
> Hi Song,
> 
> ...
> 
> Please crop emails down to the relevant section when replying.  Lots of 
> scrolling
> for everyone otherwise and sometimes things get missed when doing that.
> 

Hi Jonathan,

Sorryi for this, won't happen again.
I was intended to give a simple question, but it reminds me of a lot
unrelated stuff, next time I'll make it another email.

> > > > > + if (!buffer)
> > > > > + goto done;
> > > > > +
> > > > > + mutex_lock(>lock);
> > > > > + ret = rm3100_wait_measurement(data);
> > > > > + if (ret < 0) {
> > > > > + mutex_unlock(>lock);
> > > > > + goto done;
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < 3; i++) {
> > > > > + ret = regmap_bulk_read(regmap, RM_REG_MX2 + 3 * i,
> > > > > + buffer + 4 * i, 3);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + }  
> > > 
> > > Wouldn't it be better to read the 3 axis with one transaction here.
> > > And if required shuffle the data into the iio buffer.
> > > 
> > >   
> > 
> > Hi Phil,
> > 
> > That's surely something will make this code better!
> > 
> > But also, there is something worth discussing,
> > When triggered buffer is triggered here, I should push all the data into
> > userspace, and every time the buffer is enabled, the iio subsystem
> > automatically computes the alignment of my data. This sensor has 3
> > 24bits channels, and my original thought of data alignment should be
> > like this because my machine is 32bits:
> > +--+++++++
> > | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 8b |
> > +--+++++++
> > 3 bytes data and 1 byte for alignment, last 8 bytes for the timestamp.
> > Aligned to 4 bytes. 
> > But the actual layout is like this:
> > +--++++++++
> > | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 4b | 8b |
> > +--++++++++
> > Soon after I debugged the code of industrial io core I found that in the
> > iio_compute_scanbytes() in industialio-buffer.c, alignment was computed
> > each channel a time, and was aligned with the length of next channel. (eg:
> > In the last case, before the timestamp channel was appended, there are
> > 12 bytes already in the buffer, to align to the timetamp which is 8 
> > bytes, it injected 4 bytes to make it 16, and then append timestamp. )
> > This leads to two questions:
> > 
> > 1. I thought computer systems align their data in the memory for least
> > time access, shouldn't this always align to the bits of architecture,
> > like for a 32bits machine align to 4?? I'm a little confused here.
> > Jonathan must have considered something, Hoping to get some
> > explanations!
> 
> A couple of reasons for this:
> 
> 1. It's much nicer to have the same spacing independent of the architecture
> as it saves having to have userspace code do different things depending on
> what the kernel is running at, which may not be the same as userspace.
> 
> 2. The size of a memory read is not the only scale that makes sense for
> alignment.  In theory at least you can have fun and games like crossing of
> cacheline boundaries if you do 64 bit access only at 32bits aligned on a
> 32 bit platform.  Won't hit all that often with typically 64 byte cachelines
> but it is the sort of subtle effect you get when you don't go with
> 'naturally' aligned. (which is what aligning to an elements own size is
> normally called IIRC).
> 

I see, architecture independent surely should be the principle of the
Linux kernel, this makes sense, I think I should learn more stuff about
natural alignment. I googled something about it and understand what it
is, but still find it difficult to fully understand its benefits. I think
I'll learn more about it after. 

> > 
> > 2. In this case I have 4 channels including the timetamp one, and I
> > thought it should be aligned like the first one. But actually it's
> > aligned like the second one, when I was testing the buffer, I got
> > confused, because the in_magn_x_type returned 24/32, 24/32, 24/32
> > and 64/64. If the iio core injected some empty bytes, shouldn't it
> > notify me? Or at least change the timestamp type in scan_bytes to
> > 64/96>>0?? And when I only enabled two of three channels, the data  
> > alignment in the buffer looks like this:
> > +--+++++
> > | 3b(ytes) | 1b | 3b | 1b | 8b |
> > +--+++++
> > Notice that 4 bytes have gone due to data of two channels have already
> > aligned to 8 bytes.
> > Hoping to get some answers, I'm very curious and interested about this!
> 
> Nope it shouldn't do the 64/96 need to tell you because it uses
> the rules of natural alignment which are easy for userspace to predict.
> So in this second case there is no need to pad so it doesn't - whereas the
> 4 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 01:20:12PM +0100, Jonathan Cameron wrote:
> Hi Song,
> 
> ...
> 
> Please crop emails down to the relevant section when replying.  Lots of 
> scrolling
> for everyone otherwise and sometimes things get missed when doing that.
> 

Hi Jonathan,

Sorryi for this, won't happen again.
I was intended to give a simple question, but it reminds me of a lot
unrelated stuff, next time I'll make it another email.

> > > > > + if (!buffer)
> > > > > + goto done;
> > > > > +
> > > > > + mutex_lock(>lock);
> > > > > + ret = rm3100_wait_measurement(data);
> > > > > + if (ret < 0) {
> > > > > + mutex_unlock(>lock);
> > > > > + goto done;
> > > > > + }
> > > > > +
> > > > > + for (i = 0; i < 3; i++) {
> > > > > + ret = regmap_bulk_read(regmap, RM_REG_MX2 + 3 * i,
> > > > > + buffer + 4 * i, 3);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + }  
> > > 
> > > Wouldn't it be better to read the 3 axis with one transaction here.
> > > And if required shuffle the data into the iio buffer.
> > > 
> > >   
> > 
> > Hi Phil,
> > 
> > That's surely something will make this code better!
> > 
> > But also, there is something worth discussing,
> > When triggered buffer is triggered here, I should push all the data into
> > userspace, and every time the buffer is enabled, the iio subsystem
> > automatically computes the alignment of my data. This sensor has 3
> > 24bits channels, and my original thought of data alignment should be
> > like this because my machine is 32bits:
> > +--+++++++
> > | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 8b |
> > +--+++++++
> > 3 bytes data and 1 byte for alignment, last 8 bytes for the timestamp.
> > Aligned to 4 bytes. 
> > But the actual layout is like this:
> > +--++++++++
> > | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 4b | 8b |
> > +--++++++++
> > Soon after I debugged the code of industrial io core I found that in the
> > iio_compute_scanbytes() in industialio-buffer.c, alignment was computed
> > each channel a time, and was aligned with the length of next channel. (eg:
> > In the last case, before the timestamp channel was appended, there are
> > 12 bytes already in the buffer, to align to the timetamp which is 8 
> > bytes, it injected 4 bytes to make it 16, and then append timestamp. )
> > This leads to two questions:
> > 
> > 1. I thought computer systems align their data in the memory for least
> > time access, shouldn't this always align to the bits of architecture,
> > like for a 32bits machine align to 4?? I'm a little confused here.
> > Jonathan must have considered something, Hoping to get some
> > explanations!
> 
> A couple of reasons for this:
> 
> 1. It's much nicer to have the same spacing independent of the architecture
> as it saves having to have userspace code do different things depending on
> what the kernel is running at, which may not be the same as userspace.
> 
> 2. The size of a memory read is not the only scale that makes sense for
> alignment.  In theory at least you can have fun and games like crossing of
> cacheline boundaries if you do 64 bit access only at 32bits aligned on a
> 32 bit platform.  Won't hit all that often with typically 64 byte cachelines
> but it is the sort of subtle effect you get when you don't go with
> 'naturally' aligned. (which is what aligning to an elements own size is
> normally called IIRC).
> 

I see, architecture independent surely should be the principle of the
Linux kernel, this makes sense, I think I should learn more stuff about
natural alignment. I googled something about it and understand what it
is, but still find it difficult to fully understand its benefits. I think
I'll learn more about it after. 

> > 
> > 2. In this case I have 4 channels including the timetamp one, and I
> > thought it should be aligned like the first one. But actually it's
> > aligned like the second one, when I was testing the buffer, I got
> > confused, because the in_magn_x_type returned 24/32, 24/32, 24/32
> > and 64/64. If the iio core injected some empty bytes, shouldn't it
> > notify me? Or at least change the timestamp type in scan_bytes to
> > 64/96>>0?? And when I only enabled two of three channels, the data  
> > alignment in the buffer looks like this:
> > +--+++++
> > | 3b(ytes) | 1b | 3b | 1b | 8b |
> > +--+++++
> > Notice that 4 bytes have gone due to data of two channels have already
> > aligned to 8 bytes.
> > Hoping to get some answers, I'm very curious and interested about this!
> 
> Nope it shouldn't do the 64/96 need to tell you because it uses
> the rules of natural alignment which are easy for userspace to predict.
> So in this second case there is no need to pad so it doesn't - whereas the
> 4 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

Hi Song,

...

Please crop emails down to the relevant section when replying.  Lots of 
scrolling
for everyone otherwise and sometimes things get missed when doing that.

> > > > +   if (!buffer)
> > > > +   goto done;
> > > > +
> > > > +   mutex_lock(>lock);
> > > > +   ret = rm3100_wait_measurement(data);
> > > > +   if (ret < 0) {
> > > > +   mutex_unlock(>lock);
> > > > +   goto done;
> > > > +   }
> > > > +
> > > > +   for (i = 0; i < 3; i++) {
> > > > +   ret = regmap_bulk_read(regmap, RM_REG_MX2 + 3 * i,
> > > > +   buffer + 4 * i, 3);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > > +   }  
> > 
> > Wouldn't it be better to read the 3 axis with one transaction here.
> > And if required shuffle the data into the iio buffer.
> > 
> >   
> 
> Hi Phil,
> 
> That's surely something will make this code better!
> 
> But also, there is something worth discussing,
> When triggered buffer is triggered here, I should push all the data into
> userspace, and every time the buffer is enabled, the iio subsystem
> automatically computes the alignment of my data. This sensor has 3
> 24bits channels, and my original thought of data alignment should be
> like this because my machine is 32bits:
> +--+++++++
> | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 8b |
> +--+++++++
> 3 bytes data and 1 byte for alignment, last 8 bytes for the timestamp.
> Aligned to 4 bytes. 
> But the actual layout is like this:
> +--++++++++
> | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 4b | 8b |
> +--++++++++
> Soon after I debugged the code of industrial io core I found that in the
> iio_compute_scanbytes() in industialio-buffer.c, alignment was computed
> each channel a time, and was aligned with the length of next channel. (eg:
> In the last case, before the timestamp channel was appended, there are
> 12 bytes already in the buffer, to align to the timetamp which is 8 
> bytes, it injected 4 bytes to make it 16, and then append timestamp. )
> This leads to two questions:
> 
> 1. I thought computer systems align their data in the memory for least
> time access, shouldn't this always align to the bits of architecture,
> like for a 32bits machine align to 4?? I'm a little confused here.
> Jonathan must have considered something, Hoping to get some
> explanations!

A couple of reasons for this:

1. It's much nicer to have the same spacing independent of the architecture
as it saves having to have userspace code do different things depending on
what the kernel is running at, which may not be the same as userspace.

2. The size of a memory read is not the only scale that makes sense for
alignment.  In theory at least you can have fun and games like crossing of
cacheline boundaries if you do 64 bit access only at 32bits aligned on a
32 bit platform.  Won't hit all that often with typically 64 byte cachelines
but it is the sort of subtle effect you get when you don't go with
'naturally' aligned. (which is what aligning to an elements own size is
normally called IIRC).

> 
> 2. In this case I have 4 channels including the timetamp one, and I
> thought it should be aligned like the first one. But actually it's
> aligned like the second one, when I was testing the buffer, I got
> confused, because the in_magn_x_type returned 24/32, 24/32, 24/32
> and 64/64. If the iio core injected some empty bytes, shouldn't it
> notify me? Or at least change the timestamp type in scan_bytes to
> 64/96>>0?? And when I only enabled two of three channels, the data  
> alignment in the buffer looks like this:
> +--+++++
> | 3b(ytes) | 1b | 3b | 1b | 8b |
> +--+++++
> Notice that 4 bytes have gone due to data of two channels have already
> aligned to 8 bytes.
> Hoping to get some answers, I'm very curious and interested about this!

Nope it shouldn't do the 64/96 need to tell you because it uses
the rules of natural alignment which are easy for userspace to predict.
So in this second case there is no need to pad so it doesn't - whereas the
4 bytes are needed in the first case.

We could have explicitly output the offsets and kept things more flexible
but that would have prevented some general optimizations on the userspace
side for common cases.  If you are reading a 3 axis accelerometer for
example with timestamp then you can probably safely prebake optimized handling
for,

1B (x), 1B (y), 1B(z), 5B(pad), 8B(ts)

2B (x), 2B (y), 2B(z), 2B(pad), 8B(ts)

in reality no one ever goes higher than this because accelerometers aren't
accurate enough...

If we did anything more complex we'd need to describe to userspace in some
sense what was happening beyond the static data currently provided (when
combined with the ordering and enables) and that makes for a complex 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Jonathan Cameron]

Hi Song,

...

Please crop emails down to the relevant section when replying.  Lots of 
scrolling
for everyone otherwise and sometimes things get missed when doing that.

> > > > +   if (!buffer)
> > > > +   goto done;
> > > > +
> > > > +   mutex_lock(>lock);
> > > > +   ret = rm3100_wait_measurement(data);
> > > > +   if (ret < 0) {
> > > > +   mutex_unlock(>lock);
> > > > +   goto done;
> > > > +   }
> > > > +
> > > > +   for (i = 0; i < 3; i++) {
> > > > +   ret = regmap_bulk_read(regmap, RM_REG_MX2 + 3 * i,
> > > > +   buffer + 4 * i, 3);
> > > > +   if (ret < 0)
> > > > +   return ret;
> > > > +   }  
> > 
> > Wouldn't it be better to read the 3 axis with one transaction here.
> > And if required shuffle the data into the iio buffer.
> > 
> >   
> 
> Hi Phil,
> 
> That's surely something will make this code better!
> 
> But also, there is something worth discussing,
> When triggered buffer is triggered here, I should push all the data into
> userspace, and every time the buffer is enabled, the iio subsystem
> automatically computes the alignment of my data. This sensor has 3
> 24bits channels, and my original thought of data alignment should be
> like this because my machine is 32bits:
> +--+++++++
> | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 8b |
> +--+++++++
> 3 bytes data and 1 byte for alignment, last 8 bytes for the timestamp.
> Aligned to 4 bytes. 
> But the actual layout is like this:
> +--++++++++
> | 3b(ytes) | 1b | 3b | 1b | 3b | 1b | 4b | 8b |
> +--++++++++
> Soon after I debugged the code of industrial io core I found that in the
> iio_compute_scanbytes() in industialio-buffer.c, alignment was computed
> each channel a time, and was aligned with the length of next channel. (eg:
> In the last case, before the timestamp channel was appended, there are
> 12 bytes already in the buffer, to align to the timetamp which is 8 
> bytes, it injected 4 bytes to make it 16, and then append timestamp. )
> This leads to two questions:
> 
> 1. I thought computer systems align their data in the memory for least
> time access, shouldn't this always align to the bits of architecture,
> like for a 32bits machine align to 4?? I'm a little confused here.
> Jonathan must have considered something, Hoping to get some
> explanations!

A couple of reasons for this:

1. It's much nicer to have the same spacing independent of the architecture
as it saves having to have userspace code do different things depending on
what the kernel is running at, which may not be the same as userspace.

2. The size of a memory read is not the only scale that makes sense for
alignment.  In theory at least you can have fun and games like crossing of
cacheline boundaries if you do 64 bit access only at 32bits aligned on a
32 bit platform.  Won't hit all that often with typically 64 byte cachelines
but it is the sort of subtle effect you get when you don't go with
'naturally' aligned. (which is what aligning to an elements own size is
normally called IIRC).

> 
> 2. In this case I have 4 channels including the timetamp one, and I
> thought it should be aligned like the first one. But actually it's
> aligned like the second one, when I was testing the buffer, I got
> confused, because the in_magn_x_type returned 24/32, 24/32, 24/32
> and 64/64. If the iio core injected some empty bytes, shouldn't it
> notify me? Or at least change the timestamp type in scan_bytes to
> 64/96>>0?? And when I only enabled two of three channels, the data  
> alignment in the buffer looks like this:
> +--+++++
> | 3b(ytes) | 1b | 3b | 1b | 8b |
> +--+++++
> Notice that 4 bytes have gone due to data of two channels have already
> aligned to 8 bytes.
> Hoping to get some answers, I'm very curious and interested about this!

Nope it shouldn't do the 64/96 need to tell you because it uses
the rules of natural alignment which are easy for userspace to predict.
So in this second case there is no need to pad so it doesn't - whereas the
4 bytes are needed in the first case.

We could have explicitly output the offsets and kept things more flexible
but that would have prevented some general optimizations on the userspace
side for common cases.  If you are reading a 3 axis accelerometer for
example with timestamp then you can probably safely prebake optimized handling
for,

1B (x), 1B (y), 1B(z), 5B(pad), 8B(ts)

2B (x), 2B (y), 2B(z), 2B(pad), 8B(ts)

in reality no one ever goes higher than this because accelerometers aren't
accurate enough...

If we did anything more complex we'd need to describe to userspace in some
sense what was happening beyond the static data currently provided (when
combined with the ordering and enables) and that makes for a complex 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 01:07:17PM +0800, Phil Reid wrote:
> G'day Song,
> 
> One more comment below.
> On 20/09/2018 9:46 PM, Peter Meerwald-Stadler wrote:
> > On Thu, 20 Sep 2018, Song Qiang 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.
> > 
> > comments below
> > 
> > > 
> > > 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 
> > > ---
> > >   .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
> > >   .../devicetree/bindings/vendor-prefixes.txt   |   1 +
> > >   MAINTAINERS   |  10 +
> > >   drivers/iio/magnetometer/Kconfig  |  29 ++
> > >   drivers/iio/magnetometer/Makefile |   4 +
> > >   drivers/iio/magnetometer/rm3100-core.c| 399 ++
> > >   drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
> > >   drivers/iio/magnetometer/rm3100-spi.c |  72 
> > >   drivers/iio/magnetometer/rm3100.h |  90 
> > >   9 files changed, 728 insertions(+)
> > >   create mode 100644 
> > > Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100.h
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> > > b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > > new file mode 100644
> > > index ..d0d2063e943f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > > @@ -0,0 +1,57 @@
> > > +* PNI RM3100 9-axis magnetometer sensor
> > > +
> > > +I2C Bus:
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : should be "pni,rm3100-i2c"
> > > +- reg : the I2C address of the magnetometer
> > > +
> > > +Optional properties:
> > > +
> > > +- interrupts: data ready (DRDY) from the chip.
> > > +  The interrupts can be triggered on rising edges.
> > > +
> > > + Refer to interrupt-controller/interrupts.txt for generic
> > > + interrupt client node bindings.
> > > +
> > > +- pinctrl-*: pinctrl setup for DRDY line.
> > > +
> > > +Example:
> > > +
> > > +rm3100: rm3100@20 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_pins>;
> > > +
> > > + compatible = "pni,rm3100-i2c";
> > > + reg = <0x20>;
> > > + interrupt-parent = <>;
> > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > > +};
> > > +
> > > +SPI Bus:
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : should be "pni,rm3100-spi"
> > > +- reg : address of sensor, usually 0 or 1.
> > > +
> > > +Optional properties:
> > > +
> > > +- interrupts: data ready (DRDY) from the chip.
> > > +  The interrupts can be triggered on rising edges.
> > > +
> > > + Refer to interrupt-controller/interrupts.txt for generic
> > > + interrupt client node bindings.
> > > +
> > > +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> > 
> > depends
> > architecture
> > 
> > > +
> > > +Example:
> > > +
> > > +rm3100: rm3100@0{
> > > + compatible = "pni,rm3100-spi";
> > > + reg = <0>;
> > > +
> > > + interrupt-parent = <>;
> > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > > +};
> > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > index 41f0b97eb933..5bf3395fe9ae 100644
> > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > @@ -288,6 +288,7 @@ pine64Pine64
> > >   pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> > >   plathomePlat'Home Co., Ltd.
> > >   pldaPLDA
> > > +pni PNI
> > >   portwellPortwell Inc.
> > >   poslab  Poslab Technology Co., Ltd.
> > >   powervr PowerVR (deprecated, use img)
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 967ce8cdd1cc..30ee8cf98312 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
> > > 
> > >   S:  Maintained
> > >   F:  drivers/pnp/
> > > +PNI RM3100 IIO DRIVER
> > > +M:   Song Qiang 
> > > +L:   linux-...@vger.kernel.org
> > > +S:   Maintained
> > > +F:   drivers/iio/magnetometer/rm3100-core.c
> > > +F:   drivers/iio/magnetometer/rm3100-i2c.c
> > > +F:   drivers/iio/magnetometer/rm3100-spi.c
> > > +F:   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 01:07:17PM +0800, Phil Reid wrote:
> G'day Song,
> 
> One more comment below.
> On 20/09/2018 9:46 PM, Peter Meerwald-Stadler wrote:
> > On Thu, 20 Sep 2018, Song Qiang 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.
> > 
> > comments below
> > 
> > > 
> > > 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 
> > > ---
> > >   .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
> > >   .../devicetree/bindings/vendor-prefixes.txt   |   1 +
> > >   MAINTAINERS   |  10 +
> > >   drivers/iio/magnetometer/Kconfig  |  29 ++
> > >   drivers/iio/magnetometer/Makefile |   4 +
> > >   drivers/iio/magnetometer/rm3100-core.c| 399 ++
> > >   drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
> > >   drivers/iio/magnetometer/rm3100-spi.c |  72 
> > >   drivers/iio/magnetometer/rm3100.h |  90 
> > >   9 files changed, 728 insertions(+)
> > >   create mode 100644 
> > > Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> > >   create mode 100644 drivers/iio/magnetometer/rm3100.h
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> > > b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > > new file mode 100644
> > > index ..d0d2063e943f
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > > @@ -0,0 +1,57 @@
> > > +* PNI RM3100 9-axis magnetometer sensor
> > > +
> > > +I2C Bus:
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : should be "pni,rm3100-i2c"
> > > +- reg : the I2C address of the magnetometer
> > > +
> > > +Optional properties:
> > > +
> > > +- interrupts: data ready (DRDY) from the chip.
> > > +  The interrupts can be triggered on rising edges.
> > > +
> > > + Refer to interrupt-controller/interrupts.txt for generic
> > > + interrupt client node bindings.
> > > +
> > > +- pinctrl-*: pinctrl setup for DRDY line.
> > > +
> > > +Example:
> > > +
> > > +rm3100: rm3100@20 {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_pins>;
> > > +
> > > + compatible = "pni,rm3100-i2c";
> > > + reg = <0x20>;
> > > + interrupt-parent = <>;
> > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > > +};
> > > +
> > > +SPI Bus:
> > > +
> > > +Required properties:
> > > +
> > > +- compatible : should be "pni,rm3100-spi"
> > > +- reg : address of sensor, usually 0 or 1.
> > > +
> > > +Optional properties:
> > > +
> > > +- interrupts: data ready (DRDY) from the chip.
> > > +  The interrupts can be triggered on rising edges.
> > > +
> > > + Refer to interrupt-controller/interrupts.txt for generic
> > > + interrupt client node bindings.
> > > +
> > > +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> > 
> > depends
> > architecture
> > 
> > > +
> > > +Example:
> > > +
> > > +rm3100: rm3100@0{
> > > + compatible = "pni,rm3100-spi";
> > > + reg = <0>;
> > > +
> > > + interrupt-parent = <>;
> > > + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > > +};
> > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > index 41f0b97eb933..5bf3395fe9ae 100644
> > > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > > @@ -288,6 +288,7 @@ pine64Pine64
> > >   pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> > >   plathomePlat'Home Co., Ltd.
> > >   pldaPLDA
> > > +pni PNI
> > >   portwellPortwell Inc.
> > >   poslab  Poslab Technology Co., Ltd.
> > >   powervr PowerVR (deprecated, use img)
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 967ce8cdd1cc..30ee8cf98312 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
> > > 
> > >   S:  Maintained
> > >   F:  drivers/pnp/
> > > +PNI RM3100 IIO DRIVER
> > > +M:   Song Qiang 
> > > +L:   linux-...@vger.kernel.org
> > > +S:   Maintained
> > > +F:   drivers/iio/magnetometer/rm3100-core.c
> > > +F:   drivers/iio/magnetometer/rm3100-i2c.c
> > > +F:   drivers/iio/magnetometer/rm3100-spi.c
> > > +F:   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 10:05:38AM +0800, Phil Reid wrote:
> On 20/09/2018 9:13 PM, Song Qiang 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.
> > 
> 
> In the subject: Isn't the RM3100 a 3axis mag.
> The 9axis bit comes when you combine it with an accel / gryo I think.
> 
> ... snip
> 

Hi Phil,

That's right!
The first time I got this sensor I googled it on the internet and the
abstract of the first webpage told me that it's a 9-axis sensor, and
it's from its offcial website! I just googled it and saw it's still
there, but after I entered the website, there isn't anything on the
webpage about '9-axis', maybe I just saw the old wrong web cache of 
google. Newer websites do say it's a 3-axis magnetometer sensor, and 
it only has three channels of data can be read. 3-axis it is.

> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > @@ -0,0 +1,57 @@
> > +* PNI RM3100 9-axis magnetometer sensor
> > +
> > +I2C Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-i2c"
> > +- reg : the I2C address of the magnetometer
> > +
> 
> ... snip
> 
> > +SPI Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-spi"
> > +- reg : address of sensor, usually 0 or 1.
> > +
> 
> Looking at other drivers supporting i2c / spi.
> They use the same compatible for both.
> 
> eg: see iio/accel/adxl345_*.c
> 
> and it's binding doc:
> 
> Required properties:
>  - compatible : should be "adi,adxl345"
>  - reg : the I2C address or SPI chip select number of the sensor
> 

Agreed, since nodes of this sensor should be already on the bus where it
should be in DT, there's no need for compitable string to identify which
bus it's on.

yours,
Song Qiang

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Fri, Sep 21, 2018 at 10:05:38AM +0800, Phil Reid wrote:
> On 20/09/2018 9:13 PM, Song Qiang 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.
> > 
> 
> In the subject: Isn't the RM3100 a 3axis mag.
> The 9axis bit comes when you combine it with an accel / gryo I think.
> 
> ... snip
> 

Hi Phil,

That's right!
The first time I got this sensor I googled it on the internet and the
abstract of the first webpage told me that it's a 9-axis sensor, and
it's from its offcial website! I just googled it and saw it's still
there, but after I entered the website, there isn't anything on the
webpage about '9-axis', maybe I just saw the old wrong web cache of 
google. Newer websites do say it's a 3-axis magnetometer sensor, and 
it only has three channels of data can be read. 3-axis it is.

> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > @@ -0,0 +1,57 @@
> > +* PNI RM3100 9-axis magnetometer sensor
> > +
> > +I2C Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-i2c"
> > +- reg : the I2C address of the magnetometer
> > +
> 
> ... snip
> 
> > +SPI Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-spi"
> > +- reg : address of sensor, usually 0 or 1.
> > +
> 
> Looking at other drivers supporting i2c / spi.
> They use the same compatible for both.
> 
> eg: see iio/accel/adxl345_*.c
> 
> and it's binding doc:
> 
> Required properties:
>  - compatible : should be "adi,adxl345"
>  - reg : the I2C address or SPI chip select number of the sensor
> 

Agreed, since nodes of this sensor should be already on the bus where it
should be in DT, there's no need for compitable string to identify which
bus it's on.

yours,
Song Qiang

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

G'day Song,

One more comment below.
On 20/09/2018 9:46 PM, Peter Meerwald-Stadler wrote:

On Thu, 20 Sep 2018, Song Qiang 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.


comments below



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 
---
  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
  MAINTAINERS   |  10 +
  drivers/iio/magnetometer/Kconfig  |  29 ++
  drivers/iio/magnetometer/Makefile |   4 +
  drivers/iio/magnetometer/rm3100-core.c| 399 ++
  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
  drivers/iio/magnetometer/rm3100-spi.c |  72 
  drivers/iio/magnetometer/rm3100.h |  90 
  9 files changed, 728 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
  create mode 100644 drivers/iio/magnetometer/rm3100.h

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
new file mode 100644
index ..d0d2063e943f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+
+Optional properties:
+
+- interrupts: data ready (DRDY) from the chip.
+  The interrupts can be triggered on rising edges.
+
+   Refer to interrupt-controller/interrupts.txt for generic
+   interrupt client node bindings.
+
+- pinctrl-*: pinctrl setup for DRDY line.
+
+Example:
+
+rm3100: rm3100@20 {
+   pinctrl-names = "default";
+   pinctrl-0 = <_pins>;
+
+   compatible = "pni,rm3100-i2c";
+   reg = <0x20>;
+   interrupt-parent = <>;
+   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+};
+
+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+
+Optional properties:
+
+- interrupts: data ready (DRDY) from the chip.
+  The interrupts can be triggered on rising edges.
+
+   Refer to interrupt-controller/interrupts.txt for generic
+   interrupt client node bindings.
+
+- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.


depends
architecture


+
+Example:
+
+rm3100: rm3100@0{
+   compatible = "pni,rm3100-spi";
+   reg = <0>;
+
+   interrupt-parent = <>;
+   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 41f0b97eb933..5bf3395fe9ae 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -288,6 +288,7 @@ pine64  Pine64
  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
  plathome  Plat'Home Co., Ltd.
  plda  PLDA
+pni PNI
  portwell  Portwell Inc.
  poslabPoslab Technology Co., Ltd.
  powervr   PowerVR (deprecated, use img)
diff --git a/MAINTAINERS b/MAINTAINERS
index 967ce8cdd1cc..30ee8cf98312 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11393,6 +11393,16 @@ M: "Rafael J. Wysocki" 
  S:Maintained
  F:drivers/pnp/
  
+PNI RM3100 IIO DRIVER

+M: Song Qiang 
+L: linux-...@vger.kernel.org
+S: Maintained
+F: drivers/iio/magnetometer/rm3100-core.c
+F: drivers/iio/magnetometer/rm3100-i2c.c
+F: drivers/iio/magnetometer/rm3100-spi.c
+F: drivers/iio/magnetometer/rm3100.h
+F: Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
+
  POSIX CLOCKS and TIMERS
  M:Thomas Gleixner 
  L:linux-kernel@vger.kernel.org
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index ed9d776d01af..f130b866a4fc 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
  - hmc5843_core (core functions)
  - hmc5843_spi (support for HMC5983)
  
+config SENSORS_RM3100

+   tristate
+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+
+config SENSORS_RM3100_I2C
+   tristate "PNI RM3100 9-Axis Magnetometer (I2C)"
+   depends on I2C
+   select SENSORS_RM3100
+   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

G'day Song,

One more comment below.
On 20/09/2018 9:46 PM, Peter Meerwald-Stadler wrote:

On Thu, 20 Sep 2018, Song Qiang 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.


comments below



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 
---
  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
  MAINTAINERS   |  10 +
  drivers/iio/magnetometer/Kconfig  |  29 ++
  drivers/iio/magnetometer/Makefile |   4 +
  drivers/iio/magnetometer/rm3100-core.c| 399 ++
  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
  drivers/iio/magnetometer/rm3100-spi.c |  72 
  drivers/iio/magnetometer/rm3100.h |  90 
  9 files changed, 728 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
  create mode 100644 drivers/iio/magnetometer/rm3100.h

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
new file mode 100644
index ..d0d2063e943f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+
+Optional properties:
+
+- interrupts: data ready (DRDY) from the chip.
+  The interrupts can be triggered on rising edges.
+
+   Refer to interrupt-controller/interrupts.txt for generic
+   interrupt client node bindings.
+
+- pinctrl-*: pinctrl setup for DRDY line.
+
+Example:
+
+rm3100: rm3100@20 {
+   pinctrl-names = "default";
+   pinctrl-0 = <_pins>;
+
+   compatible = "pni,rm3100-i2c";
+   reg = <0x20>;
+   interrupt-parent = <>;
+   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+};
+
+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+
+Optional properties:
+
+- interrupts: data ready (DRDY) from the chip.
+  The interrupts can be triggered on rising edges.
+
+   Refer to interrupt-controller/interrupts.txt for generic
+   interrupt client node bindings.
+
+- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.


depends
architecture


+
+Example:
+
+rm3100: rm3100@0{
+   compatible = "pni,rm3100-spi";
+   reg = <0>;
+
+   interrupt-parent = <>;
+   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 41f0b97eb933..5bf3395fe9ae 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -288,6 +288,7 @@ pine64  Pine64
  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
  plathome  Plat'Home Co., Ltd.
  plda  PLDA
+pni PNI
  portwell  Portwell Inc.
  poslabPoslab Technology Co., Ltd.
  powervr   PowerVR (deprecated, use img)
diff --git a/MAINTAINERS b/MAINTAINERS
index 967ce8cdd1cc..30ee8cf98312 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11393,6 +11393,16 @@ M: "Rafael J. Wysocki" 
  S:Maintained
  F:drivers/pnp/
  
+PNI RM3100 IIO DRIVER

+M: Song Qiang 
+L: linux-...@vger.kernel.org
+S: Maintained
+F: drivers/iio/magnetometer/rm3100-core.c
+F: drivers/iio/magnetometer/rm3100-i2c.c
+F: drivers/iio/magnetometer/rm3100-spi.c
+F: drivers/iio/magnetometer/rm3100.h
+F: Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
+
  POSIX CLOCKS and TIMERS
  M:Thomas Gleixner 
  L:linux-kernel@vger.kernel.org
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index ed9d776d01af..f130b866a4fc 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
  - hmc5843_core (core functions)
  - hmc5843_spi (support for HMC5983)
  
+config SENSORS_RM3100

+   tristate
+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+
+config SENSORS_RM3100_I2C
+   tristate "PNI RM3100 9-Axis Magnetometer (I2C)"
+   depends on I2C
+   select SENSORS_RM3100
+   

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

On 20/09/2018 9:13 PM, Song Qiang 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.



In the subject: Isn't the RM3100 a 3axis mag.
The 9axis bit comes when you combine it with an accel / gryo I think.

... snip


+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+


... snip


+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+


Looking at other drivers supporting i2c / spi.
They use the same compatible for both.

eg: see iio/accel/adxl345_*.c

and it's binding doc:

Required properties:
 - compatible : should be "adi,adxl345"
 - reg : the I2C address or SPI chip select number of the sensor

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Phil Reid]

On 20/09/2018 9:13 PM, Song Qiang 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.



In the subject: Isn't the RM3100 a 3axis mag.
The 9axis bit comes when you combine it with an accel / gryo I think.

... snip


+++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
@@ -0,0 +1,57 @@
+* PNI RM3100 9-axis magnetometer sensor
+
+I2C Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-i2c"
+- reg : the I2C address of the magnetometer
+


... snip


+SPI Bus:
+
+Required properties:
+
+- compatible : should be "pni,rm3100-spi"
+- reg : address of sensor, usually 0 or 1.
+


Looking at other drivers supporting i2c / spi.
They use the same compatible for both.

eg: see iio/accel/adxl345_*.c

and it's binding doc:

Required properties:
 - compatible : should be "adi,adxl345"
 - reg : the I2C address or SPI chip select number of the sensor

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Thu, Sep 20, 2018 at 03:46:03PM +0200, Peter Meerwald-Stadler wrote:
> On Thu, 20 Sep 2018, Song Qiang 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.
> 
> comments below
> 
> > 
> > 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 
> > ---
> >  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
> >  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
> >  MAINTAINERS   |  10 +
> >  drivers/iio/magnetometer/Kconfig  |  29 ++
> >  drivers/iio/magnetometer/Makefile |   4 +
> >  drivers/iio/magnetometer/rm3100-core.c| 399 ++
> >  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
> >  drivers/iio/magnetometer/rm3100-spi.c |  72 
> >  drivers/iio/magnetometer/rm3100.h |  90 
> >  9 files changed, 728 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> >  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100.h
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> > b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > new file mode 100644
> > index ..d0d2063e943f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > @@ -0,0 +1,57 @@
> > +* PNI RM3100 9-axis magnetometer sensor
> > +
> > +I2C Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-i2c"
> > +- reg : the I2C address of the magnetometer
> > +
> > +Optional properties:
> > +
> > +- interrupts: data ready (DRDY) from the chip.
> > +  The interrupts can be triggered on rising edges.
> > +
> > +   Refer to interrupt-controller/interrupts.txt for generic
> > +   interrupt client node bindings.
> > +
> > +- pinctrl-*: pinctrl setup for DRDY line.
> > +
> > +Example:
> > +
> > +rm3100: rm3100@20 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_pins>;
> > +
> > +   compatible = "pni,rm3100-i2c";
> > +   reg = <0x20>;
> > +   interrupt-parent = <>;
> > +   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > +};
> > +
> > +SPI Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-spi"
> > +- reg : address of sensor, usually 0 or 1.
> > +
> > +Optional properties:
> > +
> > +- interrupts: data ready (DRDY) from the chip.
> > +  The interrupts can be triggered on rising edges.
> > +
> > +   Refer to interrupt-controller/interrupts.txt for generic
> > +   interrupt client node bindings.
> > +
> > +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> 
> depends
> architecture
> 

Hi Peter,

Thanks for spending time with my patch!
Sorry for my English, I'll use a spell checker next time.

> > +
> > +Example:
> > +
> > +rm3100: rm3100@0{
> > +   compatible = "pni,rm3100-spi";
> > +   reg = <0>;
> > +
> > +   interrupt-parent = <>;
> > +   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > +};
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 41f0b97eb933..5bf3395fe9ae 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -288,6 +288,7 @@ pine64  Pine64
> >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> >  plathome   Plat'Home Co., Ltd.
> >  plda   PLDA
> > +pni PNI
> >  portwell   Portwell Inc.
> >  poslab Poslab Technology Co., Ltd.
> >  powervrPowerVR (deprecated, use img)
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 967ce8cdd1cc..30ee8cf98312 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11393,6 +11393,16 @@ M: "Rafael J. Wysocki" 
> >  S: Maintained
> >  F: drivers/pnp/
> >  
> > +PNI RM3100 IIO DRIVER
> > +M: Song Qiang 
> > +L: linux-...@vger.kernel.org
> > +S: Maintained
> > +F: drivers/iio/magnetometer/rm3100-core.c
> > +F: drivers/iio/magnetometer/rm3100-i2c.c
> > +F: drivers/iio/magnetometer/rm3100-spi.c
> > +F: drivers/iio/magnetometer/rm3100.h
> > +F: Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
> > +
> >  POSIX CLOCKS and TIMERS
> >  M: Thomas Gleixner 
> >  L: linux-kernel@vger.kernel.org
> > diff --git a/drivers/iio/magnetometer/Kconfig 
> > b/drivers/iio/magnetometer/Kconfig
> > index 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Song Qiang]

On Thu, Sep 20, 2018 at 03:46:03PM +0200, Peter Meerwald-Stadler wrote:
> On Thu, 20 Sep 2018, Song Qiang 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.
> 
> comments below
> 
> > 
> > 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 
> > ---
> >  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
> >  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
> >  MAINTAINERS   |  10 +
> >  drivers/iio/magnetometer/Kconfig  |  29 ++
> >  drivers/iio/magnetometer/Makefile |   4 +
> >  drivers/iio/magnetometer/rm3100-core.c| 399 ++
> >  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
> >  drivers/iio/magnetometer/rm3100-spi.c |  72 
> >  drivers/iio/magnetometer/rm3100.h |  90 
> >  9 files changed, 728 insertions(+)
> >  create mode 100644 
> > Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> >  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> >  create mode 100644 drivers/iio/magnetometer/rm3100.h
> > 
> > diff --git 
> > a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> > b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > new file mode 100644
> > index ..d0d2063e943f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> > @@ -0,0 +1,57 @@
> > +* PNI RM3100 9-axis magnetometer sensor
> > +
> > +I2C Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-i2c"
> > +- reg : the I2C address of the magnetometer
> > +
> > +Optional properties:
> > +
> > +- interrupts: data ready (DRDY) from the chip.
> > +  The interrupts can be triggered on rising edges.
> > +
> > +   Refer to interrupt-controller/interrupts.txt for generic
> > +   interrupt client node bindings.
> > +
> > +- pinctrl-*: pinctrl setup for DRDY line.
> > +
> > +Example:
> > +
> > +rm3100: rm3100@20 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <_pins>;
> > +
> > +   compatible = "pni,rm3100-i2c";
> > +   reg = <0x20>;
> > +   interrupt-parent = <>;
> > +   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > +};
> > +
> > +SPI Bus:
> > +
> > +Required properties:
> > +
> > +- compatible : should be "pni,rm3100-spi"
> > +- reg : address of sensor, usually 0 or 1.
> > +
> > +Optional properties:
> > +
> > +- interrupts: data ready (DRDY) from the chip.
> > +  The interrupts can be triggered on rising edges.
> > +
> > +   Refer to interrupt-controller/interrupts.txt for generic
> > +   interrupt client node bindings.
> > +
> > +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.
> 
> depends
> architecture
> 

Hi Peter,

Thanks for spending time with my patch!
Sorry for my English, I'll use a spell checker next time.

> > +
> > +Example:
> > +
> > +rm3100: rm3100@0{
> > +   compatible = "pni,rm3100-spi";
> > +   reg = <0>;
> > +
> > +   interrupt-parent = <>;
> > +   interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> > +};
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> > b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > index 41f0b97eb933..5bf3395fe9ae 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> > @@ -288,6 +288,7 @@ pine64  Pine64
> >  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
> >  plathome   Plat'Home Co., Ltd.
> >  plda   PLDA
> > +pni PNI
> >  portwell   Portwell Inc.
> >  poslab Poslab Technology Co., Ltd.
> >  powervrPowerVR (deprecated, use img)
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 967ce8cdd1cc..30ee8cf98312 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11393,6 +11393,16 @@ M: "Rafael J. Wysocki" 
> >  S: Maintained
> >  F: drivers/pnp/
> >  
> > +PNI RM3100 IIO DRIVER
> > +M: Song Qiang 
> > +L: linux-...@vger.kernel.org
> > +S: Maintained
> > +F: drivers/iio/magnetometer/rm3100-core.c
> > +F: drivers/iio/magnetometer/rm3100-i2c.c
> > +F: drivers/iio/magnetometer/rm3100-spi.c
> > +F: drivers/iio/magnetometer/rm3100.h
> > +F: Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
> > +
> >  POSIX CLOCKS and TIMERS
> >  M: Thomas Gleixner 
> >  L: linux-kernel@vger.kernel.org
> > diff --git a/drivers/iio/magnetometer/Kconfig 
> > b/drivers/iio/magnetometer/Kconfig
> > index 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Peter Meerwald-Stadler]

On Thu, 20 Sep 2018, Song Qiang 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.

comments below

> 
> 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 
> ---
>  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
>  MAINTAINERS   |  10 +
>  drivers/iio/magnetometer/Kconfig  |  29 ++
>  drivers/iio/magnetometer/Makefile |   4 +
>  drivers/iio/magnetometer/rm3100-core.c| 399 ++
>  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
>  drivers/iio/magnetometer/rm3100-spi.c |  72 
>  drivers/iio/magnetometer/rm3100.h |  90 
>  9 files changed, 728 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
>  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
>  create mode 100644 drivers/iio/magnetometer/rm3100.h
> 
> diff --git 
> a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> new file mode 100644
> index ..d0d2063e943f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> @@ -0,0 +1,57 @@
> +* PNI RM3100 9-axis magnetometer sensor
> +
> +I2C Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-i2c"
> +- reg : the I2C address of the magnetometer
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line.
> +
> +Example:
> +
> +rm3100: rm3100@20 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> +
> + compatible = "pni,rm3100-i2c";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> +
> +SPI Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-spi"
> +- reg : address of sensor, usually 0 or 1.
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.

depends
architecture

> +
> +Example:
> +
> +rm3100: rm3100@0{
> + compatible = "pni,rm3100-spi";
> + reg = <0>;
> +
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 41f0b97eb933..5bf3395fe9ae 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -288,6 +288,7 @@ pine64Pine64
>  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
>  plathome Plat'Home Co., Ltd.
>  plda PLDA
> +pni PNI
>  portwell Portwell Inc.
>  poslab   Poslab Technology Co., Ltd.
>  powervr  PowerVR (deprecated, use img)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..30ee8cf98312 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
>  S:   Maintained
>  F:   drivers/pnp/
>  
> +PNI RM3100 IIO DRIVER
> +M:   Song Qiang 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/iio/magnetometer/rm3100-core.c
> +F:   drivers/iio/magnetometer/rm3100-i2c.c
> +F:   drivers/iio/magnetometer/rm3100-spi.c
> +F:   drivers/iio/magnetometer/rm3100.h
> +F:   Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
> +
>  POSIX CLOCKS and TIMERS
>  M:   Thomas Gleixner 
>  L:   linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/magnetometer/Kconfig 
> b/drivers/iio/magnetometer/Kconfig
> index ed9d776d01af..f130b866a4fc 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>  
> +config SENSORS_RM3100
> + tristate
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> +
> +config SENSORS_RM3100_I2C
> + tristate "PNI 

Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis magnetometer

[Peter Meerwald-Stadler]

On Thu, 20 Sep 2018, Song Qiang 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.

comments below

> 
> 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 
> ---
>  .../bindings/iio/magnetometer/pni,rm3100.txt  |  57 +++
>  .../devicetree/bindings/vendor-prefixes.txt   |   1 +
>  MAINTAINERS   |  10 +
>  drivers/iio/magnetometer/Kconfig  |  29 ++
>  drivers/iio/magnetometer/Makefile |   4 +
>  drivers/iio/magnetometer/rm3100-core.c| 399 ++
>  drivers/iio/magnetometer/rm3100-i2c.c |  66 +++
>  drivers/iio/magnetometer/rm3100-spi.c |  72 
>  drivers/iio/magnetometer/rm3100.h |  90 
>  9 files changed, 728 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
>  create mode 100644 drivers/iio/magnetometer/rm3100-core.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
>  create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
>  create mode 100644 drivers/iio/magnetometer/rm3100.h
> 
> diff --git 
> a/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt 
> b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> new file mode 100644
> index ..d0d2063e943f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> @@ -0,0 +1,57 @@
> +* PNI RM3100 9-axis magnetometer sensor
> +
> +I2C Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-i2c"
> +- reg : the I2C address of the magnetometer
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line.
> +
> +Example:
> +
> +rm3100: rm3100@20 {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pins>;
> +
> + compatible = "pni,rm3100-i2c";
> + reg = <0x20>;
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> +
> +SPI Bus:
> +
> +Required properties:
> +
> +- compatible : should be "pni,rm3100-spi"
> +- reg : address of sensor, usually 0 or 1.
> +
> +Optional properties:
> +
> +- interrupts: data ready (DRDY) from the chip.
> +  The interrupts can be triggered on rising edges.
> +
> + Refer to interrupt-controller/interrupts.txt for generic
> + interrupt client node bindings.
> +
> +- pinctrl-*: pinctrl setup for DRDY line, depands on archtechture.

depends
architecture

> +
> +Example:
> +
> +rm3100: rm3100@0{
> + compatible = "pni,rm3100-spi";
> + reg = <0>;
> +
> + interrupt-parent = <>;
> + interrupts = <4 IRQ_TYPE_EDGE_RISING>;
> +};
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt 
> b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 41f0b97eb933..5bf3395fe9ae 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -288,6 +288,7 @@ pine64Pine64
>  pixcir  PIXCIR MICROELECTRONICS Co., Ltd
>  plathome Plat'Home Co., Ltd.
>  plda PLDA
> +pni PNI
>  portwell Portwell Inc.
>  poslab   Poslab Technology Co., Ltd.
>  powervr  PowerVR (deprecated, use img)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..30ee8cf98312 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,16 @@ M:   "Rafael J. Wysocki" 
>  S:   Maintained
>  F:   drivers/pnp/
>  
> +PNI RM3100 IIO DRIVER
> +M:   Song Qiang 
> +L:   linux-...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/iio/magnetometer/rm3100-core.c
> +F:   drivers/iio/magnetometer/rm3100-i2c.c
> +F:   drivers/iio/magnetometer/rm3100-spi.c
> +F:   drivers/iio/magnetometer/rm3100.h
> +F:   Documentation/devicetree/bindings/iio/magnetometer/rm3100.txt
> +
>  POSIX CLOCKS and TIMERS
>  M:   Thomas Gleixner 
>  L:   linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/magnetometer/Kconfig 
> b/drivers/iio/magnetometer/Kconfig
> index ed9d776d01af..f130b866a4fc 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>  
> +config SENSORS_RM3100
> + tristate
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> +
> +config SENSORS_RM3100_I2C
> + tristate "PNI