linux-kernel - Re: [PATCH 9/9] iio: adc: ad7606: Add support for writing registers when using backend

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: <69352c32-56fb-4bfe-aead-4126e144a1e6@baylibre.com>
Date: Tue, 26 Nov 2024 13:13:00 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>, Guillaume Stols <gstols@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sa <nuno.sa@...log.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, jstephan@...libre.com, aardelean@...libre.com,
 adureghello@...libre.com
Subject: Re: [PATCH 9/9] iio: adc: ad7606: Add support for writing registers
 when using backend

On 11/26/24 12:48 PM, Jonathan Cameron wrote:
> On Thu, 21 Nov 2024 10:18:31 +0000
> Guillaume Stols <gstols@...libre.com> wrote:
> 
>> Adds the logic for effectively enabling the software mode for the
>> iio-backend, i.e enabling the software mode channel configuration and
>> implementing the register writing functions.
>>
>> Signed-off-by: Guillaume Stols <gstols@...libre.com>
> 
> A few comments inline, but basically looks fine to me.
> 
> Thanks,
> 
> Jonathan
> 
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index a25182a3daa7..0c1177f436f3 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
> 
>>  static int ad7606_bi_update_scan_mode(struct iio_dev *indio_dev, const unsigned long *scan_mask)
>>  {
>>  	struct ad7606_state *st = iio_priv(indio_dev);
>> @@ -70,7 +83,7 @@ static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio
>>  	if (ret)
>>  		return ret;
>>  
>> -	ret = devm_iio_backend_enable(dev, st->back);
>> +	ret = devm_iio_backend_enable(st->dev, st->back);
> 
> Is that a different dev? That's not obvious...
> 
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -86,9 +99,52 @@ static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio
>>  	return 0;
>>  }
>>  
>> +static int ad7606_bi_reg_read(struct iio_dev *indio_dev, unsigned int addr)
>> +{
>> +	struct ad7606_state *st = iio_priv(indio_dev);
>> +	int val, ret;
>> +	struct ad7606_platform_data *pdata =  st->dev->platform_data;
>> +
>> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>> +		ret = pdata->bus_reg_read(st->back,
>> +					addr,
>> +					&val);
> 
> As below.
> 
>> +	}
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return val;
>> +}
>> +
>> +static int ad7606_bi_reg_write(struct iio_dev *indio_dev,
>> +			       unsigned int addr,
>> +			       unsigned int val)
>> +{
>> +	struct ad7606_state *st = iio_priv(indio_dev);
>> +	struct ad7606_platform_data *pdata =  st->dev->platform_data;
>> +	int ret;
>> +
>> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> 
> Given David's if_not_cond_guard() should land shortly I'd prefer
> to use that going forwards for cases like this.

Well, Torvalds wasn't happy with the patch and suggested we should
give up on trying to do conditional guards altogether in cleanup.h.

[1]: https://lore.kernel.org/all/CAHk-=whn07tnDosPfn+UcAtWHBcLg=KqA16SHVv0GV4t8P1fHw@mail.gmail.com/

So I'm tempted to just revert the if_not_cond_guard() patch rather
than trying to fix it.

> 
>> +	ret = pdata->bus_reg_write(st->back,
>> +					addr,
>> +					val);
> Put parameters all on one line.
> + return here (which needs the new if_not_cond_guard() to avoid
> confusing the compiler).
> 
>> +	}
>> +	return ret;
>> +}