linux-kernel - Re: [PATCH v3 2/2] iio: frequency: adf4350: add clk provider

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: <20240612-traverse-scion-1c5fd444f9c2@spud>
Date: Wed, 12 Jun 2024 13:07:07 +0100
From: Conor Dooley <conor@...nel.org>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Antoniu Miclaus <antoniu.miclaus@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: frequency: adf4350: add clk provider

On Wed, Jun 12, 2024 at 01:54:07PM +0200, Nuno Sá wrote:
> Hi Antonium
> 
> On Wed, 2024-06-12 at 13:45 +0300, Antoniu Miclaus wrote:
> > Add clk provider feature for the adf4350.
> > 
> > Even though the driver was sent as an IIO driver in most cases the
> > device is actually seen as a clock provider.
> > 
> > This patch aims to cover actual usecases requested by users in order to
> > completely control the output frequencies from userspace.
> > 
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > ---
> > changes in v3:
> >  - use container_of to directly access the adf4350_state structure.
> >  drivers/iio/frequency/adf4350.c | 118 ++++++++++++++++++++++++++++++++
> >  1 file changed, 118 insertions(+)
> > 
> > diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> > index 4abf80f75ef5..f716f744baa9 100644
> > --- a/drivers/iio/frequency/adf4350.c
> > +++ b/drivers/iio/frequency/adf4350.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <asm/div64.h>
> >  #include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > @@ -36,6 +37,9 @@ struct adf4350_state {
> >  	struct gpio_desc		*lock_detect_gpiod;
> >  	struct adf4350_platform_data	*pdata;
> >  	struct clk			*clk;
> > +	struct clk			*clkout;
> > +	const char			*clk_out_name;
> > +	struct clk_hw			hw;
> >  	unsigned long			clkin;
> >  	unsigned long			chspc; /* Channel Spacing */
> >  	unsigned long			fpfd; /* Phase Frequency Detector */
> > @@ -61,6 +65,8 @@ struct adf4350_state {
> >  	__be32				val __aligned(IIO_DMA_MINALIGN);
> >  };
> >  
> > +#define to_state(_hw) container_of(_hw, struct adf4350_state, hw)
> > +
> 
> nit: to_adf4350_state() would be neater...
> 
> >  static struct adf4350_platform_data default_pdata = {
> >  	.channel_spacing = 10000,
> >  	.r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
> > @@ -264,6 +270,10 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
> >  	mutex_lock(&st->lock);
> >  	switch ((u32)private) {
> >  	case ADF4350_FREQ:
> > +		if (st->clkout) {
> > +			ret = clk_set_rate(st->clkout, readin);
> > +			break;
> > +		}
> 
> So, apparently you forgot or decided otherwise to not go with the suggestion of
> not including the IIO interface (at least he channel one - debugfs could be
> maintained I guess) or with the more in the middle approach Michael suggested.
> Just not allowing ADF4350_FREQ and ADF4350_FREQ_REFIN.
> 
> Hence, I would expect at least some justification to keep the above in your v3
> changelog. Also note that keeping ADF4350_FREQ_REFIN while being a clock
> provider seems pointless and maybe even be wrong (as the clock framework should
> take care of the parent clock). This also brings another question... see below
> 

I think also there was a request to include the clk list and
maintainers. It's possible that Stephen might want something like the
auxbus to be used here so that the clock code ends up in the clock
subsystem.

Thanks,
Conor.


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)