On Mon, 2016-05-23 at 22:53 +0000, Opensource [Adam Thomson] wrote:
On May 19, 2015 14:28, Andy Shevchenko wrote:
-static struct da7219_aad_pdata *da7219_aad_of_to_pdata(struct snd_soc_codec *codec) +static struct da7219_aad_pdata *da7219_aad_fw_to_pdata(struct snd_soc_codec *codec) {
- struct device_node *np = codec->dev->of_node;
- struct device_node *aad_np = of_find_node_by_name(np,
"da7219_aad");
- struct device *dev = codec->dev;
- struct i2c_client *i2c = to_i2c_client(dev);
- struct fwnode_handle *aad_np =
device_get_named_child_node(dev, "da7219_aad");I would suggest to do an assignment below...
struct da7219_aad_pdata *aad_pdata;
- const char *of_str;
- u32 of_val32;
- const char *fw_str;
- u32 fw_val32;
...right here. Same amount of LOC, but less difficult to see from where aad_np comes.
if (!aad_np) return NULL;
To be fair the allocation of 'aad_np' is only a few lines above so this really doesn't seem to make much difference in my opinion. It really shouldn't be hard for someone to spot where it's allocated.
Better to have it exactly before check. Just a readability and future maintenance. (Someone might insert something in between, and a matter of fact already did)
Though I agree this is minor.
@@ -769,9 +768,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))
da7219->pdata->aad_pdata =da7219_aad_of_to_pdata(codec);
- /* Handle any DT/ACPI/platform data */
+ if ((da7219->pdata) && (!da7219->pdata->aad_pdata))
Redundant parens, twice.
Not essential, but looks cleaner to me. Unless there's a real demand to change, I'd like to leave this as is.
It's really unusual pattern and doesn't add any value
Compare if ((da7219->pdata) && (!da7219->pdata->aad_pdata)) to if (da7219->pdata && !da7219->pdata->aad_pdata)
Latter looks cleaner.