On Tue, 9 Aug 2022 09:47:54 +0000
Dmitry Rokosov [off-list ref] wrote:

Hello Jonathan,

On Sat, Aug 06, 2022 at 04:32:04PM +0100, Jonathan Cameron wrote:

[...]

quoted

quoted

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

quoted

quoted

+
+	struct iio_trigger *new_data_trig;
+	struct regulator *vdd;
+};
 

quoted

+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);  

quoted

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

[...]

quoted

quoted

+	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@mail.gmail.com/ (local)

[...]

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