From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 30621C2BA15 for ; Tue, 18 Jun 2024 09:54:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=5M8Gw3B3hhX6fHbXglMbtgP+L3mj6hKvpY+AUem+Im4=; b=CFsfVe02Lyh6ALtIcwsi5K44Rk CYYe0SRx0ZaECNaA5fp+21ZJvfZ5ZMR5YEUh+Af+np0qPnX2ONF/2mcWbuMyi/Lpk/2cz8T+kHYuD TzS0W2h7SyIP2bMxMos6mMxrdWmbd7+cUOPVnRbveV0rZtrvdWy0jMGZDFT0gBL2QME3kGQRixOQP 7ni7+mydOpn4Yppt5ZZeC83nGCjPo+RAVUoKlkmAkkFAb/zAppAiNWoqyQcP+QIE0BaaviIPDKh71 /QPDqy7Eo4BnyvK6m3ZYe+XExx+VaZd/9awFBmTwul2GhZMU5DQRK9QjwKwDM0BgRWIoE/8iDTvRq fDshQzZg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJVXQ-0000000EEGr-06vZ; Tue, 18 Jun 2024 09:54:04 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJVXN-0000000EEGG-24kU for linux-arm-kernel@lists.infradead.org; Tue, 18 Jun 2024 09:54:03 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 55C55DA7; Tue, 18 Jun 2024 02:54:23 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 942BF3F6A8; Tue, 18 Jun 2024 02:53:56 -0700 (PDT) Date: Tue, 18 Jun 2024 10:53:54 +0100 From: Cristian Marussi To: "Peng Fan (OSS)" Cc: Jonathan Corbet , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , Sudeep Holla , Cristian Marussi , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Peng Fan , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org Subject: Re: [PATCH v4 5/6] firmware: imx: support i.MX95 BBM module Message-ID: References: <20240524-imx95-bbm-misc-v2-v4-0-dc456995d590@nxp.com> <20240524-imx95-bbm-misc-v2-v4-5-dc456995d590@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240524-imx95-bbm-misc-v2-v4-5-dc456995d590@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240618_025401_653224_CA65883E X-CRM114-Status: GOOD ( 26.11 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, May 24, 2024 at 04:56:47PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan > > The BBM module provides RTC and BUTTON feature. To i.MX95, this module > is managed by System Manager. Linux could use i.MX SCMI BBM Extension > protocol to use RTC and BUTTON feature. > > This driver is to use SCMI interface to get/set RTC, enable pwrkey. > > Signed-off-by: Peng Fan > --- > drivers/firmware/imx/Makefile | 1 + > drivers/firmware/imx/sm-bbm.c | 314 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 315 insertions(+) > > diff --git a/drivers/firmware/imx/Makefile b/drivers/firmware/imx/Makefile > index 8f9f04a513a8..fb20e22074e1 100644 > --- a/drivers/firmware/imx/Makefile > +++ b/drivers/firmware/imx/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_IMX_DSP) += imx-dsp.o > obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o imx-scu-irq.o rm.o imx-scu-soc.o > +obj-${CONFIG_IMX_SCMI_BBM_EXT} += sm-bbm.o > diff --git a/drivers/firmware/imx/sm-bbm.c b/drivers/firmware/imx/sm-bbm.c > new file mode 100644 > index 000000000000..5e7083bf8fd3 > --- /dev/null > +++ b/drivers/firmware/imx/sm-bbm.c > @@ -0,0 +1,314 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2024 NXP. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DEBOUNCE_TIME 30 > +#define REPEAT_INTERVAL 60 > + > +struct scmi_imx_bbm { > + struct rtc_device *rtc_dev; > + struct scmi_protocol_handle *ph; > + const struct scmi_imx_bbm_proto_ops *ops; > + struct notifier_block nb; > + int keycode; > + int keystate; /* 1:pressed */ > + bool suspended; > + struct delayed_work check_work; > + struct input_dev *input; > +}; You could pull out the *ops in a global like static const struct scmi_imx_bbm_proto_ops *bbm_ops; since the protocol ops handle are always the same you will end up overwriting it with the same value if this driver is probed multiple times (which is harmless)...you will get anyway a different *ph handle to operate on that you are already storing... ..up to you really > + > +static int scmi_imx_bbm_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + struct scmi_protocol_handle *ph = bbnsm->ph; > + u64 val; > + int ret; > + > + ret = bbnsm->ops->rtc_time_get(ph, 0, &val); > + if (ret) > + dev_err(dev, "%s: %d\n", __func__, ret); > + ..so if you fail to get the time via SCMI you just carry on without caring ? > + rtc_time64_to_tm(val, tm); ... converting some random val from the stack into tm ? > + > + return 0; > +} > + > +static int scmi_imx_bbm_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + struct scmi_protocol_handle *ph = bbnsm->ph; > + u64 val; > + int ret; > + > + val = rtc_tm_to_time64(tm); > + > + ret = bbnsm->ops->rtc_time_set(ph, 0, val); > + if (ret) > + dev_err(dev, "%s: %d\n", __func__, ret); > + same here why you dont report any error ? > + return 0; > +} > + > +static int scmi_imx_bbm_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ > + return 0; > +} > + > +static int scmi_imx_bbm_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev); > + struct scmi_protocol_handle *ph = bbnsm->ph; > + struct rtc_time *alrm_tm = &alrm->time; > + u64 val; > + int ret; > + > + val = rtc_tm_to_time64(alrm_tm); > + > + ret = bbnsm->ops->rtc_alarm_set(ph, 0, val); > + if (ret) > + dev_err(dev, "%s: %d\n", __func__, ret); > + same ... ....mmm... hang on ... this reminds me of something... https://lore.kernel.org/linux-arm-kernel/ZdjgSxFx9YRP107y@pluto/ ...I did exactly the same comments on V1 around these 2 BBM/MISC SCMI drivers....but you never replied, addressed or disputed those issues. You DID address other review/comments in protocols and DT in later versions so I suppose you just forget these latter drivers reviewes and just rebased on. So, this review stops here for now, please address or reply to my V1 concerns on these last 2 BBM/MISC drivers. Thanks, Cristian