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.