Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0fc2ba5d-8357-6dfb-4aa4-26de6b497c13@linaro.org>
Date:   Wed, 26 Jul 2023 20:39:44 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/2] drivers: misc: adi-axi-tdd: Add new TDD engine driver
Content-Language: en-US
To:     Eliza Balas <eliza.balas@analog.com>
Cc:     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>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20230726071103.12172-1-eliza.balas@analog.com>
 <20230726071103.12172-2-eliza.balas@analog.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230726071103.12172-2-eliza.balas@analog.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 26/07/2023 09:11, Eliza Balas wrote:
> This patch introduces the driver for the new ADI TDD engine HDL.
> The generic TDD controller is in essence a waveform generator
> capable of addressing RF applications which require Time Division
> Duplexing, as well as controlling other modules of general
> applications through its dedicated 32 channel outputs.
> 
> The reason of creating the generic TDD controller was to reduce
> the naming confusion around the existing repurposed TDD core
> built for AD9361, as well as expanding its number of output
> channels for systems which require more than six controlling signals.
> 
> Signed-off-by: Eliza Balas <eliza.balas@analog.com>
> ---
>  .../sysfs-bus-platform-drivers-adi-axi-tdd    | 158 ++++
>  MAINTAINERS                                   |   2 +
>  drivers/misc/Kconfig                          |  10 +
>  drivers/misc/Makefile                         |   1 +
>  drivers/misc/adi-axi-tdd.c                    | 753 ++++++++++++++++++
>  5 files changed, 924 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
>  create mode 100644 drivers/misc/adi-axi-tdd.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd b/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
> new file mode 100644
> index 000000000000..eb5f3db7d0cb
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-platform-drivers-adi-axi-tdd
> @@ -0,0 +1,158 @@
> +What:           /sys/bus/platform/drivers/adi-axi-tdd/*/burst_count
> +Date:           July 2023
> +KernelVersion:  6.5

We are in 6.5 now, so there is no way your driver will be in 6.5. Target
6.6 and use phb crystall ball for next release date (September).

...

> +
> +enum adi_axi_tdd_attribute_id {
> +	ADI_TDD_ATTR_VERSION,
> +	ADI_TDD_ATTR_CORE_ID,
> +	ADI_TDD_ATTR_SCRATCH,
> +	ADI_TDD_ATTR_MAGIC,
> +

...

> +
> +static int adi_axi_tdd_probe(struct platform_device *pdev)
> +{
> +	unsigned int expected_version, version, data;
> +	struct adi_axi_tdd_state *st;
> +	struct clk *aclk;
> +	int ret;
> +
> +	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	st->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(st->base))
> +		return PTR_ERR(st->base);
> +
> +	platform_set_drvdata(pdev, st);
> +
> +	aclk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");
> +	if (IS_ERR(aclk))
> +		return PTR_ERR(aclk);
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_tdd_clk_disable, aclk);

Looks you have here double disable.

> +	if (ret)
> +		return ret;
> +
> +	st->clk.clk = devm_clk_get(&pdev->dev, "intf_clk");
> +	if (IS_ERR(st->clk.clk))
> +		return PTR_ERR(st->clk.clk);
> +
> +	ret = clk_prepare_enable(st->clk.clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_tdd_clk_disable, st->clk.clk);

Looks you have here double disable.


> +	if (ret)
> +		return ret;
> +
> +	st->clk.rate = clk_get_rate(st->clk.clk);
> +	st->clk.nb.notifier_call = adi_axi_tdd_rate_change;
> +	ret = clk_notifier_register(st->clk.clk, &st->clk.nb);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, adi_axi_tdd_clk_notifier_unreg, st->clk.clk);

Wrap your lines. Limit is 80.

> +	if (ret)
> +		return ret;
> +
> +	st->regs = devm_regmap_init_mmio(&pdev->dev, st->base,
> +					 &adi_axi_tdd_regmap_cfg);
> +	if (IS_ERR(st->regs))
> +		return PTR_ERR(st->regs);
> +
> +	ret = regmap_read(st->regs, ADI_AXI_REG_VERSION, &version);
> +	if (ret)
> +		return ret;
> +


Best regards,
Krzysztof