Received-SPF: pass (google.com: domain of linux-kernel+bounces-152514-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <9ba1665e-70b0-4aea-9cab-b62660953f6b@ieee.org>
Date: Sun, 21 Apr 2024 09:29:47 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Patch v2] staging: greybus: Replace gcam macros with direct dev
 log calls
Content-Language: en-US
To: Jackson Chui <jacksonchui.qwerty@gmail.com>
Cc: Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
 linux-kernel@vger.kernel.org
References: <ZhRzWNiak1qOdJLL@jc-ubuntu-dev-korn-1>
From: Alex Elder <elder@ieee.org>
In-Reply-To: <ZhRzWNiak1qOdJLL@jc-ubuntu-dev-korn-1>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 4/8/24 5:44 PM, Jackson Chui wrote:
> Reported by checkpatch:
> 
> CHECK: Macro argument 'gcam' may be better as '(gcam)' to avoid
> precedence issues
> 
> Inline standard calls to 'dev_*' kernel logging functions, in favor
> of 'gcam_*' macros, to clear up gcam-related logging.
> 
> Signed-off-by: Jackson Chui <jacksonchui.qwerty@gmail.com>

This looks good, thanks for doing this.

Nit: for the future, on a few of the dev_err() calls, the new first
argument makes the line even wider than before.  Lines wider than
80 columns are acceptable--especially when they contain formatted
output--but you could have put the format string on a new line in
a few of the cases.  I'm old-skool and prefer making things fit
in 80 columns unless it gets too ugly.

Reviewed-by: Alex Elder <elder@linaro.org>


