| Message ID | 1453401227-27135-1-git-send-email-srinivas.kandagatla@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 21, 2016 at 06:33:47PM +0000, Srinivas Kandagatla wrote: > This driver reuses pdev->id for spi bus numbers resulting in random > or very large bus numbering when used with device trees. pdev->id > is not the correct choice when using device trees. So add code to What makes you say this, why is pdev->id not "correct"? It is worrying if anything cares what number we pick. > get bus numbers via device tree aliases and if it fails then generate > a unique bus number. The other question is even if this is a good idea why is it something that should be open coded in individual drivers, if we want to change the policy we should be consistent between drivers.
On 21/01/16 19:03, Mark Brown wrote: > On Thu, Jan 21, 2016 at 06:47:34PM +0000, Srinivas Kandagatla wrote: >> On 21/01/16 18:38, Mark Brown wrote: >>> On Thu, Jan 21, 2016 at 06:33:47PM +0000, Srinivas Kandagatla wrote: > >>>> This driver reuses pdev->id for spi bus numbers resulting in random >>>> or very large bus numbering when used with device trees. pdev->id >>>> is not the correct choice when using device trees. So add code to > >>> What makes you say this, why is pdev->id not "correct"? It is worrying >>> if anything cares what number we pick. > >> Issue is that using pdev->id for bus number, as pdev->id does not get >> populated in device tree cases. > > That's a statement of what currently happens... > >> The end users who are reading the schematics would not be able to map the >> actual bus numbers on the schematics with the bus numbers allocated using >> pdev->id. It add more confusion. > >> Without this patch the bus number allocated to this driver is 32766. >> This does not really reflect the actual bus numbers on the boards >> schematics. > > Is this really causing anyone any confusion? Normally people are > looking at the devices on the SPI bus rather than the bus itself... In > any case if this *is* causing confusion should we not be doing something > at the bus core level that allows us to assign a descriptive name since > this doesn't seem in the least bit SPI specific but could apply to any > bus? > > There's also the problem that if someone has decided to label the bus > with a descriptive name in their schematic (eg, "SPI_FLASH") then being > able to assign a number doesn't do much to help, we'd need to be able to > provide strings. A brief survey of schematics I have to hand suggests > that this is a thing people do. > >>>> get bus numbers via device tree aliases and if it fails then generate >>>> a unique bus number. > >>> The other question is even if this is a good idea why is it something >>> that should be open coded in individual drivers, if we want to change >>> the policy we should be consistent between drivers. > >> Device tree aliases seems used very much in many drivers. >> The unique bus number scheme was actually inspired by the >> driver/tty/serial/msm_serial.c > > That doesn't help explain why it is a good idea to open code this in > individual drivers. I was asking why it's a good idea to do this in a > single driver rather than at a higher level. Oops!!, I should have looked at spi.c which already does exactly same thing. I think the logic did not get triggered because (int)-1 overflowed into s16 busnum. --srini > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 21, 2016 at 07:26:19PM +0000, Srinivas Kandagatla wrote: > On 21/01/16 19:03, Mark Brown wrote: > >That doesn't help explain why it is a good idea to open code this in > >individual drivers. I was asking why it's a good idea to do this in a > >single driver rather than at a higher level. > Oops!!, I should have looked at spi.c which already does exactly same thing. > I think the logic did not get triggered because (int)-1 overflowed into s16 > busnum. Ah, that sounds like a bug... I'd actually forgotten that we did that, it should work though since the code is there and it (hopefully) worked at some point.
diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c index 810a7fa..2bc67a9 100644 --- a/drivers/spi/spi-qup.c +++ b/drivers/spi/spi-qup.c @@ -127,6 +127,8 @@ #define SPI_DELAY_THRESHOLD 1 #define SPI_DELAY_RETRY 10 +static atomic_t spi_qup_next_id = ATOMIC_INIT(0); + struct spi_qup { void __iomem *base; struct device *dev; @@ -759,7 +761,7 @@ static int spi_qup_probe(struct platform_device *pdev) struct device *dev; void __iomem *base; u32 max_freq, iomode, num_cs; - int ret, irq, size; + int ret, irq, size, bus_num; dev = &pdev->dev; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); @@ -816,7 +818,15 @@ static int spi_qup_probe(struct platform_device *pdev) else master->num_chipselect = num_cs; - master->bus_num = pdev->id; + if (dev->of_node) + bus_num = of_alias_get_id(dev->of_node, "spi"); + else + bus_num = pdev->id; + + if (bus_num < 0) + bus_num = atomic_inc_return(&spi_qup_next_id) - 1; + + master->bus_num = bus_num; master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP; master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32); master->max_speed_hz = max_freq;
This driver reuses pdev->id for spi bus numbers resulting in random or very large bus numbering when used with device trees. pdev->id is not the correct choice when using device trees. So add code to get bus numbers via device tree aliases and if it fails then generate a unique bus number. Without this patch the driver get a random and useless bus number. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/spi/spi-qup.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html