From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mta0.migadu.com (out-109.mta0.migadu.com [91.218.175.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4658E450909 for ; Tue, 15 Sep 2026 19:51:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.109 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789501873; cv=none; b=ETF/0SV0KtInKbJ745j1bEhv8L543hyEmbRrVaHtFaQ7bmiLSVGZLuNwUQhe71axZj9N376RpsuiGHXJHo5PLXx1WI5W7DVsShgF1SKmSchiQa+xd85WhlJ8ficE2GvO99t85x3B2CiYJIoD7bxZY53IuUlr5ADibg1uCCJxmDk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1789501873; c=relaxed/simple; bh=uAf2DJcdhrhTYI1UJ6GNxaESDfQLXKd+SMlyrQoroTM=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=dZLdWGkhYBp1l0mLoSIzYEBOuavSeT2/sPfLbo6hXWKJGSMVZuAcNInlmkpFWWK+ItMp0aC+8msa2L1CFoMM2vutXF/YbYEST557rEPqa2JzPmL3PNbARqCf74j1xSUFx7IZoovM8rzlOFRstSlJF/csY4U8DtBFyoZSM72pASE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=nsj8AtOt; arc=none smtp.client-ip=91.218.175.109 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="nsj8AtOt" X-Envelope-To: linux-kernel@vger.kernel.org DKIM-Signature: a=rsa-sha256; bh=uAf2DJcdhrhTYI1UJ6GNxaESDfQLXKd+SMlyrQoroTM=; c=simple/simple; d=linux.dev; h=from:to:subject:date:message-id:mime-version:content-type; s=key1; t=1789501869; v=1; x=1790106669; b=nsj8AtOtPOgMApO0KqyHXvOYlEPTN8afxEZ27h/qxar0Ia4CYNAKqzFVWTd8hq/rMEZZWlhA 8HZNq8O1Y6emzDzslFPnxL7R6E9NgSrxF10YI1RLZoflX/ahjLiolNQurCGGHi5eGZxcpTb1DUj dDtI1QlNTbzdFPgJTnVd0zw4= X-Envelope-To: linux-kernel@vger.kernel.org Received: by smtp.migadu.com with ESMTPS id df467dddeed73729; Tue, 15 Sep 2026 19:51:09 +0000 X-Mizu-Trace-ID: df467dddeed73729 X-Migadu-Flow: FLOW_OUT Date: Tue, 15 Sep 2026 21:51:03 +0200 From: Richard Leitner To: Dave Stevenson Cc: Sakari Ailus , Mauro Carvalho Chehab , Martina Krasteva , "Paul J. Murphy" , Daniele Alessandrelli , Hans Verkuil , Mauro Carvalho Chehab , Gyula Kelemen , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 08/10] media: i2c: ov9282: harmonize dev_err_probe usage Message-ID: References: <20260914-ov9282-fixes-v1-0-f520af59df1b@linux.dev> <20260914-ov9282-fixes-v1-8-f520af59df1b@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Hi Dave, thanks for the review! On Tue, Sep 15, 2026 at 03:55:01PM +0100, Dave Stevenson wrote: > Hi Richard > > On Mon, 14 Sept 2026 at 20:21, Richard Leitner > wrote: > > > > Use dev_err_probe() for all error messages during probing. This ensures > > there's a common "look-and-feel" in the drivers source code as well as > > the system log. > > > > Signed-off-by: Richard Leitner > > --- > > drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++-------------------- > > 1 file changed, 18 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c > > index 28f8b05b4c09e..f728709fcf0a6 100644 > > --- a/drivers/media/i2c/ov9282.c > > +++ b/drivers/media/i2c/ov9282.c > > @@ -1109,26 +1109,25 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282) > > ov9282->reset_gpio = devm_gpiod_get_optional(ov9282->dev, "reset", > > GPIOD_OUT_LOW); > > if (IS_ERR(ov9282->reset_gpio)) { > > - dev_err(ov9282->dev, "failed to get reset gpio %pe", > > - ov9282->reset_gpio); > > - return PTR_ERR(ov9282->reset_gpio); > > + return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->reset_gpio), > > + "failed to get reset gpio"); > > } > > > > /* Get sensor input clock */ > > ov9282->inclk = devm_v4l2_sensor_clk_get(ov9282->dev, NULL); > > if (IS_ERR(ov9282->inclk)) > > return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->inclk), > > - "could not get inclk\n"); > > + "could not get inclk"); > > My understanding is that dev_err_probe should always have the \n on > the end of the log text. > Admittedly "failed to get reset gpio %pe" above is missing it, but > removing it off all the other instances seems to be the wrong fix. Thanks for the pointer. I wasn't aware of this convention. Also the documentation of dev_err_probe mentions nothing. Nonetheless when grepping through the kernel source I totally agree. "\n" is definitely preferred. I will adapt this for v2. regards;rl > > Otherwise the patch looks fine. > > Dave > > > > > ret = ov9282_configure_regulators(ov9282); > > if (ret) > > return dev_err_probe(ov9282->dev, ret, > > - "Failed to get power regulators\n"); > > + "Failed to get power regulators"); > > > > rate = clk_get_rate(ov9282->inclk); > > if (rate != OV9282_INCLK_RATE) { > > - dev_err(ov9282->dev, "inclk frequency mismatch"); > > - return -EINVAL; > > + return dev_err_probe(ov9282->dev, -EINVAL, > > + "inclk frequency mismatch"); > > } > > > > ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > > @@ -1144,16 +1143,15 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282) > > bus_cfg.bus.mipi_csi2.flags & V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK; > > > > if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV9282_NUM_DATA_LANES) { > > - dev_err(ov9282->dev, > > - "number of CSI2 data lanes %d is not supported", > > - bus_cfg.bus.mipi_csi2.num_data_lanes); > > - ret = -EINVAL; > > + ret = dev_err_probe(ov9282->dev, -EINVAL, > > + "number of CSI2 data lanes %d is not supported", > > + bus_cfg.bus.mipi_csi2.num_data_lanes); > > goto done_endpoint_free; > > } > > > > if (!bus_cfg.nr_of_link_frequencies) { > > - dev_err(ov9282->dev, "no link frequencies defined"); > > - ret = -EINVAL; > > + ret = dev_err_probe(ov9282->dev, -EINVAL, > > + "no link frequencies defined"); > > goto done_endpoint_free; > > } > > > > @@ -1382,14 +1380,14 @@ static int ov9282_probe(struct i2c_client *client) > > > > ret = ov9282_parse_hw_config(ov9282); > > if (ret) { > > - dev_err(ov9282->dev, "HW configuration is not supported"); > > - return ret; > > + return dev_err_probe(ov9282->dev, ret, > > + "HW configuration is not supported"); > > } > > > > ov9282->regmap = devm_cci_regmap_init_i2c(client, 16); > > if (IS_ERR(ov9282->regmap)) > > return dev_err_probe(ov9282->dev, PTR_ERR(ov9282->regmap), > > - "Failed to init CCI\n"); > > + "Failed to init CCI"); > > > > ret = ov9282_power_on(ov9282->dev); > > if (ret) > > @@ -1399,7 +1397,7 @@ static int ov9282_probe(struct i2c_client *client) > > /* Check module identity */ > > ret = ov9282_detect(ov9282); > > if (ret) { > > - dev_err(ov9282->dev, "failed to find sensor: %d", ret); > > + dev_err_probe(ov9282->dev, ret, "failed to find sensor"); > > goto error_power_off; > > } > > > > @@ -1409,7 +1407,7 @@ static int ov9282_probe(struct i2c_client *client) > > > > ret = ov9282_init_controls(ov9282); > > if (ret) { > > - dev_err(ov9282->dev, "failed to init controls: %d", ret); > > + dev_err_probe(ov9282->dev, ret, "failed to init controls"); > > goto error_power_off; > > } > > > > @@ -1422,14 +1420,14 @@ static int ov9282_probe(struct i2c_client *client) > > ov9282->pad.flags = MEDIA_PAD_FL_SOURCE; > > ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad); > > if (ret) { > > - dev_err(ov9282->dev, "failed to init entity pads: %d", ret); > > + dev_err_probe(ov9282->dev, ret, "failed to init entity pads"); > > goto error_handler_free; > > } > > > > ov9282->sd.state_lock = ov9282->ctrl_handler.lock; > > ret = v4l2_subdev_init_finalize(&ov9282->sd); > > if (ret < 0) { > > - dev_err_probe(ov9282->dev, ret, "failed to init subdev\n"); > > + dev_err_probe(ov9282->dev, ret, "failed to init subdev"); > > goto error_media_entity; > > } > > > > > > -- > > 2.53.0 > > > >