| 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
| ||
|
Message-ID: <2f4171f8-badb-a33a-7ae6-c375d9d725c3@amd.com>
Date: Mon, 23 Jan 2023 13:44:56 +0100
From: Michal Simek <michal.simek@....com>
To: Amit Kumar Mahapatra <amit.kumar-mahapatra@....com>,
<broonie@...nel.org>, <miquel.raynal@...tlin.com>,
<richard@....at>, <vigneshr@...com>, <jic23@...nel.org>,
<tudor.ambarus@...rochip.com>, <pratyush@...nel.org>,
<sanju.mehta@....com>, <chin-ting_kuo@...eedtech.com>,
<clg@...d.org>, <kdasu.kdev@...il.com>, <f.fainelli@...il.com>,
<rjui@...adcom.com>, <sbranden@...adcom.com>,
<eajames@...ux.ibm.com>, <olteanv@...il.com>, <han.xu@....com>,
<john.garry@...wei.com>, <shawnguo@...nel.org>,
<s.hauer@...gutronix.de>, <narmstrong@...libre.com>,
<khilman@...libre.com>, <matthias.bgg@...il.com>,
<haibo.chen@....com>, <linus.walleij@...aro.org>,
<daniel@...que.org>, <haojian.zhuang@...il.com>,
<robert.jarzmik@...e.fr>, <agross@...nel.org>,
<bjorn.andersson@...aro.org>, <heiko@...ech.de>,
<krzysztof.kozlowski@...aro.org>, <andi@...zian.org>,
<mcoquelin.stm32@...il.com>, <alexandre.torgue@...s.st.com>,
<wens@...e.org>, <jernej.skrabec@...il.com>, <samuel@...lland.org>,
<masahisa.kojima@...aro.org>, <jaswinder.singh@...aro.org>,
<rostedt@...dmis.org>, <mingo@...hat.com>,
<l.stelmach@...sung.com>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<alex.aring@...il.com>, <stefan@...enfreihafen.org>,
<kvalo@...nel.org>, <thierry.reding@...il.com>,
<jonathanh@...dia.com>, <skomatineni@...dia.com>,
<sumit.semwal@...aro.org>, <christian.koenig@....com>,
<j.neuschaefer@....net>, <vireshk@...nel.org>, <rmfrfs@...il.com>,
<johan@...nel.org>, <elder@...nel.org>,
<gregkh@...uxfoundation.org>
CC: <git@....com>, <linux-spi@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <joel@....id.au>,
<andrew@...id.au>, <radu_nicolae.pirea@....ro>,
<nicolas.ferre@...rochip.com>, <alexandre.belloni@...tlin.com>,
<claudiu.beznea@...rochip.com>,
<bcm-kernel-feedback-list@...adcom.com>, <fancer.lancer@...il.com>,
<kernel@...gutronix.de>, <festevam@...il.com>, <linux-imx@....com>,
<jbrunet@...libre.com>, <martin.blumenstingl@...glemail.com>,
<avifishman70@...il.com>, <tmaimon77@...il.com>,
<tali.perry1@...il.com>, <venture@...gle.com>, <yuenn@...gle.com>,
<benjaminfair@...gle.com>, <yogeshgaur.83@...il.com>,
<konrad.dybcio@...ainline.org>, <alim.akhtar@...sung.com>,
<ldewangan@...dia.com>, <linux-aspeed@...ts.ozlabs.org>,
<openbmc@...ts.ozlabs.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-rpi-kernel@...ts.infradead.org>,
<linux-amlogic@...ts.infradead.org>,
<linux-mediatek@...ts.infradead.org>,
<linux-arm-msm@...r.kernel.org>,
<linux-rockchip@...ts.infradead.org>,
<linux-samsung-soc@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-sunxi@...ts.linux.dev>, <linux-tegra@...r.kernel.org>,
<netdev@...r.kernel.org>, <linux-wpan@...r.kernel.org>,
<libertas-dev@...ts.infradead.org>,
<linux-wireless@...r.kernel.org>, <linux-mtd@...ts.infradead.org>,
<lars@...afoo.de>, <Michael.Hennerich@...log.com>,
<linux-iio@...r.kernel.org>, <michael@...le.cc>,
<palmer@...belt.com>, <linux-riscv@...ts.infradead.org>,
<linux-media@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
<greybus-dev@...ts.linaro.org>, <linux-staging@...ts.linux.dev>,
<amitrkcian2002@...il.com>
Subject: Re: [PATCH v2 01/13] spi: Add APIs in spi core to set/get
spi->chip_select and spi->cs_gpiod
On 1/19/23 19:53, Amit Kumar Mahapatra wrote:
> Supporting multi-cs in spi core and spi controller drivers would require
> the chip_select & cs_gpiod members of struct spi_device to be an array.
> But changing the type of these members to array would break the spi driver
> functionality. To make the transition smoother introduced four new APIs to
> get/set the spi->chip_select & spi->cs_gpiod and replaced all
> spi->chip_select and spi->cs_gpiod references in spi core with the API
> calls.
> While adding multi-cs support in further patches the chip_select & cs_gpiod
> members of the spi_device structure would be converted to arrays & the
> "idx" parameter of the APIs would be used as array index i.e.,
> spi->chip_select[idx] & spi->cs_gpiod[idx] respectively.
>
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@....com>
> ---
> drivers/spi/spi.c | 45 ++++++++++++++++++++---------------------
> include/linux/spi/spi.h | 20 ++++++++++++++++++
> 2 files changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 3cc7bb4d03de..38421e831a7d 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -604,7 +604,7 @@ static void spi_dev_set_name(struct spi_device *spi)
> }
>
> dev_set_name(&spi->dev, "%s.%u", dev_name(&spi->controller->dev),
> - spi->chip_select);
> + spi_get_chipselect(spi, 0));
> }
>
> static int spi_dev_check(struct device *dev, void *data)
> @@ -613,7 +613,7 @@ static int spi_dev_check(struct device *dev, void *data)
> struct spi_device *new_spi = data;
>
> if (spi->controller == new_spi->controller &&
> - spi->chip_select == new_spi->chip_select)
> + spi_get_chipselect(spi, 0) == spi_get_chipselect(new_spi, 0))
> return -EBUSY;
> return 0;
> }
> @@ -638,7 +638,7 @@ static int __spi_add_device(struct spi_device *spi)
> status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
> if (status) {
> dev_err(dev, "chipselect %d already in use\n",
> - spi->chip_select);
> + spi_get_chipselect(spi, 0));
> return status;
> }
>
> @@ -649,7 +649,7 @@ static int __spi_add_device(struct spi_device *spi)
> }
>
> if (ctlr->cs_gpiods)
> - spi->cs_gpiod = ctlr->cs_gpiods[spi->chip_select];
> + spi_set_csgpiod(spi, 0, ctlr->cs_gpiods[spi_get_chipselect(spi, 0)]);
>
> /*
> * Drivers may modify this initial i/o setup, but will
> @@ -692,8 +692,8 @@ int spi_add_device(struct spi_device *spi)
> int status;
>
> /* Chipselects are numbered 0..max; validate. */
> - if (spi->chip_select >= ctlr->num_chipselect) {
> - dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
> + if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) {
> + dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0),
> ctlr->num_chipselect);
> return -EINVAL;
> }
> @@ -714,8 +714,8 @@ static int spi_add_device_locked(struct spi_device *spi)
> struct device *dev = ctlr->dev.parent;
>
> /* Chipselects are numbered 0..max; validate. */
> - if (spi->chip_select >= ctlr->num_chipselect) {
> - dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
> + if (spi_get_chipselect(spi, 0) >= ctlr->num_chipselect) {
> + dev_err(dev, "cs%d >= max %d\n", spi_get_chipselect(spi, 0),
> ctlr->num_chipselect);
> return -EINVAL;
> }
> @@ -761,7 +761,7 @@ struct spi_device *spi_new_device(struct spi_controller *ctlr,
>
> WARN_ON(strlen(chip->modalias) >= sizeof(proxy->modalias));
>
> - proxy->chip_select = chip->chip_select;
> + spi_set_chipselect(proxy, 0, chip->chip_select);
> proxy->max_speed_hz = chip->max_speed_hz;
> proxy->mode = chip->mode;
> proxy->irq = chip->irq;
> @@ -970,24 +970,23 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
> * Avoid calling into the driver (or doing delays) if the chip select
> * isn't actually changing from the last time this was called.
> */
> - if (!force && ((enable && spi->controller->last_cs == spi->chip_select) ||
> - (!enable && spi->controller->last_cs != spi->chip_select)) &&
> + if (!force && ((enable && spi->controller->last_cs == spi_get_chipselect(spi, 0)) ||
> + (!enable && spi->controller->last_cs != spi_get_chipselect(spi, 0))) &&
> (spi->controller->last_cs_mode_high == (spi->mode & SPI_CS_HIGH)))
> return;
>
> trace_spi_set_cs(spi, activate);
>
> - spi->controller->last_cs = enable ? spi->chip_select : -1;
> + spi->controller->last_cs = enable ? spi_get_chipselect(spi, 0) : -1;
> spi->controller->last_cs_mode_high = spi->mode & SPI_CS_HIGH;
>
> - if ((spi->cs_gpiod || !spi->controller->set_cs_timing) && !activate) {
> + if ((spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) && !activate)
> spi_delay_exec(&spi->cs_hold, NULL);
> - }
>
> if (spi->mode & SPI_CS_HIGH)
> enable = !enable;
>
> - if (spi->cs_gpiod) {
> + if (spi_get_csgpiod(spi, 0)) {
> if (!(spi->mode & SPI_NO_CS)) {
> /*
> * Historically ACPI has no means of the GPIO polarity and
> @@ -1000,10 +999,10 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
> * into account.
> */
> if (has_acpi_companion(&spi->dev))
> - gpiod_set_value_cansleep(spi->cs_gpiod, !enable);
> + gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), !enable);
> else
> /* Polarity handled by GPIO library */
> - gpiod_set_value_cansleep(spi->cs_gpiod, activate);
> + gpiod_set_value_cansleep(spi_get_csgpiod(spi, 0), activate);
> }
> /* Some SPI masters need both GPIO CS & slave_select */
> if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
> @@ -1013,7 +1012,7 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
> spi->controller->set_cs(spi, !enable);
> }
>
> - if (spi->cs_gpiod || !spi->controller->set_cs_timing) {
> + if (spi_get_csgpiod(spi, 0) || !spi->controller->set_cs_timing) {
> if (activate)
> spi_delay_exec(&spi->cs_setup, NULL);
> else
> @@ -2304,7 +2303,7 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
> nc, rc);
> return rc;
> }
> - spi->chip_select = value;
> + spi_set_chipselect(spi, 0, value);
>
> /* Device speed */
> if (!of_property_read_u32(nc, "spi-max-frequency", &value))
> @@ -2423,7 +2422,7 @@ struct spi_device *spi_new_ancillary_device(struct spi_device *spi,
> strscpy(ancillary->modalias, "dummy", sizeof(ancillary->modalias));
>
> /* Use provided chip-select for ancillary device */
> - ancillary->chip_select = chip_select;
> + spi_set_chipselect(ancillary, 0, chip_select);
>
> /* Take over SPI mode/speed from SPI main device */
> ancillary->max_speed_hz = spi->max_speed_hz;
> @@ -2670,7 +2669,7 @@ struct spi_device *acpi_spi_device_alloc(struct spi_controller *ctlr,
> spi->mode |= lookup.mode;
> spi->irq = lookup.irq;
> spi->bits_per_word = lookup.bits_per_word;
> - spi->chip_select = lookup.chip_select;
> + spi_set_chipselect(spi, 0, lookup.chip_select);
>
> return spi;
> }
> @@ -3632,7 +3631,7 @@ static int spi_set_cs_timing(struct spi_device *spi)
> struct device *parent = spi->controller->dev.parent;
> int status = 0;
>
> - if (spi->controller->set_cs_timing && !spi->cs_gpiod) {
> + if (spi->controller->set_cs_timing && !spi_get_csgpiod(spi, 0)) {
> if (spi->controller->auto_runtime_pm) {
> status = pm_runtime_get_sync(parent);
> if (status < 0) {
> @@ -3837,7 +3836,7 @@ static int __spi_validate(struct spi_device *spi, struct spi_message *message)
> * cs_change is set for each transfer.
> */
> if ((spi->mode & SPI_CS_WORD) && (!(ctlr->mode_bits & SPI_CS_WORD) ||
> - spi->cs_gpiod)) {
> + spi_get_csgpiod(spi, 0))) {
> size_t maxsize;
> int ret;
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 9a32495fbb1f..9b23a1d0dd0d 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -263,6 +263,26 @@ static inline void *spi_get_drvdata(struct spi_device *spi)
> return dev_get_drvdata(&spi->dev);
> }
>
> +static inline u8 spi_get_chipselect(struct spi_device *spi, u8 idx)
> +{
> + return spi->chip_select;
> +}
> +
> +static inline void spi_set_chipselect(struct spi_device *spi, u8 idx, u8 chipselect)
> +{
> + spi->chip_select = chipselect;
> +}
> +
> +static inline struct gpio_desc *spi_get_csgpiod(struct spi_device *spi, u8 idx)
> +{
> + return spi->cs_gpiod;
> +}
> +
> +static inline void spi_set_csgpiod(struct spi_device *spi, u8 idx, struct gpio_desc *csgpiod)
> +{
> + spi->cs_gpiod = csgpiod;
> +}
> +
> struct spi_message;
>
> /**
Lars suggested this style in v1 version of this patch here.
https://lore.kernel.org/all/12fe1b84-1981-bf56-9323-b7f5b698c196@metafoo.de/
That's why let me also add his
Suggested-by: Lars-Peter Clausen <lars@...afoo.de>
And
Reviewed-by: Michal Simek <michal.simek@....com>
Thanks,
Michal
Powered by blists - more mailing lists