Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Saturday, November 10, 2012 10:14:47 AM Bjorn Helgaas wrote: > On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki wrote: > > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: > >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely > >> wrote: > >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas > >> > wrote: > >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely > >> >> wrote: > >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas > >> >>> wrote: > >> [+cc Greg, Peter, Tony since they acked the original patch [1]] > >> > >> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > >> wrote: > >> > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > >> >> Struct device_driver is a generic structure, so it seems strange to > >> >> have to include non-generic things like of_device_id and now > >> >> acpi_match_table there. > >> > > >> > Yes, but in a sense the DT and ACPI are "generic". So that they are > >> > used to > >> > describe the configuration of a machine. > >> > >> What I meant by "generic" was "useful across all architectures." The > >> new acpi_match_table and acpi_handle fields [1] are not generic in > >> that sense because they're present on all architectures but used only > >> on x86 and ia64. The existing of_match_table and of_node are > >> similarly unused on many architectures. This doesn't seem like a > >> scalable strategy to me. Are we going to add a pnpbios_node for x86 > >> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > >> etc.? > >> > >> [1] https://patchwork.kernel.org/patch/1677221/ > >> >>> > >> >>> Ultimately yes, I think that is what we want to do, > >> >> > >> >> Just to be clear, you think we *should* add things like pnpbios_node, > >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling > >> >> the OS about non-enumerable devices, where only one of the N fields is > >> >> used on any given machine? That seems surprising to me, but maybe I > >> >> just need to be educated :) > >> > > >> > Ah, I see what you're asking. > >> > > >> > In the short term, yes but only because we don't have any other > >> > alternative. What I'd really rather have is a safe way to attach datum > >> > (ie. acpi_device or device_node) to a struct device and get it back > >> > later in a type safe way. > >> > >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: > >> > >> #define dev_is_acpi(d)((d)->bus == _bus_type) > > > > No, that's not right. It won't work for things like SPI and I2C with a > > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > > > >> #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) > >> d->datum : NULL) > > > > The problem basically is how we can tell that the given struct device has > > a "backing" object containing device information (e.g. resources) and what > > that "backing" object is. For device trees that would be a struct > > device_node > > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And > > by > > the way, they _can_ be used simultaneously, in principle. > > > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > > something following two pointers or calling a function just in order to > > check > > if the pointer _is_ _there_ in either case. > > > > And since we added of_node to struct device at one point, it is only > > logical to > > treat ACPI in the same way. If we come up with a better idea _later_, then > > we > > can convert _all_ things to this new idea, whatever it is. > > > > Are you seriously expecting us to come up with such an idea on the fly just > > so > > that we can use ACPI support, which already is there in the form of > > archdata.acpi_handle anyway, on equal footing with Device Trees? > > Of course not. I'm just trying to understand where we're headed. > That was not obvious from the patches I've seen so far. No, it wasn't, fair enough. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki wrote: > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely >> wrote: >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas wrote: >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely >> >> wrote: >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas >> >>> wrote: >> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> wrote: >> > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> >> Struct device_driver is a generic structure, so it seems strange to >> >> have to include non-generic things like of_device_id and now >> >> acpi_match_table there. >> > >> > Yes, but in a sense the DT and ACPI are "generic". So that they are >> > used to >> > describe the configuration of a machine. >> >> What I meant by "generic" was "useful across all architectures." The >> new acpi_match_table and acpi_handle fields [1] are not generic in >> that sense because they're present on all architectures but used only >> on x86 and ia64. The existing of_match_table and of_node are >> similarly unused on many architectures. This doesn't seem like a >> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> etc.? >> >> [1] https://patchwork.kernel.org/patch/1677221/ >> >>> >> >>> Ultimately yes, I think that is what we want to do, >> >> >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> >> the OS about non-enumerable devices, where only one of the N fields is >> >> used on any given machine? That seems surprising to me, but maybe I >> >> just need to be educated :) >> > >> > Ah, I see what you're asking. >> > >> > In the short term, yes but only because we don't have any other >> > alternative. What I'd really rather have is a safe way to attach datum >> > (ie. acpi_device or device_node) to a struct device and get it back >> > later in a type safe way. >> >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: >> >> #define dev_is_acpi(d)((d)->bus == _bus_type) > > No, that's not right. It won't work for things like SPI and I2C with a > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > >> #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) >> d->datum : NULL) > > The problem basically is how we can tell that the given struct device has > a "backing" object containing device information (e.g. resources) and what > that "backing" object is. For device trees that would be a struct device_node > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by > the way, they _can_ be used simultaneously, in principle. > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > something following two pointers or calling a function just in order to check > if the pointer _is_ _there_ in either case. > > And since we added of_node to struct device at one point, it is only logical > to > treat ACPI in the same way. If we come up with a better idea _later_, then we > can convert _all_ things to this new idea, whatever it is. > > Are you seriously expecting us to come up with such an idea on the fly just so > that we can use ACPI support, which already is there in the form of > archdata.acpi_handle anyway, on equal footing with Device Trees? Of course not. I'm just trying to understand where we're headed. That was not obvious from the patches I've seen so far. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 10, 2012 at 11:10 AM, Rafael J. Wysocki wrote: > On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: >> On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely >> wrote: >> > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas wrote: >> >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely >> >> wrote: >> >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas >> >>> wrote: >> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> wrote: >> > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> >> Struct device_driver is a generic structure, so it seems strange to >> >> have to include non-generic things like of_device_id and now >> >> acpi_match_table there. >> > >> > Yes, but in a sense the DT and ACPI are "generic". So that they are >> > used to >> > describe the configuration of a machine. >> >> What I meant by "generic" was "useful across all architectures." The >> new acpi_match_table and acpi_handle fields [1] are not generic in >> that sense because they're present on all architectures but used only >> on x86 and ia64. The existing of_match_table and of_node are >> similarly unused on many architectures. This doesn't seem like a >> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> etc.? >> >> [1] https://patchwork.kernel.org/patch/1677221/ >> >>> >> >>> Ultimately yes, I think that is what we want to do, >> >> >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> >> the OS about non-enumerable devices, where only one of the N fields is >> >> used on any given machine? That seems surprising to me, but maybe I >> >> just need to be educated :) >> > >> > Ah, I see what you're asking. >> > >> > In the short term, yes but only because we don't have any other >> > alternative. What I'd really rather have is a safe way to attach datum >> > (ie. acpi_device or device_node) to a struct device and get it back >> > later in a type safe way. >> >> Yep, *that* makes perfect sense to me. Something along these lines, maybe: >> >> #define dev_is_acpi(d)((d)->bus == _bus_type) > > No, that's not right. It won't work for things like SPI and I2C with a > "backing" ACPI device node anyway (and for PCI too, by the way :-)). > >> #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) >> d->datum : NULL) > > The problem basically is how we can tell that the given struct device has > a "backing" object containing device information (e.g. resources) and what > that "backing" object is. For device trees that would be a struct device_node > and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by > the way, they _can_ be used simultaneously, in principle. > > So we need something like of_node(dev) or acpi_node(dev), but that can't be > something following two pointers or calling a function just in order to check > if the pointer _is_ _there_ in either case. > > And since we added of_node to struct device at one point, it is only logical > to > treat ACPI in the same way. If we come up with a better idea _later_, then we > can convert _all_ things to this new idea, whatever it is. > > Are you seriously expecting us to come up with such an idea on the fly just so > that we can use ACPI support, which already is there in the form of > archdata.acpi_handle anyway, on equal footing with Device Trees? I'm certainly not. I agree with adding it to struct device now and replace it later if someone designs something better. I also agree with using a dev_acpi_node() macro as you described above. I went the opposite way with device tree, and I absolutely regret it. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: > On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely > wrote: > > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas wrote: > >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely > >> wrote: > >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas wrote: > [+cc Greg, Peter, Tony since they acked the original patch [1]] > > On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > wrote: > > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > >> Struct device_driver is a generic structure, so it seems strange to > >> have to include non-generic things like of_device_id and now > >> acpi_match_table there. > > > > Yes, but in a sense the DT and ACPI are "generic". So that they are > > used to > > describe the configuration of a machine. > > What I meant by "generic" was "useful across all architectures." The > new acpi_match_table and acpi_handle fields [1] are not generic in > that sense because they're present on all architectures but used only > on x86 and ia64. The existing of_match_table and of_node are > similarly unused on many architectures. This doesn't seem like a > scalable strategy to me. Are we going to add a pnpbios_node for x86 > PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > etc.? > > [1] https://patchwork.kernel.org/patch/1677221/ > >>> > >>> Ultimately yes, I think that is what we want to do, > >> > >> Just to be clear, you think we *should* add things like pnpbios_node, > >> pdc_hpa, etc., to struct device, one field for every scheme of telling > >> the OS about non-enumerable devices, where only one of the N fields is > >> used on any given machine? That seems surprising to me, but maybe I > >> just need to be educated :) > > > > Ah, I see what you're asking. > > > > In the short term, yes but only because we don't have any other > > alternative. What I'd really rather have is a safe way to attach datum > > (ie. acpi_device or device_node) to a struct device and get it back > > later in a type safe way. > > Yep, *that* makes perfect sense to me. Something along these lines, maybe: > > #define dev_is_acpi(d)((d)->bus == _bus_type) No, that's not right. It won't work for things like SPI and I2C with a "backing" ACPI device node anyway (and for PCI too, by the way :-)). > #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) > d->datum : NULL) The problem basically is how we can tell that the given struct device has a "backing" object containing device information (e.g. resources) and what that "backing" object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)-bus == acpi_bus_type) No, that's not right. It won't work for things like SPI and I2C with a backing ACPI device node anyway (and for PCI too, by the way :-)). #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d-datum : NULL) The problem basically is how we can tell that the given struct device has a backing object containing device information (e.g. resources) and what that backing object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 10, 2012 at 11:10 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)-bus == acpi_bus_type) No, that's not right. It won't work for things like SPI and I2C with a backing ACPI device node anyway (and for PCI too, by the way :-)). #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d-datum : NULL) The problem basically is how we can tell that the given struct device has a backing object containing device information (e.g. resources) and what that backing object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? I'm certainly not. I agree with adding it to struct device now and replace it later if someone designs something better. I also agree with using a dev_acpi_node() macro as you described above. I went the opposite way with device tree, and I absolutely regret it. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)-bus == acpi_bus_type) No, that's not right. It won't work for things like SPI and I2C with a backing ACPI device node anyway (and for PCI too, by the way :-)). #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d-datum : NULL) The problem basically is how we can tell that the given struct device has a backing object containing device information (e.g. resources) and what that backing object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? Of course not. I'm just trying to understand where we're headed. That was not obvious from the patches I've seen so far. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Saturday, November 10, 2012 10:14:47 AM Bjorn Helgaas wrote: On Sat, Nov 10, 2012 at 4:10 AM, Rafael J. Wysocki r...@sisk.pl wrote: On Friday, November 09, 2012 09:53:26 AM Bjorn Helgaas wrote: On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)-bus == acpi_bus_type) No, that's not right. It won't work for things like SPI and I2C with a backing ACPI device node anyway (and for PCI too, by the way :-)). #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d-datum : NULL) The problem basically is how we can tell that the given struct device has a backing object containing device information (e.g. resources) and what that backing object is. For device trees that would be a struct device_node and for ACPI that would be an acpi_handle or a struct acpi_device etc. And by the way, they _can_ be used simultaneously, in principle. So we need something like of_node(dev) or acpi_node(dev), but that can't be something following two pointers or calling a function just in order to check if the pointer _is_ _there_ in either case. And since we added of_node to struct device at one point, it is only logical to treat ACPI in the same way. If we come up with a better idea _later_, then we can convert _all_ things to this new idea, whatever it is. Are you seriously expecting us to come up with such an idea on the fly just so that we can use ACPI support, which already is there in the form of archdata.acpi_handle anyway, on equal footing with Device Trees? Of course not. I'm just trying to understand where we're headed. That was not obvious from the patches I've seen so far. No, it wasn't, fair enough. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely wrote: > On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas wrote: >> On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely >> wrote: >>> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg wrote: > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> Struct device_driver is a generic structure, so it seems strange to >> have to include non-generic things like of_device_id and now >> acpi_match_table there. > > Yes, but in a sense the DT and ACPI are "generic". So that they are used > to > describe the configuration of a machine. What I meant by "generic" was "useful across all architectures." The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ >>> >>> Ultimately yes, I think that is what we want to do, >> >> Just to be clear, you think we *should* add things like pnpbios_node, >> pdc_hpa, etc., to struct device, one field for every scheme of telling >> the OS about non-enumerable devices, where only one of the N fields is >> used on any given machine? That seems surprising to me, but maybe I >> just need to be educated :) > > Ah, I see what you're asking. > > In the short term, yes but only because we don't have any other > alternative. What I'd really rather have is a safe way to attach datum > (ie. acpi_device or device_node) to a struct device and get it back > later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)->bus == _bus_type) #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d->datum : NULL) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 09, 2012 at 04:43:27PM +, Grant Likely wrote: > In the short term, yes but only because we don't have any other > alternative. What I'd really rather have is a safe way to attach datum > (ie. acpi_device or device_node) to a struct device and get it back > later in a type safe way. It would actually be useful for all manner > of things, not just ACPI/DT. I experimented a bit with trying to > implement something a year back, but never spent enough time on it. devres might already do what you need here if you are OK with the performance (and frees things too for super bonus fun points!). That's how dev_get_regmap() is implemented, it's not awesome in a fast path but it seems OK otherwise. signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas wrote: > On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely > wrote: >> On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas wrote: >>> [+cc Greg, Peter, Tony since they acked the original patch [1]] >>> >>> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >>> wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: > Struct device_driver is a generic structure, so it seems strange to > have to include non-generic things like of_device_id and now > acpi_match_table there. Yes, but in a sense the DT and ACPI are "generic". So that they are used to describe the configuration of a machine. >>> >>> What I meant by "generic" was "useful across all architectures." The >>> new acpi_match_table and acpi_handle fields [1] are not generic in >>> that sense because they're present on all architectures but used only >>> on x86 and ia64. The existing of_match_table and of_node are >>> similarly unused on many architectures. This doesn't seem like a >>> scalable strategy to me. Are we going to add a pnpbios_node for x86 >>> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >>> etc.? >>> >>> [1] https://patchwork.kernel.org/patch/1677221/ >> >> Ultimately yes, I think that is what we want to do, > > Just to be clear, you think we *should* add things like pnpbios_node, > pdc_hpa, etc., to struct device, one field for every scheme of telling > the OS about non-enumerable devices, where only one of the N fields is > used on any given machine? That seems surprising to me, but maybe I > just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. It would actually be useful for all manner of things, not just ACPI/DT. I experimented a bit with trying to implement something a year back, but never spent enough time on it. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely wrote: > On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas wrote: >> [+cc Greg, Peter, Tony since they acked the original patch [1]] >> >> On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg >> wrote: >>> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. >>> >>> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >>> describe the configuration of a machine. >> >> What I meant by "generic" was "useful across all architectures." The >> new acpi_match_table and acpi_handle fields [1] are not generic in >> that sense because they're present on all architectures but used only >> on x86 and ia64. The existing of_match_table and of_node are >> similarly unused on many architectures. This doesn't seem like a >> scalable strategy to me. Are we going to add a pnpbios_node for x86 >> PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, >> etc.? >> >> [1] https://patchwork.kernel.org/patch/1677221/ > > Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) > but there is first > the non-trivial problem to solve of figuring out how ACPI/DT/whatever > data maps into what the driver expects. For example, say a device uses > two GPIOs (A & B) and we have a generic get_gpio(int index) function > that works for both ACPI and DT. But what if the ACPI binding has the > gpios in the order A,B and DT orders them B,A? I do want to coordinate > between the DT and ACPI camps to avoid those situations as much as > possible, but they will happen. When they do the driver will still > need firmware specific data. It doesn't make any sense to put that > stuff outside the driver because only that specific driver needs the > extra information. Sure. This seems like just a special case of "drivers need a way to access the underlying ACPI/DT/whatever-specific functionality," e.g., gpio = get_gpio(dev, dev_is_acpi(dev) ? 1 : 0); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas wrote: > [+cc Greg, Peter, Tony since they acked the original patch [1]] > > On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg > wrote: >> On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >>> Struct device_driver is a generic structure, so it seems strange to >>> have to include non-generic things like of_device_id and now >>> acpi_match_table there. >> >> Yes, but in a sense the DT and ACPI are "generic". So that they are used to >> describe the configuration of a machine. > > What I meant by "generic" was "useful across all architectures." The > new acpi_match_table and acpi_handle fields [1] are not generic in > that sense because they're present on all architectures but used only > on x86 and ia64. The existing of_match_table and of_node are > similarly unused on many architectures. This doesn't seem like a > scalable strategy to me. Are we going to add a pnpbios_node for x86 > PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, > etc.? > > [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, but there is first the non-trivial problem to solve of figuring out how ACPI/DT/whatever data maps into what the driver expects. For example, say a device uses two GPIOs (A & B) and we have a generic get_gpio(int index) function that works for both ACPI and DT. But what if the ACPI binding has the gpios in the order A,B and DT orders them B,A? I do want to coordinate between the DT and ACPI camps to avoid those situations as much as possible, but they will happen. When they do the driver will still need firmware specific data. It doesn't make any sense to put that stuff outside the driver because only that specific driver needs the extra information. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
[+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg wrote: > On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: >> Struct device_driver is a generic structure, so it seems strange to >> have to include non-generic things like of_device_id and now >> acpi_match_table there. > > Yes, but in a sense the DT and ACPI are "generic". So that they are used to > describe the configuration of a machine. What I meant by "generic" was "useful across all architectures." The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
[+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, but there is first the non-trivial problem to solve of figuring out how ACPI/DT/whatever data maps into what the driver expects. For example, say a device uses two GPIOs (A B) and we have a generic get_gpio(int index) function that works for both ACPI and DT. But what if the ACPI binding has the gpios in the order A,B and DT orders them B,A? I do want to coordinate between the DT and ACPI camps to avoid those situations as much as possible, but they will happen. When they do the driver will still need firmware specific data. It doesn't make any sense to put that stuff outside the driver because only that specific driver needs the extra information. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) but there is first the non-trivial problem to solve of figuring out how ACPI/DT/whatever data maps into what the driver expects. For example, say a device uses two GPIOs (A B) and we have a generic get_gpio(int index) function that works for both ACPI and DT. But what if the ACPI binding has the gpios in the order A,B and DT orders them B,A? I do want to coordinate between the DT and ACPI camps to avoid those situations as much as possible, but they will happen. When they do the driver will still need firmware specific data. It doesn't make any sense to put that stuff outside the driver because only that specific driver needs the extra information. Sure. This seems like just a special case of drivers need a way to access the underlying ACPI/DT/whatever-specific functionality, e.g., gpio = get_gpio(dev, dev_is_acpi(dev) ? 1 : 0); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. It would actually be useful for all manner of things, not just ACPI/DT. I experimented a bit with trying to implement something a year back, but never spent enough time on it. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 09, 2012 at 04:43:27PM +, Grant Likely wrote: In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. It would actually be useful for all manner of things, not just ACPI/DT. I experimented a bit with trying to implement something a year back, but never spent enough time on it. devres might already do what you need here if you are OK with the performance (and frees things too for super bonus fun points!). That's how dev_get_regmap() is implemented, it's not awesome in a fast path but it seems OK otherwise. signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Fri, Nov 9, 2012 at 9:43 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 4:35 PM, Bjorn Helgaas bhelg...@google.com wrote: On Fri, Nov 9, 2012 at 8:45 AM, Grant Likely grant.lik...@secretlab.ca wrote: On Fri, Nov 9, 2012 at 3:11 PM, Bjorn Helgaas bhelg...@google.com wrote: [+cc Greg, Peter, Tony since they acked the original patch [1]] On Thu, Nov 8, 2012 at 1:04 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. What I meant by generic was useful across all architectures. The new acpi_match_table and acpi_handle fields [1] are not generic in that sense because they're present on all architectures but used only on x86 and ia64. The existing of_match_table and of_node are similarly unused on many architectures. This doesn't seem like a scalable strategy to me. Are we going to add a pnpbios_node for x86 PNPBIOS machines without ACPI, a pdc_hpa for parisc machines with PDC, etc.? [1] https://patchwork.kernel.org/patch/1677221/ Ultimately yes, I think that is what we want to do, Just to be clear, you think we *should* add things like pnpbios_node, pdc_hpa, etc., to struct device, one field for every scheme of telling the OS about non-enumerable devices, where only one of the N fields is used on any given machine? That seems surprising to me, but maybe I just need to be educated :) Ah, I see what you're asking. In the short term, yes but only because we don't have any other alternative. What I'd really rather have is a safe way to attach datum (ie. acpi_device or device_node) to a struct device and get it back later in a type safe way. Yep, *that* makes perfect sense to me. Something along these lines, maybe: #define dev_is_acpi(d)((d)-bus == acpi_bus_type) #define dev_acpi_handle(d)(dev_is_acpi(d) ? (acpi_handle) d-datum : NULL) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 06:48:05PM +, Grant Likely wrote:
> On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg
> wrote:
> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> > configure the SPI slave devices behind the SPI controller. This patch adds
> > support for this to the SPI core.
> >
> > In addition we bind ACPI nodes to SPI devices. This makes it possible for
> > the slave drivers to get the ACPI handle for further configuration.
> >
> > Signed-off-by: Mika Westerberg
> > Acked-by: Rafael J. Wysocki
> > ---
> > drivers/spi/spi.c | 231
> > -
> > 1 file changed, 230 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> > index 84c2861..de22a6e 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -35,6 +35,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > static void spidev_release(struct device *dev)
> > {
> > @@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct
> > device_driver *drv)
> > if (of_driver_match_device(dev, drv))
> > return 1;
> >
> > + /* Then try ACPI */
> > + if (acpi_driver_match_device(dev, drv))
> > + return 1;
> > +
> > if (sdrv->id_table)
> > return !!spi_match_id(sdrv->id_table, spi);
> >
> > @@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master
> > *master)
> > static void of_register_spi_devices(struct spi_master *master) { }
> > #endif
> >
> > +#ifdef CONFIG_ACPI
> > +struct acpi_spi {
> > + acpi_status (*callback)(struct acpi_device *, void *);
> > + void *data;
> > +};
> > +
> > +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
> > +void *data, void
> > **return_value)
> > +{
> > + struct acpi_spi *acpi_spi = data;
> > + struct acpi_device *adev;
> > +
> > + if (acpi_bus_get_device(handle, ))
> > + return AE_OK;
> > + if (acpi_bus_get_status(adev) || !adev->status.present)
> > + return AE_OK;
> > +
> > + return acpi_spi->callback(adev, acpi_spi->data);
> > +}
> > +
> > +static acpi_status acpi_spi_enumerate(acpi_handle handle,
> > + acpi_status (*callback)(struct acpi_device *, void *), void *data)
> > +{
> > + struct acpi_spi acpi_spi;
> > +
> > + acpi_spi.callback = callback;
> > + acpi_spi.data = data;
> > +
> > + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> > + acpi_spi_enumerate_device, NULL,
> > + _spi, NULL);
> > +}
>
> >From my reading of this, the block causes 2 levels of callback
> indirection. First to either acpi_spi_find_child or
> acpi_spi_add_device and second to acpi_spi_enumerate_device. All to
> share about 4 lines of code in acpi_spi_enumerate_device. It took me a
> while to unravel it. I think acpi_spi_find_child and
> acpi_spi_add_device should be passed directly to acpi_walk_namespace.
> Is there anything that prevents that?
No, I'll fix that up in the next version of the series.
> I also agree with the discussion that the actual parsing code for the
> resources should be common,. Retrieving things like IRQs and address
> resources should be function calls into ACPI helpers instead of open
> coding it in the spi core code.
We are working on that and I'm hoping the second version will use the
resources as provided by the ACPI core instead of calling _CRS directly
here.
> Otherwise the patch looks sane to me.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 8, 2012 at 9:06 PM, Rafael J. Wysocki wrote: > On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: >> On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg >> wrote: >> > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: >> >> > >> >> > OK, but then we need to pass the information obtained from _CRS >> >> > (presumably after some adjustments through _SRS) to drivers, or rather >> >> > to >> >> > things like the SPI core, I2C core etc. so that they can create device >> >> > objects for drivers to bind to and quite frankly I don't see why not to >> >> > use >> >> > ACPI resources for that. >> >> >> >> Nevertheless, the routines for parsing those resources should belong >> >> to the ACPI core, mostly to avoid code duplication. >> > >> > Rafael, >> > >> > So is the idea now that the ACPI core parses the resources and passes them >> > forward via struct acpi_device? > > Not exactly. The idea is to execute _CRS in the core and attach the result > as a list of resources the struct acpi_device object representing the given > device node. > >> > I'm just wondering how to proceed with these I2C and SPI enumeration >> > patches. >> >> From my experience with device tree, that seems the wrong way around. >> Device Tree used to have a separate "of_device" which is analogous to >> an acpi_device. > > No, it is not. If anything, struct acpi_device is a counterpart of struct > device_node. :-) > > Yes, the name is misleading and it should be something like struct > acpi_dev_node. > Yes, these objects _are_ registered as devices with the driver model and there > are drivers that bind to some of them. Yes, this is a mistake, but fixing it > will take quite some time, because it involves converting the drivers in > question. Okay, fair enough. I was indeed thrown off by the fact that it embeds a struct device and there are drivers that bind against it. At least that is the sort of thing that can be fixed over the long haul without undue pain. I can certainly see the advantage of having acpi nodes appear in sysfs. Interestingly enough I'm currently playing with a patch set that makes struct device_node a kobject so it can do the same. It is quite nice to get a back symlink from a struct device to a struct device_node when the pointer is populated. >> Plus individual drivers can call the same functions if (and only if) the >> needed resources cannot fit into the bus type's native format. > > I'm kind of cautious about this particular thing. I've seen enough cases where something doesn't quite fit into the model provided by a subsystem and it requires the driver to go asking for something specific. But I completely agree that caution is absolutely required and it shouldn't be done casually. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: > On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg > wrote: > > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > >> > > >> > OK, but then we need to pass the information obtained from _CRS > >> > (presumably after some adjustments through _SRS) to drivers, or rather to > >> > things like the SPI core, I2C core etc. so that they can create device > >> > objects for drivers to bind to and quite frankly I don't see why not to > >> > use > >> > ACPI resources for that. > >> > >> Nevertheless, the routines for parsing those resources should belong > >> to the ACPI core, mostly to avoid code duplication. > > > > Rafael, > > > > So is the idea now that the ACPI core parses the resources and passes them > > forward via struct acpi_device? Not exactly. The idea is to execute _CRS in the core and attach the result as a list of resources the struct acpi_device object representing the given device node. > > I'm just wondering how to proceed with these I2C and SPI enumeration > > patches. > > From my experience with device tree, that seems the wrong way around. > Device Tree used to have a separate "of_device" which is analogous to > an acpi_device. No, it is not. If anything, struct acpi_device is a counterpart of struct device_node. :-) Yes, the name is misleading and it should be something like struct acpi_dev_node. Yes, these objects _are_ registered as devices with the driver model and there are drivers that bind to some of them. Yes, this is a mistake, but fixing it will take quite some time, because it involves converting the drivers in question. No, acpi_handle is not analogous to struct device_node, because it only is a pointer to a structure used internally by the AML interpreter. It only is useful for executing AML methods with the help of the interpreter, but there are reasons why _CRS, in particular, should only be executed by the ACPI core. > The problem was always that of_devices never fit into > the view that Linux has of the system. That would mean having both an > of_device and and spi_device in completely separate parts of the > driver model tree to support an spi device. Same for platform, i2c and > onewire and others. Blech. > > So, yes I agree that ACPI core should have the tools for parsing the > resources, but it makes sense for those functions to be helpers that > the spi core acpi support and the i2c core acpi support use to > populate the native spi_device and i2c_client structures. Yes, that exactly is the plan, although I2C and SPI will not receive the resources directly from _CRS. :-) > Plus individual drivers can call the same functions if (and only if) the > needed resources cannot fit into the bus type's native format. I'm kind of cautious about this particular thing. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thursday, November 08, 2012 10:20:42 PM Mika Westerberg wrote: > On Thu, Nov 08, 2012 at 01:46:24AM +0100, Rafael J. Wysocki wrote: > > On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote: > > > On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: > > > > > So is the idea now that the ACPI core parses the resources and passes > > > > > them > > > > > forward via struct acpi_device? I'm just wondering how to proceed with > > > > > these I2C and SPI enumeration patches. > > > > > > > > Well, we definitely don't want to duplicate what > > > > drivers/pnp/pnpacpi/rsparser.c > > > > does, so the idea is to move the code from there to the core in such a > > > > way that > > > > both the SPI/I2C patches and the PNP layer can use it. > > > > > > Ok. > > > > > > > I'll have some prototype code ready shortly, hopefully, and I'll post it > > > > in that form for comments (and so that you know what to expect). > > > > > > Sounds good. Thanks! > > > > There you go. > > > > I haven't even try to compile it, so most likely it breaks things left, > > right > > and in between, but I hope it shows the idea. > > Thanks Rafael! > > I'll try this tomorrow (we had problems with the HW today so I wasn't able > to do any testing). > > I'll convert the SPI and I2C enumeration patches to use this method. OK, but I still need to move parsers of interrupt resources from rsparser.c to resource.c. :-) > > It does a couple of things at the same time, so it should be split into a > > few > > simpler patches. First, it moves some code from > > drivers/pnp/pnpacpi/rsparser.c > > to a new file drivers/acpi/resource.c and makes pnpacpi use functions from > > there. Second, it changes acpi_platform.c to use those functions too. > > Finally, it adds a list of ACPI resources to struct acpi_device and > > makes acpi_platform.c use that list intead of evaluating _CRS and parsing > > its > > output with acpi_walk_resources(). > > > > While changing acpi_platform.c I noticed that we had a bug in there, because > > GSIs were registered for the struct acpi_device object, although they > > should be > > registered for the struct platform_device one created by that code. I > > didn't > > try to fix that in the patch below, but it generally needs fixing. > > Good point. > > I wonder if the acpi_register_gsi() wants a device that is registered to > the Linux device framework? No, it doesn't, as far as I can tell. At least the pnpacpi code adds devices after registering GSIs for them. Also the existing implementations of acpi_register_gsi() don't require that. > At least with the SPI and I2C we generally > don't have such until we call i2c_new_device() or spi_add_device() and they > are getting passed the IRQ number which should be translated to the Linux > IRQ before that... That's correct. It looks like we could follow the hpet code and pass NULL as the dev argument to acpi_register_gsi() from there, as apparently the dev argument is only used to special-case PCI devices in mp_config_acpi_gsi(). Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 01:46:24AM +0100, Rafael J. Wysocki wrote: > On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote: > > On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: > > > > So is the idea now that the ACPI core parses the resources and passes > > > > them > > > > forward via struct acpi_device? I'm just wondering how to proceed with > > > > these I2C and SPI enumeration patches. > > > > > > Well, we definitely don't want to duplicate what > > > drivers/pnp/pnpacpi/rsparser.c > > > does, so the idea is to move the code from there to the core in such a > > > way that > > > both the SPI/I2C patches and the PNP layer can use it. > > > > Ok. > > > > > I'll have some prototype code ready shortly, hopefully, and I'll post it > > > in that form for comments (and so that you know what to expect). > > > > Sounds good. Thanks! > > There you go. > > I haven't even try to compile it, so most likely it breaks things left, right > and in between, but I hope it shows the idea. Thanks Rafael! I'll try this tomorrow (we had problems with the HW today so I wasn't able to do any testing). I'll convert the SPI and I2C enumeration patches to use this method. > It does a couple of things at the same time, so it should be split into a few > simpler patches. First, it moves some code from > drivers/pnp/pnpacpi/rsparser.c > to a new file drivers/acpi/resource.c and makes pnpacpi use functions from > there. Second, it changes acpi_platform.c to use those functions too. > Finally, it adds a list of ACPI resources to struct acpi_device and > makes acpi_platform.c use that list intead of evaluating _CRS and parsing its > output with acpi_walk_resources(). > > While changing acpi_platform.c I noticed that we had a bug in there, because > GSIs were registered for the struct acpi_device object, although they should > be > registered for the struct platform_device one created by that code. I didn't > try to fix that in the patch below, but it generally needs fixing. Good point. I wonder if the acpi_register_gsi() wants a device that is registered to the Linux device framework? At least with the SPI and I2C we generally don't have such until we call i2c_new_device() or spi_add_device() and they are getting passed the IRQ number which should be translated to the Linux IRQ before that... -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote:
> Struct device_driver is a generic structure, so it seems strange to
> have to include non-generic things like of_device_id and now
> acpi_match_table there.
Yes, but in a sense the DT and ACPI are "generic". So that they are used to
describe the configuration of a machine.
> I'm actually interested in the details you didn't include above, too.
> For example, I don't know of a generic way to get resource information
> from a "struct device *", so I assume you need to figure out what sort
> of device it is and then do the appropriate PCI/ACPI/OF/DT/etc
> operations to learn the resources?
Right. Typically you check, dev->of_node (or dev->acpi_handle) in a driver
and if it is non-NULL you can extract the resources using DT or ACPI
specific API calls.
Some things like IRQs and MMIO addresses can be passed to the driver using
the struct resource (and we do that already) but others we can't, like
GPIOs and some DT specific properties. Also with ACPI there might be need
to call some ACPI method, like _DSM where we need to have access to the
ACPI handle.
> I think it would be cool if there *were* a generic way to get "struct
> device" resources. Then you could imagine a mechanism where a driver
> supplied a list of identifiers it could claim, e.g.,
> PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART),
> ACPI_ID("PNP0501"), etc., and it might not need to know anything more
> than what the identifier is.
Indeed that would be cool, and we should probably try to implement
something like that, eventually. If you have followed the discusion there
is already talks about having a single API to get GPIOs to the driver in a
generic way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 7, 2012 at 2:56 AM, Mika Westerberg
wrote:
> On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote:
>> > How is the SPI controller different than this? Is there some logical
>> > difference that requires a different framework? Or are you proposing
>> > that we get rid of acpi_bus_register_driver() and migrate everything
>> > to this new framework?
>>
>> Yes, I do, but let's just do it gradually.
>
> Bjorn, here is a concrete example how this is supposed to be used.
>
> Lets say we have an existing SPI slave driver that we want to extend to
> support enumeration from ACPI. Instead of writing acpi_driver glue for that
> (and registering it using acpi_bus_register_driver()) what we do is simple
> add these to the existing SPI driver:
>
> #ifdef CONFIG_ACPI
> static struct acpi_device_id my_spidrv_match[] = {
> /* ACPI IDs here */
> { }
> };
> MODULE_DEVICE_TABLE(acpi, my_spidrv_match);
> #endif
>
> static struct spi_driver my_spidrv = {
> ...
> .driver = {
> .acpi_match_table = ACPI_PTR(my_spidrv_match),
> },
> };
>
> The same thing works with platform, I2c and SPI drivers and can be extented
> to others as well. If the driver needs to do some ACPI specific
> configuration it can get the ACPI handle using its dev->acpi_handle.
>
> The above example now supports both, normal SPI (where the devices are
> enumerated by populating spi_board_info()) and ACPI. Adding support for
> Device Tree is similar than ACPI so a single driver can support all three
> easily at the same time.
Thanks for the concrete example; that helps me a lot.
Struct device_driver is a generic structure, so it seems strange to
have to include non-generic things like of_device_id and now
acpi_match_table there.
I'm actually interested in the details you didn't include above, too.
For example, I don't know of a generic way to get resource information
from a "struct device *", so I assume you need to figure out what sort
of device it is and then do the appropriate PCI/ACPI/OF/DT/etc
operations to learn the resources?
I think it would be cool if there *were* a generic way to get "struct
device" resources. Then you could imagine a mechanism where a driver
supplied a list of identifiers it could claim, e.g.,
PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART),
ACPI_ID("PNP0501"), etc., and it might not need to know anything more
than what the identifier is.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg
wrote:
> ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> configure the SPI slave devices behind the SPI controller. This patch adds
> support for this to the SPI core.
>
> In addition we bind ACPI nodes to SPI devices. This makes it possible for
> the slave drivers to get the ACPI handle for further configuration.
>
> Signed-off-by: Mika Westerberg
> Acked-by: Rafael J. Wysocki
> ---
> drivers/spi/spi.c | 231
> -
> 1 file changed, 230 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 84c2861..de22a6e 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
>
> static void spidev_release(struct device *dev)
> {
> @@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct
> device_driver *drv)
> if (of_driver_match_device(dev, drv))
> return 1;
>
> + /* Then try ACPI */
> + if (acpi_driver_match_device(dev, drv))
> + return 1;
> +
> if (sdrv->id_table)
> return !!spi_match_id(sdrv->id_table, spi);
>
> @@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master
> *master)
> static void of_register_spi_devices(struct spi_master *master) { }
> #endif
>
> +#ifdef CONFIG_ACPI
> +struct acpi_spi {
> + acpi_status (*callback)(struct acpi_device *, void *);
> + void *data;
> +};
> +
> +static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
> +void *data, void **return_value)
> +{
> + struct acpi_spi *acpi_spi = data;
> + struct acpi_device *adev;
> +
> + if (acpi_bus_get_device(handle, ))
> + return AE_OK;
> + if (acpi_bus_get_status(adev) || !adev->status.present)
> + return AE_OK;
> +
> + return acpi_spi->callback(adev, acpi_spi->data);
> +}
> +
> +static acpi_status acpi_spi_enumerate(acpi_handle handle,
> + acpi_status (*callback)(struct acpi_device *, void *), void *data)
> +{
> + struct acpi_spi acpi_spi;
> +
> + acpi_spi.callback = callback;
> + acpi_spi.data = data;
> +
> + return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> + acpi_spi_enumerate_device, NULL,
> + _spi, NULL);
> +}
>From my reading of this, the block causes 2 levels of callback
indirection. First to either acpi_spi_find_child or
acpi_spi_add_device and second to acpi_spi_enumerate_device. All to
share about 4 lines of code in acpi_spi_enumerate_device. It took me a
while to unravel it. I think acpi_spi_find_child and
acpi_spi_add_device should be passed directly to acpi_walk_namespace.
Is there anything that prevents that?
I also agree with the discussion that the actual parsing code for the
resources should be common,. Retrieving things like IRQs and address
resources should be function calls into ACPI helpers instead of open
coding it in the spi core code.
Otherwise the patch looks sane to me.
g.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg wrote: > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: >> > >> > OK, but then we need to pass the information obtained from _CRS >> > (presumably after some adjustments through _SRS) to drivers, or rather to >> > things like the SPI core, I2C core etc. so that they can create device >> > objects for drivers to bind to and quite frankly I don't see why not to use >> > ACPI resources for that. >> >> Nevertheless, the routines for parsing those resources should belong >> to the ACPI core, mostly to avoid code duplication. > > Rafael, > > So is the idea now that the ACPI core parses the resources and passes them > forward via struct acpi_device? I'm just wondering how to proceed with > these I2C and SPI enumeration patches. >From my experience with device tree, that seems the wrong way around. Device Tree used to have a separate "of_device" which is analogous to an acpi_device. The problem was always that of_devices never fit into the view that Linux has of the system. That would mean having both an of_device and and spi_device in completely separate parts of the driver model tree to support an spi device. Same for platform, i2c and onewire and others. Blech. So, yes I agree that ACPI core should have the tools for parsing the resources, but it makes sense for those functions to be helpers that the spi core acpi support and the i2c core acpi support use to populate the native spi_device and i2c_client structures. Plus individual drivers can call the same functions if (and only if) the needed resources cannot fit into the bus type's native format. We really could also use more common code between bus types for storing various kinds of resources, but that's a separate issue and doesn't affect this discussion. g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. From my experience with device tree, that seems the wrong way around. Device Tree used to have a separate of_device which is analogous to an acpi_device. The problem was always that of_devices never fit into the view that Linux has of the system. That would mean having both an of_device and and spi_device in completely separate parts of the driver model tree to support an spi device. Same for platform, i2c and onewire and others. Blech. So, yes I agree that ACPI core should have the tools for parsing the resources, but it makes sense for those functions to be helpers that the spi core acpi support and the i2c core acpi support use to populate the native spi_device and i2c_client structures. Plus individual drivers can call the same functions if (and only if) the needed resources cannot fit into the bus type's native format. We really could also use more common code between bus types for storing various kinds of resources, but that's a separate issue and doesn't affect this discussion. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch adds
support for this to the SPI core.
In addition we bind ACPI nodes to SPI devices. This makes it possible for
the slave drivers to get the ACPI handle for further configuration.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/spi/spi.c | 231
-
1 file changed, 230 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84c2861..de22a6e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -35,6 +35,7 @@
#include linux/sched.h
#include linux/delay.h
#include linux/kthread.h
+#include linux/acpi.h
static void spidev_release(struct device *dev)
{
@@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct
device_driver *drv)
if (of_driver_match_device(dev, drv))
return 1;
+ /* Then try ACPI */
+ if (acpi_driver_match_device(dev, drv))
+ return 1;
+
if (sdrv-id_table)
return !!spi_match_id(sdrv-id_table, spi);
@@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master
*master)
static void of_register_spi_devices(struct spi_master *master) { }
#endif
+#ifdef CONFIG_ACPI
+struct acpi_spi {
+ acpi_status (*callback)(struct acpi_device *, void *);
+ void *data;
+};
+
+static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
+void *data, void **return_value)
+{
+ struct acpi_spi *acpi_spi = data;
+ struct acpi_device *adev;
+
+ if (acpi_bus_get_device(handle, adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adev) || !adev-status.present)
+ return AE_OK;
+
+ return acpi_spi-callback(adev, acpi_spi-data);
+}
+
+static acpi_status acpi_spi_enumerate(acpi_handle handle,
+ acpi_status (*callback)(struct acpi_device *, void *), void *data)
+{
+ struct acpi_spi acpi_spi;
+
+ acpi_spi.callback = callback;
+ acpi_spi.data = data;
+
+ return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_spi_enumerate_device, NULL,
+ acpi_spi, NULL);
+}
From my reading of this, the block causes 2 levels of callback
indirection. First to either acpi_spi_find_child or
acpi_spi_add_device and second to acpi_spi_enumerate_device. All to
share about 4 lines of code in acpi_spi_enumerate_device. It took me a
while to unravel it. I think acpi_spi_find_child and
acpi_spi_add_device should be passed directly to acpi_walk_namespace.
Is there anything that prevents that?
I also agree with the discussion that the actual parsing code for the
resources should be common,. Retrieving things like IRQs and address
resources should be function calls into ACPI helpers instead of open
coding it in the spi core code.
Otherwise the patch looks sane to me.
g.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 7, 2012 at 2:56 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote:
How is the SPI controller different than this? Is there some logical
difference that requires a different framework? Or are you proposing
that we get rid of acpi_bus_register_driver() and migrate everything
to this new framework?
Yes, I do, but let's just do it gradually.
Bjorn, here is a concrete example how this is supposed to be used.
Lets say we have an existing SPI slave driver that we want to extend to
support enumeration from ACPI. Instead of writing acpi_driver glue for that
(and registering it using acpi_bus_register_driver()) what we do is simple
add these to the existing SPI driver:
#ifdef CONFIG_ACPI
static struct acpi_device_id my_spidrv_match[] = {
/* ACPI IDs here */
{ }
};
MODULE_DEVICE_TABLE(acpi, my_spidrv_match);
#endif
static struct spi_driver my_spidrv = {
...
.driver = {
.acpi_match_table = ACPI_PTR(my_spidrv_match),
},
};
The same thing works with platform, I2c and SPI drivers and can be extented
to others as well. If the driver needs to do some ACPI specific
configuration it can get the ACPI handle using its dev-acpi_handle.
The above example now supports both, normal SPI (where the devices are
enumerated by populating spi_board_info()) and ACPI. Adding support for
Device Tree is similar than ACPI so a single driver can support all three
easily at the same time.
Thanks for the concrete example; that helps me a lot.
Struct device_driver is a generic structure, so it seems strange to
have to include non-generic things like of_device_id and now
acpi_match_table there.
I'm actually interested in the details you didn't include above, too.
For example, I don't know of a generic way to get resource information
from a struct device *, so I assume you need to figure out what sort
of device it is and then do the appropriate PCI/ACPI/OF/DT/etc
operations to learn the resources?
I think it would be cool if there *were* a generic way to get struct
device resources. Then you could imagine a mechanism where a driver
supplied a list of identifiers it could claim, e.g.,
PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART),
ACPI_ID(PNP0501), etc., and it might not need to know anything more
than what the identifier is.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 12:32:25PM -0700, Bjorn Helgaas wrote: Struct device_driver is a generic structure, so it seems strange to have to include non-generic things like of_device_id and now acpi_match_table there. Yes, but in a sense the DT and ACPI are generic. So that they are used to describe the configuration of a machine. I'm actually interested in the details you didn't include above, too. For example, I don't know of a generic way to get resource information from a struct device *, so I assume you need to figure out what sort of device it is and then do the appropriate PCI/ACPI/OF/DT/etc operations to learn the resources? Right. Typically you check, dev-of_node (or dev-acpi_handle) in a driver and if it is non-NULL you can extract the resources using DT or ACPI specific API calls. Some things like IRQs and MMIO addresses can be passed to the driver using the struct resource (and we do that already) but others we can't, like GPIOs and some DT specific properties. Also with ACPI there might be need to call some ACPI method, like _DSM where we need to have access to the ACPI handle. I think it would be cool if there *were* a generic way to get struct device resources. Then you could imagine a mechanism where a driver supplied a list of identifiers it could claim, e.g., PCI_VEN_DEV(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_CE4100_UART), ACPI_ID(PNP0501), etc., and it might not need to know anything more than what the identifier is. Indeed that would be cool, and we should probably try to implement something like that, eventually. If you have followed the discusion there is already talks about having a single API to get GPIOs to the driver in a generic way. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 01:46:24AM +0100, Rafael J. Wysocki wrote: On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote: On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. Ok. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Sounds good. Thanks! There you go. I haven't even try to compile it, so most likely it breaks things left, right and in between, but I hope it shows the idea. Thanks Rafael! I'll try this tomorrow (we had problems with the HW today so I wasn't able to do any testing). I'll convert the SPI and I2C enumeration patches to use this method. It does a couple of things at the same time, so it should be split into a few simpler patches. First, it moves some code from drivers/pnp/pnpacpi/rsparser.c to a new file drivers/acpi/resource.c and makes pnpacpi use functions from there. Second, it changes acpi_platform.c to use those functions too. Finally, it adds a list of ACPI resources to struct acpi_device and makes acpi_platform.c use that list intead of evaluating _CRS and parsing its output with acpi_walk_resources(). While changing acpi_platform.c I noticed that we had a bug in there, because GSIs were registered for the struct acpi_device object, although they should be registered for the struct platform_device one created by that code. I didn't try to fix that in the patch below, but it generally needs fixing. Good point. I wonder if the acpi_register_gsi() wants a device that is registered to the Linux device framework? At least with the SPI and I2C we generally don't have such until we call i2c_new_device() or spi_add_device() and they are getting passed the IRQ number which should be translated to the Linux IRQ before that... -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thursday, November 08, 2012 10:20:42 PM Mika Westerberg wrote: On Thu, Nov 08, 2012 at 01:46:24AM +0100, Rafael J. Wysocki wrote: On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote: On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. Ok. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Sounds good. Thanks! There you go. I haven't even try to compile it, so most likely it breaks things left, right and in between, but I hope it shows the idea. Thanks Rafael! I'll try this tomorrow (we had problems with the HW today so I wasn't able to do any testing). I'll convert the SPI and I2C enumeration patches to use this method. OK, but I still need to move parsers of interrupt resources from rsparser.c to resource.c. :-) It does a couple of things at the same time, so it should be split into a few simpler patches. First, it moves some code from drivers/pnp/pnpacpi/rsparser.c to a new file drivers/acpi/resource.c and makes pnpacpi use functions from there. Second, it changes acpi_platform.c to use those functions too. Finally, it adds a list of ACPI resources to struct acpi_device and makes acpi_platform.c use that list intead of evaluating _CRS and parsing its output with acpi_walk_resources(). While changing acpi_platform.c I noticed that we had a bug in there, because GSIs were registered for the struct acpi_device object, although they should be registered for the struct platform_device one created by that code. I didn't try to fix that in the patch below, but it generally needs fixing. Good point. I wonder if the acpi_register_gsi() wants a device that is registered to the Linux device framework? No, it doesn't, as far as I can tell. At least the pnpacpi code adds devices after registering GSIs for them. Also the existing implementations of acpi_register_gsi() don't require that. At least with the SPI and I2C we generally don't have such until we call i2c_new_device() or spi_add_device() and they are getting passed the IRQ number which should be translated to the Linux IRQ before that... That's correct. It looks like we could follow the hpet code and pass NULL as the dev argument to acpi_register_gsi() from there, as apparently the dev argument is only used to special-case PCI devices in mp_config_acpi_gsi(). Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? Not exactly. The idea is to execute _CRS in the core and attach the result as a list of resources the struct acpi_device object representing the given device node. I'm just wondering how to proceed with these I2C and SPI enumeration patches. From my experience with device tree, that seems the wrong way around. Device Tree used to have a separate of_device which is analogous to an acpi_device. No, it is not. If anything, struct acpi_device is a counterpart of struct device_node. :-) Yes, the name is misleading and it should be something like struct acpi_dev_node. Yes, these objects _are_ registered as devices with the driver model and there are drivers that bind to some of them. Yes, this is a mistake, but fixing it will take quite some time, because it involves converting the drivers in question. No, acpi_handle is not analogous to struct device_node, because it only is a pointer to a structure used internally by the AML interpreter. It only is useful for executing AML methods with the help of the interpreter, but there are reasons why _CRS, in particular, should only be executed by the ACPI core. The problem was always that of_devices never fit into the view that Linux has of the system. That would mean having both an of_device and and spi_device in completely separate parts of the driver model tree to support an spi device. Same for platform, i2c and onewire and others. Blech. So, yes I agree that ACPI core should have the tools for parsing the resources, but it makes sense for those functions to be helpers that the spi core acpi support and the i2c core acpi support use to populate the native spi_device and i2c_client structures. Yes, that exactly is the plan, although I2C and SPI will not receive the resources directly from _CRS. :-) Plus individual drivers can call the same functions if (and only if) the needed resources cannot fit into the bus type's native format. I'm kind of cautious about this particular thing. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 8, 2012 at 9:06 PM, Rafael J. Wysocki r...@sisk.pl wrote: On Thursday, November 08, 2012 06:05:23 PM Grant Likely wrote: On Wed, Nov 7, 2012 at 9:58 AM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? Not exactly. The idea is to execute _CRS in the core and attach the result as a list of resources the struct acpi_device object representing the given device node. I'm just wondering how to proceed with these I2C and SPI enumeration patches. From my experience with device tree, that seems the wrong way around. Device Tree used to have a separate of_device which is analogous to an acpi_device. No, it is not. If anything, struct acpi_device is a counterpart of struct device_node. :-) Yes, the name is misleading and it should be something like struct acpi_dev_node. Yes, these objects _are_ registered as devices with the driver model and there are drivers that bind to some of them. Yes, this is a mistake, but fixing it will take quite some time, because it involves converting the drivers in question. Okay, fair enough. I was indeed thrown off by the fact that it embeds a struct device and there are drivers that bind against it. At least that is the sort of thing that can be fixed over the long haul without undue pain. I can certainly see the advantage of having acpi nodes appear in sysfs. Interestingly enough I'm currently playing with a patch set that makes struct device_node a kobject so it can do the same. It is quite nice to get a back symlink from a struct device to a struct device_node when the pointer is populated. Plus individual drivers can call the same functions if (and only if) the needed resources cannot fit into the bus type's native format. I'm kind of cautious about this particular thing. I've seen enough cases where something doesn't quite fit into the model provided by a subsystem and it requires the driver to go asking for something specific. But I completely agree that caution is absolutely required and it shouldn't be done casually. g. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Thu, Nov 08, 2012 at 06:48:05PM +, Grant Likely wrote:
On Sat, Nov 3, 2012 at 7:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch adds
support for this to the SPI core.
In addition we bind ACPI nodes to SPI devices. This makes it possible for
the slave drivers to get the ACPI handle for further configuration.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/spi/spi.c | 231
-
1 file changed, 230 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84c2861..de22a6e 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -35,6 +35,7 @@
#include linux/sched.h
#include linux/delay.h
#include linux/kthread.h
+#include linux/acpi.h
static void spidev_release(struct device *dev)
{
@@ -93,6 +94,10 @@ static int spi_match_device(struct device *dev, struct
device_driver *drv)
if (of_driver_match_device(dev, drv))
return 1;
+ /* Then try ACPI */
+ if (acpi_driver_match_device(dev, drv))
+ return 1;
+
if (sdrv-id_table)
return !!spi_match_id(sdrv-id_table, spi);
@@ -888,6 +893,227 @@ static void of_register_spi_devices(struct spi_master
*master)
static void of_register_spi_devices(struct spi_master *master) { }
#endif
+#ifdef CONFIG_ACPI
+struct acpi_spi {
+ acpi_status (*callback)(struct acpi_device *, void *);
+ void *data;
+};
+
+static acpi_status acpi_spi_enumerate_device(acpi_handle handle, u32 level,
+void *data, void
**return_value)
+{
+ struct acpi_spi *acpi_spi = data;
+ struct acpi_device *adev;
+
+ if (acpi_bus_get_device(handle, adev))
+ return AE_OK;
+ if (acpi_bus_get_status(adev) || !adev-status.present)
+ return AE_OK;
+
+ return acpi_spi-callback(adev, acpi_spi-data);
+}
+
+static acpi_status acpi_spi_enumerate(acpi_handle handle,
+ acpi_status (*callback)(struct acpi_device *, void *), void *data)
+{
+ struct acpi_spi acpi_spi;
+
+ acpi_spi.callback = callback;
+ acpi_spi.data = data;
+
+ return acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+ acpi_spi_enumerate_device, NULL,
+ acpi_spi, NULL);
+}
From my reading of this, the block causes 2 levels of callback
indirection. First to either acpi_spi_find_child or
acpi_spi_add_device and second to acpi_spi_enumerate_device. All to
share about 4 lines of code in acpi_spi_enumerate_device. It took me a
while to unravel it. I think acpi_spi_find_child and
acpi_spi_add_device should be passed directly to acpi_walk_namespace.
Is there anything that prevents that?
No, I'll fix that up in the next version of the series.
I also agree with the discussion that the actual parsing code for the
resources should be common,. Retrieving things like IRQs and address
resources should be function calls into ACPI helpers instead of open
coding it in the spi core code.
We are working on that and I'm hoping the second version will use the
resources as provided by the ACPI core instead of calling _CRS directly
here.
Otherwise the patch looks sane to me.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote:
> On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote:
> > > So is the idea now that the ACPI core parses the resources and passes them
> > > forward via struct acpi_device? I'm just wondering how to proceed with
> > > these I2C and SPI enumeration patches.
> >
> > Well, we definitely don't want to duplicate what
> > drivers/pnp/pnpacpi/rsparser.c
> > does, so the idea is to move the code from there to the core in such a way
> > that
> > both the SPI/I2C patches and the PNP layer can use it.
>
> Ok.
>
> > I'll have some prototype code ready shortly, hopefully, and I'll post it
> > in that form for comments (and so that you know what to expect).
>
> Sounds good. Thanks!
There you go.
I haven't even try to compile it, so most likely it breaks things left, right
and in between, but I hope it shows the idea.
It does a couple of things at the same time, so it should be split into a few
simpler patches. First, it moves some code from drivers/pnp/pnpacpi/rsparser.c
to a new file drivers/acpi/resource.c and makes pnpacpi use functions from
there. Second, it changes acpi_platform.c to use those functions too.
Finally, it adds a list of ACPI resources to struct acpi_device and
makes acpi_platform.c use that list intead of evaluating _CRS and parsing its
output with acpi_walk_resources().
While changing acpi_platform.c I noticed that we had a bug in there, because
GSIs were registered for the struct acpi_device object, although they should be
registered for the struct platform_device one created by that code. I didn't
try to fix that in the patch below, but it generally needs fixing.
Thanks,
Rafael
Prototype, no sign-off.
---
drivers/acpi/Makefile |1
drivers/acpi/acpi_platform.c | 122 +-
drivers/acpi/resource.c| 269 +
drivers/acpi/scan.c| 48 +++
drivers/pnp/base.h |2
drivers/pnp/pnpacpi/rsparser.c | 150 +-
drivers/pnp/resource.c | 16 ++
include/acpi/acpi_bus.h|6
8 files changed, 386 insertions(+), 228 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -259,6 +259,11 @@ struct acpi_device_physical_node {
struct device *dev;
};
+struct acpi_resource_list_entry {
+ struct list_head node;
+ struct acpi_resource resource;
+};
+
/* set maximum of physical nodes to 32 for expansibility */
#define ACPI_MAX_PHYSICAL_NODE 32
@@ -269,6 +274,7 @@ struct acpi_device {
struct acpi_device *parent;
struct list_head children;
struct list_head node;
+ struct list_head resources; /* Device resources. */
struct list_head wakeup_list;
struct acpi_device_status status;
struct acpi_device_flags flags;
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -670,6 +670,8 @@ end:
static void acpi_device_unregister(struct acpi_device *device, int type)
{
+ struct acpi_resource_list_entry *entry, *s;
+
mutex_lock(_device_lock);
if (device->parent)
list_del(>node);
@@ -681,6 +683,9 @@ static void acpi_device_unregister(struc
acpi_device_remove_files(device);
device_unregister(>dev);
+
+ list_for_each_entry_safe(entry, s, >resources, node)
+ kfree(entry);
}
/* --
@@ -990,6 +995,40 @@ static void acpi_bus_get_wakeup_device_f
"error in _DSW or _PSW evaluation\n"));
}
+static acpi_status acpi_bus_add_resource(struct acpi_resource *res,
+void *context)
+{
+ struct list_head *list = context;
+ struct acpi_resource_list_entry *entry;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return AE_NO_MEMORY;
+
+ entry->resource = *res;
+ list_add_tail(>node, list);
+ return AE_OK;
+}
+
+static int acpi_bus_get_resources(struct acpi_device *adev)
+{
+ acpi_status status;
+ int ret = 0;
+
+ status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
+acpi_bus_add_resource, >resources);
+ if (ACPI_FAILURE(status)) {
+ switch(status) {
+ case AE_NO_MEMORY:
+ ret = -ENOMEM;
+ break;
+ default:
+ ret = -EIO;
+ }
+ }
+ return ret;
+}
+
static void acpi_bus_add_power_resource(acpi_handle handle);
static int acpi_bus_get_power_flags(struct acpi_device
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: > > So is the idea now that the ACPI core parses the resources and passes them > > forward via struct acpi_device? I'm just wondering how to proceed with > > these I2C and SPI enumeration patches. > > Well, we definitely don't want to duplicate what > drivers/pnp/pnpacpi/rsparser.c > does, so the idea is to move the code from there to the core in such a way > that > both the SPI/I2C patches and the PNP layer can use it. Ok. > I'll have some prototype code ready shortly, hopefully, and I'll post it > in that form for comments (and so that you know what to expect). Sounds good. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wednesday, November 07, 2012 11:58:42 AM Mika Westerberg wrote: > On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > > > > > > OK, but then we need to pass the information obtained from _CRS > > > (presumably after some adjustments through _SRS) to drivers, or rather to > > > things like the SPI core, I2C core etc. so that they can create device > > > objects for drivers to bind to and quite frankly I don't see why not to > > > use > > > ACPI resources for that. > > > > Nevertheless, the routines for parsing those resources should belong > > to the ACPI core, mostly to avoid code duplication. > > Rafael, > > So is the idea now that the ACPI core parses the resources and passes them > forward via struct acpi_device? I'm just wondering how to proceed with > these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: > > > > OK, but then we need to pass the information obtained from _CRS > > (presumably after some adjustments through _SRS) to drivers, or rather to > > things like the SPI core, I2C core etc. so that they can create device > > objects for drivers to bind to and quite frankly I don't see why not to use > > ACPI resources for that. > > Nevertheless, the routines for parsing those resources should belong > to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote:
> > How is the SPI controller different than this? Is there some logical
> > difference that requires a different framework? Or are you proposing
> > that we get rid of acpi_bus_register_driver() and migrate everything
> > to this new framework?
>
> Yes, I do, but let's just do it gradually.
Bjorn, here is a concrete example how this is supposed to be used.
Lets say we have an existing SPI slave driver that we want to extend to
support enumeration from ACPI. Instead of writing acpi_driver glue for that
(and registering it using acpi_bus_register_driver()) what we do is simple
add these to the existing SPI driver:
#ifdef CONFIG_ACPI
static struct acpi_device_id my_spidrv_match[] = {
/* ACPI IDs here */
{ }
};
MODULE_DEVICE_TABLE(acpi, my_spidrv_match);
#endif
static struct spi_driver my_spidrv = {
...
.driver = {
.acpi_match_table = ACPI_PTR(my_spidrv_match),
},
};
The same thing works with platform, I2c and SPI drivers and can be extented
to others as well. If the driver needs to do some ACPI specific
configuration it can get the ACPI handle using its dev->acpi_handle.
The above example now supports both, normal SPI (where the devices are
enumerated by populating spi_board_info()) and ACPI. Adding support for
Device Tree is similar than ACPI so a single driver can support all three
easily at the same time.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 06, 2012 at 11:18:11PM +0100, Rafael J. Wysocki wrote:
How is the SPI controller different than this? Is there some logical
difference that requires a different framework? Or are you proposing
that we get rid of acpi_bus_register_driver() and migrate everything
to this new framework?
Yes, I do, but let's just do it gradually.
Bjorn, here is a concrete example how this is supposed to be used.
Lets say we have an existing SPI slave driver that we want to extend to
support enumeration from ACPI. Instead of writing acpi_driver glue for that
(and registering it using acpi_bus_register_driver()) what we do is simple
add these to the existing SPI driver:
#ifdef CONFIG_ACPI
static struct acpi_device_id my_spidrv_match[] = {
/* ACPI IDs here */
{ }
};
MODULE_DEVICE_TABLE(acpi, my_spidrv_match);
#endif
static struct spi_driver my_spidrv = {
...
.driver = {
.acpi_match_table = ACPI_PTR(my_spidrv_match),
},
};
The same thing works with platform, I2c and SPI drivers and can be extented
to others as well. If the driver needs to do some ACPI specific
configuration it can get the ACPI handle using its dev-acpi_handle.
The above example now supports both, normal SPI (where the devices are
enumerated by populating spi_board_info()) and ACPI. Adding support for
Device Tree is similar than ACPI so a single driver can support all three
easily at the same time.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wednesday, November 07, 2012 11:58:42 AM Mika Westerberg wrote: On Tue, Nov 06, 2012 at 11:36:08PM +0100, Rafael J. Wysocki wrote: OK, but then we need to pass the information obtained from _CRS (presumably after some adjustments through _SRS) to drivers, or rather to things like the SPI core, I2C core etc. so that they can create device objects for drivers to bind to and quite frankly I don't see why not to use ACPI resources for that. Nevertheless, the routines for parsing those resources should belong to the ACPI core, mostly to avoid code duplication. Rafael, So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote: So is the idea now that the ACPI core parses the resources and passes them forward via struct acpi_device? I'm just wondering how to proceed with these I2C and SPI enumeration patches. Well, we definitely don't want to duplicate what drivers/pnp/pnpacpi/rsparser.c does, so the idea is to move the code from there to the core in such a way that both the SPI/I2C patches and the PNP layer can use it. Ok. I'll have some prototype code ready shortly, hopefully, and I'll post it in that form for comments (and so that you know what to expect). Sounds good. Thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Wednesday, November 07, 2012 03:05:48 PM Mika Westerberg wrote:
On Wed, Nov 07, 2012 at 12:14:31PM +0100, Rafael J. Wysocki wrote:
So is the idea now that the ACPI core parses the resources and passes them
forward via struct acpi_device? I'm just wondering how to proceed with
these I2C and SPI enumeration patches.
Well, we definitely don't want to duplicate what
drivers/pnp/pnpacpi/rsparser.c
does, so the idea is to move the code from there to the core in such a way
that
both the SPI/I2C patches and the PNP layer can use it.
Ok.
I'll have some prototype code ready shortly, hopefully, and I'll post it
in that form for comments (and so that you know what to expect).
Sounds good. Thanks!
There you go.
I haven't even try to compile it, so most likely it breaks things left, right
and in between, but I hope it shows the idea.
It does a couple of things at the same time, so it should be split into a few
simpler patches. First, it moves some code from drivers/pnp/pnpacpi/rsparser.c
to a new file drivers/acpi/resource.c and makes pnpacpi use functions from
there. Second, it changes acpi_platform.c to use those functions too.
Finally, it adds a list of ACPI resources to struct acpi_device and
makes acpi_platform.c use that list intead of evaluating _CRS and parsing its
output with acpi_walk_resources().
While changing acpi_platform.c I noticed that we had a bug in there, because
GSIs were registered for the struct acpi_device object, although they should be
registered for the struct platform_device one created by that code. I didn't
try to fix that in the patch below, but it generally needs fixing.
Thanks,
Rafael
Prototype, no sign-off.
---
drivers/acpi/Makefile |1
drivers/acpi/acpi_platform.c | 122 +-
drivers/acpi/resource.c| 269 +
drivers/acpi/scan.c| 48 +++
drivers/pnp/base.h |2
drivers/pnp/pnpacpi/rsparser.c | 150 +-
drivers/pnp/resource.c | 16 ++
include/acpi/acpi_bus.h|6
8 files changed, 386 insertions(+), 228 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -259,6 +259,11 @@ struct acpi_device_physical_node {
struct device *dev;
};
+struct acpi_resource_list_entry {
+ struct list_head node;
+ struct acpi_resource resource;
+};
+
/* set maximum of physical nodes to 32 for expansibility */
#define ACPI_MAX_PHYSICAL_NODE 32
@@ -269,6 +274,7 @@ struct acpi_device {
struct acpi_device *parent;
struct list_head children;
struct list_head node;
+ struct list_head resources; /* Device resources. */
struct list_head wakeup_list;
struct acpi_device_status status;
struct acpi_device_flags flags;
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -670,6 +670,8 @@ end:
static void acpi_device_unregister(struct acpi_device *device, int type)
{
+ struct acpi_resource_list_entry *entry, *s;
+
mutex_lock(acpi_device_lock);
if (device-parent)
list_del(device-node);
@@ -681,6 +683,9 @@ static void acpi_device_unregister(struc
acpi_device_remove_files(device);
device_unregister(device-dev);
+
+ list_for_each_entry_safe(entry, s, device-resources, node)
+ kfree(entry);
}
/* --
@@ -990,6 +995,40 @@ static void acpi_bus_get_wakeup_device_f
error in _DSW or _PSW evaluation\n));
}
+static acpi_status acpi_bus_add_resource(struct acpi_resource *res,
+void *context)
+{
+ struct list_head *list = context;
+ struct acpi_resource_list_entry *entry;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return AE_NO_MEMORY;
+
+ entry-resource = *res;
+ list_add_tail(entry-node, list);
+ return AE_OK;
+}
+
+static int acpi_bus_get_resources(struct acpi_device *adev)
+{
+ acpi_status status;
+ int ret = 0;
+
+ status = acpi_walk_resources(adev-handle, METHOD_NAME__CRS,
+acpi_bus_add_resource, adev-resources);
+ if (ACPI_FAILURE(status)) {
+ switch(status) {
+ case AE_NO_MEMORY:
+ ret = -ENOMEM;
+ break;
+ default:
+ ret = -EIO;
+ }
+ }
+ return ret;
+}
+
static void acpi_bus_add_power_resource(acpi_handle handle);
static int acpi_bus_get_power_flags(struct acpi_device *device)
@@
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 11:28:37 PM Rafael J. Wysocki wrote:
> On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote:
> > On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki wrote:
> > > On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
> > >> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki wrote:
> > >> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
> > >> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
> > >> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
> > >> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> > >> >> > > wrote:
> > >> >> > > > ACPI 5 introduced SPISerialBus resource that allows us to
> > >> >> > > > enumerate and
> > >> >> > > > configure the SPI slave devices behind the SPI controller. This
> > >> >> > > > patch adds
> > >> >> > > > support for this to the SPI core.
> > >> > [...]
> > >> >> > And if the ACPI core parses the _CRS, how does it pass all the
> > >> >> > resources to
> > >> >> > the drivers?
> > >> >>
> > >> >> Pretty much the same way the $subject patch does.
> > >> >>
> > >> >> Instead of parsing the entire subtree below an SPI controller and
> > >> >> trying
> > >> >> acpi_spi_add_device() for each device node in there, it could call
> > >> >> acpi_spi_add_device() whenever it finds a device of type
> > >> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
> > >> >> The only problem is how to pass "master" to it.
> > >> >>
> > >> >> So Bjorn, do you have any idea how we could pass the "master" pointer
> > >> >> from the
> > >> >> ACPI core to acpi_spi_add_device() in a sensible way?
> > >> >>
> > >> >> An alternative might be to store the information obtained from _CRS in
> > >> >> struct acpi_device objects created by the ACPI core while parsing the
> > >> >> namespace. We do that already for things like _PRW, so we might as
> > >> >> well do it
> > >> >> for _CRS. Then, the SPI core could just walk the subtree of the
> > >> >> device hierarchy
> > >> >> below the SPI controller's acpi_device to extract that information.
> > >> >> Maybe that's the way to go?
> > >> >
> > >> > The general idea is to move the _CRS parsing routine from
> > >> > acpi_platform.c
> > >> > to scan.c and make it attach resource objects to struct acpi_device.
> > >> >
> > >> > I'm thinking about adding a list head to struct acpi_device pointing
> > >> > to a
> > >> > list of entries like:
> > >> >
> > >> > struct resource_list_entry {
> > >> > struct list_head node;
> > >> > struct resource *resources;
> > >> > unsigned int count;
> > >> > };
> > >> >
> > >> > where "resources" is an array of resources (e.g. interrupts) in the
> > >> > given
> > >> > entry and count is the size of that array.
> > >> >
> > >> > That list would contain common resources like
> > >> > ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> > >> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
> > >> > ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
> > >>
> > >> This is exactly what PNPACPI already does. For every Device node in
> > >> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
> > >> struct resources that correspond to it. If you put this functionality
> > >> in acpi/scan.c, should we get rid of PNPACPI?
> > >
> > > Quite likely. At least this part of it, if you want the core to parse
> > > resources.
> > >
> > > That said, I actually tried to abstract out resource parsing in a more
> > > generic
> > > fashion on the basis of our new platform device support code, but quite
> > > frankly
> > > I wasn't able to.
> > >
> > > The problem is that struct resource is too simple to be useful for
> > > representing
> > > all of the information that can be encoded in ACPI resources. As a
> > > result, some
> > > information have to be stored directly in things like struct pnp_dev,
> > > struct
> > > platform_device etc. and if we want to represent it generically, the only
> > > way
> > > to do that seems to be using the ACPICA resource types as defined in
> > > acrestyp.h.
> >
> > Really? I didn't have that impression. It might be that the new GPIO
> > and SERIAL_BUS stuff makes it too complicated for a struct resource,
> > but prior to that, I don't think it was. It's true that there are
> > many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
> > only the core needs to be aware of the encoding of all those formats.
> > As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
> > MEM resources, and those fit easily in a struct resource.
>
> However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an
> array of struct resource objects before anyone actually needs it seems a
> bit wasteful to me. Let alone registering GSI for the interrupts while
> we're parsing those resources.
>
> > I want to expand on what I said before about _CRS being the domain of
> > the core, not drivers.
>
> Well, I see a difference between
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote:
> On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki wrote:
> > On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
> >> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki wrote:
> >> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
> >> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
> >> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
> >> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> >> >> > > wrote:
> >> >> > > > ACPI 5 introduced SPISerialBus resource that allows us to
> >> >> > > > enumerate and
> >> >> > > > configure the SPI slave devices behind the SPI controller. This
> >> >> > > > patch adds
> >> >> > > > support for this to the SPI core.
> >> > [...]
> >> >> > And if the ACPI core parses the _CRS, how does it pass all the
> >> >> > resources to
> >> >> > the drivers?
> >> >>
> >> >> Pretty much the same way the $subject patch does.
> >> >>
> >> >> Instead of parsing the entire subtree below an SPI controller and trying
> >> >> acpi_spi_add_device() for each device node in there, it could call
> >> >> acpi_spi_add_device() whenever it finds a device of type
> >> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
> >> >> The only problem is how to pass "master" to it.
> >> >>
> >> >> So Bjorn, do you have any idea how we could pass the "master" pointer
> >> >> from the
> >> >> ACPI core to acpi_spi_add_device() in a sensible way?
> >> >>
> >> >> An alternative might be to store the information obtained from _CRS in
> >> >> struct acpi_device objects created by the ACPI core while parsing the
> >> >> namespace. We do that already for things like _PRW, so we might as
> >> >> well do it
> >> >> for _CRS. Then, the SPI core could just walk the subtree of the device
> >> >> hierarchy
> >> >> below the SPI controller's acpi_device to extract that information.
> >> >> Maybe that's the way to go?
> >> >
> >> > The general idea is to move the _CRS parsing routine from acpi_platform.c
> >> > to scan.c and make it attach resource objects to struct acpi_device.
> >> >
> >> > I'm thinking about adding a list head to struct acpi_device pointing to a
> >> > list of entries like:
> >> >
> >> > struct resource_list_entry {
> >> > struct list_head node;
> >> > struct resource *resources;
> >> > unsigned int count;
> >> > };
> >> >
> >> > where "resources" is an array of resources (e.g. interrupts) in the given
> >> > entry and count is the size of that array.
> >> >
> >> > That list would contain common resources like
> >> > ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> >> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
> >> > ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
> >>
> >> This is exactly what PNPACPI already does. For every Device node in
> >> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
> >> struct resources that correspond to it. If you put this functionality
> >> in acpi/scan.c, should we get rid of PNPACPI?
> >
> > Quite likely. At least this part of it, if you want the core to parse
> > resources.
> >
> > That said, I actually tried to abstract out resource parsing in a more
> > generic
> > fashion on the basis of our new platform device support code, but quite
> > frankly
> > I wasn't able to.
> >
> > The problem is that struct resource is too simple to be useful for
> > representing
> > all of the information that can be encoded in ACPI resources. As a result,
> > some
> > information have to be stored directly in things like struct pnp_dev, struct
> > platform_device etc. and if we want to represent it generically, the only
> > way
> > to do that seems to be using the ACPICA resource types as defined in
> > acrestyp.h.
>
> Really? I didn't have that impression. It might be that the new GPIO
> and SERIAL_BUS stuff makes it too complicated for a struct resource,
> but prior to that, I don't think it was. It's true that there are
> many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
> only the core needs to be aware of the encoding of all those formats.
> As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
> MEM resources, and those fit easily in a struct resource.
However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an
array of struct resource objects before anyone actually needs it seems a
bit wasteful to me. Let alone registering GSI for the interrupts while
we're parsing those resources.
> I want to expand on what I said before about _CRS being the domain of
> the core, not drivers.
Well, I see a difference between _evaluating_ _CRS and _parsing_ its
output. In particular, I don't see a reason to do those two things in
one operation. In fact, I see reasons to do otherwise. :-)
> The core must evaluate _CRS to know where
> devices are and avoid conflicts when assigning resources. The core
> must evaluate _PRS and _SRS when
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 01:53:01 PM Bjorn Helgaas wrote:
> On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki wrote:
> > On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
> >> On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki wrote:
> >> > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
> >> >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> >> >> wrote:
> >> >> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate
> >> >> > and
> >> >> > configure the SPI slave devices behind the SPI controller. This patch
> >> >> > adds
> >> >> > support for this to the SPI core.
> >> >> >
> >> >> > In addition we bind ACPI nodes to SPI devices. This makes it possible
> >> >> > for
> >> >> > the slave drivers to get the ACPI handle for further configuration.
> >> >> >
> >> >> > Signed-off-by: Mika Westerberg
> >> >> > Acked-by: Rafael J. Wysocki
> >> >> > ---
> >> >> > drivers/spi/spi.c | 231
> >> >> > -
> >>
> >> >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
> >> >> > void *data)
> >> >> > +{
> >> >> > + struct acpi_spi_device_info *info = data;
> >> >> > + struct acpi_resource_spi_serialbus *sb;
> >> >> > + struct spi_device *spi = info->spi;
> >> >> > +
> >> >> > + switch (res->type) {
> >> >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> >> >> > + sb = >data.spi_serial_bus;
> >> >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
> >> >> > + spi->chip_select = sb->device_selection;
> >> >> > + spi->max_speed_hz = sb->connection_speed;
> >> >> > +
> >> >> > + /* Mode (clock phase/polarity/etc. */
> >> >> > + if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
> >> >> > + spi->mode |= SPI_CPHA;
> >> >> > + if (sb->clock_polarity == ACPI_SPI_START_HIGH)
> >> >> > + spi->mode |= SPI_CPOL;
> >> >> > + if (sb->device_polarity ==
> >> >> > ACPI_SPI_ACTIVE_HIGH)
> >> >> > + spi->mode |= SPI_CS_HIGH;
> >> >> > +
> >> >> > + /*
> >> >> > +* The info is valid once we have found the
> >> >> > +* SPISerialBus resource.
> >> >> > +*/
> >> >> > + info->valid = true;
> >> >> > + }
> >> >> > + break;
> >> >> > +
> >> >> > + case ACPI_RESOURCE_TYPE_IRQ:
> >> >> > + info->gsi = res->data.irq.interrupts[0];
> >> >> > + info->triggering = res->data.irq.triggering;
> >> >> > + info->polarity = res->data.irq.polarity;
> >> >> > + break;
> >> >> > +
> >> >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> >> >> > + info->gsi = res->data.extended_irq.interrupts[0];
> >> >> > + info->triggering = res->data.extended_irq.triggering;
> >> >> > + info->polarity = res->data.extended_irq.polarity;
> >> >>
> >> >> A driver doesn't seem like the right place for _CRS parsing code.
> >> >
> >> > This is not a driver, however. :-)
> >>
> >> OK. Can you describe what this is? I assume the picture involves an
> >> SPI master device and one or more SPI slave devices, and the ACPI
> >> namespace tells us something about them, and somehow this code is
> >> related to those devices because it's looking at _CRS for them.
> >
> > This is part of the SPI core that looks for SPI devices below a controller.
> >
> >> A _CRS method is associated with a Device in the namespace. What is
> >> that device in this case?
> >
> > An SPI device below a controller.
> >
> >> What is its PNP ID?
> >
> > I can't answer that from the top of my head, sorry.
> >
> >> What driver claims devices with that ID?
> >
> > The driver that declares support for it in its acpi_match_table[] array.
>
> OK, I guess my problem is in understanding the difference between (1)
> a platform device where we use the acpi_match_table[] to locate
> devices with matching ACPI IDs, and (2) a device where we use
> pnp_register_driver() or acpi_bus_register_driver() to locate devices
> with matching ACPI IDs.
>
> They seem similar to me. For example, take a PNP0A03 PCI host bridge.
> This seems like a platform device since there's no native hardware
> method to enumerate it. The ACPI namespace gives us a way to find it.
> We use acpi_bus_register_driver() to bind a driver to it. The driver
> has the struct acpi_device * and can access any ACPI state it needs.
> The driver supplies .add() and .remove() methods, so in principle the
> driver doesn't need to know anything about hotplug (assuming the ACPI
> core handles hotplug correctly, which it doesn't yet).
Well, I hope this is the last time I need to explain that. :-)
Please see this
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki wrote:
> On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
>> On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki wrote:
>> > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
>> >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>> >> wrote:
>> >> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
>> >> > configure the SPI slave devices behind the SPI controller. This patch
>> >> > adds
>> >> > support for this to the SPI core.
>> >> >
>> >> > In addition we bind ACPI nodes to SPI devices. This makes it possible
>> >> > for
>> >> > the slave drivers to get the ACPI handle for further configuration.
>> >> >
>> >> > Signed-off-by: Mika Westerberg
>> >> > Acked-by: Rafael J. Wysocki
>> >> > ---
>> >> > drivers/spi/spi.c | 231
>> >> > -
>>
>> >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
>> >> > void *data)
>> >> > +{
>> >> > + struct acpi_spi_device_info *info = data;
>> >> > + struct acpi_resource_spi_serialbus *sb;
>> >> > + struct spi_device *spi = info->spi;
>> >> > +
>> >> > + switch (res->type) {
>> >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
>> >> > + sb = >data.spi_serial_bus;
>> >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
>> >> > + spi->chip_select = sb->device_selection;
>> >> > + spi->max_speed_hz = sb->connection_speed;
>> >> > +
>> >> > + /* Mode (clock phase/polarity/etc. */
>> >> > + if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
>> >> > + spi->mode |= SPI_CPHA;
>> >> > + if (sb->clock_polarity == ACPI_SPI_START_HIGH)
>> >> > + spi->mode |= SPI_CPOL;
>> >> > + if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
>> >> > + spi->mode |= SPI_CS_HIGH;
>> >> > +
>> >> > + /*
>> >> > +* The info is valid once we have found the
>> >> > +* SPISerialBus resource.
>> >> > +*/
>> >> > + info->valid = true;
>> >> > + }
>> >> > + break;
>> >> > +
>> >> > + case ACPI_RESOURCE_TYPE_IRQ:
>> >> > + info->gsi = res->data.irq.interrupts[0];
>> >> > + info->triggering = res->data.irq.triggering;
>> >> > + info->polarity = res->data.irq.polarity;
>> >> > + break;
>> >> > +
>> >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>> >> > + info->gsi = res->data.extended_irq.interrupts[0];
>> >> > + info->triggering = res->data.extended_irq.triggering;
>> >> > + info->polarity = res->data.extended_irq.polarity;
>> >>
>> >> A driver doesn't seem like the right place for _CRS parsing code.
>> >
>> > This is not a driver, however. :-)
>>
>> OK. Can you describe what this is? I assume the picture involves an
>> SPI master device and one or more SPI slave devices, and the ACPI
>> namespace tells us something about them, and somehow this code is
>> related to those devices because it's looking at _CRS for them.
>
> This is part of the SPI core that looks for SPI devices below a controller.
>
>> A _CRS method is associated with a Device in the namespace. What is
>> that device in this case?
>
> An SPI device below a controller.
>
>> What is its PNP ID?
>
> I can't answer that from the top of my head, sorry.
>
>> What driver claims devices with that ID?
>
> The driver that declares support for it in its acpi_match_table[] array.
OK, I guess my problem is in understanding the difference between (1)
a platform device where we use the acpi_match_table[] to locate
devices with matching ACPI IDs, and (2) a device where we use
pnp_register_driver() or acpi_bus_register_driver() to locate devices
with matching ACPI IDs.
They seem similar to me. For example, take a PNP0A03 PCI host bridge.
This seems like a platform device since there's no native hardware
method to enumerate it. The ACPI namespace gives us a way to find it.
We use acpi_bus_register_driver() to bind a driver to it. The driver
has the struct acpi_device * and can access any ACPI state it needs.
The driver supplies .add() and .remove() methods, so in principle the
driver doesn't need to know anything about hotplug (assuming the ACPI
core handles hotplug correctly, which it doesn't yet).
How is the SPI controller different than this? Is there some logical
difference that requires a different framework? Or are you proposing
that we get rid of acpi_bus_register_driver() and migrate everything
to this new framework?
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki wrote:
> On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
>> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki wrote:
>> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
>> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
>> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
>> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>> >> > > wrote:
>> >> > > > ACPI 5 introduced SPISerialBus resource that allows us to enumerate
>> >> > > > and
>> >> > > > configure the SPI slave devices behind the SPI controller. This
>> >> > > > patch adds
>> >> > > > support for this to the SPI core.
>> > [...]
>> >> > And if the ACPI core parses the _CRS, how does it pass all the
>> >> > resources to
>> >> > the drivers?
>> >>
>> >> Pretty much the same way the $subject patch does.
>> >>
>> >> Instead of parsing the entire subtree below an SPI controller and trying
>> >> acpi_spi_add_device() for each device node in there, it could call
>> >> acpi_spi_add_device() whenever it finds a device of type
>> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
>> >> The only problem is how to pass "master" to it.
>> >>
>> >> So Bjorn, do you have any idea how we could pass the "master" pointer
>> >> from the
>> >> ACPI core to acpi_spi_add_device() in a sensible way?
>> >>
>> >> An alternative might be to store the information obtained from _CRS in
>> >> struct acpi_device objects created by the ACPI core while parsing the
>> >> namespace. We do that already for things like _PRW, so we might as well
>> >> do it
>> >> for _CRS. Then, the SPI core could just walk the subtree of the device
>> >> hierarchy
>> >> below the SPI controller's acpi_device to extract that information.
>> >> Maybe that's the way to go?
>> >
>> > The general idea is to move the _CRS parsing routine from acpi_platform.c
>> > to scan.c and make it attach resource objects to struct acpi_device.
>> >
>> > I'm thinking about adding a list head to struct acpi_device pointing to a
>> > list of entries like:
>> >
>> > struct resource_list_entry {
>> > struct list_head node;
>> > struct resource *resources;
>> > unsigned int count;
>> > };
>> >
>> > where "resources" is an array of resources (e.g. interrupts) in the given
>> > entry and count is the size of that array.
>> >
>> > That list would contain common resources like
>> > ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
>> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
>> > ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
>>
>> This is exactly what PNPACPI already does. For every Device node in
>> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
>> struct resources that correspond to it. If you put this functionality
>> in acpi/scan.c, should we get rid of PNPACPI?
>
> Quite likely. At least this part of it, if you want the core to parse
> resources.
>
> That said, I actually tried to abstract out resource parsing in a more generic
> fashion on the basis of our new platform device support code, but quite
> frankly
> I wasn't able to.
>
> The problem is that struct resource is too simple to be useful for
> representing
> all of the information that can be encoded in ACPI resources. As a result,
> some
> information have to be stored directly in things like struct pnp_dev, struct
> platform_device etc. and if we want to represent it generically, the only way
> to do that seems to be using the ACPICA resource types as defined in
> acrestyp.h.
Really? I didn't have that impression. It might be that the new GPIO
and SERIAL_BUS stuff makes it too complicated for a struct resource,
but prior to that, I don't think it was. It's true that there are
many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
only the core needs to be aware of the encoding of all those formats.
As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
MEM resources, and those fit easily in a struct resource.
I want to expand on what I said before about _CRS being the domain of
the core, not drivers. The core must evaluate _CRS to know where
devices are and avoid conflicts when assigning resources. The core
must evaluate _PRS and _SRS when making resource assignments. It
doesn't make sense for drivers to be using _PRS and _SRS; they don't
have a global view of resources, and any assignment they make would
likely cause conflicts with other devices. I think it makes sense to
consolidate all _CRS, _PRS, and _SRS management in the core rather
than splitting it up. This is exactly analogous to PCI BAR
management, and we don't intend drivers to read and write BARs
directly.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
> On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki wrote:
> > On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
> >> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
> >> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
> >> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> >> > > wrote:
> >> > > > ACPI 5 introduced SPISerialBus resource that allows us to enumerate
> >> > > > and
> >> > > > configure the SPI slave devices behind the SPI controller. This
> >> > > > patch adds
> >> > > > support for this to the SPI core.
> > [...]
> >> > And if the ACPI core parses the _CRS, how does it pass all the resources
> >> > to
> >> > the drivers?
> >>
> >> Pretty much the same way the $subject patch does.
> >>
> >> Instead of parsing the entire subtree below an SPI controller and trying
> >> acpi_spi_add_device() for each device node in there, it could call
> >> acpi_spi_add_device() whenever it finds a device of type
> >> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
> >> The only problem is how to pass "master" to it.
> >>
> >> So Bjorn, do you have any idea how we could pass the "master" pointer from
> >> the
> >> ACPI core to acpi_spi_add_device() in a sensible way?
> >>
> >> An alternative might be to store the information obtained from _CRS in
> >> struct acpi_device objects created by the ACPI core while parsing the
> >> namespace. We do that already for things like _PRW, so we might as well
> >> do it
> >> for _CRS. Then, the SPI core could just walk the subtree of the device
> >> hierarchy
> >> below the SPI controller's acpi_device to extract that information.
> >> Maybe that's the way to go?
> >
> > The general idea is to move the _CRS parsing routine from acpi_platform.c
> > to scan.c and make it attach resource objects to struct acpi_device.
> >
> > I'm thinking about adding a list head to struct acpi_device pointing to a
> > list of entries like:
> >
> > struct resource_list_entry {
> > struct list_head node;
> > struct resource *resources;
> > unsigned int count;
> > };
> >
> > where "resources" is an array of resources (e.g. interrupts) in the given
> > entry and count is the size of that array.
> >
> > That list would contain common resources like
> > ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> > ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
> > ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
>
> This is exactly what PNPACPI already does. For every Device node in
> the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
> struct resources that correspond to it. If you put this functionality
> in acpi/scan.c, should we get rid of PNPACPI?
Quite likely. At least this part of it, if you want the core to parse
resources.
That said, I actually tried to abstract out resource parsing in a more generic
fashion on the basis of our new platform device support code, but quite frankly
I wasn't able to.
The problem is that struct resource is too simple to be useful for representing
all of the information that can be encoded in ACPI resources. As a result, some
information have to be stored directly in things like struct pnp_dev, struct
platform_device etc. and if we want to represent it generically, the only way
to do that seems to be using the ACPICA resource types as defined in acrestyp.h.
So we can attach a list of things like
struct acpi_resource_list_entry {
struct list_head node;
struct acpi_resource resource;
};
to struct acpi_device, but that doesn't help much, because the different
bus types will then need to extract the information they need from that list
anyway. It would reduce the number of _CRS invocations, but beyond that I'm
not sure how usefult it's going to be.
Still, we probably should centralize things like
pnpacpi_parse_allocated_irqresource() to avoid duplicating that code.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
> On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki wrote:
> > On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
> >> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> >> wrote:
> >> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> >> > configure the SPI slave devices behind the SPI controller. This patch
> >> > adds
> >> > support for this to the SPI core.
> >> >
> >> > In addition we bind ACPI nodes to SPI devices. This makes it possible for
> >> > the slave drivers to get the ACPI handle for further configuration.
> >> >
> >> > Signed-off-by: Mika Westerberg
> >> > Acked-by: Rafael J. Wysocki
> >> > ---
> >> > drivers/spi/spi.c | 231
> >> > -
>
> >> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
> >> > void *data)
> >> > +{
> >> > + struct acpi_spi_device_info *info = data;
> >> > + struct acpi_resource_spi_serialbus *sb;
> >> > + struct spi_device *spi = info->spi;
> >> > +
> >> > + switch (res->type) {
> >> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
> >> > + sb = >data.spi_serial_bus;
> >> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
> >> > + spi->chip_select = sb->device_selection;
> >> > + spi->max_speed_hz = sb->connection_speed;
> >> > +
> >> > + /* Mode (clock phase/polarity/etc. */
> >> > + if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
> >> > + spi->mode |= SPI_CPHA;
> >> > + if (sb->clock_polarity == ACPI_SPI_START_HIGH)
> >> > + spi->mode |= SPI_CPOL;
> >> > + if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
> >> > + spi->mode |= SPI_CS_HIGH;
> >> > +
> >> > + /*
> >> > +* The info is valid once we have found the
> >> > +* SPISerialBus resource.
> >> > +*/
> >> > + info->valid = true;
> >> > + }
> >> > + break;
> >> > +
> >> > + case ACPI_RESOURCE_TYPE_IRQ:
> >> > + info->gsi = res->data.irq.interrupts[0];
> >> > + info->triggering = res->data.irq.triggering;
> >> > + info->polarity = res->data.irq.polarity;
> >> > + break;
> >> > +
> >> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
> >> > + info->gsi = res->data.extended_irq.interrupts[0];
> >> > + info->triggering = res->data.extended_irq.triggering;
> >> > + info->polarity = res->data.extended_irq.polarity;
> >>
> >> A driver doesn't seem like the right place for _CRS parsing code.
> >
> > This is not a driver, however. :-)
>
> OK. Can you describe what this is? I assume the picture involves an
> SPI master device and one or more SPI slave devices, and the ACPI
> namespace tells us something about them, and somehow this code is
> related to those devices because it's looking at _CRS for them.
This is part of the SPI core that looks for SPI devices below a controller.
> A _CRS method is associated with a Device in the namespace. What is
> that device in this case?
An SPI device below a controller.
> What is its PNP ID?
I can't answer that from the top of my head, sorry.
> What driver claims devices with that ID?
The driver that declares support for it in its acpi_match_table[] array.
> >> > +static void acpi_register_spi_devices(struct spi_master *master)
> >> > +{
> >> > + acpi_status status;
> >> > + acpi_handle handle;
> >> > +
> >> > + handle = master->dev.acpi_handle;
> >> > + if (!handle)
> >> > + return;
> >> > +
> >> > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master);
> >>
> >> How does this work with hot-plug? acpi_spi_enumerate() walks a
> >> portion of the namespace. How do we deal with changes to that part of
> >> the namespace? For example, what happens if this part of the
> >> namespace gets pruned because an enclosing device is removed? Is
> >> there a way to discover new SPI devices if they get added?
> >
> > Yes, there should be a way to do that eventually. No, we don't have any
> > removable SPI devices described by ACPI yet, as far as I know. So even if
> > we added code for that now, we wouldn't be able to test it anyway with any
> > real hardware until such devices become available. I have no idea when
> > that's
> > going to happen, though.
>
> I'm not asking about hotplug of devices on an SPI bus. I'm asking
> about hotplug of SPI masters. For example, let's say you hot-add a
> whole node that contains an SPI master. I assume there's some ACPI
> Device node that describes the SPI master, and a hot-add would add a
>
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 02:02:19PM +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > I've got practical systems where there are multiple buses physically > > connected, though in practice almost always only one is actually used at > > runtime when it's I2C and SPI there are some systems (usually with other > > buses) where you might want to use more than one bus. Not sure those > > buses will fit in here though. > Yeah, I just went through DSDT table of one of our machines and found a > device that actually has two I2CSerialBus connectors (and those are to the > same controller). What I'm not sure is that is it used to select between > two different addresses or doest the device really have two physical I2C > connections. That one is likely to be one device that responds on two addresses as others have mentioned. You do also get devices which have multiple physical interfaces for various reasons, such as having a dedicated bus for some functions that need to respond quickly. signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 02:02:19PM +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: I've got practical systems where there are multiple buses physically connected, though in practice almost always only one is actually used at runtime when it's I2C and SPI there are some systems (usually with other buses) where you might want to use more than one bus. Not sure those buses will fit in here though. Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. That one is likely to be one device that responds on two addresses as others have mentioned. You do also get devices which have multiple physical interfaces for various reasons, such as having a dedicated bus for some functions that need to respond quickly. signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch
adds
support for this to the SPI core.
In addition we bind ACPI nodes to SPI devices. This makes it possible for
the slave drivers to get the ACPI handle for further configuration.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/spi/spi.c | 231
-
+static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
void *data)
+{
+ struct acpi_spi_device_info *info = data;
+ struct acpi_resource_spi_serialbus *sb;
+ struct spi_device *spi = info-spi;
+
+ switch (res-type) {
+ case ACPI_RESOURCE_TYPE_SERIAL_BUS:
+ sb = res-data.spi_serial_bus;
+ if (sb-type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
+ spi-chip_select = sb-device_selection;
+ spi-max_speed_hz = sb-connection_speed;
+
+ /* Mode (clock phase/polarity/etc. */
+ if (sb-clock_phase == ACPI_SPI_SECOND_PHASE)
+ spi-mode |= SPI_CPHA;
+ if (sb-clock_polarity == ACPI_SPI_START_HIGH)
+ spi-mode |= SPI_CPOL;
+ if (sb-device_polarity == ACPI_SPI_ACTIVE_HIGH)
+ spi-mode |= SPI_CS_HIGH;
+
+ /*
+* The info is valid once we have found the
+* SPISerialBus resource.
+*/
+ info-valid = true;
+ }
+ break;
+
+ case ACPI_RESOURCE_TYPE_IRQ:
+ info-gsi = res-data.irq.interrupts[0];
+ info-triggering = res-data.irq.triggering;
+ info-polarity = res-data.irq.polarity;
+ break;
+
+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+ info-gsi = res-data.extended_irq.interrupts[0];
+ info-triggering = res-data.extended_irq.triggering;
+ info-polarity = res-data.extended_irq.polarity;
A driver doesn't seem like the right place for _CRS parsing code.
This is not a driver, however. :-)
OK. Can you describe what this is? I assume the picture involves an
SPI master device and one or more SPI slave devices, and the ACPI
namespace tells us something about them, and somehow this code is
related to those devices because it's looking at _CRS for them.
This is part of the SPI core that looks for SPI devices below a controller.
A _CRS method is associated with a Device in the namespace. What is
that device in this case?
An SPI device below a controller.
What is its PNP ID?
I can't answer that from the top of my head, sorry.
What driver claims devices with that ID?
The driver that declares support for it in its acpi_match_table[] array.
+static void acpi_register_spi_devices(struct spi_master *master)
+{
+ acpi_status status;
+ acpi_handle handle;
+
+ handle = master-dev.acpi_handle;
+ if (!handle)
+ return;
+
+ status = acpi_spi_enumerate(handle, acpi_spi_add_device, master);
How does this work with hot-plug? acpi_spi_enumerate() walks a
portion of the namespace. How do we deal with changes to that part of
the namespace? For example, what happens if this part of the
namespace gets pruned because an enclosing device is removed? Is
there a way to discover new SPI devices if they get added?
Yes, there should be a way to do that eventually. No, we don't have any
removable SPI devices described by ACPI yet, as far as I know. So even if
we added code for that now, we wouldn't be able to test it anyway with any
real hardware until such devices become available. I have no idea when
that's
going to happen, though.
I'm not asking about hotplug of devices on an SPI bus. I'm asking
about hotplug of SPI masters. For example, let's say you hot-add a
whole node that contains an SPI master. I assume there's some ACPI
Device node that describes the SPI master, and a hot-add would add a
subtree to the namespace that contains a new SPI master Device.
The master will be a platform device and again, we're going to add support
for the hotplug of those, but the SPI controllers that we currently can test
it with are not removable.
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate
and
configure the SPI slave devices behind the SPI controller. This
patch adds
support for this to the SPI core.
[...]
And if the ACPI core parses the _CRS, how does it pass all the resources
to
the drivers?
Pretty much the same way the $subject patch does.
Instead of parsing the entire subtree below an SPI controller and trying
acpi_spi_add_device() for each device node in there, it could call
acpi_spi_add_device() whenever it finds a device of type
ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
The only problem is how to pass master to it.
So Bjorn, do you have any idea how we could pass the master pointer from
the
ACPI core to acpi_spi_add_device() in a sensible way?
An alternative might be to store the information obtained from _CRS in
struct acpi_device objects created by the ACPI core while parsing the
namespace. We do that already for things like _PRW, so we might as well
do it
for _CRS. Then, the SPI core could just walk the subtree of the device
hierarchy
below the SPI controller's acpi_device to extract that information.
Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like
ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
This is exactly what PNPACPI already does. For every Device node in
the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
struct resources that correspond to it. If you put this functionality
in acpi/scan.c, should we get rid of PNPACPI?
Quite likely. At least this part of it, if you want the core to parse
resources.
That said, I actually tried to abstract out resource parsing in a more generic
fashion on the basis of our new platform device support code, but quite frankly
I wasn't able to.
The problem is that struct resource is too simple to be useful for representing
all of the information that can be encoded in ACPI resources. As a result, some
information have to be stored directly in things like struct pnp_dev, struct
platform_device etc. and if we want to represent it generically, the only way
to do that seems to be using the ACPICA resource types as defined in acrestyp.h.
So we can attach a list of things like
struct acpi_resource_list_entry {
struct list_head node;
struct acpi_resource resource;
};
to struct acpi_device, but that doesn't help much, because the different
bus types will then need to extract the information they need from that list
anyway. It would reduce the number of _CRS invocations, but beyond that I'm
not sure how usefult it's going to be.
Still, we probably should centralize things like
pnpacpi_parse_allocated_irqresource() to avoid duplicating that code.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate
and
configure the SPI slave devices behind the SPI controller. This
patch adds
support for this to the SPI core.
[...]
And if the ACPI core parses the _CRS, how does it pass all the
resources to
the drivers?
Pretty much the same way the $subject patch does.
Instead of parsing the entire subtree below an SPI controller and trying
acpi_spi_add_device() for each device node in there, it could call
acpi_spi_add_device() whenever it finds a device of type
ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
The only problem is how to pass master to it.
So Bjorn, do you have any idea how we could pass the master pointer
from the
ACPI core to acpi_spi_add_device() in a sensible way?
An alternative might be to store the information obtained from _CRS in
struct acpi_device objects created by the ACPI core while parsing the
namespace. We do that already for things like _PRW, so we might as well
do it
for _CRS. Then, the SPI core could just walk the subtree of the device
hierarchy
below the SPI controller's acpi_device to extract that information.
Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like
ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
This is exactly what PNPACPI already does. For every Device node in
the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
struct resources that correspond to it. If you put this functionality
in acpi/scan.c, should we get rid of PNPACPI?
Quite likely. At least this part of it, if you want the core to parse
resources.
That said, I actually tried to abstract out resource parsing in a more generic
fashion on the basis of our new platform device support code, but quite
frankly
I wasn't able to.
The problem is that struct resource is too simple to be useful for
representing
all of the information that can be encoded in ACPI resources. As a result,
some
information have to be stored directly in things like struct pnp_dev, struct
platform_device etc. and if we want to represent it generically, the only way
to do that seems to be using the ACPICA resource types as defined in
acrestyp.h.
Really? I didn't have that impression. It might be that the new GPIO
and SERIAL_BUS stuff makes it too complicated for a struct resource,
but prior to that, I don't think it was. It's true that there are
many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
only the core needs to be aware of the encoding of all those formats.
As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
MEM resources, and those fit easily in a struct resource.
I want to expand on what I said before about _CRS being the domain of
the core, not drivers. The core must evaluate _CRS to know where
devices are and avoid conflicts when assigning resources. The core
must evaluate _PRS and _SRS when making resource assignments. It
doesn't make sense for drivers to be using _PRS and _SRS; they don't
have a global view of resources, and any assignment they make would
likely cause conflicts with other devices. I think it makes sense to
consolidate all _CRS, _PRS, and _SRS management in the core rather
than splitting it up. This is exactly analogous to PCI BAR
management, and we don't intend drivers to read and write BARs
directly.
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch
adds
support for this to the SPI core.
In addition we bind ACPI nodes to SPI devices. This makes it possible
for
the slave drivers to get the ACPI handle for further configuration.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/spi/spi.c | 231
-
+static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
void *data)
+{
+ struct acpi_spi_device_info *info = data;
+ struct acpi_resource_spi_serialbus *sb;
+ struct spi_device *spi = info-spi;
+
+ switch (res-type) {
+ case ACPI_RESOURCE_TYPE_SERIAL_BUS:
+ sb = res-data.spi_serial_bus;
+ if (sb-type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
+ spi-chip_select = sb-device_selection;
+ spi-max_speed_hz = sb-connection_speed;
+
+ /* Mode (clock phase/polarity/etc. */
+ if (sb-clock_phase == ACPI_SPI_SECOND_PHASE)
+ spi-mode |= SPI_CPHA;
+ if (sb-clock_polarity == ACPI_SPI_START_HIGH)
+ spi-mode |= SPI_CPOL;
+ if (sb-device_polarity == ACPI_SPI_ACTIVE_HIGH)
+ spi-mode |= SPI_CS_HIGH;
+
+ /*
+* The info is valid once we have found the
+* SPISerialBus resource.
+*/
+ info-valid = true;
+ }
+ break;
+
+ case ACPI_RESOURCE_TYPE_IRQ:
+ info-gsi = res-data.irq.interrupts[0];
+ info-triggering = res-data.irq.triggering;
+ info-polarity = res-data.irq.polarity;
+ break;
+
+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+ info-gsi = res-data.extended_irq.interrupts[0];
+ info-triggering = res-data.extended_irq.triggering;
+ info-polarity = res-data.extended_irq.polarity;
A driver doesn't seem like the right place for _CRS parsing code.
This is not a driver, however. :-)
OK. Can you describe what this is? I assume the picture involves an
SPI master device and one or more SPI slave devices, and the ACPI
namespace tells us something about them, and somehow this code is
related to those devices because it's looking at _CRS for them.
This is part of the SPI core that looks for SPI devices below a controller.
A _CRS method is associated with a Device in the namespace. What is
that device in this case?
An SPI device below a controller.
What is its PNP ID?
I can't answer that from the top of my head, sorry.
What driver claims devices with that ID?
The driver that declares support for it in its acpi_match_table[] array.
OK, I guess my problem is in understanding the difference between (1)
a platform device where we use the acpi_match_table[] to locate
devices with matching ACPI IDs, and (2) a device where we use
pnp_register_driver() or acpi_bus_register_driver() to locate devices
with matching ACPI IDs.
They seem similar to me. For example, take a PNP0A03 PCI host bridge.
This seems like a platform device since there's no native hardware
method to enumerate it. The ACPI namespace gives us a way to find it.
We use acpi_bus_register_driver() to bind a driver to it. The driver
has the struct acpi_device * and can access any ACPI state it needs.
The driver supplies .add() and .remove() methods, so in principle the
driver doesn't need to know anything about hotplug (assuming the ACPI
core handles hotplug correctly, which it doesn't yet).
How is the SPI controller different than this? Is there some logical
difference that requires a different framework? Or are you proposing
that we get rid of acpi_bus_register_driver() and migrate everything
to this new framework?
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 01:53:01 PM Bjorn Helgaas wrote:
On Tue, Nov 6, 2012 at 6:16 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Monday, November 05, 2012 09:54:37 AM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate
and
configure the SPI slave devices behind the SPI controller. This patch
adds
support for this to the SPI core.
In addition we bind ACPI nodes to SPI devices. This makes it possible
for
the slave drivers to get the ACPI handle for further configuration.
Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/spi/spi.c | 231
-
+static acpi_status acpi_spi_add_resources(struct acpi_resource *res,
void *data)
+{
+ struct acpi_spi_device_info *info = data;
+ struct acpi_resource_spi_serialbus *sb;
+ struct spi_device *spi = info-spi;
+
+ switch (res-type) {
+ case ACPI_RESOURCE_TYPE_SERIAL_BUS:
+ sb = res-data.spi_serial_bus;
+ if (sb-type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
+ spi-chip_select = sb-device_selection;
+ spi-max_speed_hz = sb-connection_speed;
+
+ /* Mode (clock phase/polarity/etc. */
+ if (sb-clock_phase == ACPI_SPI_SECOND_PHASE)
+ spi-mode |= SPI_CPHA;
+ if (sb-clock_polarity == ACPI_SPI_START_HIGH)
+ spi-mode |= SPI_CPOL;
+ if (sb-device_polarity ==
ACPI_SPI_ACTIVE_HIGH)
+ spi-mode |= SPI_CS_HIGH;
+
+ /*
+* The info is valid once we have found the
+* SPISerialBus resource.
+*/
+ info-valid = true;
+ }
+ break;
+
+ case ACPI_RESOURCE_TYPE_IRQ:
+ info-gsi = res-data.irq.interrupts[0];
+ info-triggering = res-data.irq.triggering;
+ info-polarity = res-data.irq.polarity;
+ break;
+
+ case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
+ info-gsi = res-data.extended_irq.interrupts[0];
+ info-triggering = res-data.extended_irq.triggering;
+ info-polarity = res-data.extended_irq.polarity;
A driver doesn't seem like the right place for _CRS parsing code.
This is not a driver, however. :-)
OK. Can you describe what this is? I assume the picture involves an
SPI master device and one or more SPI slave devices, and the ACPI
namespace tells us something about them, and somehow this code is
related to those devices because it's looking at _CRS for them.
This is part of the SPI core that looks for SPI devices below a controller.
A _CRS method is associated with a Device in the namespace. What is
that device in this case?
An SPI device below a controller.
What is its PNP ID?
I can't answer that from the top of my head, sorry.
What driver claims devices with that ID?
The driver that declares support for it in its acpi_match_table[] array.
OK, I guess my problem is in understanding the difference between (1)
a platform device where we use the acpi_match_table[] to locate
devices with matching ACPI IDs, and (2) a device where we use
pnp_register_driver() or acpi_bus_register_driver() to locate devices
with matching ACPI IDs.
They seem similar to me. For example, take a PNP0A03 PCI host bridge.
This seems like a platform device since there's no native hardware
method to enumerate it. The ACPI namespace gives us a way to find it.
We use acpi_bus_register_driver() to bind a driver to it. The driver
has the struct acpi_device * and can access any ACPI state it needs.
The driver supplies .add() and .remove() methods, so in principle the
driver doesn't need to know anything about hotplug (assuming the ACPI
core handles hotplug correctly, which it doesn't yet).
Well, I hope this is the last time I need to explain that. :-)
Please see this message for detailed discussion:
http://marc.info/?l=linux-kernelm=135180467616074w=4
How is the SPI controller different than this? Is there some logical
difference that requires a different framework? Or are you proposing
that we get rid of acpi_bus_register_driver() and migrate everything
to this new framework?
Yes, I do, but let's just do it
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote:
On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to
enumerate and
configure the SPI slave devices behind the SPI controller. This
patch adds
support for this to the SPI core.
[...]
And if the ACPI core parses the _CRS, how does it pass all the
resources to
the drivers?
Pretty much the same way the $subject patch does.
Instead of parsing the entire subtree below an SPI controller and trying
acpi_spi_add_device() for each device node in there, it could call
acpi_spi_add_device() whenever it finds a device of type
ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
The only problem is how to pass master to it.
So Bjorn, do you have any idea how we could pass the master pointer
from the
ACPI core to acpi_spi_add_device() in a sensible way?
An alternative might be to store the information obtained from _CRS in
struct acpi_device objects created by the ACPI core while parsing the
namespace. We do that already for things like _PRW, so we might as
well do it
for _CRS. Then, the SPI core could just walk the subtree of the device
hierarchy
below the SPI controller's acpi_device to extract that information.
Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like
ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
This is exactly what PNPACPI already does. For every Device node in
the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
struct resources that correspond to it. If you put this functionality
in acpi/scan.c, should we get rid of PNPACPI?
Quite likely. At least this part of it, if you want the core to parse
resources.
That said, I actually tried to abstract out resource parsing in a more
generic
fashion on the basis of our new platform device support code, but quite
frankly
I wasn't able to.
The problem is that struct resource is too simple to be useful for
representing
all of the information that can be encoded in ACPI resources. As a result,
some
information have to be stored directly in things like struct pnp_dev, struct
platform_device etc. and if we want to represent it generically, the only
way
to do that seems to be using the ACPICA resource types as defined in
acrestyp.h.
Really? I didn't have that impression. It might be that the new GPIO
and SERIAL_BUS stuff makes it too complicated for a struct resource,
but prior to that, I don't think it was. It's true that there are
many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
only the core needs to be aware of the encoding of all those formats.
As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
MEM resources, and those fit easily in a struct resource.
However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an
array of struct resource objects before anyone actually needs it seems a
bit wasteful to me. Let alone registering GSI for the interrupts while
we're parsing those resources.
I want to expand on what I said before about _CRS being the domain of
the core, not drivers.
Well, I see a difference between _evaluating_ _CRS and _parsing_ its
output. In particular, I don't see a reason to do those two things in
one operation. In fact, I see reasons to do otherwise. :-)
The core must evaluate _CRS to know where
devices are and avoid conflicts when assigning resources. The core
must evaluate _PRS and _SRS when making resource assignments. It
doesn't make sense for drivers to be using _PRS and _SRS; they don't
have a global view of resources, and any assignment they make would
likely cause conflicts with other devices. I think it makes sense to
consolidate all _CRS, _PRS, and _SRS management in the core
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Tuesday, November 06, 2012 11:28:37 PM Rafael J. Wysocki wrote:
On Tuesday, November 06, 2012 01:35:56 PM Bjorn Helgaas wrote:
On Tue, Nov 6, 2012 at 6:43 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Monday, November 05, 2012 09:54:42 AM Bjorn Helgaas wrote:
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki r...@sisk.pl wrote:
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to
enumerate and
configure the SPI slave devices behind the SPI controller. This
patch adds
support for this to the SPI core.
[...]
And if the ACPI core parses the _CRS, how does it pass all the
resources to
the drivers?
Pretty much the same way the $subject patch does.
Instead of parsing the entire subtree below an SPI controller and
trying
acpi_spi_add_device() for each device node in there, it could call
acpi_spi_add_device() whenever it finds a device of type
ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
The only problem is how to pass master to it.
So Bjorn, do you have any idea how we could pass the master pointer
from the
ACPI core to acpi_spi_add_device() in a sensible way?
An alternative might be to store the information obtained from _CRS in
struct acpi_device objects created by the ACPI core while parsing the
namespace. We do that already for things like _PRW, so we might as
well do it
for _CRS. Then, the SPI core could just walk the subtree of the
device hierarchy
below the SPI controller's acpi_device to extract that information.
Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from
acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing
to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the
given
entry and count is the size of that array.
That list would contain common resources like
ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
This is exactly what PNPACPI already does. For every Device node in
the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
struct resources that correspond to it. If you put this functionality
in acpi/scan.c, should we get rid of PNPACPI?
Quite likely. At least this part of it, if you want the core to parse
resources.
That said, I actually tried to abstract out resource parsing in a more
generic
fashion on the basis of our new platform device support code, but quite
frankly
I wasn't able to.
The problem is that struct resource is too simple to be useful for
representing
all of the information that can be encoded in ACPI resources. As a
result, some
information have to be stored directly in things like struct pnp_dev,
struct
platform_device etc. and if we want to represent it generically, the only
way
to do that seems to be using the ACPICA resource types as defined in
acrestyp.h.
Really? I didn't have that impression. It might be that the new GPIO
and SERIAL_BUS stuff makes it too complicated for a struct resource,
but prior to that, I don't think it was. It's true that there are
many different formats (IO, FIXED_IO, MEMORY24, MEMORY32, etc.), but
only the core needs to be aware of the encoding of all those formats.
As far as a *driver* is concerned, there are only IRQ, DMA, IO, and
MEM resources, and those fit easily in a struct resource.
However, for example expanding ACPI_RESOURCE_TYPE_EXTENDED_IRQ into an
array of struct resource objects before anyone actually needs it seems a
bit wasteful to me. Let alone registering GSI for the interrupts while
we're parsing those resources.
I want to expand on what I said before about _CRS being the domain of
the core, not drivers.
Well, I see a difference between _evaluating_ _CRS and _parsing_ its
output. In particular, I don't see a reason to do those two things in
one operation. In fact, I see reasons to do otherwise. :-)
The core must evaluate _CRS to know where
devices are and avoid conflicts when assigning resources. The core
must evaluate _PRS and _SRS when making resource assignments. It
doesn't make sense for drivers to be using _PRS and _SRS; they don't
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 5, 2012 at 3:03 PM, Jean Delvare wrote: (...) >> If the addresses are consecutive like ours it makes sense >> to just instantiate one device and have the driver know that it will >> also be able to access address +1, +2 ... +n. So is it possible >> to group the consecutive related addresses after each other >> here at the acpi-spi level and just create a single device? > > We were actually discussing I2C here, although I admit not in the right > thread, and maybe some of this applies to SPI as well. This is my typo. The AB8500 example is indeed I2C. > So if multiple slave addresses must be > supported, please do not limit this support to consecutive addresses. I buy that ... in Nomadik we even have an instance of a single device with two discrete *physical* interfaces connected to it, on both of them it responds to the same address, but with totally different register layouts ... hardware engineers rule! :-) > There really is no reason to limit us that way anyway, as i2c-core > supports attaching any additional slave address to en existing > i2c_client using i2c_new_dummy(). Yes, this is how we do it in AB8500. We just take advantage of the fact that the addresses are consecutive. > Note for DDR3 DIMM SPD chips we currently have two different drivers > handling the two slave addresses (eeprom/at24 and jc42.) I don't know > if this is something that could be instantiated from ACPI. So it seems > we really have two different cases when a single chip replies to > multiple slave addresses: either they refer to the same function and we > want a single driver for all of them, or they are for unrelated > functions and we want separate drivers (and thus separate i2c_clients.) > Not sure how we can always handle that properly on the ACPI side. If they can be accessed concurrently and do not share any locks or code they should be separate I think? When they have strong dependencies we immediately sort of fall into the MFD class where you need a mediator I think. >> If the addresses are not consecutive I guess you need to have >> one device driver bind to several devices and return -EPROBE_DEFER >> until the driver has been probed for all of them or something like >> that, this is what multi-block GPIO drivers sometimes do. > > Consecutive or not makes no difference really, as long as the driver > can deduce the additional addresses from the main address or register > contents accessible from the main address. This has always been the > case so far. You're right. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 10:43:17AM -0700, Bjorn Helgaas wrote: > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > > achieve that. > > Using only _HID and _UID to guarantee uniqueness means you're relying > on a property of the BIOS, so you're vulnerable to BIOS bugs. > > If there's an ACPI Device for I2C adapters, why wouldn't you just use > its device name as set in acpi_device_register() (basically a _HID + > instance number)? That's a good point - we could change to use that instead (the platform code in linux-pm tree uses _HID + _UID but I guess it is pretty trivial to change). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, 5 Nov 2012 19:12:48 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: > > I2C core fears that you're mixing up everything ;) I2C adapter devices > > are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave > > devices. There's nothing wrong with i2c_clients sharing ->name, that's > > even how device driver matching is achieved. The uniqueness of > > i2c_clients is on their bus_id which is the combination of i2c adapter > > number and slave address (e.g. 0-0050) > > Yeah, I mixed I2C adapter and client. Thanks for correcting. > > So if we create one I2C adapter from the platform bus code as we do now and > then for each I2CSerialBus connector we create one I2C client (well, the > one that is created when i2c_new_device() is called), everything should > work, right? Yes. > Then I suggest that we have a list of serial bus resources in the struct > acpi_device and create the I2C clients based on that. > > > i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we > > usually append the base I/O address at the end of the name to guarantee > > that. ACPI will have to come up with something similar. > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > achieve that. Perfect. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 5, 2012 at 10:12 AM, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: >> On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: >> > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: >> > > >> > > In the ACPI namespace we have device nodes and serial interfaces below >> > > them. >> > > In the above case we see that a single device node supports two different >> > > interfaces and in that case we probably should create two different >> > > struct i2c_adapter objects for the same ACPI device node. >> > > >> > > Mika, what do you think? >> > >> > I agree. >> > >> > Only problem I see is that then we have two I2C adapter devices with the >> > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C >> > core thinks about that. >> >> I2C core fears that you're mixing up everything ;) I2C adapter devices >> are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave >> devices. There's nothing wrong with i2c_clients sharing ->name, that's >> even how device driver matching is achieved. The uniqueness of >> i2c_clients is on their bus_id which is the combination of i2c adapter >> number and slave address (e.g. 0-0050) > > Yeah, I mixed I2C adapter and client. Thanks for correcting. > > So if we create one I2C adapter from the platform bus code as we do now and > then for each I2CSerialBus connector we create one I2C client (well, the > one that is created when i2c_new_device() is called), everything should > work, right? > > Then I suggest that we have a list of serial bus resources in the struct > acpi_device and create the I2C clients based on that. > >> i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we >> usually append the base I/O address at the end of the name to guarantee >> that. ACPI will have to come up with something similar. > > It should already be unique in case of ACPI. We use ACPI _HID and _UID to > achieve that. Using only _HID and _UID to guarantee uniqueness means you're relying on a property of the BIOS, so you're vulnerable to BIOS bugs. If there's an ACPI Device for I2C adapters, why wouldn't you just use its device name as set in acpi_device_register() (basically a _HID + instance number)? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 04:19:20PM +0100, Jean Delvare wrote: > On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: > > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > > > > > In the ACPI namespace we have device nodes and serial interfaces below > > > them. > > > In the above case we see that a single device node supports two different > > > interfaces and in that case we probably should create two different > > > struct i2c_adapter objects for the same ACPI device node. > > > > > > Mika, what do you think? > > > > I agree. > > > > Only problem I see is that then we have two I2C adapter devices with the > > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C > > core thinks about that. > > I2C core fears that you're mixing up everything ;) I2C adapter devices > are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave > devices. There's nothing wrong with i2c_clients sharing ->name, that's > even how device driver matching is achieved. The uniqueness of > i2c_clients is on their bus_id which is the combination of i2c adapter > number and slave address (e.g. 0-0050) Yeah, I mixed I2C adapter and client. Thanks for correcting. So if we create one I2C adapter from the platform bus code as we do now and then for each I2CSerialBus connector we create one I2C client (well, the one that is created when i2c_new_device() is called), everything should work, right? Then I suggest that we have a list of serial bus resources in the struct acpi_device and create the I2C clients based on that. > i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we > usually append the base I/O address at the end of the name to guarantee > that. ACPI will have to come up with something similar. It should already be unique in case of ACPI. We use ACPI _HID and _UID to achieve that. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 3, 2012 at 2:39 PM, Rafael J. Wysocki wrote:
> On Saturday, November 03, 2012 01:42:02 PM Bjorn Helgaas wrote:
>> On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>> wrote:
>> > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
>> > configure the SPI slave devices behind the SPI controller. This patch adds
>> > support for this to the SPI core.
>> >
>> > In addition we bind ACPI nodes to SPI devices. This makes it possible for
>> > the slave drivers to get the ACPI handle for further configuration.
>> >
>> > Signed-off-by: Mika Westerberg
>> > Acked-by: Rafael J. Wysocki
>> > ---
>> > drivers/spi/spi.c | 231
>> > -
>> > +static acpi_status acpi_spi_add_resources(struct acpi_resource *res, void
>> > *data)
>> > +{
>> > + struct acpi_spi_device_info *info = data;
>> > + struct acpi_resource_spi_serialbus *sb;
>> > + struct spi_device *spi = info->spi;
>> > +
>> > + switch (res->type) {
>> > + case ACPI_RESOURCE_TYPE_SERIAL_BUS:
>> > + sb = >data.spi_serial_bus;
>> > + if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_SPI) {
>> > + spi->chip_select = sb->device_selection;
>> > + spi->max_speed_hz = sb->connection_speed;
>> > +
>> > + /* Mode (clock phase/polarity/etc. */
>> > + if (sb->clock_phase == ACPI_SPI_SECOND_PHASE)
>> > + spi->mode |= SPI_CPHA;
>> > + if (sb->clock_polarity == ACPI_SPI_START_HIGH)
>> > + spi->mode |= SPI_CPOL;
>> > + if (sb->device_polarity == ACPI_SPI_ACTIVE_HIGH)
>> > + spi->mode |= SPI_CS_HIGH;
>> > +
>> > + /*
>> > +* The info is valid once we have found the
>> > +* SPISerialBus resource.
>> > +*/
>> > + info->valid = true;
>> > + }
>> > + break;
>> > +
>> > + case ACPI_RESOURCE_TYPE_IRQ:
>> > + info->gsi = res->data.irq.interrupts[0];
>> > + info->triggering = res->data.irq.triggering;
>> > + info->polarity = res->data.irq.polarity;
>> > + break;
>> > +
>> > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ:
>> > + info->gsi = res->data.extended_irq.interrupts[0];
>> > + info->triggering = res->data.extended_irq.triggering;
>> > + info->polarity = res->data.extended_irq.polarity;
>>
>> A driver doesn't seem like the right place for _CRS parsing code.
>
> This is not a driver, however. :-)
OK. Can you describe what this is? I assume the picture involves an
SPI master device and one or more SPI slave devices, and the ACPI
namespace tells us something about them, and somehow this code is
related to those devices because it's looking at _CRS for them.
A _CRS method is associated with a Device in the namespace. What is
that device in this case? What is its PNP ID? What driver claims
devices with that ID?
>> > +static void acpi_register_spi_devices(struct spi_master *master)
>> > +{
>> > + acpi_status status;
>> > + acpi_handle handle;
>> > +
>> > + handle = master->dev.acpi_handle;
>> > + if (!handle)
>> > + return;
>> > +
>> > + status = acpi_spi_enumerate(handle, acpi_spi_add_device, master);
>>
>> How does this work with hot-plug? acpi_spi_enumerate() walks a
>> portion of the namespace. How do we deal with changes to that part of
>> the namespace? For example, what happens if this part of the
>> namespace gets pruned because an enclosing device is removed? Is
>> there a way to discover new SPI devices if they get added?
>
> Yes, there should be a way to do that eventually. No, we don't have any
> removable SPI devices described by ACPI yet, as far as I know. So even if
> we added code for that now, we wouldn't be able to test it anyway with any
> real hardware until such devices become available. I have no idea when that's
> going to happen, though.
I'm not asking about hotplug of devices on an SPI bus. I'm asking
about hotplug of SPI masters. For example, let's say you hot-add a
whole node that contains an SPI master. I assume there's some ACPI
Device node that describes the SPI master, and a hot-add would add a
subtree to the namespace that contains a new SPI master Device.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 5, 2012 at 3:31 AM, Rafael J. Wysocki wrote:
> On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
>> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
>> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
>> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
>> > > wrote:
>> > > > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
>> > > > configure the SPI slave devices behind the SPI controller. This patch
>> > > > adds
>> > > > support for this to the SPI core.
> [...]
>> > And if the ACPI core parses the _CRS, how does it pass all the resources to
>> > the drivers?
>>
>> Pretty much the same way the $subject patch does.
>>
>> Instead of parsing the entire subtree below an SPI controller and trying
>> acpi_spi_add_device() for each device node in there, it could call
>> acpi_spi_add_device() whenever it finds a device of type
>> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
>> The only problem is how to pass "master" to it.
>>
>> So Bjorn, do you have any idea how we could pass the "master" pointer from
>> the
>> ACPI core to acpi_spi_add_device() in a sensible way?
>>
>> An alternative might be to store the information obtained from _CRS in
>> struct acpi_device objects created by the ACPI core while parsing the
>> namespace. We do that already for things like _PRW, so we might as well do
>> it
>> for _CRS. Then, the SPI core could just walk the subtree of the device
>> hierarchy
>> below the SPI controller's acpi_device to extract that information.
>> Maybe that's the way to go?
>
> The general idea is to move the _CRS parsing routine from acpi_platform.c
> to scan.c and make it attach resource objects to struct acpi_device.
>
> I'm thinking about adding a list head to struct acpi_device pointing to a
> list of entries like:
>
> struct resource_list_entry {
> struct list_head node;
> struct resource *resources;
> unsigned int count;
> };
>
> where "resources" is an array of resources (e.g. interrupts) in the given
> entry and count is the size of that array.
>
> That list would contain common resources like
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
> ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
This is exactly what PNPACPI already does. For every Device node in
the namespace, pnpacpi/rsparser.c parses _CRS and builds a list of
struct resources that correspond to it. If you put this functionality
in acpi/scan.c, should we get rid of PNPACPI?
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, 5 Nov 2012 16:53:15 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > > > In the ACPI namespace we have device nodes and serial interfaces below them. > > In the above case we see that a single device node supports two different > > interfaces and in that case we probably should create two different > > struct i2c_adapter objects for the same ACPI device node. > > > > Mika, what do you think? > > I agree. > > Only problem I see is that then we have two I2C adapter devices with the > same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C > core thinks about that. I2C core fears that you're mixing up everything ;) I2C adapter devices are struct i2c_adapter aka i2c-0, i2c-1 etc. i2c_client is for slave devices. There's nothing wrong with i2c_clients sharing ->name, that's even how device driver matching is achieved. The uniqueness of i2c_clients is on their bus_id which is the combination of i2c adapter number and slave address (e.g. 0-0050) i2c_adapter->name should, OTOH, be unique. In i2c bus drivers we usually append the base I/O address at the end of the name to guarantee that. ACPI will have to come up with something similar. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 03:19:58PM +0100, Rafael J. Wysocki wrote: > > In the ACPI namespace we have device nodes and serial interfaces below them. > In the above case we see that a single device node supports two different > interfaces and in that case we probably should create two different > struct i2c_adapter objects for the same ACPI device node. > > Mika, what do you think? I agree. Only problem I see is that then we have two I2C adapter devices with the same ACPI ID (and hence the same i2c_client->name). I wonder what the I2C core thinks about that. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 03:03:26 PM Jean Delvare wrote: > Hi Linus, > > On Mon, 5 Nov 2012 14:20:52 +0100, Linus Walleij wrote: > > On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg > > wrote: > > > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > > >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > > >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > (...) > > >> > > Yeah, I just went through DSDT table of one of our machines and > > >> > > found a > > >> > > device that actually has two I2CSerialBus connectors (and those are > > >> > > to the > > >> > > same controller). What I'm not sure is that is it used to select > > >> > > between > > >> > > two different addresses or doest the device really have two physical > > >> > > I2C > > >> > > connections. > > >> > > > >> > Neither would make sense from a hardware perspective. > > >> > > >> Well, interesting. :-) > > > > > > It looks like some PMICs for example have two I2C control interfaces, like > > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > > controller with different address, you have the situation like above. > > Ah, OK, if this is a degenerated case of a more complex initial design > then yes it makes some sense. I had never met chips like that and did > not know such chips existed. > > That being said, from a software perspective, there is no difference > between one or two I2C pin pairs being soldered. All we care about is > which master they are ultimately connected to, and to which slave > address(es) the chip replies. > > > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > > has this. The reason is usually that the device has more than 256 registers > > to the address space simply is not big enough. > > This is completely different from the case being discussed above. Even > the most complex addressing scheme can be implemented using a single > hardware I2C interface. You can use 16-bit register addresses (if you > can live without SMBus compatibility, AT24C32 and larger EEPROMs do > that), or implement internal banks (using one of the registers as the > bank selector, hardware monitoring chips do that), or you can use > multiple slave addresses (AT24C04 to C16 EEPROMs do that.) > > > What we do is refer to the subaddresses as "banks" and they happen > > to always be at the next consecutive address so n+1. > > > > So the same device appear behind several addresses just to support > > a lot of registers. > > > > So you're not actually modelling the devices on the other end but the > > multiple addresses of a single device. > > > > If the addresses are consecutive like ours it makes sense > > to just instantiate one device and have the driver know that it will > > also be able to access address +1, +2 ... +n. So is it possible > > to group the consecutive related addresses after each other > > here at the acpi-spi level and just create a single device? > > We were actually discussing I2C here, although I admit not in the right > thread, and maybe some of this applies to SPI as well. > > There are I2C devices replying to multiple non-consecutive slave > addresses. Most notably Winbond hardware monitoring chips replying to > one address in 0x2c-0x2f and 2 addresses in 0x48-0x4f. And of course > there are the DDR3 DIMM SPD chips which have an EEPROM at 0x50-0x57 and > a thermal sensor at 0x18-0x1f. So if multiple slave addresses must be > supported, please do not limit this support to consecutive addresses. > There really is no reason to limit us that way anyway, as i2c-core > supports attaching any additional slave address to en existing > i2c_client using i2c_new_dummy(). > > Note for DDR3 DIMM SPD chips we currently have two different drivers > handling the two slave addresses (eeprom/at24 and jc42.) I don't know > if this is something that could be instantiated from ACPI. So it seems > we really have two different cases when a single chip replies to > multiple slave addresses: either they refer to the same function and we > want a single driver for all of them, or they are for unrelated > functions and we want separate drivers (and thus separate i2c_clients.) > Not sure how we can always handle that properly on the ACPI side. > > > If the addresses are not consecutive I guess you need to have > > one device driver bind to several devices and return -EPROBE_DEFER > > until the driver has been probed for all of them or something like > > that, this is what multi-block GPIO drivers sometimes do. > > Consecutive or not makes no difference really, as long as the driver > can deduce the additional addresses from the main address or register > contents accessible from the main address. This has always been the > case so far. In the ACPI namespace we have device nodes and serial interfaces below them. In the above case we see that a single device node
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
Hi Linus, On Mon, 5 Nov 2012 14:20:52 +0100, Linus Walleij wrote: > On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg > wrote: > > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > (...) > >> > > Yeah, I just went through DSDT table of one of our machines and found a > >> > > device that actually has two I2CSerialBus connectors (and those are to > >> > > the > >> > > same controller). What I'm not sure is that is it used to select > >> > > between > >> > > two different addresses or doest the device really have two physical > >> > > I2C > >> > > connections. > >> > > >> > Neither would make sense from a hardware perspective. > >> > >> Well, interesting. :-) > > > > It looks like some PMICs for example have two I2C control interfaces, like > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > controller with different address, you have the situation like above. Ah, OK, if this is a degenerated case of a more complex initial design then yes it makes some sense. I had never met chips like that and did not know such chips existed. That being said, from a software perspective, there is no difference between one or two I2C pin pairs being soldered. All we care about is which master they are ultimately connected to, and to which slave address(es) the chip replies. > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > has this. The reason is usually that the device has more than 256 registers > to the address space simply is not big enough. This is completely different from the case being discussed above. Even the most complex addressing scheme can be implemented using a single hardware I2C interface. You can use 16-bit register addresses (if you can live without SMBus compatibility, AT24C32 and larger EEPROMs do that), or implement internal banks (using one of the registers as the bank selector, hardware monitoring chips do that), or you can use multiple slave addresses (AT24C04 to C16 EEPROMs do that.) > What we do is refer to the subaddresses as "banks" and they happen > to always be at the next consecutive address so n+1. > > So the same device appear behind several addresses just to support > a lot of registers. > > So you're not actually modelling the devices on the other end but the > multiple addresses of a single device. > > If the addresses are consecutive like ours it makes sense > to just instantiate one device and have the driver know that it will > also be able to access address +1, +2 ... +n. So is it possible > to group the consecutive related addresses after each other > here at the acpi-spi level and just create a single device? We were actually discussing I2C here, although I admit not in the right thread, and maybe some of this applies to SPI as well. There are I2C devices replying to multiple non-consecutive slave addresses. Most notably Winbond hardware monitoring chips replying to one address in 0x2c-0x2f and 2 addresses in 0x48-0x4f. And of course there are the DDR3 DIMM SPD chips which have an EEPROM at 0x50-0x57 and a thermal sensor at 0x18-0x1f. So if multiple slave addresses must be supported, please do not limit this support to consecutive addresses. There really is no reason to limit us that way anyway, as i2c-core supports attaching any additional slave address to en existing i2c_client using i2c_new_dummy(). Note for DDR3 DIMM SPD chips we currently have two different drivers handling the two slave addresses (eeprom/at24 and jc42.) I don't know if this is something that could be instantiated from ACPI. So it seems we really have two different cases when a single chip replies to multiple slave addresses: either they refer to the same function and we want a single driver for all of them, or they are for unrelated functions and we want separate drivers (and thus separate i2c_clients.) Not sure how we can always handle that properly on the ACPI side. > If the addresses are not consecutive I guess you need to have > one device driver bind to several devices and return -EPROBE_DEFER > until the driver has been probed for all of them or something like > that, this is what multi-block GPIO drivers sometimes do. Consecutive or not makes no difference really, as long as the driver can deduce the additional addresses from the main address or register contents accessible from the main address. This has always been the case so far. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 02:20:52PM +0100, Linus Walleij wrote: > > > > It looks like some PMICs for example have two I2C control interfaces, like > > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > > controller with different address, you have the situation like above. > > This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) > has this. The reason is usually that the device has more than 256 registers > to the address space simply is not big enough. > > What we do is refer to the subaddresses as "banks" and they happen > to always be at the next consecutive address so n+1. > > So the same device appear behind several addresses just to support > a lot of registers. > > So you're not actually modelling the devices on the other end but the > multiple addresses of a single device. That makes sense. Thanks for the explanation. > If the addresses are consecutive like ours it makes sense > to just instantiate one device and have the driver know that it will > also be able to access address +1, +2 ... +n. So is it possible > to group the consecutive related addresses after each other > here at the acpi-spi level and just create a single device? Not sure if it should be done there, unless we really can be sure that we have a single device with multiple addresses (and that is usually something that is only known by the actual driver) and not some device that has two unrelated interfaces connecting the same controller. > If the addresses are not consecutive I guess you need to have > one device driver bind to several devices and return -EPROBE_DEFER > until the driver has been probed for all of them or something like > that, this is what multi-block GPIO drivers sometimes do. The addresses in the DSDT table for this particular device are not consecutive so we could do something like you propose above. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: >> On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: >> > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: >> > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: (...) >> > > Yeah, I just went through DSDT table of one of our machines and found a >> > > device that actually has two I2CSerialBus connectors (and those are to >> > > the >> > > same controller). What I'm not sure is that is it used to select between >> > > two different addresses or doest the device really have two physical I2C >> > > connections. >> > >> > Neither would make sense from a hardware perspective. >> >> Well, interesting. :-) > > It looks like some PMICs for example have two I2C control interfaces, like > TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C > controller with different address, you have the situation like above. This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) has this. The reason is usually that the device has more than 256 registers to the address space simply is not big enough. What we do is refer to the subaddresses as "banks" and they happen to always be at the next consecutive address so n+1. So the same device appear behind several addresses just to support a lot of registers. So you're not actually modelling the devices on the other end but the multiple addresses of a single device. If the addresses are consecutive like ours it makes sense to just instantiate one device and have the driver know that it will also be able to access address +1, +2 ... +n. So is it possible to group the consecutive related addresses after each other here at the acpi-spi level and just create a single device? If the addresses are not consecutive I guess you need to have one device driver bind to several devices and return -EPROBE_DEFER until the driver has been probed for all of them or something like that, this is what multi-block GPIO drivers sometimes do. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: > On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > > > I've got practical systems where there are multiple buses physically > > > > connected, though in practice almost always only one is actually used at > > > > runtime when it's I2C and SPI there are some systems (usually with other > > > > buses) where you might want to use more than one bus. Not sure those > > > > buses will fit in here though. > > > > > > Yeah, I just went through DSDT table of one of our machines and found a > > > device that actually has two I2CSerialBus connectors (and those are to the > > > same controller). What I'm not sure is that is it used to select between > > > two different addresses or doest the device really have two physical I2C > > > connections. > > > > Neither would make sense from a hardware perspective. > > Well, interesting. :-) It looks like some PMICs for example have two I2C control interfaces, like TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C controller with different address, you have the situation like above. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: > On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > > I've got practical systems where there are multiple buses physically > > > connected, though in practice almost always only one is actually used at > > > runtime when it's I2C and SPI there are some systems (usually with other > > > buses) where you might want to use more than one bus. Not sure those > > > buses will fit in here though. > > > > Yeah, I just went through DSDT table of one of our machines and found a > > device that actually has two I2CSerialBus connectors (and those are to the > > same controller). What I'm not sure is that is it used to select between > > two different addresses or doest the device really have two physical I2C > > connections. > > Neither would make sense from a hardware perspective. Well, interesting. :-) I wonder what we're supposed to do with things like that? It looks like our patch [3/3] will use the last one found, but is that correct? Should it use the first one instead, perhaps? That little conundrum aside, the observations above seem to indicate that we'd need a list of "serial" resources per struct acpi_device too. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: > > I've got practical systems where there are multiple buses physically > > connected, though in practice almost always only one is actually used at > > runtime when it's I2C and SPI there are some systems (usually with other > > buses) where you might want to use more than one bus. Not sure those > > buses will fit in here though. > > Yeah, I just went through DSDT table of one of our machines and found a > device that actually has two I2CSerialBus connectors (and those are to the > same controller). What I'm not sure is that is it used to select between > two different addresses or doest the device really have two physical I2C > connections. Neither would make sense from a hardware perspective. -- Jean Delvare -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote:
> On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote:
>
> > > struct acpi_device {
> > > ...
> > > union acpi_resource_serial_bus *serial;
> > > ...
> > > };
>
> > It is also possible to have several serial bus connectors on a single
> > device (although we've seen only one connector per device but it should not
> > be limited to that).
>
> I've got practical systems where there are multiple buses physically
> connected, though in practice almost always only one is actually used at
> runtime when it's I2C and SPI there are some systems (usually with other
> buses) where you might want to use more than one bus. Not sure those
> buses will fit in here though.
Yeah, I just went through DSDT table of one of our machines and found a
device that actually has two I2CSerialBus connectors (and those are to the
same controller). What I'm not sure is that is it used to select between
two different addresses or doest the device really have two physical I2C
connections.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 01:03:22PM +0200, Mika Westerberg wrote: > On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: > > On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > > + if (info.gsi >= 0) > > > + spi->irq = acpi_register_gsi(>dev, info.gsi, > > > + info.triggering, info.polarity); > > > + request_module(spi->modalias); > > request_module()? Why? > The DT code does the same. I have no idea why it is there, really :-) > I can remove it in the next version if you think it is not needed. Well, if you can't identify what it does... signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: > On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > > > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > > + if (info.gsi >= 0) > > + spi->irq = acpi_register_gsi(>dev, info.gsi, > > +info.triggering, info.polarity); > > + request_module(spi->modalias); > > request_module()? Why? The DT code does the same. I have no idea why it is there, really :-) I can remove it in the next version if you think it is not needed. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote:
> > struct acpi_device {
> > ...
> > union acpi_resource_serial_bus *serial;
> > ...
> > };
> It is also possible to have several serial bus connectors on a single
> device (although we've seen only one connector per device but it should not
> be limited to that).
I've got practical systems where there are multiple buses physically
connected, though in practice almost always only one is actually used at
runtime when it's I2C and SPI there are some systems (usually with other
buses) where you might want to use more than one bus. Not sure those
buses will fit in here though.
signature.asc
Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: > + strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > + if (info.gsi >= 0) > + spi->irq = acpi_register_gsi(>dev, info.gsi, > + info.triggering, info.polarity); > + request_module(spi->modalias); request_module()? Why? signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 03, 2012 at 10:13:10PM +0200, Mika Westerberg wrote: > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: > > > + case ACPI_RESOURCE_TYPE_IRQ: > > > + info->gsi = res->data.irq.interrupts[0]; > > > + info->triggering = res->data.irq.triggering; > > > + info->polarity = res->data.irq.polarity; > > > + break; > > > + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: > > > + info->gsi = res->data.extended_irq.interrupts[0]; > > > + info->triggering = res->data.extended_irq.triggering; > > > + info->polarity = res->data.extended_irq.polarity; > > A driver doesn't seem like the right place for _CRS parsing code. I > > think the intent of _CRS is to describe resources that need to be > > coordinated across all devices, e.g., MMIO space, I/O port space, and > > IRQs. Since these resources require system-wide coordination, even > > when we don't have drivers for some devices, the ACPI core should be > > able to parse _CRS without needing any device-specific knowledge. > I think the driver is the only one who really knows the resources it needs > in order to talk the hardware. Generic SPI drivers expect the subsystem to supply them with an interrupt number; if there is a single interrupt it seems reasonable for the generic code to continue to do that for them when they're instantiated from ACPI. On the other hand if there are muliple interrupts it should probably give up and punt to the driver. > The purpose of the above code is to extract the resources in a suitable > form so that we can create a struct spi_device out of them automatically, > in a similar way than the Device Tree does. Definitely agreed. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:31:19AM +0100, Rafael J. Wysocki wrote:
> The general idea is to move the _CRS parsing routine from acpi_platform.c
> to scan.c and make it attach resource objects to struct acpi_device.
>
> I'm thinking about adding a list head to struct acpi_device pointing to a
> list of entries like:
>
> struct resource_list_entry {
> struct list_head node;
> struct resource *resources;
> unsigned int count;
> };
>
> where "resources" is an array of resources (e.g. interrupts) in the given
> entry and count is the size of that array.
>
> That list would contain common resources like
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
> ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
> ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
> I think adding it would allow us to make acpi_create_platform_device(),
> acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward
> (and
> remove some code duplication between the last two routines).
This certainly sounds good to me.
> In addition to that, I'd add an entry containing serial bus information, if
> applicable, for the given struct acpi_device, something like:
>
> union acpi_resource_serial_bus {
> struct acpi_resource_common_serialbus;
> struct acpi_resource_spi_serialbus;
> struct acpi_resource_i2c_serialbus;
> struct acpi_resource_uart_serialbus;
> };
>
> struct acpi_device {
> ...
> union acpi_resource_serial_bus *serial;
> ...
> };
It is also possible to have several serial bus connectors on a single
device (although we've seen only one connector per device but it should not
be limited to that).
> Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would
> be able to use struct acpi_device objects directly without running any AML.
> That could be done on top of the current series and I'm going to prepare a
> patch
> for that in the next few days if I find some time between the LCE sessions.
Cool :-) Let me know if you need any help, like testing the patches etc.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
> On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
> > On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
> > > On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
> > > wrote:
> > > > ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
> > > > configure the SPI slave devices behind the SPI controller. This patch
> > > > adds
> > > > support for this to the SPI core.
[...]
> > And if the ACPI core parses the _CRS, how does it pass all the resources to
> > the drivers?
>
> Pretty much the same way the $subject patch does.
>
> Instead of parsing the entire subtree below an SPI controller and trying
> acpi_spi_add_device() for each device node in there, it could call
> acpi_spi_add_device() whenever it finds a device of type
> ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
> The only problem is how to pass "master" to it.
>
> So Bjorn, do you have any idea how we could pass the "master" pointer from the
> ACPI core to acpi_spi_add_device() in a sensible way?
>
> An alternative might be to store the information obtained from _CRS in
> struct acpi_device objects created by the ACPI core while parsing the
> namespace. We do that already for things like _PRW, so we might as well do it
> for _CRS. Then, the SPI core could just walk the subtree of the device
> hierarchy
> below the SPI controller's acpi_device to extract that information.
> Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where "resources" is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
I think adding it would allow us to make acpi_create_platform_device(),
acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward (and
remove some code duplication between the last two routines).
In addition to that, I'd add an entry containing serial bus information, if
applicable, for the given struct acpi_device, something like:
union acpi_resource_serial_bus {
struct acpi_resource_common_serialbus;
struct acpi_resource_spi_serialbus;
struct acpi_resource_i2c_serialbus;
struct acpi_resource_uart_serialbus;
};
struct acpi_device {
...
union acpi_resource_serial_bus *serial;
...
};
Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would
be able to use struct acpi_device objects directly without running any AML.
That could be done on top of the current series and I'm going to prepare a patch
for that in the next few days if I find some time between the LCE sessions.
Thanks,
Rafael
---
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Saturday, November 03, 2012 09:59:28 PM Rafael J. Wysocki wrote:
On Saturday, November 03, 2012 10:13:10 PM Mika Westerberg wrote:
On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote:
On Sat, Nov 3, 2012 at 1:46 AM, Mika Westerberg
mika.westerb...@linux.intel.com wrote:
ACPI 5 introduced SPISerialBus resource that allows us to enumerate and
configure the SPI slave devices behind the SPI controller. This patch
adds
support for this to the SPI core.
[...]
And if the ACPI core parses the _CRS, how does it pass all the resources to
the drivers?
Pretty much the same way the $subject patch does.
Instead of parsing the entire subtree below an SPI controller and trying
acpi_spi_add_device() for each device node in there, it could call
acpi_spi_add_device() whenever it finds a device of type
ACPI_RESOURCE_TYPE_SERIAL_BUS/ACPI_RESOURCE_SERIAL_TYPE_SPI.
The only problem is how to pass master to it.
So Bjorn, do you have any idea how we could pass the master pointer from the
ACPI core to acpi_spi_add_device() in a sensible way?
An alternative might be to store the information obtained from _CRS in
struct acpi_device objects created by the ACPI core while parsing the
namespace. We do that already for things like _PRW, so we might as well do it
for _CRS. Then, the SPI core could just walk the subtree of the device
hierarchy
below the SPI controller's acpi_device to extract that information.
Maybe that's the way to go?
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
I think adding it would allow us to make acpi_create_platform_device(),
acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward (and
remove some code duplication between the last two routines).
In addition to that, I'd add an entry containing serial bus information, if
applicable, for the given struct acpi_device, something like:
union acpi_resource_serial_bus {
struct acpi_resource_common_serialbus;
struct acpi_resource_spi_serialbus;
struct acpi_resource_i2c_serialbus;
struct acpi_resource_uart_serialbus;
};
struct acpi_device {
...
union acpi_resource_serial_bus *serial;
...
};
Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would
be able to use struct acpi_device objects directly without running any AML.
That could be done on top of the current series and I'm going to prepare a patch
for that in the next few days if I find some time between the LCE sessions.
Thanks,
Rafael
---
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:31:19AM +0100, Rafael J. Wysocki wrote:
The general idea is to move the _CRS parsing routine from acpi_platform.c
to scan.c and make it attach resource objects to struct acpi_device.
I'm thinking about adding a list head to struct acpi_device pointing to a
list of entries like:
struct resource_list_entry {
struct list_head node;
struct resource *resources;
unsigned int count;
};
where resources is an array of resources (e.g. interrupts) in the given
entry and count is the size of that array.
That list would contain common resources like
ACPI_RESOURCE_TYPE_FIXED_MEMORY32,
ACPI_RESOURCE_TYPE_IRQ, ACPI_RESOURCE_TYPE_ADDRESS32,
ACPI_RESOURCE_TYPE_EXTENDED_IRQ.
I think adding it would allow us to make acpi_create_platform_device(),
acpi_spi_add_resources() and acpi_i2c_add_resources() more straightforward
(and
remove some code duplication between the last two routines).
This certainly sounds good to me.
In addition to that, I'd add an entry containing serial bus information, if
applicable, for the given struct acpi_device, something like:
union acpi_resource_serial_bus {
struct acpi_resource_common_serialbus;
struct acpi_resource_spi_serialbus;
struct acpi_resource_i2c_serialbus;
struct acpi_resource_uart_serialbus;
};
struct acpi_device {
...
union acpi_resource_serial_bus *serial;
...
};
It is also possible to have several serial bus connectors on a single
device (although we've seen only one connector per device but it should not
be limited to that).
Then, things like acpi_spi_add_resources() and acpi_i2c_add_resources() would
be able to use struct acpi_device objects directly without running any AML.
That could be done on top of the current series and I'm going to prepare a
patch
for that in the next few days if I find some time between the LCE sessions.
Cool :-) Let me know if you need any help, like testing the patches etc.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 03, 2012 at 10:13:10PM +0200, Mika Westerberg wrote: On Sat, Nov 03, 2012 at 01:42:02PM -0600, Bjorn Helgaas wrote: + case ACPI_RESOURCE_TYPE_IRQ: + info-gsi = res-data.irq.interrupts[0]; + info-triggering = res-data.irq.triggering; + info-polarity = res-data.irq.polarity; + break; + case ACPI_RESOURCE_TYPE_EXTENDED_IRQ: + info-gsi = res-data.extended_irq.interrupts[0]; + info-triggering = res-data.extended_irq.triggering; + info-polarity = res-data.extended_irq.polarity; A driver doesn't seem like the right place for _CRS parsing code. I think the intent of _CRS is to describe resources that need to be coordinated across all devices, e.g., MMIO space, I/O port space, and IRQs. Since these resources require system-wide coordination, even when we don't have drivers for some devices, the ACPI core should be able to parse _CRS without needing any device-specific knowledge. I think the driver is the only one who really knows the resources it needs in order to talk the hardware. Generic SPI drivers expect the subsystem to supply them with an interrupt number; if there is a single interrupt it seems reasonable for the generic code to continue to do that for them when they're instantiated from ACPI. On the other hand if there are muliple interrupts it should probably give up and punt to the driver. The purpose of the above code is to extract the resources in a suitable form so that we can create a struct spi_device out of them automatically, in a similar way than the Device Tree does. Definitely agreed. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: + strlcpy(spi-modalias, acpi_device_hid(adev), sizeof(spi-modalias)); + if (info.gsi = 0) + spi-irq = acpi_register_gsi(adev-dev, info.gsi, + info.triggering, info.polarity); + request_module(spi-modalias); request_module()? Why? signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote:
struct acpi_device {
...
union acpi_resource_serial_bus *serial;
...
};
It is also possible to have several serial bus connectors on a single
device (although we've seen only one connector per device but it should not
be limited to that).
I've got practical systems where there are multiple buses physically
connected, though in practice almost always only one is actually used at
runtime when it's I2C and SPI there are some systems (usually with other
buses) where you might want to use more than one bus. Not sure those
buses will fit in here though.
signature.asc
Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: + strlcpy(spi-modalias, acpi_device_hid(adev), sizeof(spi-modalias)); + if (info.gsi = 0) + spi-irq = acpi_register_gsi(adev-dev, info.gsi, +info.triggering, info.polarity); + request_module(spi-modalias); request_module()? Why? The DT code does the same. I have no idea why it is there, really :-) I can remove it in the next version if you think it is not needed. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 01:03:22PM +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:54:55AM +0100, Mark Brown wrote: On Sat, Nov 03, 2012 at 09:46:32AM +0200, Mika Westerberg wrote: + strlcpy(spi-modalias, acpi_device_hid(adev), sizeof(spi-modalias)); + if (info.gsi = 0) + spi-irq = acpi_register_gsi(adev-dev, info.gsi, + info.triggering, info.polarity); + request_module(spi-modalias); request_module()? Why? The DT code does the same. I have no idea why it is there, really :-) I can remove it in the next version if you think it is not needed. Well, if you can't identify what it does... signature.asc Description: Digital signature
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote:
On Mon, Nov 05, 2012 at 12:56:02PM +0200, Mika Westerberg wrote:
struct acpi_device {
...
union acpi_resource_serial_bus *serial;
...
};
It is also possible to have several serial bus connectors on a single
device (although we've seen only one connector per device but it should not
be limited to that).
I've got practical systems where there are multiple buses physically
connected, though in practice almost always only one is actually used at
runtime when it's I2C and SPI there are some systems (usually with other
buses) where you might want to use more than one bus. Not sure those
buses will fit in here though.
Yeah, I just went through DSDT table of one of our machines and found a
device that actually has two I2CSerialBus connectors (and those are to the
same controller). What I'm not sure is that is it used to select between
two different addresses or doest the device really have two physical I2C
connections.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: I've got practical systems where there are multiple buses physically connected, though in practice almost always only one is actually used at runtime when it's I2C and SPI there are some systems (usually with other buses) where you might want to use more than one bus. Not sure those buses will fit in here though. Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. Neither would make sense from a hardware perspective. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: I've got practical systems where there are multiple buses physically connected, though in practice almost always only one is actually used at runtime when it's I2C and SPI there are some systems (usually with other buses) where you might want to use more than one bus. Not sure those buses will fit in here though. Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. Neither would make sense from a hardware perspective. Well, interesting. :-) I wonder what we're supposed to do with things like that? It looks like our patch [3/3] will use the last one found, but is that correct? Should it use the first one instead, perhaps? That little conundrum aside, the observations above seem to indicate that we'd need a list of serial resources per struct acpi_device too. Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: I've got practical systems where there are multiple buses physically connected, though in practice almost always only one is actually used at runtime when it's I2C and SPI there are some systems (usually with other buses) where you might want to use more than one bus. Not sure those buses will fit in here though. Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. Neither would make sense from a hardware perspective. Well, interesting. :-) It looks like some PMICs for example have two I2C control interfaces, like TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C controller with different address, you have the situation like above. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] spi / ACPI: add ACPI enumeration support
On Mon, Nov 5, 2012 at 2:15 PM, Mika Westerberg mika.westerb...@linux.intel.com wrote: On Mon, Nov 05, 2012 at 01:59:23PM +0100, Rafael J. Wysocki wrote: On Monday, November 05, 2012 01:23:50 PM Jean Delvare wrote: On Mon, 5 Nov 2012 14:02:19 +0200, Mika Westerberg wrote: On Mon, Nov 05, 2012 at 11:56:39AM +0100, Mark Brown wrote: (...) Yeah, I just went through DSDT table of one of our machines and found a device that actually has two I2CSerialBus connectors (and those are to the same controller). What I'm not sure is that is it used to select between two different addresses or doest the device really have two physical I2C connections. Neither would make sense from a hardware perspective. Well, interesting. :-) It looks like some PMICs for example have two I2C control interfaces, like TI's TWL6030 if I'm not mistaken. If both are put behind the same I2C controller with different address, you have the situation like above. This is quite common. So for example the AB8500 (drivers/mfd/ab8500-core.c) has this. The reason is usually that the device has more than 256 registers to the address space simply is not big enough. What we do is refer to the subaddresses as banks and they happen to always be at the next consecutive address so n+1. So the same device appear behind several addresses just to support a lot of registers. So you're not actually modelling the devices on the other end but the multiple addresses of a single device. If the addresses are consecutive like ours it makes sense to just instantiate one device and have the driver know that it will also be able to access address +1, +2 ... +n. So is it possible to group the consecutive related addresses after each other here at the acpi-spi level and just create a single device? If the addresses are not consecutive I guess you need to have one device driver bind to several devices and return -EPROBE_DEFER until the driver has been probed for all of them or something like that, this is what multi-block GPIO drivers sometimes do. Yours, Linus Walleij -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
