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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250211191900.36fdd4d9@jic23-huawei>
Date: Tue, 11 Feb 2025 19:19:00 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Nuno Sa
 <nuno.sa@...log.com>, David Lechner <dlechner@...libre.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC


> >> +
> >> +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
> 
>