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: <5f728a46-d222-a734-ce69-5adb695fb374@linaro.org>
Date:   Thu, 28 Jul 2022 15:44:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH v2 2/2] iio: time: capture-tiecap: capture driver support
 for ECAP
Content-Language: en-US
To:     Julien Panis <jpanis@baylibre.com>, jic23@kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org
Cc:     lars@metafoo.de, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        mranostay@ti.com
References: <20220728125212.76728-1-jpanis@baylibre.com>
 <20220728125212.76728-3-jpanis@baylibre.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220728125212.76728-3-jpanis@baylibre.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 28/07/2022 14:52, Julien Panis wrote:
> ECAP hardware on AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
> 
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
> 
> In the ECAP hardware, capture pin can also be configured to be in

Thank you for your patch. There is something to discuss/improve.

(...)

> +static int ecap_iio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_iio_dev *ecap_dev;
> +	struct iio_dev *indio_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(indio_dev)) {
> +		dev_err(dev, "failed to allocate memory for iio device\n");

Do not print messages, which core takes care of.

> +		return PTR_ERR(indio_dev);
> +	}
> +
> +	ecap_dev = iio_priv(indio_dev);
> +
> +	ecap_dev->clk = devm_clk_get(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk)) {
> +		dev_err(dev, "failed to get clock\n");
> +		return PTR_ERR(ecap_dev->clk);
> +	}
> +
> +	ret = clk_prepare_enable(ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ecap_iio_clk_disable, ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock disable action\n");
> +		return ret;
> +	}
> +
> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> +	if (!ecap_dev->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	if (prescaler > ECAP_PS_MAX_VAL) {
> +		prescaler = ECAP_PS_MAX_VAL;
> +		dev_warn(dev, "prescaler out of range, forced to %d\n", prescaler);
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base)) {
> +		dev_err(dev, "failed to remap io\n");

No need for msg.

> +		return PTR_ERR(mmio_base);
> +	}
> +
> +	ecap_dev->regmap = regmap_init_mmio(dev, mmio_base, &ecap_iio_regmap_config);
> +	if (IS_ERR(ecap_dev->regmap)) {
> +		dev_err(dev, "failed to init regmap\n");
> +		return PTR_ERR(ecap_dev->regmap);
> +	}
> +
> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL,
> +					 "ecap-iio-%p", mmio_base);
> +	indio_dev->info = &ecap_iio_info;
> +	indio_dev->channels = ecap_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ecap_iio_channels);
> +	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL, NULL);
> +	if (ret) {
> +		dev_err(dev, "failed to setup iio buffer\n");
> +		return ret;
> +	}
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get irq\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, ret, ecap_iio_isr, 0, pdev->name, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		return ret;
> +	}

Best regards,
Krzysztof