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

[Dmitry Rokosov]

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

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

[...]

-- 
Thank you,
Dmitry