| Message ID | 1d26e956f12fcd86ad915659adeb86e242b96ac7.1468409668.git.leonard.crestez@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Jul 13, 2016 at 1:53 PM, Crestez Dan Leonard <leonard.crestez@intel.com> wrote: > When using devicetree spi_device.modalias is set to the compatible > string with the vendor prefix removed. For SPI devices described via > ACPI the i2c_board_info.type 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> Same as for the i2c patch: it looks good to me, but I'd prefer to get a word from Mika on it too. > --- > drivers/spi/spi.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 7589c8a..49fdf6e 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -1679,13 +1679,22 @@ static acpi_status acpi_register_spi_device(struct spi_master *master, > return AE_OK; > } > > + if (adev->data.of_compatible) { > + ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)); > + if (ret) { > + spi_dev_put(spi); > + return AE_NOT_FOUND; > + } > + } else { > + 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", > -- 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 Wed, Jul 13, 2016 at 02:22:02PM +0200, Rafael J. Wysocki wrote: > On Wed, Jul 13, 2016 at 1:53 PM, Crestez Dan Leonard > <leonard.crestez@intel.com> wrote: > > When using devicetree spi_device.modalias is set to the compatible > > string with the vendor prefix removed. For SPI devices described via > > ACPI the i2c_board_info.type 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> > > Same as for the i2c patch: it looks good to me, but I'd prefer to get > a word from Mika on it too. Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- 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, Jul 13, 2016 at 02:53:42PM +0300, Crestez Dan Leonard wrote: > When using devicetree spi_device.modalias is set to the compatible > string with the vendor prefix removed. For SPI devices described via > ACPI the i2c_board_info.type string is initialized by acpi_device_hid. > When using ACPI and DT ids this string ends up something like "PRP0001". Please submit patches using subject lines reflecting the style for the subsystem. This makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. > 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. This is basically fine but... > + if (adev->data.of_compatible) { > + ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)); > + if (ret) { > + spi_dev_put(spi); > + return AE_NOT_FOUND; > + } The only reason this could fail currently is that there wasn't a compatible in the first place so why don't we just handle it like the no compatible case? It's probably not realistic but it seems like there's a small chance this could regress some platform if we do add more error detection in acpi_of_modalias(). I'm still not sure why if we want to translate ACPI into DT why we don't actually translate and avoid all this special casing and churn.
On 07/19/2016 01:22 PM, Mark Brown wrote: > On Wed, Jul 13, 2016 at 02:53:42PM +0300, Crestez Dan Leonard wrote: >> When using devicetree spi_device.modalias is set to the compatible >> string with the vendor prefix removed. For SPI devices described via >> ACPI the i2c_board_info.type string is initialized by acpi_device_hid. >> When using ACPI and DT ids this string ends up something like "PRP0001". > > Please submit patches using subject lines reflecting the style for the > subsystem. This makes it easier for people to identify relevant > patches. Look at what existing commits in the area you're changing are > doing and make sure your subject lines visually resemble what they're > doing. So the prefix should be something like "spi: acpi: "? >> 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. > > This is basically fine but... > >> + if (adev->data.of_compatible) { >> + ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)); >> + if (ret) { >> + spi_dev_put(spi); >> + return AE_NOT_FOUND; >> + } > > The only reason this could fail currently is that there wasn't a > compatible in the first place so why don't we just handle it like the no > compatible case? It's probably not realistic but it seems like there's > a small chance this could regress some platform if we do add more error > detection in acpi_of_modalias(). If acpi_of_modalias fails for some new reason wouldn't it be better to fail explicitly rather than ignore it?
On Wed, Jul 20, 2016 at 02:21:55PM +0300, Crestez Dan Leonard wrote: > On 07/19/2016 01:22 PM, Mark Brown wrote: > > Please submit patches using subject lines reflecting the style for the > > subsystem. This makes it easier for people to identify relevant > > patches. Look at what existing commits in the area you're changing are > > doing and make sure your subject lines visually resemble what they're > > doing. > So the prefix should be something like "spi: acpi: "? Yes. > >> + if (adev->data.of_compatible) { > >> + ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)); > >> + if (ret) { > >> + spi_dev_put(spi); > >> + return AE_NOT_FOUND; > >> + } > > The only reason this could fail currently is that there wasn't a > > compatible in the first place so why don't we just handle it like the no > > compatible case? It's probably not realistic but it seems like there's > > a small chance this could regress some platform if we do add more error > > detection in acpi_of_modalias(). > If acpi_of_modalias fails for some new reason wouldn't it be better to > fail explicitly rather than ignore it? The current code will happily proceed to create a device without doing this parsing so clearly we can do that. It's not clear to me that it's better to refuse to create the device at all than to soldier on and create a device with only the native ACPI information, perhaps it just needs a comment explaining why we do that.
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 7589c8a..49fdf6e 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1679,13 +1679,22 @@ static acpi_status acpi_register_spi_device(struct spi_master *master, return AE_OK; } + if (adev->data.of_compatible) { + ret = acpi_of_modalias(adev, spi->modalias, sizeof(spi->modalias)); + if (ret) { + spi_dev_put(spi); + return AE_NOT_FOUND; + } + } else { + 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",
When using devicetree spi_device.modalias is set to the compatible string with the vendor prefix removed. For SPI devices described via ACPI the i2c_board_info.type 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> --- drivers/spi/spi.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)