Received: by 2002:a05:6a10:1a4d:0:0:0:0 with SMTP id nk13csp895141pxb; Tue, 1 Feb 2022 12:39:27 -0800 (PST) X-Google-Smtp-Source: ABdhPJwOuzjpe6rdyFR/DgmYPpyGNEsns7shd0SZ3K+wWuAx0atsssMIpPuiEBOFJ1zyJ1/tvy/P X-Received: by 2002:a17:90a:e288:: with SMTP id d8mr4400317pjz.196.1643747967583; Tue, 01 Feb 2022 12:39:27 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1643747967; cv=none; d=google.com; s=arc-20160816; b=QFm8/jXqu8aUDXoOC/skMJYQShTXzxMR4ze/7kW94OU9nSQUG6T3QjM5EovLxJA+vM CpEHF4T+klhtbE1b1TjngzR0VbX783/0U+0Iat+j6EqYVSDIcIM2P9lk7B8tmjAhIWDd JuV304t5sgcmSlm0zaH5ogEJH7fMPiFmAlqs8kJocXAQ4dxgKKRE9DZ0uTNROubktpYu Hp3DFw0Q29GPnyX/i/x0UwQxlqboleL9TRqd9rmntjW1xkqgAcGFfYY2onzouN2JjCmN /FyoQr0erTi/wDXCxJvS8d9m3pKzTswphVkHC/58escONMLdnivTKg57E+qT0gTW0WOE 9J6w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:organization:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=flplWeFrXwC9LfjqyzPj/ssbtA9umekooqoDxkifDXs=; b=b8E04iy0SyXzXUcNevFE4uVIVVoNPLDe/g2croOtsf4F07bqOANToHqbjq526kHiQC dOOipwdjC6bRb+0TOOGjcra7+H5uvsObR/OnrgCFlVlWCqEt0U+QxiS4evsZhxT6+gXM 7kL3z2/HDuGq1rnG9/FpoHwJE0akFmA/vwza2UaIJliO9ZjU6zr1mgsR9gZRv/qtVS2j kz4rz2B2ivbaM6Y0O/TEECeHxry3zWe2wHVw+rR5jk+tbZzeLY7YNg96KbxJSuAaMRpa NGKqfEu+QUw5lVniuN7R0NQy922gelmDh9ZMTr3GBHI3U/NIY/6O+FOvlz0w3feSy4S3 /xTQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=DwekNq4a; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id p7si3200510pjn.30.2022.02.01.12.39.16; Tue, 01 Feb 2022 12:39:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=DwekNq4a; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1378806AbiAaNdI (ORCPT + 99 others); Mon, 31 Jan 2022 08:33:08 -0500 Received: from mga09.intel.com ([134.134.136.24]:53848 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1379103AbiAaNcz (ORCPT ); Mon, 31 Jan 2022 08:32:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643635975; x=1675171975; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=21GzmZFe7LvEpvmmANbf4casorIY5q1J72Vb+xIjsBY=; b=DwekNq4aEQrFaYegLaNkdMJZKC01D59bisJk9H+Uxe2yZVOPTC19nC2v cjiJ+sLCi07LXJ55icTJgk6u3HJrE2ahNG0S2GlkWIuggB/a4lHF9qLFs r4xG6JUqtUxt0a5QE9aHBLBAevV1Y/o39G6Ll+LnkENX76QeKJ0ydBm7c ZqZqiJsKCc0SvVuhmMGPREYTH3rISPU1LcrNVR3czsYGrqV6OS2W3JRCL SDaBLIaKBZ/kCuzxEThCY9KoU4ebowjTwbq+T+Ad5WKX9es6NDai1QAyG a0hvU3DH+7LskxLs8VgaF3GWpZ5V68THz6x7F2lkijmTzUTYrpSBb5PdQ w==; X-IronPort-AV: E=McAfee;i="6200,9189,10243"; a="247230404" X-IronPort-AV: E=Sophos;i="5.88,331,1635231600"; d="scan'208";a="247230404" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Jan 2022 05:32:55 -0800 X-IronPort-AV: E=Sophos;i="5.88,331,1635231600"; d="scan'208";a="534176210" Received: from smile.fi.intel.com ([10.237.72.61]) by fmsmga007-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 31 Jan 2022 05:32:51 -0800 Received: from andy by smile.fi.intel.com with local (Exim 4.95) (envelope-from ) id 1nEWmZ-00Gslu-NT; Mon, 31 Jan 2022 15:31:47 +0200 Date: Mon, 31 Jan 2022 15:31:47 +0200 From: Andy Shevchenko To: Jan =?utf-8?B?RMSFYnJvxZs=?= Cc: Linux Kernel Mailing List , linux-i2c , Jarkko Nikula , Mika Westerberg , Hans de Goede , Wolfram Sang , Raul E Rangel , Marcin Wojtas , Grzegorz Jaszczyk , upstream@semihalf.com, Tom Lendacky , "Deucher, Alexander" , "Easow, Nimesh" , "Limonciello, Mario" , kernel test robot Subject: Re: [PATCH v2 2/2] i2c: designware: Add AMD PSP I2C bus support Message-ID: References: <20220120001621.705352-2-jsd@semihalf.com> <20220128144811.783279-1-jsd@semihalf.com> <20220128144811.783279-3-jsd@semihalf.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 Mon, Jan 31, 2022 at 12:56:27PM +0100, Jan Dąbroś wrote: > pt., 28 sty 2022 o 16:50 Andy Shevchenko > napisał(a): > > On Fri, Jan 28, 2022 at 03:59:40PM +0100, Jan Dąbroś wrote: > > > pt., 28 sty 2022 o 15:48 Jan Dabros napisał(a): ... > > > > +struct psp_mbox { > > > > + u32 cmd_fields; > > > > > > + phys_addr_t i2c_req_addr; > > > > But phys_addr_t is platform-dependent type. Perhaps you meant to use u64 > > here > > always? > > Once I remove the "depends on X86_64" I believe this should be left > platform-dependent. If it's a protocol or HW layout, it may not be platform-dependent. > > > > +} __packed; ... > > > > + if (psp_send_cmd(req)) > > > > > > + return -EIO; > > > > Why is error code shadowed? > > > > Just as a side note - it wasn't modified in v2 when moving above to > psp_send_check_i2c_req(), but let me explain why I have introduced this > initially. > > We have two means of timeouts in the context of this driver: > 1. Timeout of internal mailbox, which means we cannot communicate with a > PSP for a programmed timeout. This timeout is encountered inside > psp_send_cmd(). > 2. Timeout of i2c arbitration - which means that we can communicate with > PSP, but PSP refuses to release i2c bus for too long. This timeout is > returned by psp_send_i2c_req() in case of error. > (side note: both error conditions are very unlikely to happen at runtime) > > I wanted to clearly distinguish between these two and thus put all errors > around mailbox into "-EIO category", which is actually true. At very least this code needs more or less the above to be put as a comment. ... > > > > +cleanup: > > > > + mutex_unlock(&psp_i2c_access_mutex); > > > > + return 0; > > > > Not sure I understand why we ignore all above errors here. > > > > Actually we are not ignoring them, since each error sets "psp_i2c_mbox_fail > = true;". This means that if there is any error on x86-PSP interface, we > are ignoring i2c-arbitration and just fall back to normal (that is > no-quirk) operation. > > From the i2c-client perspective (who is eventually gathering error code > from above) I think we can claim that everything is fine, since bus is > granted to it. For developers there is an error message in case some debug > will be necessary. Perhaps needs a comment (sorry, if I overlooked it). ... > > > > + if (!dev || !dev->dev) > > > > + return -ENODEV; > > > > At which circumstances may we get > > dev != NULL > > dev->dev == NULL > > ? > > > > ... > > > > > > if (!dev || !dev->dev) > > > > - return 0; > > > > + return -ENODEV; > > > > I see the same here, perhaps Hans knows the answer :-) > > Right, so I must admit that I simply used *-baytrail.c as a reference and > thinking that additional check shouldn't hurt us (always better than not > enough safety..). Looking more at this now - `dw_i2c_plat_probe()` will > boil-out earlier if `dev->dev == NULL`. Should I remove this extra check in > *-baytrail.c in the same commit? Maybe. Please, double check that it's not needed indeed. -- With Best Regards, Andy Shevchenko