Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron <jic23@kernel.org>]

> >> +
> >> +static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
> >> +{
> >> +	int ret, evbit = BIT(IIO_EV_DIR_RISING);
> >> +
> >> +	if (!(data->alarm_suppressed[channel] & evbit))
> >> +		return;
> >> +
> >> +	data->alarm_suppressed[channel] &= (~evbit);
> >> +
> >> +	if (!(data->alarm_monitored[channel] & evbit))
> >> +		return;  
> > This lot is very similar to the lo variant. Can we combine them or
> > use some helper for both?  
> 
> Initially I did this.
> 
> But the code looked a bit dull because the evbitm, alarm-limit array and 
> prints depend on the direction. Furthermore, the caller already knows 
> the direction (as the caller does also handle directions separately), so 
> doing:
> 
> foo(dir)
> {
> 	if (dir == bar)
> 		...
> 	else
> 		...
> }
> 
> ...
> 
> if (dir == bar)
> 	foo(dir);
> else
> 	foo(dir);
> 
> started to feel just a bit, meh. Hence I separated the stuff to own _lo 
> and _hi functions.
Hmm. I was thinking of something like

static void bd79124_re_enable_xx(struct bd79124_data *data, unsigned int channel,
				 unsigned int reg, u16 limit,
				 enum iio_event_dir dir)
{
	int ret, evbit = BIT(dir);

	if (!(data->alarm_suppressed[channel] & evbit))
		return;

	data->alarm_suppressed[channel] &= (~evbit);

	if (!(data->alarm_monitored[channel] & evbit))
		return;

	ret = bd79124_write_int_to_reg(data, reg, limit);
	if (ret)
		dev_warn(data->dev, "Low limit enabling failed for channel%d\n",
			 channel);
}

static void bd71924_reenable_hi(struct bd79124_data *data, unsigned int channel)
{
	return bd71924_reenable_x(data, channel, BD79124_GET_HIGH_LIMIT_REG(channel)
				  data->alarm_r_limit[channel], IIO_EV_DIR_RISING);
}

static void bd71924_reenable_lo(struct bd79124_data *data, unsigned int channel)
{
	return bd71924_reenable_x(data, channel, BD79124_GET_LOW_LIMIT_REG(channel)
				  data->alarm_f_limit[channel], IIO_EV_DIR_FALLING);
}

But I guess not really a saving in the end.

> 
> 
> >> +
> >> +	ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel),
> >> +				       data->alarm_r_limit[channel]);
> >> +	if (ret)
> >> +		dev_warn(data->dev, "High limit enabling failed for channel%d\n",
> >> +			 channel);
> >> +}  
> >   
> 
> Yours,
>    -- Matti
> 
>