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;
Date:   Thu, 22 Sep 2022 12:18:50 +0100
From:   Jonathan Cameron <Jonathan.Cameron@huawei.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC:     Mark Brown <broonie@kernel.org>, <linux-spi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        Wei Yongjun <weiyongjun@huaweicloud.com>,
        Jonathan Cameron <jic23@kernel.org>,
        <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v1 1/1] spi: Introduce spi_get_device_match_data()
 helper
Message-ID: <20220922121850.000006f3@huawei.com>
In-Reply-To: <20220921204520.23984-1-andriy.shevchenko@linux.intel.com>
References: <20220921204520.23984-1-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 21 Sep 2022 23:45:20 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The proposed spi_get_device_match_data() helper is for retrieving
> a driver data associated with the ID in an ID table. First, it tries
> to get driver data of the device enumerated by firmware interface
> (usually Device Tree or ACPI). If none is found it falls back to
> the SPI ID table matching.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>


I like this in general, but we need to keep a close eye on usecases
that are introduced to make sure that there is alignment between the
contents of the tables.

Perhaps the text should also explain a bit about why we would allow
both paths (in many case, the fallback will work fine)...
Previously a strong reason for that was that there was nothing to ensure
that all dt table entries were mirrored in the spi_device_id table.
As a side effect of the check to ensure module autoloading works, there
is now a check for that.

My personal view is we should still use the more generic path, even if
magic under the hood puts the data in spi_id->driver_data.

Jonathan


> ---
>  drivers/spi/spi.c       | 12 ++++++++++++
>  include/linux/spi/spi.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index ad254b94308e..a0947d63afbc 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -360,6 +360,18 @@ const struct spi_device_id *spi_get_device_id(const struct spi_device *sdev)
>  }
>  EXPORT_SYMBOL_GPL(spi_get_device_id);
>  
> +const void *spi_get_device_match_data(const struct spi_device *sdev)
> +{
> +	const void *match;
> +
> +	match = device_get_match_data(&sdev->dev);
> +	if (match)
> +		return match;
> +
> +	return (const void *)spi_get_device_id(sdev)->driver_data;
> +}
> +EXPORT_SYMBOL_GPL(spi_get_device_match_data);
> +
>  static int spi_match_device(struct device *dev, struct device_driver *drv)
>  {
>  	const struct spi_device	*spi = to_spi_device(dev);
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 6ea889df0813..f2565c24ef27 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -1510,6 +1510,9 @@ extern void spi_unregister_device(struct spi_device *spi);
>  extern const struct spi_device_id *
>  spi_get_device_id(const struct spi_device *sdev);
>  
> +extern const void *
> +spi_get_device_match_data(const struct spi_device *sdev);
> +
>  static inline bool
>  spi_transfer_is_last(struct spi_controller *ctlr, struct spi_transfer *xfer)
>  {