Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

[Jonathan Cameron]

On Tue, 9 Aug 2022 09:47:54 +0000
Dmitry Rokosov <DDRokosov@xxxxxxxxxxxxxx> wrote:

> Hello Jonathan,
> 
> On Sat, Aug 06, 2022 at 04:32:04PM +0100, Jonathan Cameron wrote:
> 
> [...]
> 
> > > +/**
> > > + * struct msa311_priv - MSA311 internal private state
> > > + * @regs: Underlying I2C bus adapter used to abstract slave
> > > + *        register accesses
> > > + * @fields: Abstract objects for each registers fields access
> > > + * @dev: Device handler associated with appropriate bus client
> > > + * @lock: Protects msa311 device state between setup and data access routines
> > > + *        (power transitions, samp_freq/scale tune, retrieving axes data, etc)
> > > + * @new_data_trig: Optional NEW_DATA interrupt driven trigger used
> > > + *                 to notify external consumers a new sample is ready
> > > + * @vdd: Optional external voltage regulator for the device power supply
> > > + */
> > > +struct msa311_priv {
> > > +	struct regmap *regs;
> > > +	struct regmap_field *fields[F_MAX_FIELDS];
> > > +
> > > +	struct device *dev;
> > > +	struct mutex lock; /* state guard */  
> > 
> > Shouldn't need this comment given documentation above that provides
> > more information.  
> 
> Without this comment checkpatch.pl raises a warning about uncommented
> lock definition.
> I agree with you, above comment is redundant, but is it okay to ignore
> such warnings before sending the patch?
> 
> I'm talking about below checkpatch condition:
> =====
> # check for spinlock_t definitions without a comment.
> 		if ($line =~ /^.\s*(struct\s+mutex|spinlock_t)\s+\S+;/ ||
> 		    $line =~ /^.\s*(DEFINE_MUTEX)\s*\(/) {
> 			my $which = $1;
> 			if (!ctx_has_comment($first_line, $linenr)) {
> 				CHK("UNCOMMENTED_DEFINITION",
> 				    "$1 definition without comment\n" . $herecurr);
> 			}
> 		}
> =====

Hmm. I guess checkpatch is more stupid than I thought on this. Definitely
fine to ignore that shortcoming of checkpatch.

> 
> >   
> > > +
> > > +	struct iio_trigger *new_data_trig;
> > > +	struct regulator *vdd;
> > > +};
> > >  
> > 
> >   
> > > +static irqreturn_t msa311_irq_thread(int irq, void *p)
> > > +{
> > > +	struct msa311_priv *msa311 = iio_priv(p);
> > > +	unsigned int new_data_int_enabled;
> > > +	struct device *dev = msa311->dev;
> > > +	int err;
> > > +
> > > +	mutex_lock(&msa311->lock);  
> >   
> > > +
> > > +	/*
> > > +	 * We do not check NEW_DATA int status, because of based on
> > > +	 * specification it's cleared automatically after a fixed time.
> > > +	 * So just check that is enabled by driver logic.  
> > 
> > That is going to be very problematic if we can have this and events coming
> > through the same interrupt pin.  Not harmful for now though given you are
> > only supporting NEW_DATA for now.  Just something to watch out for.
> >   
> 
> Actually, I have run some experiments with NEW_DATA status bits. And
> looks like we can't determince actual status of NEW_DATA virtual
> interrupt when physical IRQ is raised. I will back to this problem when
> begin Motion Events feature implementation.
> 
> [...]
> 
> > > +	err = devm_pm_runtime_enable(dev);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	pm_runtime_get_noresume(dev);
> > > +	pm_runtime_set_autosuspend_delay(dev, MSA311_PWR_SLEEP_DELAY_MS);
> > > +	pm_runtime_use_autosuspend(dev);
> > > +
> > > +	err = msa311_chip_init(msa311);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	indio_dev->modes = 0; /* setup buffered mode later */  
> > 
> > As per other branch, I led you astray here it seems.
> >   
> 
> Sorry, I've made a mistake. Comment about INDIO_DIRECT_MODE was left
> by Andy here:
> 
> https://lore.kernel.org/linux-iio/CAHp75Vc0+ckNnm2tzLMPrjeFRjwoj3zy0C4koNShFRG3kP8b6w@xxxxxxxxxxxxxx/
> 
> [...]

No problem. That's an odd historical corner case.   I liked having
clear values for 'currentmode' (now in iio_opaque) and those matching
the bits set in available modes.  So even if we didn't set it we'd end up
with a reserved bit which would add extra confusion.

The direct mode is currently just used as a placeholder for 'not a buffered mode',
rather than the state variable has never been set.

Jonathan

>