Re: [PATCH v2 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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