Message-ID: <20180922104244.651c802d@archlinux>
Date:   Sat, 22 Sep 2018 10:42:44 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Song Qiang <songqiang1304521@...il.com>
Cc:     Peter Meerwald-Stadler <pmeerw@...erw.net>, lars@...afoo.de,
        knaack.h@....de, robh+dt@...nel.org, mark.rutland@....com,
        devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: magnetometer: Add support for PNI RM3100 9-axis
 magnetometer

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 000000000000..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 <songqiang1304521@...il.com>
> > > + *
> > > + * User Manual available at
> > > + * <https://www.pnicorp.com/download/rm3100-user-manual/>
> > > + *
> > > + * 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 measurement frequency, and
> they are used for wait time, I thought a little longer is better.
> I think the comment above this structure isn't very clear, I'll find a
> better way to explain it.
Please also use kernel standard comment syntax

/*
 * Frequency...
 */

> 
...
> > > +	if (i != RM_SAMP_NUM) {
> > > +		mutex_lock(&data->lock);
> > > +		ret = regmap_write(regmap, RM_REG_TMRC, i + RM_TMRC_OFFSET);
> > > +		if (ret < 0)  
> > 
> > unlock?
> >   
> 
> These actions are for changing the sampling frequency. This device
> cannot start conversion if CMM register is not reset after reading from
> CCX/CCY/CCZ registers. So I unlock it later since conversion should have
> already been stopped and other threads should not access the bus.
Hmm.  If you are holding the lock across function calls you need
to look at lockdep annotations. 

Also, I suspect something is wrong here as you are unlocking in
the good path but not the bad one which seems unlikely to be 
as intended...

> 
> > > +			return ret;
> > > +
> > > +		/* Checking if cycle count registers need changing. */
> > > +		if (val == 600 && cycle_count == 200) {
> > > +			for (i = 0; i < 3; i++) {
> > > +				regmap_write(regmap, RM_REG_CCXL + 2 * i, 100);
> > > +				if (ret < 0)  
> > 
> > unlock?
> >   
> > > +					return ret;
> > > +			}
> > > +		} else if (val != 600 && cycle_count == 100) {
> > > +			for (i = 0; i < 3; i++) {
> > > +				regmap_write(regmap, RM_REG_CCXL + 2 * i, 200);
> > > +				if (ret < 0)  
> > 
> > unlock?
> >   
> > > +					return ret;
> > > +			}
> > > +		}
> > > +		/* Writing TMRC registers requires CMM reset. */
> > > +		ret = regmap_write(regmap, RM_REG_CMM, 0);
> > > +		if (ret < 0)  
> > 
> > unlock?
> >   
> > > +			return ret;
> > > +		ret = regmap_write(regmap, RM_REG_CMM, RM_CMM_PMX |
> > > +			RM_CMM_PMY | RM_CMM_PMZ | RM_CMM_START);
> > > +		if (ret < 0)  
> > 
> > unlock?
> >   
> > > +			return ret;
> > > +		mutex_unlock(&data->lock);
> > > +
> > > +		data->conversion_time = rm3100_samp_rates[i][2] + 3000;
> > > +		return 0;
> > > +	}
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int rm3100_read_raw(struct iio_dev *indio_dev,
> > > +			   const struct iio_chan_spec *chan,
> > > +			   int *val, int *val2, long mask)
> > > +{
> > > +	struct rm3100_data *data = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = iio_device_claim_direct_mode(indio_dev);
> > > +		if (ret < 0)  
> > 
> > release_direct_mode() here?
> >   
> 
> Oh..yes!

This should have stopped you reading more than once so I'm surprised
this slipped through.  I guess the usual last minute change problem ;)
(we all do it however much we know we should retest properly)

Jonathan

Powered by blists - more mailing lists