| Message ID | 1485187737-22414-4-git-send-email-dan@emutex.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Jan 23, 2017 at 6:08 PM, Dan O'Donovan <dan@emutex.com> wrote: > From: Crestez Dan Leonard <leonard.crestez@intel.com> > > When using devicetree spi_device.modalias is set to the compatible > string with the vendor prefix removed. For SPI devices described via > ACPI the spi_device.modalias string is initialized by acpi_device_hid. > When using ACPI and DT ids this string ends up something like "PRP0001". > > Change acpi_register_spi_device to use the of_compatible property if > present. This makes it easier to instantiate spi drivers through ACPI > with DT ids. > + /* > + * Populate modalias from compatible property if available, > + * otherwise use native ACPI information > + */ > + if ((!adev->data.of_compatible) || > + acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias))) Same comment as in patch 2. > + strlcpy(spi->modalias, acpi_device_hid(adev), > + sizeof(spi->modalias)); Could this be one line?
On 01/23/2017 05:11 PM, Andy Shevchenko wrote: > On Mon, Jan 23, 2017 at 6:08 PM, Dan O'Donovan <dan@emutex.com> wrote: >> From: Crestez Dan Leonard <leonard.crestez@intel.com> >> >> When using devicetree spi_device.modalias is set to the compatible >> string with the vendor prefix removed. For SPI devices described via >> ACPI the spi_device.modalias string is initialized by acpi_device_hid. >> When using ACPI and DT ids this string ends up something like "PRP0001". >> >> Change acpi_register_spi_device to use the of_compatible property if >> present. This makes it easier to instantiate spi drivers through ACPI >> with DT ids. >> + /* >> + * Populate modalias from compatible property if available, >> + * otherwise use native ACPI information >> + */ >> + if ((!adev->data.of_compatible) || >> + acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias))) > Same comment as in patch 2. Thanks for the feedback, Andy. The check on of_compatible is redundant, because its repeated in acpi_of_modalias(), so I'll remove it here (and in patch 2) to reduce this to one line. v4 on the way. >> + strlcpy(spi->modalias, acpi_device_hid(adev), >> + sizeof(spi->modalias)); > Could this be one line? I couldn't see a way to reduce this to one line without exceeding 80 chars or adding another line somewhere else, so I'll leave this one as it is if that's ok. -- 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, Jan 27, 2017 at 12:35 PM, Dan O'Donovan <dan@emutex.com> wrote: > On 01/23/2017 05:11 PM, Andy Shevchenko wrote: >> On Mon, Jan 23, 2017 at 6:08 PM, Dan O'Donovan <dan@emutex.com> wrote: >>> + strlcpy(spi->modalias, acpi_device_hid(adev), >>> + sizeof(spi->modalias)); >> Could this be one line? > I couldn't see a way to reduce this to one line without exceeding 80 > chars or adding another line somewhere else, so I'll leave this one as > it is if that's ok. 80 characters per line is not carved in stone. How many do you have over? 3 like "s);"?
On Mon, Jan 23, 2017 at 5:08 PM, Dan O'Donovan <dan@emutex.com> wrote: > From: Crestez Dan Leonard <leonard.crestez@intel.com> > > When using devicetree spi_device.modalias is set to the compatible > string with the vendor prefix removed. For SPI devices described via > ACPI the spi_device.modalias string is initialized by acpi_device_hid. > When using ACPI and DT ids this string ends up something like "PRP0001". > > Change acpi_register_spi_device to use the of_compatible property if > present. This makes it easier to instantiate spi drivers through ACPI > with DT ids. > > Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com> > Signed-off-by: Dan O'Donovan <dan@emutex.com> > --- > drivers/spi/spi.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 656dd3e..4b562e8 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1722,13 +1722,21 @@ static acpi_status acpi_register_spi_device(struct spi_master *master, > return AE_OK; > } > > + /* > + * Populate modalias from compatible property if available, > + * otherwise use native ACPI information > + */ > + if ((!adev->data.of_compatible) || > + acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias))) > + strlcpy(spi->modalias, acpi_device_hid(adev), > + sizeof(spi->modalias)); > + > if (spi->irq < 0) > spi->irq = acpi_dev_gpio_irq_get(adev, 0); > > acpi_device_set_enumerated(adev); > > adev->power.flags.ignore_parent = true; > - strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); > if (spi_add_device(spi)) { > adev->power.flags.ignore_parent = false; > dev_err(&master->dev, "failed to add SPI device %s from ACPI\n", > -- Mark, any objections here? Thanks, Rafael -- 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 Mon, Jan 30, 2017 at 10:41:33AM +0100, Rafael J. Wysocki wrote: > On Mon, Jan 23, 2017 at 5:08 PM, Dan O'Donovan <dan@emutex.com> wrote: > Mark, any objections here? I've not looked yet, please allow a reasonable time for review (there still seemed to be ongoing discussion at the ACPI level). I continue to be concerned about the amount of duplication and special casing that the various firmware paths seem to entail.
On Tue, Jan 31, 2017 at 9:17 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Jan 30, 2017 at 10:41:33AM +0100, Rafael J. Wysocki wrote: >> On Mon, Jan 23, 2017 at 5:08 PM, Dan O'Donovan <dan@emutex.com> wrote: > >> Mark, any objections here? > > I've not looked yet, please allow a reasonable time for review Sure, no problem at all. > (there still seemed to be ongoing discussion at the ACPI level). Honestly, I'm not sure what you mean. The first patch in the series (which is the ACPI level here) does not seem to be controversial at all and the i2c one has been acked already. > I continue to be concerned about the amount of duplication and special casing that the > various firmware paths seem to entail. Well, this particular change is relatively straightforward and one can argue that it fixes a bug. It does address a problem for sure. Thanks, Rafael -- 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 656dd3e..4b562e8 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1722,13 +1722,21 @@ static acpi_status acpi_register_spi_device(struct spi_master *master, return AE_OK; } + /* + * Populate modalias from compatible property if available, + * otherwise use native ACPI information + */ + if ((!adev->data.of_compatible) || + acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias))) + strlcpy(spi->modalias, acpi_device_hid(adev), + sizeof(spi->modalias)); + if (spi->irq < 0) spi->irq = acpi_dev_gpio_irq_get(adev, 0); acpi_device_set_enumerated(adev); adev->power.flags.ignore_parent = true; - strlcpy(spi->modalias, acpi_device_hid(adev), sizeof(spi->modalias)); if (spi_add_device(spi)) { adev->power.flags.ignore_parent = false; dev_err(&master->dev, "failed to add SPI device %s from ACPI\n",