Message-ID: <20180926013353.GA6152@Eros>
Date:   Wed, 26 Sep 2018 09:33:53 +0800
From:   Song Qiang <songqiang1304521@...il.com>
To:     Jonathan Cameron <jonathan.cameron@...wei.com>
Cc:     jic23@...nel.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, robh+dt@...nel.org, mark.rutland@....com,
        preid@...ctromag.com.au, rtresidd@...ctromag.com.au,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

On Tue, Sep 25, 2018 at 02:30:54PM +0100, Jonathan Cameron wrote:
> On Tue, 25 Sep 2018 11:17:24 +0800
> Song Qiang <songqiang1304521@...il.com> wrote:
> 
> > PNI RM3100 is a high resolution, large signal immunity magnetometer,
> > composed of 3 single sensors and a processing chip with MagI2C Interface.
> > 
> > Following functions are available:
> > - Single-shot measurement from
> >   /sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> > - Triggerd buffer measurement.
> > - Both i2c and spi interface are supported.
> > - Both interrupt and polling measurement is supported, depands on if
> >   the 'interrupts' in DT is declared.
> > 
> > Signed-off-by: Song Qiang <songqiang1304521@...il.com>
> Various minor comments inline.  Definitely heading in the right direction.
> 
> Good work.
> 
> Thanks,
> 
> Jonathan
> 

...

