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
