From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f169.google.com (mail-pl1-f169.google.com [209.85.214.169]) (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 064A513635B for ; Tue, 7 Jan 2025 04:21:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736223710; cv=none; b=juYOCcjxR1NlgBnJzoCFJXn8s42e13q6KCRPvGfwbiqtv70Ntc3noJQzdSLtVV0ru3HAIVwvG/NFSr7aaEdWSs0PWsmX5jZLpkyO9PgJRhQu//JeMouwDfLVHsWIv5sTgl8yR0P+6Tz046mrYrCrN50bLB/W5VmxfiQJ0z8BK1M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736223710; c=relaxed/simple; bh=Ki0xkd7PBDiC5LHkO0khrUp2v48Baiae90mDC4Y9eSM=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=ZE58khgRnHlncUratHOC9k3ymZtOEDvLums/0+IbXzmmUWBC3fs575TuD2odYVxqOAJ8sKTp/dDzSAuZd9+sLXT1QO2YNLgnI4VUrDd4rE5sHRDGQq+YHSYcB4h7POnYYxClceevAEPBwO3SPkyz59h2QAMjS5yXwNRGbv6fwIM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Clep/91r; arc=none smtp.client-ip=209.85.214.169 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Clep/91r" Received: by mail-pl1-f169.google.com with SMTP id d9443c01a7336-21683192bf9so215294475ad.3 for ; Mon, 06 Jan 2025 20:21:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1736223707; x=1736828507; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=w6CQdG/Y4q2PglLk/FT3YvtdH/92Je/f7NVKLOTx5Co=; b=Clep/91rDOG4T7OjKe+b15hTn9yJZ7RPwFg5ouFhQURzM413M5nK5m80ZXnBZk/7bG iKgWgd85rFcnfdQ6QdSgE8GnJCzI9E8bE8XrbV3JGqBX5oh7VlrndNdK5ST2nbwXjVhp 8PwWNq5srAHsoEk+GAnidqZy8yqvfnx48NGfLfB57eCnpG/xkPjjeDwPf+CeoQbX7xY7 CVF45HKOeNuhmvE+/Ri+HDFawrfz1Xd/Y99ctu7NbesBEZA1dpOYx5I/s/UzpEnUY2ZE CK0VgxHFFNRzZ2VQ0Zl+9TQRi3mHrSx2Tr6pM3Hh3U5ORIQWfKL+vmr8Qj9lI0emWncg PmhQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736223707; x=1736828507; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=w6CQdG/Y4q2PglLk/FT3YvtdH/92Je/f7NVKLOTx5Co=; b=xHwEwg+LE1K0okmmM6IhP0FJjCb8yrNoj7ZZiTxTA4hufLCQBa3f+ejH78kNusoFqo mioDotcd2C9/xPZbawpquRimVtvKvk32+WH1lX4XAdXwKvdwJIc5L+RhJgoO5TYS9tNs U0QpPUwc19hi6hGc730P7OxQ7tpnlS9IWt33+zTHjiivLJY0bvh7Hh3MjnDv8cNGwlaM /FMweZBivANt+ZarJgAqafKr43q6wkzCtnGbT8lE5zVdw/DUEuJ6VRUQ+WD1hMAGjv1Z gsGVjx0Ms9+ljusPf+aUQkzbPDndIZO7EHyGrOtTFIUwPbf/MolZYDQ19stt4Pspt9aP 1uYQ== X-Forwarded-Encrypted: i=1; AJvYcCVHotYC/PUfLgBIjWZ6q+0M0/0MxSmqvBrF738KNMAPQo8wztT3zVlun0mlgHg49fnHKNaKKobgB20Or/k=@vger.kernel.org X-Gm-Message-State: AOJu0YyexhOLNMkXbGFIcThwqQo7dXMWKGXzXy/iwIQ/JW8/CSG0VVKn EF63UBfgjbINIWBjU6E4D4HxE1FBpKRhuogcVulsBNE+/OcxI9gz X-Gm-Gg: ASbGnctTj7HUQ2v1AU+AfZULcEN+H+T+ZqktyJjYVLQHs03uJnkw+i/x4YdXtmyk4aj z37jdVPAjXzkhNUCMHLxhC/zbADUOd5DG8SknxyNmOrTarG5/vDNwOmT/nLmIl8WZrMMu96dbWh Mg9p15JpW7+/VOtd+f18c8ejqVHzn77bm5f28PVSPvoiFk8j1REVgESjcp/5vXn/X7rhFAO8wee CpHl/ZA8jeFsxlFziit3l0RIp3NrIrmYM63/c15jxqnEUr1MMLsyMqfjLWu X-Google-Smtp-Source: AGHT+IGQP+p250RicnrOecD1qZzwLXKfTbwqgnYA71mtSK1nRSXJmRGp4vGU/0NrWqqg8k1UH71nhw== X-Received: by 2002:a17:903:41c3:b0:216:485f:bf90 with SMTP id d9443c01a7336-219e6ebad3fmr772319765ad.27.1736223707140; Mon, 06 Jan 2025 20:21:47 -0800 (PST) Received: from [10.3.72.248] ([59.152.80.69]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-219dc9d4474sm302366525ad.142.2025.01.06.20.21.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jan 2025 20:21:46 -0800 (PST) Message-ID: <70f0dbf2-dd84-4a50-94cc-1d388c5c93fe@gmail.com> Date: Tue, 7 Jan 2025 09:51:41 +0530 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] drm/panel: xinpeng-xpp055c272: transition to mipi_dsi wrapped functions To: Doug Anderson , neil.armstrong@linaro.org Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org, tzimmermann@suse.de, airlied@gmail.com, simona@ffwll.ch, quic_jesszhan@quicinc.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20241223052049.419831-1-tejasvipin76@gmail.com> <47738b2b-351b-4df9-a50a-f4dff51441c8@linaro.org> Content-Language: en-US From: Tejas Vipin In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On 1/7/25 5:37 AM, Doug Anderson wrote: > Hi, > > On Mon, Dec 30, 2024 at 2:10 AM wrote: >> >>> static int xpp055c272_unprepare(struct drm_panel *panel) >>> { >>> struct xpp055c272 *ctx = panel_to_xpp055c272(panel); >>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >>> - int ret; >>> - >>> - ret = mipi_dsi_dcs_set_display_off(dsi); >>> - if (ret < 0) >>> - dev_err(ctx->dev, "failed to set display off: %d\n", ret); >>> - >>> - mipi_dsi_dcs_enter_sleep_mode(dsi); >>> - if (ret < 0) { >>> - dev_err(ctx->dev, "failed to enter sleep mode: %d\n", ret); >>> - return ret; >>> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; >>> + >>> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx); >>> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx); >>> + if (dsi_ctx.accum_err) { >>> + dev_err(ctx->dev, "failed to enter sleep mode: %d\n", >>> + dsi_ctx.accum_err); > > You should delete the above error message, right? > mipi_dsi_dcs_enter_sleep_mode_multi() reports the error for you, I > think. > > >>> @@ -155,17 +147,19 @@ static int xpp055c272_prepare(struct drm_panel *panel) >>> { >>> struct xpp055c272 *ctx = panel_to_xpp055c272(panel); >>> struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev); >>> - int ret; >>> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi }; >>> >>> dev_dbg(ctx->dev, "Resetting the panel\n"); >>> - ret = regulator_enable(ctx->vci); >>> - if (ret < 0) { >>> - dev_err(ctx->dev, "Failed to enable vci supply: %d\n", ret); >>> - return ret; >>> + dsi_ctx.accum_err = regulator_enable(ctx->vci); >>> + if (dsi_ctx.accum_err) { >> >> I would rather keep ret instead of abusing dsi_ctx.accum_err, but it's already like >> that in other converted driver so I won't oppose it... > > FWIW, we had this discussion before. I agree with what Tejas did here > and I managed to convince Dmitry Baryshkov in the past. See: > > https://lore.kernel.org/all/CAA8EJpr_HYkXnP3XR9LpDhi1xkQfE_CKJzfzGrO5qd_pQYtiOw@mail.gmail.com/ > > Looking specifically at this driver, using "ret" would have added > complexity when we wanted to do "goto disable_vci" because in some > cases the error code would be in "ret" and sometimes in "accum_err"... > > >>> @@ -175,30 +169,19 @@ static int xpp055c272_prepare(struct drm_panel *panel) >>> gpiod_set_value_cansleep(ctx->reset_gpio, 0); >>> >>> /* T8: 20ms */ >>> - msleep(20); >>> + mipi_dsi_msleep(&dsi_ctx, 20); > > Personally, I would have left the above msleep() alone. There can be > no errors at this point in the code, right? > > >>> - ret = xpp055c272_init_sequence(ctx); >>> - if (ret < 0) { >>> - dev_err(ctx->dev, "Panel init sequence failed: %d\n", ret); >>> - goto disable_iovcc; >>> - } >>> - >>> - ret = mipi_dsi_dcs_exit_sleep_mode(dsi); >>> - if (ret < 0) { >>> - dev_err(ctx->dev, "Failed to exit sleep mode: %d\n", ret); >>> - goto disable_iovcc; >>> - } >>> + xpp055c272_init_sequence(&dsi_ctx); >>> + dev_dbg(ctx->dev, "Panel init sequence done\n"); > > Should the above print be only if "accum_err" is 0? That would match > the previous behavior. I guess I would have also left the print as > part of xpp055c272_init_sequence() unless there's a reason for moving > it... I don't think it should print only if accum_err is 0. In the previous code, it would just print after all the msleeps and write_seqs are done, with no error checking at any point. The reason I've moved the print outside the function is because we are able to reduce a couple lines of code by passing dsi_ctx to the function instead of ctx. If I'd kept the print inside, it would require us to declare a `struct device*` variable which would require ctx as far as I've seen and just overall introduces some lines that we could otherwise avoid. I've done this in a couple other panels too. I'll do a v2 with the other suggested changes. -- Tejas Vipin