linux-kernel - Re: [PATCH] ARM: at91: spi: request all csgpio in spi probe

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140728223859.GA3214@piout.net>
Date:	Tue, 29 Jul 2014 00:38:59 +0200
From:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:	Jiří Prchal <jiri.prchal@...ignal.cz>
Cc:	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, nicolas.ferre@...el.com,
	voice.shen@...el.com, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH] ARM: at91: spi: request all csgpio in spi probe

(Adding Mark in Cc:)

On 28/07/2014 at 15:06:34 +0200, Jiří Prchal wrote :
> >>+#define DRIVER_NAME "atmel-spi"
> >>+
> >
> >This is not really related to the issue solved by that patch, maybe it
> >could go in another patch ?
> Probably yes, but is used for this patch, I based it upon spi-efm32.
> >

Yeah, I wouldn't use it anyway, see my comment below.

> >>  	asd->csr = csr;
> >>@@ -1290,6 +1276,15 @@ static int atmel_spi_probe(struct platform_device *pdev)
> >>  	int			ret;
> >>  	struct spi_master	*master;
> >>  	struct atmel_spi	*as;
> >>+	struct device_node *np = pdev->dev.of_node;
> >>+	int num_cs, i;
> >>+
> >>+	if (!np)
> >>+		return -EINVAL;
> >>+
> >>+	num_cs = of_gpio_named_count(np, "cs-gpios");
> >>+	if (num_cs < 0)
> >>+		return num_cs;
> >>
> >
> >This driver can still be probed without DT, this will break here, please
> >don't assume that DT is present.
> Yes, but I copied it from spi-efm32 and looked at others, is almost the same.

Maybe it is similar in other drivers but at91 platforms can still be
booted without DT, This will fail with:
atmel_spi: probe of atmel_spi.0 failed with error -22

The efm32 only supports DT, it doesn't have board files/platform data.

> >
> >
> >>  	/* Select default pin state */
> >>  	pinctrl_pm_select_default_state(&pdev->dev);
> >>@@ -1308,7 +1303,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
> >>
> >>  	/* setup spi core then atmel-specific driver state */
> >>  	ret = -ENOMEM;
> >>-	master = spi_alloc_master(&pdev->dev, sizeof(*as));
> >>+	master = spi_alloc_master(&pdev->dev, sizeof(*as) + num_cs * sizeof(unsigned));
> >>  	if (!master)
> >>  		goto out_free;
> >>
> >>@@ -1317,7 +1312,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
> >>  	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 16);
> >>  	master->dev.of_node = pdev->dev.of_node;
> >>  	master->bus_num = pdev->id;
> >>-	master->num_chipselect = master->dev.of_node ? 0 : 4;
> >>+	master->num_chipselect = num_cs;
> >
> >My guess is that you don't need to change that part.
> I think I need it. What about 3 cs?

This will be taken care of by the spi core, in of_spi_register_master():

	nb = of_gpio_named_count(np, "cs-gpios");
	master->num_chipselect = max_t(int, nb, master->num_chipselect);


> >
> >>  	master->setup = atmel_spi_setup;
> >>  	master->transfer_one_message = atmel_spi_transfer_one_message;
> >>  	master->cleanup = atmel_spi_cleanup;
> >>@@ -1325,6 +1320,25 @@ static int atmel_spi_probe(struct platform_device *pdev)
> >>
> >>  	as = spi_master_get_devdata(master);
> >>
> >>+	for (i = 0; i < master->num_chipselect; ++i) {
> >>+		ret = of_get_named_gpio(np, "cs-gpios", i);
> >
> >Again, DT maybe not be compiled in or that driver may be probed from
> >platform data.
> Is another way how to do it? I see cs-gpios in master struct, but where it gets filled.
> >

It is filled when registering the master, here when calling
devm_spi_register_master(). It may be too late at that point.

> >>+		if (ret < 0) {
> >>+			dev_err(&pdev->dev, "failed to get csgpio#%u (%d)\n",
> >>+				i, ret);
> >>+			goto out_free;
> >>+		}
> >>+		as->csgpio[i] = ret;
> >>+		dev_dbg(&pdev->dev, "csgpio#%u = %u\n", i, as->csgpio[i]);
> >>+		ret = devm_gpio_request_one(&pdev->dev, as->csgpio[i],
> >>+					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);

Maybe you could use the CS number ifor the label here instead of
DRIVER_NAME, at least, use pdev->dev->name and completely drop
DRIVER_NAME.

> >>+		if (ret < 0) {
> >>+			dev_err(&pdev->dev,
> >>+				"failed to configure csgpio#%u (%d)\n",
> >>+				i, ret);
> >>+			goto out_free;
> >>+		}
> >>+	}
> >>+

Mark: maybe it would make sense to do devm_gpio_request_one() in
of_spi_register_master(), after of_get_named_gpio.

While this solves the particular issue Jiří is seeing, this will not
solve the case where PA14 (CS0) is not used by the spi driver at all. It
will remained muxed as CS0 and toggle when the spi master needs to
access CS0 until another driver muxes it to something else. I still
believe we should explicitly ask pinctrl to mux them as gpios.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/