Re: [PATCH v3] iio: accel: adxl345: separate event state from query errors

[Iron Berrington]

Hi Andy,

Thanks for the review. In v4, I'll replace the two breaks in the
coupling query helper with direct returns, and move the error checks
into the rising and falling cases in adxl345_read_mag_config().

Best regards,
Wenqiang


On Mon, 19 Oct 2026 13:20:42 +0300, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
> On Sun, Oct 18, 2026 at 09:42:00PM +0800, Wenqiang Zhao via B4 Relay wrote:
> > From: Wenqiang Zhao <berringtoni36@xxxxxxxxx>
> >
> > The activity/inactivity query helpers return either a boolean result or a
> > negative regmap error. adxl345_read_mag_config() converts the latter to 1
> > with !!, reporting a failed read as an enabled event. The disable path
> > also turns a coupling query error into a successful write.
> >
> > Return query status separately from the boolean result by passing bool
> > output parameters to both helpers. Rename them from is_* to get_* and
> > update the event read, disable and link-bit callers to check the status
> > before using the result. Preserve the successful coupling-mismatch
> > no-op when disabling an event.
>
> ...
>
> >  	switch (type) {
> >  	case ADXL345_ACTIVITY:
> >  	case ADXL345_INACTIVITY:
> > -		return coupling == ADXL345_COUPLING_DC;
> > +		*matches = coupling == ADXL345_COUPLING_DC;
> > +		break;
> >  	case ADXL345_ACTIVITY_AC:
> >  	case ADXL345_INACTIVITY_AC:
> > -		return coupling == ADXL345_COUPLING_AC;
> > +		*matches = coupling == ADXL345_COUPLING_AC;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > +
> > +	return 0;
>
> Replace 'break;':s with the direct 'return 0;':s.
>
> ...
>
> >  static int adxl345_read_mag_config(struct adxl345_state *st,
> >  				   enum adxl345_activity_type type_act,
> >  				   enum adxl345_activity_type type_inact)
> >  {
> > +	bool enabled;
> > +	int ret;
> > +
> >  	switch (dir) {
> >  	case IIO_EV_DIR_RISING:
> > -		ret = adxl345_get_act_inact_en(st, type_act, &enabled);
> > +		ret = adxl345_get_act_inact_en(st, type_act, &enabled);
> >  		break;
>
> It's better to spread
>
> 		if (ret)
> 			return ret;
>
> here...
>
> >  	case IIO_EV_DIR_FALLING:
> > -		ret = adxl345_get_act_inact_en(st, type_inact, &enabled);
> > +		ret = adxl345_get_act_inact_en(st, type_inact, &enabled);
> >  		break;
>
> ...and here...
>
> >  	default:
> >  		return -EINVAL;
> >  	}
>
> > +	if (ret)
> > +		return ret;
> > +
>
> ...and avoid adding this one.
>
> >  	return enabled;
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko