Re: [PATCH v2 18/23] counter: ftm-quaddec: Convert to new counter registration

From: Jonathan Cameron <jic23@kernel.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: William Breathitt Gray <vilhelm.gray@gmail.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	kernel@pengutronix.de,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	linux-iio@vger.kernel.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org,
	Syed Nayyar Waris <syednwaris@gmail.com>,
	Patrick Havelange <patrick.havelange@essensium.com>
Subject: Re: [PATCH v2 18/23] counter: ftm-quaddec: Convert to new counter registration
Date: Tue, 28 Dec 2021 18:23:32 +0000	[thread overview]
Message-ID: <20211228182332.7a96041e@jic23-huawei> (raw)
In-Reply-To: <20211227094526.698714-19-u.kleine-koenig@pengutronix.de>

On Mon, 27 Dec 2021 10:45:21 +0100
Uwe Kleine-König         <u.kleine-koenig@pengutronix.de> wrote:

> This fixes device lifetime issues where it was possible to free a live
> struct device.
> 
> Fixes: a3b9a99980d9 ("counter: add FlexTimer Module Quadrature decoder counter driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Assuming you move the 104-quad-8 stuff to the earlier patch.

> ---
>  drivers/counter/104-quad-8.c  |  7 ++++---
>  drivers/counter/ftm-quaddec.c | 27 +++++++++++++--------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/counter/104-quad-8.c b/drivers/counter/104-quad-8.c
> index 4315b14f239e..680c7ba943a4 100644
> --- a/drivers/counter/104-quad-8.c
> +++ b/drivers/counter/104-quad-8.c

Block in wrong patch. I'll assume you'll fix that and ignore it beyond saying
this change is fine so keep my RB on the earlier patch once you've added this block.

> @@ -1084,7 +1084,8 @@ static struct counter_count quad8_counts[] = {
>  
>  static irqreturn_t quad8_irq_handler(int irq, void *private)
>  {
> -	struct quad8 *const priv = private;
> +	struct counter_device *counter = private;
> +	struct quad8 *const priv = counter_priv(counter);
>  	const unsigned long base = priv->base;
>  	unsigned long irq_status;
>  	unsigned long channel;
> @@ -1115,7 +1116,7 @@ static irqreturn_t quad8_irq_handler(int irq, void *private)
>  			continue;
>  		}
>  
> -		counter_push_event(&priv->counter, event, channel);
> +		counter_push_event(counter, event, channel);
>  	}
>  
>  	/* Clear pending interrupts on device */
> @@ -1192,7 +1193,7 @@ static int quad8_probe(struct device *dev, unsigned int id)
>  	outb(QUAD8_CHAN_OP_ENABLE_INTERRUPT_FUNC, base[id] + QUAD8_REG_CHAN_OP);
>  
>  	err = devm_request_irq(dev, irq[id], quad8_irq_handler, IRQF_SHARED,
> -			       counter->name, priv);
> +			       counter->name, counter);
>  	if (err)
>  		return err;
>  
> diff --git a/drivers/counter/ftm-quaddec.c b/drivers/counter/ftm-quaddec.c
> index f5d92df6a611..c636183b1337 100644
> --- a/drivers/counter/ftm-quaddec.c
> +++ b/drivers/counter/ftm-quaddec.c
> @@ -26,7 +26,6 @@
>  	})
>  
>  struct ftm_quaddec {
> -	struct counter_device counter;
>  	struct platform_device *pdev;
>  	void __iomem *ftm_base;
>  	bool big_endian;
> @@ -259,15 +258,16 @@ static struct counter_count ftm_quaddec_counts = {
>  
>  static int ftm_quaddec_probe(struct platform_device *pdev)
>  {
> +	struct counter_device *counter;
>  	struct ftm_quaddec *ftm;
> -

Try to avoid white space changes in a patch like these even if they seem
trivial.  The 'noise' adds up.

>  	struct device_node *node = pdev->dev.of_node;
>  	struct resource *io;
>  	int ret;
>  
> -	ftm = devm_kzalloc(&pdev->dev, sizeof(*ftm), GFP_KERNEL);
> -	if (!ftm)
> +	counter = devm_counter_alloc(&pdev->dev, sizeof(*ftm));
> +	if (!counter)
>  		return -ENOMEM;
> +	ftm = counter_priv(counter);
>  
>  	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!io) {
> @@ -283,14 +283,13 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "Failed to map memory region\n");
>  		return -EINVAL;
>  	}
> -	ftm->counter.name = dev_name(&pdev->dev);
> -	ftm->counter.parent = &pdev->dev;
> -	ftm->counter.ops = &ftm_quaddec_cnt_ops;
> -	ftm->counter.counts = &ftm_quaddec_counts;
> -	ftm->counter.num_counts = 1;
> -	ftm->counter.signals = ftm_quaddec_signals;
> -	ftm->counter.num_signals = ARRAY_SIZE(ftm_quaddec_signals);
> -	ftm->counter.priv = ftm;
> +	counter->name = dev_name(&pdev->dev);
> +	counter->parent = &pdev->dev;
> +	counter->ops = &ftm_quaddec_cnt_ops;
> +	counter->counts = &ftm_quaddec_counts;
> +	counter->num_counts = 1;
> +	counter->signals = ftm_quaddec_signals;
> +	counter->num_signals = ARRAY_SIZE(ftm_quaddec_signals);
>  
>  	mutex_init(&ftm->ftm_quaddec_mutex);
>  
> @@ -300,9 +299,9 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_counter_register(&pdev->dev, &ftm->counter);
> +	ret = devm_counter_add(&pdev->dev, counter);
>  	if (ret)
> -		return ret;
> +		return dev_err_probe(&pdev->dev, ret, "Failed to add counter\n");
>  
>  	return 0;
>  }