Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Oct 23, 2023 at 12:54:15PM +0000, Balas, Eliza wrote:
> > -----Original Message-----
> > From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Sent: Friday, October 20, 2023 17:32
> > To: Balas, Eliza <Eliza.Balas@analog.com>
> > Cc: linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; Derek Kiernan <derek.kiernan@amd.com>; Dragan
> > Cvetic <dragan.cvetic@amd.com>; Arnd Bergmann <arnd@arndb.de>
> > Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> > 
> > [External]
> > 
> > On Fri, Oct 20, 2023 at 11:18:44AM +0000, Balas, Eliza wrote:
> > > > > +static int adi_axi_tdd_parse_ms(struct adi_axi_tdd_state *st,
> > > > > +				const char *buf,
> > > > > +				u64 *res)
> > > > > +{
> > > > > +	u64 clk_rate = READ_ONCE(st->clk.rate);
> > > > > +	char *orig_str, *modf_str, *int_part, frac_part[7];
> > > > > +	long ival, frac;
> > > > > +	int ret;
> > > > > +
> > > > > +	orig_str = kstrdup(buf, GFP_KERNEL);
> > > > > +	int_part = strsep(&orig_str, ".");
> > > >
> > > > Why are we parsing floating point in the kernel?  Please just keep the
> > > > api simple so that we don't have to try to do any type of parsing other
> > > > than turning a single text number into a value.
> > > >
> > >
> > > The adi_axi_tdd_parse_ms function does almost the same thing as the
> > > iio_str_to_fixpoint() function which already exists in kernel.
> > 
> > That does not mean that this is a valid api for your device as you are
> > not an iio driver (why aren't you an iio driver?)
> > 
> > > It parses a fixed-point number from a string.
> > 
> > And as such, you shouldn't duplicate existing logic.
> > 
> > > The __iio_str_to_fixpoint is used in a similar way, as I intend to use adi_axi_tdd_parse_ms.
> > > It writes to a channel the corresponding scale (micro,nano) for a value.
> > 
> > Why not just have the api accept values in nanoseconds and then no
> > parsing is needed?
> 
> I thought this would be easier for the user, to work with smaller values,
> than using a lot of zeros for nanoseconds. I will make the changes
> to accept values in nanoseconds..

Make the kernel simpler, it's easier to make more complex userspace,
right?

> > > Since the device is not an iio device, using an iio function would be confusing.
> > 
> > Why isn't this an iio device?
> 
> The device is not registered into the IIO device tree, 
> and does not rely on IIO kernel APIs. 
> Even though there are a few attributes that resemble the
> ones from iio, and the sysfs structure is similar,
> this is not an IIO device.
> In the previous patch versions 1 and 2 we concluded
> that this device fits better in the misc subsystem.

Ok, can you point to that in the changelog where the IIO maintainer
agreed that this doesn't fit into that subsystem?

thanks,

greg k-h