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
>
