From:   "Wu, Wentong" <wentong.wu@intel.com>
To:     Hillf Danton <hdanton@sina.com>
CC:     "mchehab@kernel.org" <mchehab@kernel.org>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com>,
        "pierre-louis.bossart@linux.intel.com" 
        <pierre-louis.bossart@linux.intel.com>,
        "Wang, Zhifeng" <zhifeng.wang@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 1/3] media: pci: intel: ivsc: Add CSI submodule
Thread-Topic: [PATCH v2 1/3] media: pci: intel: ivsc: Add CSI submodule
Thread-Index: AQHZP1HqpZLwLlRt00ml9e7fRXtKOq7MO6AAgAG4EjA=
Date:   Tue, 14 Feb 2023 06:25:35 +0000
Message-ID: <DM6PR11MB4316408448DC0A07D347ED188DA29@DM6PR11MB4316.namprd11.prod.outlook.com>
References: <20230213022347.2480307-1-wentong.wu@intel.com>
 <20230213034202.2926-1-hdanton@sina.com>
In-Reply-To: <20230213034202.2926-1-hdanton@sina.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?0RwX1JhC0LRAnaeSDR9rXCXOgHBg66o18YcbMdsv6yavJwWRJt+0TOYLEVak?=
 =?us-ascii?Q?RuwcJP6Cz/NUDDay3MHDkBOxhmG/zn84XAayncUwtI20UH4jkNd7AVTFtc97?=
 =?us-ascii?Q?xpOSH31yjPlMvc/mR428HLvmVKuiTVhNcd092HPohpcqkhPgQkq+hVCL24Vr?=
 =?us-ascii?Q?//ANz+xkmsOikKgjTjexFSA9Doiz8gJ9hNeZSYrpmO2rlYsSGYC9CbN6FQRV?=
 =?us-ascii?Q?oY1HRtbjO/zQDMi8hbx40mW5e6xAYoBDuASNBVTfvtXXicwmC+y54lr4WEIx?=
 =?us-ascii?Q?O0ey7AQIog+Pbk2qxOJrsuJcCtxGJkU4Q5haw3yCChLKH2cSWPGFR1nFjQYj?=
 =?us-ascii?Q?mZCBcWJaj6d4PF/er0nPgvr4yn929d0x+WLbO3zZuk8j6/v3FhLWpmAHQeDY?=
 =?us-ascii?Q?2vClY1Hm/rHVxYoP2x8OLpEuIBTKLAiObCB7kdrtZy5YXBzqq2fQaEDmao6A?=
 =?us-ascii?Q?LJ6TsE5G4bvwaiIvxqs5ePyzai+a0V5SJ0+QtS/Pu7Ks7q1eesn/0F3jrGOu?=
 =?us-ascii?Q?Mloyl1tL9WU+0kHfzXa13lti6+2CI2bg17YEuh0gouVMTHv6QUhQXEuoDSr6?=
 =?us-ascii?Q?X+GSm8hp7KbXxS6/S+ni92Ntua/s0avm1FnBY72O4iB9K06X+FLjqyLzGH1q?=
 =?us-ascii?Q?OIG/Uv53bBqcKrrxNp4TNHI75qcQFCYCpo2KoAEaOLdXiU8Yj/GvAr9O2Gsc?=
 =?us-ascii?Q?ImH53MQNNSNHrzEX8h+3MBpsu/WRkjgouVkXgzam1SuCTLlF3CoJkyH9W0n0?=
 =?us-ascii?Q?8iouGmOVI9n9+gRTt0WSCc9Krn6IxGO76uaxR6sCNYN2SDa5OdilHRG5Qkdp?=
 =?us-ascii?Q?lTDSbhF1cD1Oii1mxOHls5gKmg4S/gGypC6aMqc6WW5OpkioVDoOdh3/bTu3?=
 =?us-ascii?Q?XlcpAFBXuZhWSmLoa7SehHpari3GpPiYi7KRijjrNF+0R+B6FAz6z9yKi6QV?=
 =?us-ascii?Q?7/qS6L0d+nkFQHnprcKfzd/JIp3sc+vcQIoIudcv8xluzjD9Cwfg1HmiiuGI?=
 =?us-ascii?Q?yjjRxunHoNcRZk3GC0UHxznvuEAutEMOR2gGgHb/aTvTjAUvOmMaBoaSojWG?=
 =?us-ascii?Q?RqtxYyuhzlx6oj1XkMrIDv2zfHSTzcnaIQVIimcKFxj7aCUXhDbXPEdhREnk?=
 =?us-ascii?Q?LvqQPTglkXgsZTDenFsZLLavDci37sfJSuo9/5hp5dnBzHEZEGwSh3siw6Q9?=
 =?us-ascii?Q?/AlY5dYGggaaKMJ6Ho2ONBHmStiZABaLU891F+IjGRmXNrYDmRedHhFYcgNQ?=
 =?us-ascii?Q?QA2dpP5KJSuRu7E6TBshyNNQNwUxVGGrMT3E+wRjMGV7l4nYZX8M4zS+RbUd?=
 =?us-ascii?Q?wvKcLH06sf11GI+nRLEYtcQcJI+3TNdLSQxopjJCqaXJ3uB70dEB2IPWEgTT?=
 =?us-ascii?Q?Vr03Pra8TwnQDctgvs+gjwwRNW3D+WMsf2hKMbqT9AU4OOrhGP8qjQYmmHox?=
 =?us-ascii?Q?2mUwMOUwMifHahwyP6fm4PTKJLajFRDL+IuSnj6KjCEfuyKfIzBvtT2gSIA9?=
 =?us-ascii?Q?X4qf321Mv2MT3B56JNbOZmlhncQCcBcBOWVLE6ciEaECI6sG9SSiV9PCzRHC?=
 =?us-ascii?Q?rwmx/VQ1vCt18FLNzhsv0sMd5dYc1N72OPaNkj8N?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4316.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9bb6ccf6-d40d-447f-1d21-08db0e544864
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Feb 2023 06:25:35.7150
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: gFjv1PJ96WpTYSDYUtBHL595QaeoYiC3qBr6gFTAzkJ24Yaf79sOl5cBzHHjAq4pR32lnstaD+MJm4mt5HyDpw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6423
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Hi Hillf Danton,

Thanks for your review

> -----Original Message-----
> From: Hillf Danton <hdanton@sina.com>
> Sent: Monday, February 13, 2023 11:42 AM
>=20
> On Mon, 13 Feb 2023 10:23:45 +0800 Wentong Wu <wentong.wu@intel.com>
> > +
> > +/* send a command to firmware and mutex must be held by caller */
> > +static int mei_csi_send(u8 *buf, size_t len) {
> > +	struct csi_cmd *cmd =3D (struct csi_cmd *)buf;
> > +	int ret;
> > +
> > +	reinit_completion(&csi->cmd_completion);
>=20
> Could you specify why reinit is needed here?

This allows new command(e.g. 'get status' command to check current firmware=
 status) to be downloaded to firmware for debugging purpose in case no resp=
onse for current command though I have never saw this happen. =20

> What is hurt without it?

No hurt for current implementation. I can remove it.

>=20
> Same question for the reinit in 2/3.

same ack for 2/3

>=20
> > +
> > +	ret =3D mei_cldev_send(csi->cldev, buf, len);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	ret =3D wait_for_completion_killable_timeout(&csi->cmd_completion,
> > +						   CSI_CMD_TIMEOUT);
> > +	if (ret < 0) {
> > +		goto out;
> > +	} else if (!ret) {
> > +		ret =3D -ETIMEDOUT;
> > +		goto out;
> > +	}