Hi Andy, On Sun, Jun 1, 2025 at 9:54 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Sun, Jun 1, 2025 at 8:22 PM Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote: > > > > Add AC coupling activity and inactivity as MAG_ADAPTIVE events. This adds > > up an additional set of threshold and period handles, verifies matching > > disabling functionality and extends setting the link bit to complementary > > event configurations. > > > > This means, e.g. either ACTIVITY or ACTIVITY_AC can be enabled. The most > > recent set will remain configured. Disabling ACTIVITY where ACTIVITY_AC was > > enabled is ignored, since it does not match (should be disabling > > ACTIVITY_AC). When INACTIVITY or INACTIVITY_AC is also enabled, the link > > bit will be set. Note, having the link bit and auto-sleep in place activity > > and inactivity indicate the power save state change and thus will only be > > triggered once a state transition occurs. Since there is a separate AC bit > > for ACTIVITY and for INACTIVITY, events can be linked independently from > > each other i.e. ACTIVITY can be linked to INACTIVITY_AC for instance. > > > > When one of both is disabled, the link bit will be removed. Hence, the > > remaining event will not indicate a plain state change anymore, but occur > > as a periodically triggered inactivity event or for each activity event > > above the threshold. > > ... > > > +/** > > + * adxl313_is_act_inact_ac() - Check if AC coupling is enabled. > > > + * > > Unneeded blank line. > > > + * @data: The device data. > > + * @type: The activity or inactivity type. > > + * > > + * Provide a type of activity or inactivity, combined with either AC coupling > > + * set, or default to DC coupling. This function verifies, if the combination is > > + * currently enabled or not. > > + * > > + * Return if the provided activity type has AC coupling enabled or a negative > > + * error value. > > Missing Return section. Always try kernel-doc validation when adding > new kernel-doc descriptions. > > > + */ > > ... > > > unsigned int regval; > > + int coupling; > > Why? Doesn't 'ret' suffice? > the coupling variable here is rather meant to provide kind of a semantic context. It shall be checked for being negative (error), or used in binary decision logic. In fact, could be done with ret as well, but then in case I'd need to comment that in this case the value of 'ret' carries either error, or the bool if we have coupling or not. I'd like to leave it like this, but let me know if better replace it by ret here. > > int axis_en, int_en, ret; > > ... > > > - int act_en, inact_en; > > - bool en; > > + int act_en, inact_en, act_ac_en, inact_ac_en; > > + bool en, act_inact_ac; > > int ret; > > For all your patches: try really hard to avoid the ping-pong coding, > i.e. when you add something in one patch in the series and change in > the other for no reason. I.o.w. when the initial code may be written > already in a form that doesn't need further changes (e.g., switch-case > vs. if). > > This patch is *very* noisy due to the above. So, just slow down, try a > new approach that you have less '-' lines in the diff:s all over the > code. Agree. I tried to follow the review comments. Probably, IMHO it's mostly about how to separate the patches. Your reviews seem to be quite focussed on the particular patch w/o taking the context of follow up patches so much into account. [At least by the way you gave me feed back here. Actually, by your vast experience I'm pretty sure you have the context of how such a driver shall look and have an excellent overview well in mind.] So, I guess you'd like to stress on certain points. I'm wondering if it might probably be better to send you this all first in one big patch, or say rather bigger patches, and then separate pieces out? Let me know what you think. Thank you so much for the reviews, let's see how this can be improved here in a v5. Best, L > > -- > With Best Regards, > Andy Shevchenko