> 
> ---
> Changes in v2:
>    - Inline 'dev_*' logging functions, over wrappers and macros
>      to just use the standard call.
>    - Remove 'gcam_*' macros, since they are no longer used.
> ---
>   drivers/staging/greybus/camera.c | 58 +++++++++++++++-----------------
>   1 file changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/greybus/camera.c b/drivers/staging/greybus/camera.c
> index a8173aa3a995..b8b2bdfa59e5 100644
> --- a/drivers/staging/greybus/camera.c
> +++ b/drivers/staging/greybus/camera.c
> @@ -180,10 +180,6 @@ static const struct gb_camera_fmt_info *gb_camera_get_format_info(u16 gb_fmt)
>   
>   #define GB_CAMERA_MAX_SETTINGS_SIZE	8192
>   
> -#define gcam_dbg(gcam, format...)	dev_dbg(&gcam->bundle->dev, format)
> -#define gcam_info(gcam, format...)	dev_info(&gcam->bundle->dev, format)
> -#define gcam_err(gcam, format...)	dev_err(&gcam->bundle->dev, format)
> -
>   static int gb_camera_operation_sync_flags(struct gb_connection *connection,
>   					  int type, unsigned int flags,
>   					  void *request, size_t request_size,
> @@ -232,8 +228,8 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
>   
>   		fmt_info = gb_camera_get_format_info(cfg->format);
>   		if (!fmt_info) {
> -			gcam_err(gcam, "unsupported greybus image format: %d\n",
> -				 cfg->format);
> +			dev_err(&gcam->bundle->dev, "unsupported greybus image format: %d\n",
> +				cfg->format);
>   			return -EIO;
>   		}
>   
> @@ -241,18 +237,18 @@ static int gb_camera_get_max_pkt_size(struct gb_camera *gcam,
>   			pkt_size = le32_to_cpu(cfg->max_pkt_size);
>   
>   			if (pkt_size == 0) {
> -				gcam_err(gcam,
> -					 "Stream %u: invalid zero maximum packet size\n",
> -					 i);
> +				dev_err(&gcam->bundle->dev,
> +					"Stream %u: invalid zero maximum packet size\n",
> +					i);
>   				return -EIO;
>   			}
>   		} else {
>   			pkt_size = le16_to_cpu(cfg->width) * fmt_info->bpp / 8;
>   
>   			if (pkt_size != le32_to_cpu(cfg->max_pkt_size)) {
> -				gcam_err(gcam,
> -					 "Stream %u: maximum packet size mismatch (%u/%u)\n",
> -					 i, pkt_size, cfg->max_pkt_size);
> +				dev_err(&gcam->bundle->dev,
> +					"Stream %u: maximum packet size mismatch (%u/%u)\n",
> +					i, pkt_size, cfg->max_pkt_size);
>   				return -EIO;
>   			}
>   		}
> @@ -275,13 +271,13 @@ static const int gb_camera_configure_streams_validate_response(struct gb_camera
>   
>   	/* Validate the returned response structure */
>   	if (resp->padding[0] || resp->padding[1]) {
> -		gcam_err(gcam, "response padding != 0\n");
> +		dev_err(&gcam->bundle->dev, "response padding != 0\n");
>   		return -EIO;
>   	}
>   
>   	if (resp->num_streams > nstreams) {
> -		gcam_err(gcam, "got #streams %u > request %u\n",
> -			 resp->num_streams, nstreams);
> +		dev_err(&gcam->bundle->dev, "got #streams %u > request %u\n",
> +			resp->num_streams, nstreams);
>   		return -EIO;
>   	}
>   
> @@ -289,7 +285,7 @@ static const int gb_camera_configure_streams_validate_response(struct gb_camera
>   		struct gb_camera_stream_config_response *cfg = &resp->config[i];
>   
>   		if (cfg->padding) {
> -			gcam_err(gcam, "stream #%u padding != 0\n", i);
> +			dev_err(&gcam->bundle->dev, "stream #%u padding != 0\n", i);
>   			return -EIO;
>   		}
>   	}
> @@ -340,16 +336,16 @@ static int gb_camera_set_power_mode(struct gb_camera *gcam, bool hs)
>   
>   	ret = gb_camera_set_intf_power_mode(gcam, intf->interface_id, hs);
>   	if (ret < 0) {
> -		gcam_err(gcam, "failed to set module interface to %s (%d)\n",
> -			 hs ? "HS" : "PWM", ret);
> +		dev_err(&gcam->bundle->dev, "failed to set module interface to %s (%d)\n",
> +			hs ? "HS" : "PWM", ret);
>   		return ret;
>   	}
>   
>   	ret = gb_camera_set_intf_power_mode(gcam, svc->ap_intf_id, hs);
>   	if (ret < 0) {
>   		gb_camera_set_intf_power_mode(gcam, intf->interface_id, !hs);
> -		gcam_err(gcam, "failed to set AP interface to %s (%d)\n",
> -			 hs ? "HS" : "PWM", ret);
> +		dev_err(&gcam->bundle->dev, "failed to set AP interface to %s (%d)\n",
> +			hs ? "HS" : "PWM", ret);
>   		return ret;
>   	}
>   
> @@ -435,7 +431,7 @@ static int gb_camera_setup_data_connection(struct gb_camera *gcam,
>   			   sizeof(csi_cfg),
>   			   GB_APB_REQUEST_CSI_TX_CONTROL, false);
>   	if (ret < 0) {
> -		gcam_err(gcam, "failed to start the CSI transmitter\n");
> +		dev_err(&gcam->bundle->dev, "failed to start the CSI transmitter\n");
>   		goto error_power;
>   	}
>   
> @@ -470,7 +466,7 @@ static void gb_camera_teardown_data_connection(struct gb_camera *gcam)
>   			   GB_APB_REQUEST_CSI_TX_CONTROL, false);
>   
>   	if (ret < 0)
> -		gcam_err(gcam, "failed to stop the CSI transmitter\n");
> +		dev_err(&gcam->bundle->dev, "failed to stop the CSI transmitter\n");
>   
>   	/* Set the UniPro link to low speed mode. */
>   	gb_camera_set_power_mode(gcam, false);
> @@ -507,7 +503,7 @@ static int gb_camera_capabilities(struct gb_camera *gcam,
>   					     NULL, 0,
>   					     (void *)capabilities, size);
>   	if (ret)
> -		gcam_err(gcam, "failed to retrieve capabilities: %d\n", ret);
> +		dev_err(&gcam->bundle->dev, "failed to retrieve capabilities: %d\n", ret);
>   
>   done:
>   	mutex_unlock(&gcam->mutex);
> @@ -723,22 +719,22 @@ static int gb_camera_request_handler(struct gb_operation *op)
>   	struct gb_message *request;
>   
>   	if (op->type != GB_CAMERA_TYPE_METADATA) {
> -		gcam_err(gcam, "Unsupported unsolicited event: %u\n", op->type);
> +		dev_err(&gcam->bundle->dev, "Unsupported unsolicited event: %u\n", op->type);
>   		return -EINVAL;
>   	}
>   
>   	request = op->request;
>   
>   	if (request->payload_size < sizeof(*payload)) {
> -		gcam_err(gcam, "Wrong event size received (%zu < %zu)\n",
> -			 request->payload_size, sizeof(*payload));
> +		dev_err(&gcam->bundle->dev, "Wrong event size received (%zu < %zu)\n",
> +			request->payload_size, sizeof(*payload));
>   		return -EINVAL;
>   	}
>   
>   	payload = request->payload;
>   
> -	gcam_dbg(gcam, "received metadata for request %u, frame %u, stream %u\n",
> -		 payload->request_id, payload->frame_number, payload->stream);
> +	dev_dbg(&gcam->bundle->dev, "received metadata for request %u, frame %u, stream %u\n",
> +		payload->request_id, payload->frame_number, payload->stream);
>   
>   	return 0;
>   }
> @@ -1347,15 +1343,15 @@ static int gb_camera_resume(struct device *dev)
>   
>   	ret = gb_connection_enable(gcam->connection);
>   	if (ret) {
> -		gcam_err(gcam, "failed to enable connection: %d\n", ret);
> +		dev_err(&gcam->bundle->dev, "failed to enable connection: %d\n", ret);
>   		return ret;
>   	}
>   
>   	if (gcam->data_connection) {
>   		ret = gb_connection_enable(gcam->data_connection);
>   		if (ret) {
> -			gcam_err(gcam,
> -				 "failed to enable data connection: %d\n", ret);
> +			dev_err(&gcam->bundle->dev,
> +				"failed to enable data connection: %d\n", ret);
>   			return ret;
>   		}
>   	}