linux-kernel - Re: [PATCH v3 1/2] iio: adc: ad7173: remove special handling for irq number

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250112103750.55bc754e@jic23-huawei>
Date: Sun, 12 Jan 2025 10:37:50 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Dumitru Ceclan <mitrutzceclan@...il.com>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Michael Walle
 <michael@...le.cc>, Andy Shevchenko <andy@...nel.org>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Uwe
 Kleine-König <u.kleine-koenig@...libre.com>, Guillaume
 Ranquet <granquet@...libre.com>
Subject: Re: [PATCH v3 1/2] iio: adc: ad7173: remove special handling for
 irq number

On Fri, 10 Jan 2025 11:40:06 -0600
David Lechner <dlechner@...libre.com> wrote:

> Remove the int irq_line field in struct ad_sigma_delta_info and all code
> that referenced it.
> 
> This struct is intended to be used as static const data. Currently, the
> only user that doesn't uses the static const struct directly, namely the
> ad7173 driver is making a copy of this struct to be able to modify the
> irq_line field. However, this field is written and never used due to the
> fact that ad_sd_init() which reads the field is called before
> ad7173_fw_parse_device_config() which writes it.
> 
> The runtime behavior does not change since ad_sd_init() was already
> (unintentionally) being called with irq_line = 0.  But, even though
> this could be considered a bug, the behavior was still correct. The SPI
> subsystem always uses the first interrupt in the interrupts array from
> the devicetree and the devicetree bindings for this family of chips
> specify that the RDY interrupt is always the first interrupt.

Binding does say that kind of, but it shouldn't - we should allow
for possibility of only the err being connected in the binding.
The driver can of course reject that.

  interrupts:
    minItems: 1
    items:
      - description: |
          Ready: multiplexed with SPI data out. While SPI CS is low,
          can be used to indicate the completion of a conversion.

      - description: |
          Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR,
          and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin.
          Therefore, the ERROR pin indicates that an error has occurred.

  interrupt-names:
    minItems: 1
    items:
      - const: rdy
      - const: err

Is the current binding that should be relaxed.

Upshot, I'd specifically check rdy is the first one.
Easy way being to see if it matches spi->irq.

Jonathan



> Therefore,
> we don't actually need the special call to fwnode_irq_get_byname(), so
> it is removed in this patch instead of moving it to the correct place.
> 
> Tested-by: Guillaume Ranquet <granquet@...libre.com>
> Signed-off-by: David Lechner <dlechner@...libre.com>
> ---
> 
> v3 changes:
> * Removed spurious change that was causing compiler error.
> * Rebased on iio/testing and resolved some merge conflicts.
> 
> v2 changes:
> * Fixed chip name is subject line
> * Uwe's comment made me realize that the special case was actually never
> being used because of the ordering bug and could safely be removed
> rather than trying to preserve it.
> ---
>  drivers/iio/adc/ad7173.c               | 6 ------
>  drivers/iio/adc/ad_sigma_delta.c       | 5 +----
>  include/linux/iio/adc/ad_sigma_delta.h | 2 --
>  3 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 6c4ed10ae580d66287857252ce9a69cfaa45db0b..b92aca39d117a315d6b55951fba7c3b51787555a 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1515,12 +1515,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
>  			return ret;
>  	}
>  
> -	ret = fwnode_irq_get_byname(dev_fwnode(dev), "rdy");
> -	if (ret < 0)
> -		return dev_err_probe(dev, ret, "Interrupt 'rdy' is required\n");
> -
> -	st->sigma_delta_info.irq_line = ret;
> -
>  	return ad7173_fw_parse_channel_config(indio_dev);
>  }
>  
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index d5d81581ab34099cef30ec63944ce1171c80ec14..38a72ced10326656b30fd39d7a72cefe8c4c1aa5 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -801,10 +801,7 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
>  
>  	spin_lock_init(&sigma_delta->irq_lock);
>  
> -	if (info->irq_line)
> -		sigma_delta->irq_line = info->irq_line;
> -	else
> -		sigma_delta->irq_line = spi->irq;
> +	sigma_delta->irq_line = spi->irq;
>  
>  	sigma_delta->rdy_gpiod = devm_gpiod_get_optional(&spi->dev, "rdy", GPIOD_IN);
>  	if (IS_ERR(sigma_delta->rdy_gpiod))
> diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
> index 417073c52380f60a1a45a4924f4f556b64832295..521e3dc95db9117b7df12710eaae3f373d1df7bc 100644
> --- a/include/linux/iio/adc/ad_sigma_delta.h
> +++ b/include/linux/iio/adc/ad_sigma_delta.h
> @@ -53,7 +53,6 @@ struct iio_dev;
>   *   be used.
>   * @irq_flags: flags for the interrupt used by the triggered buffer
>   * @num_slots: Number of sequencer slots
> - * @irq_line: IRQ for reading conversions. If 0, spi->irq will be used
>   * @num_resetclks: Number of SPI clk cycles with MOSI=1 to reset the chip.
>   */
>  struct ad_sigma_delta_info {
> @@ -70,7 +69,6 @@ struct ad_sigma_delta_info {
>  	unsigned int data_reg;
>  	unsigned long irq_flags;
>  	unsigned int num_slots;
> -	int irq_line;
>  	unsigned int num_resetclks;
>  };
>  
>