linux-kernel - Re: [PATCH v9 5/5] iio: light: Add support for APDS9306 Light Sensor

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <e61c6117-e03e-4d1d-8a3a-d4c56918169b@tweaklogic.com>
Date: Mon, 11 Mar 2024 07:22:58 +1030
From: Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Matti Vaittinen <mazziesaccount@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Marek Vasut <marex@...x.de>, Anshul Dalal <anshulusr@...il.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Matt Ranostay <matt@...ostay.sg>,
 Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 5/5] iio: light: Add support for APDS9306 Light Sensor

> 
>> +
>> +static int apds9306_write_event_config(struct iio_dev *indio_dev,
>> +				       const struct iio_chan_spec *chan,
>> +				       enum iio_event_type type,
>> +				       enum iio_event_direction dir,
>> +				       int state)
>> +{
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	struct apds9306_regfields *rf = &data->rf;
>> +	int ret, val;
>> +
>> +	state = !!state;
>> +
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH: {
>> +		guard(mutex)(&data->mutex);
>> +
>> +		/*
>> +		 * If interrupt is enabled, the channel is set before enabling
>> +		 * the interrupt. In case of disable, no need to switch
>> +		 * channels. In case of different channel is selected while
>> +		 * interrupt in on, just change the channel.
>> +		 */
>> +		if (state) {
>> +			if (chan->type == IIO_LIGHT)
>> +				val = 1;
>> +			else if (chan->type == IIO_INTENSITY)
>> +				val = 0;
>> +			else
>> +				return -EINVAL;
>> +
>> +			ret = regmap_field_write(rf->int_src, val);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		ret = regmap_field_read(rf->int_en, &val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (val == state)
>> +			return 0;
>> +
>> +		ret = regmap_field_write(rf->int_en, state);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (state)
>> +			return pm_runtime_resume_and_get(data->dev);
>> +
>> +		pm_runtime_mark_last_busy(data->dev);
>> +		pm_runtime_put_autosuspend(data->dev);
> Note this isn't a reason to do a v10, just a possible suggestion for
> what I think is more readable code.
> 
> Flow here is complex, maybe we'd have been better with skipping the
> state = !!state, rename val to more explicit enabled
> above and something like..
> 
> 		ret = regmap_field_read(rf->int_en, &enabled);
> 		if (ret)
> 			return ret;
> 
> 		if (state) {
> 			if (chan->type == IIO_LIGHT)
> 				ret = regmap_field_write(rf->int_src, 1);
> 			else if (chan->type == IIO_INTENSITY)
> 				ret = regmap_field_write(rf->int_src, 0);
> 			else
> 				return -EINVAL;
> 
> 			if (ret)
> 				return ret;
> 			if (enabled) /* Already enabled */
> 				return 0;		
> 			
> 			ret = regmap_field_write(rf->int_en, 1);
> 			if (ret)
> 				return ret;
> 
> 			return pm_runtime_resume_and_get(data->dev);
> 		} else {  // Could drop this else but I think it's useful to show the either or flow.
> 			if (!enabled)
> 				return 0;		
> 
> 			ret = regmap_field_write(rf->int_en, 0);
> 			if (ret)
> 				return ret;
> 			pm_runtime_mark_last_busy(data->dev);
> 			pm_runtime_put_autosuspend(data->dev);
> 
> 			return 0;
> 		}
> 	}	
Yes, this is much simpler and readable. I will prepare a follow up patch for this.

Thank you for reviewing and applying the series.

Regards,
Subhajit Ghosh
>> +
>> +		return 0;
>> +	}
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		return regmap_field_write(rf->int_thresh_var_en, state);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
>> +
>> +static void apds9306_powerdown(void *ptr)
>> +{
>> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
>> +	struct apds9306_regfields *rf = &data->rf;
>> +	int ret;
>> +
>> +	ret = regmap_field_write(rf->int_thresh_var_en, 0);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = regmap_field_write(rf->int_en, 0);
>> +	if (ret)
>> +		return;
>> +
>> +	apds9306_power_state(data, false);
>> +}
> 
> ...
>