From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C623BC64EC4 for ; Fri, 17 Feb 2023 11:24:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230073AbjBQLYZ (ORCPT ); Fri, 17 Feb 2023 06:24:25 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40470 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229660AbjBQLYX (ORCPT ); Fri, 17 Feb 2023 06:24:23 -0500 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CA6E6642DF; Fri, 17 Feb 2023 03:24:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1676633062; x=1708169062; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=RUHMDt6eHewLs7RsR6DcPytDd6NJdHekltVoH/mlQGQ=; b=jcZRPPKfGcl40TQJMrm2S+EnXIyDu4DnF4NM/yODAG1Wj438KFO3+qCp pI7KK3FGdc8FPQy6UqQ6ytGpzPrHRysCdowkFvNucbUtAvj9XKo/jHIoW qVH88DQLH+WfJNi/4U531BYkB4hQfzb58RinIBzCgk3rLqUURJpvCnBFX monx3F6yYZAFkeciS8IzVzB2OAtqqjvJHuTlOI6gyNh7eXHIUEqE9Xhig fau7MJxZqvp6RHHQy2aPEoGPqRG9Ao7a+pgpHSQHnh7m3aowyFKBdosIp 2SoneeDj/f4lymuK1rIVlSj2kSJoZn5FEGUMi55sZyEqFaUUPeIsPTbPH A==; X-IronPort-AV: E=McAfee;i="6500,9779,10623"; a="331967805" X-IronPort-AV: E=Sophos;i="5.97,304,1669104000"; d="scan'208";a="331967805" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Feb 2023 03:24:22 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10623"; a="916047677" X-IronPort-AV: E=Sophos;i="5.97,304,1669104000"; d="scan'208";a="916047677" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga006.fm.intel.com with ESMTP; 17 Feb 2023 03:24:16 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.96) (envelope-from ) id 1pSyqb-008BAc-2C; Fri, 17 Feb 2023 13:24:13 +0200 Date: Fri, 17 Feb 2023 13:24:13 +0200 From: Andy Shevchenko To: Tomi Valkeinen Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org, Rob Herring , Krzysztof Kozlowski , Wolfram Sang , Luca Ceresoli , Matti Vaittinen , Laurent Pinchart , Mauro Carvalho Chehab , Peter Rosin , Liam Girdwood , Mark Brown , Sakari Ailus , Michael Tretter , Shawn Tu , Hans Verkuil , Mike Pagano , Krzysztof =?utf-8?Q?Ha=C5=82asa?= , Marek Vasut , Satish Nagireddy Subject: Re: [PATCH v9 0/8] i2c-atr and FPDLink Message-ID: References: <20230216140747.445477-1-tomi.valkeinen@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 17, 2023 at 08:57:32AM +0200, Tomi Valkeinen wrote: > On 16/02/2023 17:53, Andy Shevchenko wrote: > > On Thu, Feb 16, 2023 at 04:07:39PM +0200, Tomi Valkeinen wrote: ... > > > struct i2c_board_info ser_info = { > > > - .of_node = to_of_node(rxport->remote_fwnode), > > > - .fwnode = rxport->remote_fwnode, > > > > > + .of_node = to_of_node(rxport->ser.fwnode), > > > + .fwnode = rxport->ser.fwnode, > > > > Why do you need to have both?! > > I didn't debug it, but having only fwnode there will break the probing (no > match). This needs to be investigated. The whole fwnode approach, when we have both fwnode and legacy of_node fields in the same data structure, is that fwnode _OR_ of_node initialization is enough, when both are defined the fwnode should take precedence. If your testing is correct (and I have no doubts) it means we have a serious bug lurking somewhere. > > > .platform_data = ser_pdata, > > > }; ... > > cur_vc = desc.entry[0].bus.csi2.vc; > > > > > + for (i = 0; i < desc.num_entries; ++i) { > > > + u8 vc = desc.entry[i].bus.csi2.vc; > > > > > + if (i == 0) { > > > + cur_vc = vc; > > > + continue; > > > + } > > > > This is an invariant to the loop, see above. > > Well, the current code handles the case of num_entries == 0. I can change it > as you suggest, and first check if num_entries == 0 and also start the loop > from 1. You may try to compile both variants and see which one gets lets code. I believe it will be mine or they are equivalent in case compiler is clever enough to recognize the invariant. > > > + if (vc == cur_vc) > > > + continue; > > > + > > > + dev_err(&priv->client->dev, > > > + "rx%u: source with multiple virtual-channels is not supported\n", > > > + nport); > > > + return -ENODEV; > > > + } ... > > > + for (i = 0; i < 6; ++i) > > > ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]); > > > id[6] = 0; > > > > Wondering if this magic can be defined. > > The number of ID registers? Yes, I can add a define. Yes. ... ... > > > if (ret) { > > > if (ret != -EINVAL) { > > > - dev_err(dev, > > > - "rx%u: failed to read 'ti,strobe-pos': %d\n", > > > - nport, ret); > > > + dev_err(dev, "rx%u: failed to read '%s': %d\n", nport, > > > + "ti,strobe-pos", ret); > > > return ret; > > > } > > > } else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS || > > > @@ -3512,8 +3403,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, > > > ret = fwnode_property_read_u32(link_fwnode, "ti,eq-level", &eq_level); > > > if (ret) { > > > if (ret != -EINVAL) { > > > - dev_err(dev, "rx%u: failed to read 'ti,eq-level': %d\n", > > > - nport, ret); > > > + dev_err(dev, "rx%u: failed to read '%s': %d\n", nport, > > > + "ti,eq-level", ret); > > > return ret; > > > } > > > } else if (eq_level > UB960_MAX_EQ_LEVEL) { > > > > Hmm, I noticed this one (and the one above) was missing return -EINVAL. > > > Seems like you may do (in both cases) similar to the above: > > > > var = 0; > > ret = read_u32(); > > if (ret && ret != -EINVAL) { > > // error handling > > } > > if (var > limit) { > > // another error handling > > } > > That's not the same. You'd also need to do: > > if (!ret) { > // handle the retrieved value > } > > which, I think, is not any clearer (perhaps more unclear). > > What I could do is: > > if (ret) { > if (ret != -EINVAL) { > dev_err(dev, "rx%u: failed to read '%s': %d\n", nport, > "ti,eq-level", ret); > return ret; > } > } else { > if (eq_level > UB960_MAX_EQ_LEVEL) { > dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n", > nport, eq_level); > return -EINVAL; > } > > rxport->eq.manual_eq = true; > rxport->eq.manual.eq_level = eq_level; > } > > Maybe the above style makes it clearer, as it clearly splits the "don't have > value" and "have value" branches. Up to you, but this just a good example why I do not like how optional properties are handled in a "smart" way. To me foo = DEFAULT; _property_read_(&foo); // no error checking is clean, neat, small and good enough solution. ... > > > + static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { "vpoc0", "vpoc1", > > > + "vpoc2", "vpoc3" }; > > > > Wouldn't be better to format it as > > > > static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { > > "vpoc0", "vpoc1", "vpoc2", "vpoc3", > > }; > > > > ? > > Clang-format disagrees, but I agree with you ;). So it needs to be fixed then :-) Glad that you agreed on this. -- With Best Regards, Andy Shevchenko