| Message ID | 1414572037-11306-1-git-send-email-pantelis.antoniou@konsulko.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Wolfram Sang |
| Headers | show |
Hello! On 29/10/14 09:40, ext Pantelis Antoniou wrote: > Dynamically inserting spi device nodes requires the use of a single > device registration method. Rework and export it. > > Methods to lookup a device/master using a device node are added > as well, of_find_spi_master_by_node() & of_find_spi_device_by_node(). > > Changes since v1: > * Brown paper bug with parameter on of_register_spi_device(). > > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> Acked-by: Alexander Sverdlin <alexaner.sverdlin@nsn.com> > --- > drivers/spi/spi.c | 255 +++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 157 insertions(+), 98 deletions(-)
On Wed, Oct 29, 2014 at 10:40:37AM +0200, Pantelis Antoniou wrote: > Dynamically inserting spi device nodes requires the use of a single > device registration method. Rework and export it. > > Methods to lookup a device/master using a device node are added > as well, of_find_spi_master_by_node() & of_find_spi_device_by_node(). Why do we need to do this - I would expect that adding nodes would trigger parsing in the same way we normally do it. Where is the user and how does it know that it's handling a SPI node? This feels like there is an abstraction problem somewhere, whatever code is supposed to use this is going to need to be taught about each individual bus which is going to be tedious, I would expect that we'd have something like the bus being able to provide a callback which will get invoked whenever a new node appears on the parent node for the bus. > Changes since v1: > * Brown paper bug with parameter on of_register_spi_device(). Don't include noise like this in the changelog, put it after --- like SubmittingPatches says. Please also try to keep your CC list sane, CCing random people just means that you're increasing the volume of mail they have to process. I'm surprised kernel.org accepts so many CCs. I have to say I don't recall ever seeing v1...
Hi > On Oct 29, 2014, at 12:14 , Mark Brown <broonie@kernel.org> wrote: > > On Wed, Oct 29, 2014 at 10:40:37AM +0200, Pantelis Antoniou wrote: >> Dynamically inserting spi device nodes requires the use of a single >> device registration method. Rework and export it. >> >> Methods to lookup a device/master using a device node are added >> as well, of_find_spi_master_by_node() & of_find_spi_device_by_node(). > > Why do we need to do this - I would expect that adding nodes would > trigger parsing in the same way we normally do it. Where is the user > and how does it know that it's handling a SPI node? > > This feels like there is an abstraction problem somewhere, whatever code > is supposed to use this is going to need to be taught about each > individual bus which is going to be tedious, I would expect that we'd > have something like the bus being able to provide a callback which will > get invoked whenever a new node appears on the parent node for the bus. > There’s a whole patchset that does exactly this. Look at "OF: spi: Add OF notifier handler” and you’ll where this is used. >> Changes since v1: >> * Brown paper bug with parameter on of_register_spi_device(). > > Don't include noise like this in the changelog, put it after --- like > SubmittingPatches says. Please also try to keep your CC list sane, > CCing random people just means that you're increasing the volume of mail > they have to process. I'm surprised kernel.org accepts so many CCs. > > I have to say I don't recall ever seeing v1... All of them are in the CC list for a reason. Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: > > On Oct 29, 2014, at 12:14 , Mark Brown <broonie@kernel.org> wrote: > > This feels like there is an abstraction problem somewhere, whatever code > > is supposed to use this is going to need to be taught about each > > individual bus which is going to be tedious, I would expect that we'd > > have something like the bus being able to provide a callback which will > > get invoked whenever a new node appears on the parent node for the bus. > There’s a whole patchset that does exactly this. > Look at "OF: spi: Add OF notifier handler” and you’ll where this is used. I deleted that unread I'm afraid; one of the reasons that you should use subject lines matching the styles for the subsystems is that it's one of the things people use to filter out things that actually need attention, if things are busy things that at first glance don't look terribly relevant (like changes to the OF core in this case) are likely to get looked at less urgently or just skipped. A quick glance suggests that this is adding code inside the SPI core so it's still not explaining why anything is being exported, can you clarify please? > > SubmittingPatches says. Please also try to keep your CC list sane, > > CCing random people just means that you're increasing the volume of mail > > they have to process. I'm surprised kernel.org accepts so many CCs. > > I have to say I don't recall ever seeing v1... > All of them are in the CC list for a reason. This is a single, standalone SPI patch - you didn't send it as part of a series (which is the only reason I read it).
On Wed, 29 Oct 2014 12:22:04 +0000 , Mark Brown <broonie@kernel.org> wrote: > On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: > > > On Oct 29, 2014, at 12:14 , Mark Brown <broonie@kernel.org> wrote: > > > > This feels like there is an abstraction problem somewhere, whatever code > > > is supposed to use this is going to need to be taught about each > > > individual bus which is going to be tedious, I would expect that we'd > > > have something like the bus being able to provide a callback which will > > > get invoked whenever a new node appears on the parent node for the bus. > > > There’s a whole patchset that does exactly this. > > Look at "OF: spi: Add OF notifier handler” and you’ll where this is used. > > I deleted that unread I'm afraid; one of the reasons that you should use > subject lines matching the styles for the subsystems is that it's one of > the things people use to filter out things that actually need attention, > if things are busy things that at first glance don't look terribly > relevant (like changes to the OF core in this case) are likely to get > looked at less urgently or just skipped. > > A quick glance suggests that this is adding code inside the SPI core so > it's still not explaining why anything is being exported, can you > clarify please? I have the same question. This doesn't look like it should be exporting symbols. Also, the way the patch is written causes a lot of code changes to get interleaved in the diff. It would be better to split into two patches; one that creates the new of_register_spi_device(), and a separate patch to add the other new functions. It would be certainly easier to review that way. > > > > SubmittingPatches says. Please also try to keep your CC list sane, > > > CCing random people just means that you're increasing the volume of mail > > > they have to process. I'm surprised kernel.org accepts so many CCs. > > > > I have to say I don't recall ever seeing v1... > > > All of them are in the CC list for a reason. > > This is a single, standalone SPI patch - you didn't send it as part of a > series (which is the only reason I read it). Yes, this is part of the OF overlay series. It should have at least been marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. g. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, > On Nov 21, 2014, at 17:33 , Grant Likely <grant.likely@secretlab.ca> wrote: > > On Wed, 29 Oct 2014 12:22:04 +0000 > , Mark Brown <broonie@kernel.org> > wrote: >> On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: >>>> On Oct 29, 2014, at 12:14 , Mark Brown <broonie@kernel.org> wrote: >> >>>> This feels like there is an abstraction problem somewhere, whatever code >>>> is supposed to use this is going to need to be taught about each >>>> individual bus which is going to be tedious, I would expect that we'd >>>> have something like the bus being able to provide a callback which will >>>> get invoked whenever a new node appears on the parent node for the bus. >> >>> Thereâs a whole patchset that does exactly this. >>> Look at "OF: spi: Add OF notifier handlerâ and youâll where this is used. >> >> I deleted that unread I'm afraid; one of the reasons that you should use >> subject lines matching the styles for the subsystems is that it's one of >> the things people use to filter out things that actually need attention, >> if things are busy things that at first glance don't look terribly >> relevant (like changes to the OF core in this case) are likely to get >> looked at less urgently or just skipped. >> >> A quick glance suggests that this is adding code inside the SPI core so >> it's still not explaining why anything is being exported, can you >> clarify please? > > I have the same question. This doesn't look like it should be exporting > symbols. > > Also, the way the patch is written causes a lot of code changes to get > interleaved in the diff. It would be better to split into two patches; > one that creates the new of_register_spi_device(), and a separate patch > to add the other new functions. It would be certainly easier to review > that way. > The diff does make a mess of things; it’s not that complex of a patch. Your wish shall be granted. I’ll respin this over the weekend. >> >>>> SubmittingPatches says. Please also try to keep your CC list sane, >>>> CCing random people just means that you're increasing the volume of mail >>>> they have to process. I'm surprised kernel.org accepts so many CCs. >> >>>> I have to say I don't recall ever seeing v1... >> >>> All of them are in the CC list for a reason. >> >> This is a single, standalone SPI patch - you didn't send it as part of a >> series (which is the only reason I read it). > > Yes, this is part of the OF overlay series. It should have at least been > marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. > > g. > Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 3:44 PM, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: > Hi Grant, > >> On Nov 21, 2014, at 17:33 , Grant Likely <grant.likely@secretlab.ca> wrote: >> >> On Wed, 29 Oct 2014 12:22:04 +0000 >> , Mark Brown <broonie@kernel.org> >> wrote: >>> On Wed, Oct 29, 2014 at 01:48:06PM +0200, Pantelis Antoniou wrote: >>>>> On Oct 29, 2014, at 12:14 , Mark Brown <broonie@kernel.org> wrote: >>> >>>>> This feels like there is an abstraction problem somewhere, whatever code >>>>> is supposed to use this is going to need to be taught about each >>>>> individual bus which is going to be tedious, I would expect that we'd >>>>> have something like the bus being able to provide a callback which will >>>>> get invoked whenever a new node appears on the parent node for the bus. >>> >>>> Thereâ EURO (tm)s a whole patchset that does exactly this. >>>> Look at "OF: spi: Add OF notifier handlerâ EURO and youâ EURO (tm)ll where this is used. >>> >>> I deleted that unread I'm afraid; one of the reasons that you should use >>> subject lines matching the styles for the subsystems is that it's one of >>> the things people use to filter out things that actually need attention, >>> if things are busy things that at first glance don't look terribly >>> relevant (like changes to the OF core in this case) are likely to get >>> looked at less urgently or just skipped. >>> >>> A quick glance suggests that this is adding code inside the SPI core so >>> it's still not explaining why anything is being exported, can you >>> clarify please? >> >> I have the same question. This doesn't look like it should be exporting >> symbols. >> >> Also, the way the patch is written causes a lot of code changes to get >> interleaved in the diff. It would be better to split into two patches; >> one that creates the new of_register_spi_device(), and a separate patch >> to add the other new functions. It would be certainly easier to review >> that way. >> > > The diff does make a mess of things; it's not that complex of a patch. > > Your wish shall be granted. I'll respin this over the weekend. I've fixed it in my tree by moving the match functions into the second patch. The diffs are much easier to read now. I'll post the new versions to the list shortly, but if you can test that would be fantastic. g. > >>> >>>>> SubmittingPatches says. Please also try to keep your CC list sane, >>>>> CCing random people just means that you're increasing the volume of mail >>>>> they have to process. I'm surprised kernel.org accepts so many CCs. >>> >>>>> I have to say I don't recall ever seeing v1... >>> >>>> All of them are in the CC list for a reason. >>> >>> This is a single, standalone SPI patch - you didn't send it as part of a >>> series (which is the only reason I read it). >> >> Yes, this is part of the OF overlay series. It should have at least been >> marked as [PATCH 7/8] and that it replaced the previous, buggy, patch 7. >> >> g. >> > > Regards > > -- Pantelis > -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index ebcb33d..4778366 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1220,6 +1220,122 @@ err_init_queue: /*-------------------------------------------------------------------------*/ #if defined(CONFIG_OF) + +static struct spi_device * +of_register_spi_device(struct spi_master *master, struct device_node *nc) +{ + struct spi_device *spi; + int rc; + u32 value; + + /* Alloc an spi_device */ + spi = spi_alloc_device(master); + if (!spi) { + dev_err(&master->dev, "spi_device alloc error for %s\n", + nc->full_name); + rc = -ENOMEM; + goto err_out; + } + + /* Select device driver */ + rc = of_modalias_node(nc, spi->modalias, + sizeof(spi->modalias)); + if (rc < 0) { + dev_err(&master->dev, "cannot find modalias for %s\n", + nc->full_name); + goto err_out; + } + + /* Device address */ + rc = of_property_read_u32(nc, "reg", &value); + if (rc) { + dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n", + nc->full_name, rc); + goto err_out; + } + spi->chip_select = value; + + /* Mode (clock phase/polarity/etc.) */ + if (of_find_property(nc, "spi-cpha", NULL)) + spi->mode |= SPI_CPHA; + if (of_find_property(nc, "spi-cpol", NULL)) + spi->mode |= SPI_CPOL; + if (of_find_property(nc, "spi-cs-high", NULL)) + spi->mode |= SPI_CS_HIGH; + if (of_find_property(nc, "spi-3wire", NULL)) + spi->mode |= SPI_3WIRE; + if (of_find_property(nc, "spi-lsb-first", NULL)) + spi->mode |= SPI_LSB_FIRST; + + /* Device DUAL/QUAD mode */ + if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { + switch (value) { + case 1: + break; + case 2: + spi->mode |= SPI_TX_DUAL; + break; + case 4: + spi->mode |= SPI_TX_QUAD; + break; + default: + dev_warn(&master->dev, + "spi-tx-bus-width %d not supported\n", + value); + break; + } + } + + if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) { + switch (value) { + case 1: + break; + case 2: + spi->mode |= SPI_RX_DUAL; + break; + case 4: + spi->mode |= SPI_RX_QUAD; + break; + default: + dev_warn(&master->dev, + "spi-rx-bus-width %d not supported\n", + value); + break; + } + } + + /* Device speed */ + rc = of_property_read_u32(nc, "spi-max-frequency", &value); + if (rc) { + dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n", + nc->full_name, rc); + goto err_out; + } + spi->max_speed_hz = value; + + /* IRQ */ + spi->irq = irq_of_parse_and_map(nc, 0); + + /* Store a pointer to the node in the device structure */ + of_node_get(nc); + spi->dev.of_node = nc; + + /* Register the new device */ + request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias); + rc = spi_add_device(spi); + if (rc) { + dev_err(&master->dev, "spi_device register error %s\n", + nc->full_name); + goto err_out; + } + + return spi; + +err_out: + spi_dev_put(spi); + return ERR_PTR(rc); +} + /** * of_register_spi_devices() - Register child devices onto the SPI bus * @master: Pointer to spi_master device @@ -1229,120 +1345,63 @@ err_init_queue: */ static void of_register_spi_devices(struct spi_master *master) { - struct spi_device *spi; struct device_node *nc; - int rc; - u32 value; + struct spi_device *spi; if (!master->dev.of_node) return; for_each_available_child_of_node(master->dev.of_node, nc) { - /* Alloc an spi_device */ - spi = spi_alloc_device(master); - if (!spi) { - dev_err(&master->dev, "spi_device alloc error for %s\n", + spi = of_register_spi_device(master, nc); + if (IS_ERR(spi)) + dev_warn(&master->dev, "Failed to create SPI device for %s\n", nc->full_name); - spi_dev_put(spi); - continue; - } + } +} - /* Select device driver */ - if (of_modalias_node(nc, spi->modalias, - sizeof(spi->modalias)) < 0) { - dev_err(&master->dev, "cannot find modalias for %s\n", - nc->full_name); - spi_dev_put(spi); - continue; - } +static int of_dev_node_match(struct device *dev, void *data) +{ + return dev->of_node == data; +} - /* Device address */ - rc = of_property_read_u32(nc, "reg", &value); - if (rc) { - dev_err(&master->dev, "%s has no valid 'reg' property (%d)\n", - nc->full_name, rc); - spi_dev_put(spi); - continue; - } - spi->chip_select = value; - - /* Mode (clock phase/polarity/etc.) */ - if (of_find_property(nc, "spi-cpha", NULL)) - spi->mode |= SPI_CPHA; - if (of_find_property(nc, "spi-cpol", NULL)) - spi->mode |= SPI_CPOL; - if (of_find_property(nc, "spi-cs-high", NULL)) - spi->mode |= SPI_CS_HIGH; - if (of_find_property(nc, "spi-3wire", NULL)) - spi->mode |= SPI_3WIRE; - if (of_find_property(nc, "spi-lsb-first", NULL)) - spi->mode |= SPI_LSB_FIRST; - - /* Device DUAL/QUAD mode */ - if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { - switch (value) { - case 1: - break; - case 2: - spi->mode |= SPI_TX_DUAL; - break; - case 4: - spi->mode |= SPI_TX_QUAD; - break; - default: - dev_warn(&master->dev, - "spi-tx-bus-width %d not supported\n", - value); - break; - } - } +/* bah; the match functions differ just by const-ness */ +static int of_dev_node_match_const(struct device *dev, const void *data) +{ + return dev->of_node == data; +} - if (!of_property_read_u32(nc, "spi-rx-bus-width", &value)) { - switch (value) { - case 1: - break; - case 2: - spi->mode |= SPI_RX_DUAL; - break; - case 4: - spi->mode |= SPI_RX_QUAD; - break; - default: - dev_warn(&master->dev, - "spi-rx-bus-width %d not supported\n", - value); - break; - } - } +/* must call put_device() when done with returned spi_device device */ +struct spi_device *of_find_spi_device_by_node(struct device_node *node) +{ + struct device *dev; - /* Device speed */ - rc = of_property_read_u32(nc, "spi-max-frequency", &value); - if (rc) { - dev_err(&master->dev, "%s has no valid 'spi-max-frequency' property (%d)\n", - nc->full_name, rc); - spi_dev_put(spi); - continue; - } - spi->max_speed_hz = value; + dev = bus_find_device(&spi_bus_type, NULL, node, + of_dev_node_match); + if (!dev) + return NULL; - /* IRQ */ - spi->irq = irq_of_parse_and_map(nc, 0); + return to_spi_device(dev); +} +EXPORT_SYMBOL(of_find_spi_device_by_node); - /* Store a pointer to the node in the device structure */ - of_node_get(nc); - spi->dev.of_node = nc; +/* forward decl */ +static struct class spi_master_class; - /* Register the new device */ - request_module("%s%s", SPI_MODULE_PREFIX, spi->modalias); - rc = spi_add_device(spi); - if (rc) { - dev_err(&master->dev, "spi_device register error %s\n", - nc->full_name); - spi_dev_put(spi); - } +/* the spi masters are not using spi_bus, so we find it with another way */ +struct spi_master *of_find_spi_master_by_node(struct device_node *node) +{ + struct device *dev; - } + dev = class_find_device(&spi_master_class, NULL, node, + of_dev_node_match_const); + if (!dev) + return NULL; + + /* reference got in class_find_device */ + return container_of(dev, struct spi_master, dev); } +EXPORT_SYMBOL(of_find_spi_master_by_node); + #else static void of_register_spi_devices(struct spi_master *master) { } #endif
Dynamically inserting spi device nodes requires the use of a single device registration method. Rework and export it. Methods to lookup a device/master using a device node are added as well, of_find_spi_master_by_node() & of_find_spi_device_by_node(). Changes since v1: * Brown paper bug with parameter on of_register_spi_device(). Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com> --- drivers/spi/spi.c | 255 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 157 insertions(+), 98 deletions(-)