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

[Balas, Eliza]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Friday, October 20, 2023 14:26
> To: Balas, Eliza <Eliza.Balas@xxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Derek Kiernan <derek.kiernan@xxxxxxx>; Dragan
> Cvetic <dragan.cvetic@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>
> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> 
> [External]
> 
> On 20/10/2023 13:18, Balas, Eliza wrote:
> >> -----Original Message-----
> >> From: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Sent: Thursday, October 19, 2023 20:57
> >> To: Balas, Eliza <Eliza.Balas@xxxxxxxxxx>
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; Derek Kiernan <derek.kiernan@xxxxxxx>; Dragan
> >> Cvetic <dragan.cvetic@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>
> >> Subject: Re: [PATCH v3 2/2] drivers: misc: adi-axi-tdd: Add TDD engine
> >>
> >> [External]
> >>
> >> On Thu, Oct 19, 2023 at 03:56:46PM +0300, Eliza Balas wrote:
> >>> +config ADI_AXI_TDD
> >>> +	tristate "Analog Devices TDD Engine support"
> >>> +	depends on HAS_IOMEM
> >>> +	select REGMAP_MMIO
> >>> +	help
> >>> +	  The ADI AXI TDD core is the reworked and generic TDD engine which
> >>> +	  was developed for general use in Analog Devices HDL projects. Unlike
> >>> +	  the previous TDD engine, this core can only be used standalone mode,
> >>> +	  and is not embedded into other devices.
> >>
> >> What does "previous" mean here?  That's not relevant for a kernel help
> >> text, is it?
> >>

I will redo the config help text to make it clearer.

> >> Also, what is the module name?  Why would someone enable this?  What
> >> userspace tools use it?

I will add a better description. Currently there are no userspace tools
that use the device. This platform driver gives the possibility to access
and configure the device using the sysfs interface.

> >>
> >>> +
> >>>  config DUMMY_IRQ
> >>>  	tristate "Dummy IRQ handler"
> >>>  	help
> >>
> >> Why put your entry in this specific location in the file?

I added the entry in the kconfig file based on the alphabetical order.

> >>
> >>> +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.
> > It parses a fixed-point number from a string.
> > 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.
> >
> > Since the device is not an iio device, using an iio function would be confusing.
> > That is the reason for creating the adi_axi_tdd_parse_ms function, which is easier
> > to understand since I don't have to make all the multiplications that are made
> > in the __iio_str_to_fixpoint function.
> 
> Why did you skip other comments?
> 
 
By mistake, I hit the send button. I added them now.

> >>> +	ret = kstrtol(int_part, 10, &ival);
> >>> +	if (ret || ival < 0)
> >>> +		return -EINVAL;
> >>
> >> You leaked memory :(
> >>
> >> Use the new logic in completion.h to make this simpler?
> >>
> >>> +	modf_str = strsep(&orig_str, ".");
> >>>+	if (modf_str) {
> >>>+		snprintf(frac_part, 7, "%s00000", modf_str);
> >>>+		ret = kstrtol(frac_part, 10, &frac);
> >>>+		if (ret)
> >>>+			return -EINVAL;
> >>
> >> You leaked memory :(
> >>
> >>>+	} else {
> >>>+		frac = 0;
> >>>+	}
> >>>+
> >>>+	*res = DIV_ROUND_CLOSEST_ULL((u64)ival * clk_rate, 1000)
> >>>+		+ DIV_ROUND_CLOSEST_ULL((u64)frac * clk_rate, 1000000000);
> >>
> >> Why isn't userspace doing this calculation?

The string contains a fixed-point number for a value in milliseconds,
but the value must be written in the register in clock cycles. 
The clock rate may change, and we must transform the value in
clock cycles, so this is the reason for this calculation here.

> >>
> >> I stopped reviewing here, sorry.
> >>
> >> greg k-h

Thank you,
Eliza