linux-kernel - Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features

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: <d9e4852f4ff935c151e31c938ff5f00335437713.camel@gmail.com>
Date: Fri, 06 Sep 2024 07:53:05 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Angelo Dureghello <adureghello@...libre.com>, Jonathan Cameron
	 <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>,  Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,  Olivier Moysan
 <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org,  linux-kernel@...r.kernel.org,
 dlechner@...libre.com
Subject: Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features

On Thu, 2024-09-05 at 14:11 +0200, Angelo Dureghello wrote:
> Hi,
> 
> sorry forgot to reply about the regmap,
> 
> On 05/09/24 12:49 PM, Nuno Sá wrote:
> > On Tue, 2024-09-03 at 20:16 +0100, Jonathan Cameron wrote:
> > > On Mon, 2 Sep 2024 18:04:51 +0200
> > > Angelo Dureghello <adureghello@...libre.com> wrote:
> > > 
> > > > On 31/08/24 1:34 PM, Jonathan Cameron wrote:
> > > > > On Thu, 29 Aug 2024 14:32:01 +0200
> > > > > Angelo Dureghello <adureghello@...libre.com> wrote:
> > > > >   
> > > > > > From: Angelo Dureghello <adureghello@...libre.com>
> > > > > > 
> > > > > > Extend DAC backend with new features required for the AXI driver
> > > > > > version for the a3552r DAC.
> > > > > > 
> > > > > > Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> > > > > Hi Angelo
> > > > > Minor comments inline.
> > > > > >    
> > > > > >    static int axi_dac_enable(struct iio_backend *back)
> > > > > > @@ -460,7 +493,13 @@ static int axi_dac_data_source_set(struct
> > > > > > iio_backend *back, unsigned int chan,
> > > > > >    	case IIO_BACKEND_EXTERNAL:
> > > > > >    		return regmap_update_bits(st->regmap,
> > > > > >    					
> > > > > > AXI_DAC_REG_CHAN_CNTRL_7(chan),
> > > > > > -					  AXI_DAC_DATA_SEL,
> > > > > > AXI_DAC_DATA_DMA);
> > > > > > +					  AXI_DAC_DATA_SEL,
> > > > > > +					  AXI_DAC_DATA_DMA);
> > > > > Unrelated change.   If you want to change this, separate patch.
> > > > Thanks, fixed.
> > > > >   
> > > > > > +	case IIO_BACKEND_INTERNAL_RAMP_16:
> > > > > > +		return regmap_update_bits(st->regmap,
> > > > > > +					
> > > > > > AXI_DAC_REG_CHAN_CNTRL_7(chan),
> > > > > > +					  AXI_DAC_DATA_SEL,
> > > > > > +					
> > > > > > AXI_DAC_DATA_INTERNAL_RAMP_16);
> > > > > >    	default:
> > > > > >    		return -EINVAL;
> > > > > >    	}
> > > > > > @@ -518,9 +557,204 @@ static int axi_dac_reg_access(struct iio_backend
> > > > > > *back, unsigned int reg,
> > > > > >    	return regmap_write(st->regmap, reg, writeval);
> > > > > >    }
> > > > > >    
> > > > > > +
> > > > > > +static int axi_dac_bus_reg_write(struct iio_backend *back,
> > > > > > +				 u32 reg, void *val, size_t size)
> > > > > Maybe just pass an unsigned int for val?
> > > > > So follow what regmap does? You will still need the size, but it
> > > > > will just be configuration related rather than affecting the type
> > > > > of val.
> > > > >   
> > > > void * was used since data size in the future may vary depending
> > > > on the bus physical interface.
> > > > 
> > > I doubt it will get bigger than u64.  Passing void * is always
> > > nasty if we can do something else and this is a register writing
> > > operation.  I'm yet to meet an ADC or similar with > 64 bit registers
> > > (or even one with 64 bit ones!)
> > I think the original thinking was to support thinks like appending crc to the
> > register read/write. But even in that case, u32 for val might be enough. Not
> > sure. Anyways, as you often say with the backend stuff, this is all in the
> > kernel so I guess we can change it to unsigned int and change it in the future
> > if we need to.
> > 
> > Since you mentioned regmap, I also want to bring something that was discussed
> > before the RFC. Basically we talked about having the backend registering it's
> > own regmap_bus. Then we would either:
> > 
> > 1) Have a specific get_regmap_bus() callback for the frontend to initialize a
> > regmap on;
> > 2) Pass this bus into the core and have a new frontend API like
> > devm_iio_backend_regmap_init().
> > 
> > Then, on top of the API already provided by regmap (like _update_bit()), the
> > frontend could just use regmap independent of having a backend or not.
> > 
> > The current API is likely more generic but tbh (and David and Angelo are aware
> > of it) my preferred approach it to use the regmap_bus stuff. I just don't feel
> > that strong about it :)
> 
> regmap idea seems really nice and a better style.
> 
> Honestly, if possible, would not go for it right now.
> The main reason is that i am on this work from months and it would 
> require a quite
> big rework (also rearranging more common code, retest, etc) while i am 
> trying to
> finalize a first driver.
> 

While I understand your reasoning, I can't really agree with it if we feel regmap is
the better solution. It makes no sense to add something knowing that it will removed
in the next couple of weeks. Actually (and I'm guilty of that too :)), when we say
things like that, odds are we're just leaving things like this.

> If you agree, this could come in a second "cleanup" patchset, but at 
> least i can
> provide an initial support for ad3552r.
> 

Having said the above, I'm not going to NAK this approach even if it's not my
favorite one :)

- Nuno Sá
> > >