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

Hello Jonathan,

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

[...]

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

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

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

[...]

-- 
Thank you,
Dmitry