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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Sat, 22 Sep 2018 10:42:44 +0100
Jonathan Cameron <jic23@kernel.org> 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 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@gmail.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)
Ah. Just realised. Error path :)

> 
> Jonathan