From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932363AbcEXKvz (ORCPT ); Tue, 24 May 2016 06:51:55 -0400 Received: from mga01.intel.com ([192.55.52.88]:54983 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751627AbcEXKvy (ORCPT ); Tue, 24 May 2016 06:51:54 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,359,1459839600"; d="scan'208";a="813631219" Message-ID: <1464087182.31269.44.camel@linux.intel.com> Subject: Re: [PATCH v2 2/2] ASoC: da7219: Convert driver to use generic device/fwnode functions From: Andy Shevchenko To: "Opensource [Adam Thomson]" , Mark Brown , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Greg Kroah-Hartman , Robert Moore , Lv Zheng , "Rafael J.Wysocki" , Len Brown Cc: "alsa-devel@alsa-project.org" , "linux-acpi@vger.kernel.org" , "devel@acpica.org" , "linux-kernel@vger.kernel.org" , Support Opensource , Sathyanarayana Nujella , Suravee , Bjorn Helgaas , Hanjun Guo , Mika Westerberg , Andrew Morton , Heikki Krogerus Date: Tue, 24 May 2016 13:53:02 +0300 In-Reply-To: <2E89032DDAA8B9408CB92943514A0337D463EEB5@SW-EX-MBX01.diasemi.com> References: <1463664451.31269.3.camel@linux.intel.com> <2E89032DDAA8B9408CB92943514A0337D463EEB5@SW-EX-MBX01.diasemi.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.2-2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. -- Andy Shevchenko Intel Finland Oy