> > +static int rm3100_get_samp_freq(struct rm3100_data *data, int *val, int *val2)
> > +{
> > +	int ret;
> > +	int tmp;
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = regmap_read(data->regmap, RM3100_REG_TMRC, &tmp);
> > +	mutex_unlock(&data->lock);
> > +	if (ret < 0)
> > +		return ret;
> > +	*val = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][0];
> > +	*val2 = rm3100_samp_rates[tmp - RM3100_TMRC_OFFSET][1];
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int rm3100_set_cycle_count(struct rm3100_data *data, int val)
> > +{
> > +	int ret;
> > +	u8 i;
> > +
> > +	for (i = 0; i < 3; i++) {
> > +		ret = regmap_write(data->regmap, RM3100_REG_CC_X + 2 * i, 100);
> 
> Does this write it to the same value irrespective of the value of val?
> Seems odd.
> 
Hi Jonathan,

Sometimes I see these mistakes I couldn't stop laughing at my stupiness.
Should be val, my mistake submitting without a throughout functional
check, I think I'll write a script to prevent this from happening again.

> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +	if (val == 50)
> 
> Switch would be nicer.  Also why have all other values call back to
> 2 rather than generating an error?
> 
I could have given it an error path, but I checked the code, 'val' in
this function is always given by my code. I think this leads no
possibility for val to go wrong. Do I still need an error path in this
circumstance?

> > +		data->cycle_count_index = 0;
> > +	else if (val == 100)
> > +		data->cycle_count_index = 1;
> > +	else
> > +		data->cycle_count_index = 2;
> > +
> > +	return 0;
> > +}
> > +
> > +static int rm3100_set_samp_freq(struct rm3100_data *data, int val, int val2)
> > +{
> > +	struct regmap *regmap = data->regmap;
> > +	int cycle_count;
> > +	int ret;
> > +	int i;
> > +
> > +	mutex_lock(&data->lock);
> 
> Why are you taking this lock? It's not well documented what it protects
> so it's not clear in this path why it is taken.
> (hint add a comment to the definition of the lock to make it clear!)
> - note I'm not saying it shouldn't be taken here, just that there is no
> comment to justify it..
> 
When I was considering the lock, I read Robert Love's 'Linux Kernel
Development', whose chapter 9 talks about how to understand the lock.
It was talking about protecting data, but in here, my purpose is to
protect the i2c execution consistency. I think it may be possible that
one person is extracting the data while another person is planning set
the frequency. Setting the frequency within queuing data will causing
CMM(Continuous Measurement Mode) register reset, breaking the measurement
process. That's why I added lock to callbacks where a i2c sequence can
happen. I referred code from hmc5843, which did the similar lock stuff,
am I understanding about lock right?

> > +	/* All cycle count registers use the same value. */
> > +	ret = regmap_read(regmap, RM3100_REG_CC_X, &cycle_count);
> > +	if (ret < 0)
> > +		goto unlock_return;
> > +
> > +	for (i = 0; i < RM3100_SAMP_NUM; i++) {
> > +		if (val == rm3100_samp_rates[i][0] &&
> > +		    val2 == rm3100_samp_rates[i][1])
> > +			break;
> > +	}
> > +	if (i == RM3100_SAMP_NUM) {
> > +		ret = -EINVAL;
> > +		goto unlock_return;
> > +	}
> > +
> > +	ret = regmap_write(regmap, RM3100_REG_TMRC, i + RM3100_TMRC_OFFSET);
> > +	if (ret < 0)
> > +		goto unlock_return;

...

> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
> > +	for (i = 0; i < 3; i++)
> > +		memcpy(data->buffer + i * 4, buffer + i * 3, 3);
> 
> Firstly X doesn't need copying.
> Secondly the copy of Y actually overwrites the value of Z
> XXXYYYZZZxxx
> XXXxYYYZZxxx
> XXXxYYYxYZZx
> 
> I think...
> 
They are in different memories, buffer is a temporary memory for data
reading out, data->buffer is for pushing to the userspace.

> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > +			iio_get_time_ns(indio_dev));
> 
> Align with opening bracket. 
> 
> > +
> > +done:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static void rm3100_remove(void *dh)
> > +{
> > +	struct rm3100_data *data = (struct rm3100_data *)dh;
> > +	int ret;
> > +
> > +	ret = regmap_write(data->regmap, RM3100_REG_CMM, 0x00);
> > +	if (ret < 0)
> > +		dev_err(data->dev, "failed to stop device.\n");
> > +}
> > +
> > +int rm3100_common_probe(struct device *dev, struct regmap *regmap, int irq)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct rm3100_data *data;
> > +	int tmp;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	data->dev = dev;
> > +	data->regmap = regmap;
> > +	data->buffer = devm_kzalloc(dev, RM3100_SCAN_BYTES, GFP_KERNEL);
> > +	if (!data->buffer)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	indio_dev->dev.parent = dev;
> > +	indio_dev->name = "rm3100";
> > +	indio_dev->info = &rm3100_info;
> > +	indio_dev->channels = rm3100_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(rm3100_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->available_scan_masks = rm3100_scan_masks;
> > +
> > +	if (!irq)
> > +		data->use_interrupt = false;
> > +	else {
> > +		data->use_interrupt = true;
> > +		ret = devm_request_irq(dev,
> > +				       irq,
> > +				       rm3100_irq_handler,
> > +				       IRQF_TRIGGER_RISING,
> > +				       indio_dev->name,
> > +				       data);
> > +		if (ret < 0) {
> > +			dev_err(dev, "request irq line failed.\n");
> > +			return ret;
> > +		}
> > +		init_completion(&data->measuring_done);
> > +	}
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > +					      rm3100_trigger_handler, NULL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* 3sec more wait time. */
> 
> I have no idea what this comment is referring to..
> Not seeing any waiting here...
> 
The thing is, measurement time of this sensor span from ~1.7ms to ~13
seconds. I'll need a wait time for *_wait_measurement(), and I use
measurement time + 3 seconds for waiting time. This is an initialization.
Usage of this is in *_wait_measurement.

yours,
Song Qiang

> > +	ret = regmap_read(regmap, RM3100_REG_TMRC, &tmp);
> > +	if (ret < 0)
> > +		return ret;
> > +	data->conversion_time =
> > +		rm3100_samp_rates[tmp-RM3100_TMRC_OFFSET][2] + 3000;
> 
> spacing around -  
> Please check this throughout.

...

Powered by blists - more mailing lists