| Message ID | bb23ba754ed1f51c9b20ccd4a2b87520ce7c1893.1462285398.git.Adam.Thomson.Opensource@diasemi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 11:53:05AM +0100, Adam Thomson wrote: > @@ -27,7 +28,6 @@ > #include "da7219.h" > #include "da7219-aad.h" > > - > /* > * Detection control > */ Random whitespace change. > static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, > const char *name) > { > @@ -551,6 +571,9 @@ static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, > of_node = to_of_node(child); > if (of_node_cmp(of_node->name, name) == 0) > return child; > + } else if (is_acpi_data_node(child)) { > + if (da7219_aad_of_acpi_node_matched(child, name)) > + return child; > } > } > This seems messy. It is a function with a DT specific name that's matching ACPI stuff and the fwnode API isn't hiding anything for us which suggests this isn't something that's expected to work transparently. At least the naming needs to be corrected, and if this *is* supposed to be something we do in ACPI I'd expect the handling to be pushed into the fwnode API rather than open coded in a driver - at the minute I'm unsure if this is messy because it's a bad idea to do this at all or if it's just the naming and so on. > - /* Handle any DT/platform data */ > - if ((codec->dev->of_node) && (da7219->pdata)) > + /* Handle any DT/ACPI/platform data */ > + if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) && > + (da7219->pdata)) > da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec); > > da7219_aad_handle_pdata(codec); Surely we should be able to check if there's firmware data without enumerating every possible firmware type?
On May 06, 2016, 13:39, Mark Brown wrote: > > @@ -27,7 +28,6 @@ > > #include "da7219.h" > > #include "da7219-aad.h" > > > > - > > /* > > * Detection control > > */ > > Random whitespace change. Fair point. Will sort it. > > > static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device > *dev, > > const char *name) > > { > > @@ -551,6 +571,9 @@ static struct fwnode_handle > *da7219_aad_of_named_fwhandle(struct device *dev, > > of_node = to_of_node(child); > > if (of_node_cmp(of_node->name, name) == 0) > > return child; > > + } else if (is_acpi_data_node(child)) { > > + if (da7219_aad_of_acpi_node_matched(child, name)) > > + return child; > > } > > } > > > > This seems messy. It is a function with a DT specific name that's > matching ACPI stuff and the fwnode API isn't hiding anything for us > which suggests this isn't something that's expected to work > transparently. At least the naming needs to be corrected, and if this > *is* supposed to be something we do in ACPI I'd expect the handling to > be pushed into the fwnode API rather than open coded in a driver - at > the minute I'm unsure if this is messy because it's a bad idea to do > this at all or if it's just the naming and so on. ACPI allows for data only child nodes, like DT, so I do believe this is a valid case. Also, this kind of functionality is already used in a similar-ish manner in the leds-gpio code. I agree that maybe the name should be changed though as it's misleading. Will also look at the fwnode API and see if it makes sense to move this there. > > > - /* Handle any DT/platform data */ > > - if ((codec->dev->of_node) && (da7219->pdata)) > > + /* Handle any DT/ACPI/platform data */ > > + if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) && > > + (da7219->pdata)) > > da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec); > > > > da7219_aad_handle_pdata(codec); > > Surely we should be able to check if there's firmware data without > enumerating every possible firmware type? There doesn't seem to be a unified check for this. Also, Given these are the only two types the driver expects and supports right now, I don't see a problem being explicit here in the checking.
On Fri, May 06, 2016 at 02:45:00PM +0000, Opensource [Adam Thomson] wrote: > On May 06, 2016, 13:39, Mark Brown wrote: > > > - /* Handle any DT/platform data */ > > > - if ((codec->dev->of_node) && (da7219->pdata)) > > > + /* Handle any DT/ACPI/platform data */ > > > + if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) && > > > + (da7219->pdata)) > > > da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec); > > > da7219_aad_handle_pdata(codec); > > Surely we should be able to check if there's firmware data without > > enumerating every possible firmware type? > There doesn't seem to be a unified check for this. Also, Given these are the > only two types the driver expects and supports right now, I don't see a problem > being explicit here in the checking. Again it's pointing out something that looks like it's missing from the fwnode API - if people are supposed to be able to write firmware neutral drivers they should be able to do everything they need at the fwnode level.
diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c index c4853a8..e04ee4f 100644 --- a/sound/soc/codecs/da7219-aad.c +++ b/sound/soc/codecs/da7219-aad.c @@ -15,6 +15,7 @@ #include <linux/platform_device.h> #include <linux/i2c.h> #include <linux/of_device.h> +#include <linux/acpi.h> #include <linux/property.h> #include <linux/pm_wakeirq.h> #include <linux/slab.h> @@ -27,7 +28,6 @@ #include "da7219.h" #include "da7219-aad.h" - /* * Detection control */ @@ -383,7 +383,7 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void *data) } /* - * DT to pdata conversion + * DT/ACPI to pdata conversion */ static enum da7219_aad_micbias_pulse_lvl @@ -539,6 +539,26 @@ static enum da7219_aad_adc_1bit_rpt } } +#ifdef CONFIG_ACPI +static inline bool da7219_aad_of_acpi_node_matched(struct fwnode_handle *child, + const char *name) +{ + struct acpi_data_node *acpi_node = to_acpi_data_node(child); + + if (strcmp(acpi_node->name, name) == 0) + return true; + else + return false; +} +#else +static inline bool da7219_aad_of_acpi_node_matched(struct fwnode_handle *child, + const char *name) +{ + return false; +} + +#endif /* CONFIG_ACPI */ + static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, const char *name) { @@ -551,6 +571,9 @@ static struct fwnode_handle *da7219_aad_of_named_fwhandle(struct device *dev, of_node = to_of_node(child); if (of_node_cmp(of_node->name, name) == 0) return child; + } else if (is_acpi_data_node(child)) { + if (da7219_aad_of_acpi_node_matched(child, name)) + return child; } } @@ -787,8 +810,9 @@ int da7219_aad_init(struct snd_soc_codec *codec) da7219->aad = da7219_aad; da7219_aad->codec = codec; - /* Handle any DT/platform data */ - if ((codec->dev->of_node) && (da7219->pdata)) + /* Handle any DT/ACPI/platform data */ + if (((codec->dev->of_node) || is_acpi_node(codec->dev->fwnode)) && + (da7219->pdata)) da7219->pdata->aad_pdata = da7219_aad_of_to_pdata(codec); da7219_aad_handle_pdata(codec); diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c index 6c6c8db..bc32322 100644 --- a/sound/soc/codecs/da7219.c +++ b/sound/soc/codecs/da7219.c @@ -14,6 +14,7 @@ #include <linux/clk.h> #include <linux/i2c.h> #include <linux/of_device.h> +#include <linux/acpi.h> #include <linux/property.h> #include <linux/regmap.h> #include <linux/slab.h> @@ -1418,7 +1419,7 @@ static struct snd_soc_dai_driver da7219_dai = { /* - * DT + * DT/ACPI */ static const struct of_device_id da7219_of_match[] = { @@ -1656,8 +1657,8 @@ static int da7219_probe(struct snd_soc_codec *codec) break; } - /* Handle DT/Platform data */ - if (codec->dev->of_node) + /* Handle DT/ACPI/Platform data */ + if (codec->dev->of_node || is_acpi_node(codec->dev->fwnode)) da7219->pdata = da7219_of_to_pdata(codec); else da7219->pdata = dev_get_platdata(codec->dev);