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 1989EECAAD1 for ; Thu, 1 Sep 2022 19:27:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=s9ycROA66yNTwRWLEf1XilRACgBxu1RRz8krfxFW5rc=; b=yf+LPfrl4bfkb+ rVGzIXRxD9HNWaFkUqcZX0ZIVwjt2tcEhys/oUzZ2iWZ/g2N85zdyeK+seV/DdHQhVPh6d+KdFID7 qV6sT7p8lCwDVe33IHN47+TRtP1ytELCT34VEynY2p8afsx/UBX6O42DY3K426u8hYRkSx2htDuq/ ppPOkxd95EPlwmrqko+JQZcDfYPStcHrGFiz2SX2Lsdh0VhlHrVfM/eJ0K9+3PpluAnO2RByOqlMh J7RiMrp+DTpri4CEAQQLCGB9UwTVAHVofGaHlqW5aoi2ZunBZmwE8ljKHS9QGkLCswcBrQahuVY7m Tvk8p+zsUVCA3w+VmJQA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTpq4-00EDtv-E2; Thu, 01 Sep 2022 19:26:56 +0000 Received: from mail-qt1-x82b.google.com ([2607:f8b0:4864:20::82b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTpq1-00EDsW-5e for linux-arm-kernel@lists.infradead.org; Thu, 01 Sep 2022 19:26:54 +0000 Received: by mail-qt1-x82b.google.com with SMTP id g14so14259045qto.11 for ; Thu, 01 Sep 2022 12:26:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=GwpDrpLxHphzM2y1BmvfF0fkc6vmYO6P4//roMJ5Xlo=; b=GYhX1Vl/+JSq6BUxkOtP8ctOOINt6dbz71dV61BRJOuNshjDKMTdVSG9Cda7fi6F4m H6jGdvVTWZFUkLtNzDb36YP1CfqmrISJmDnnrUGtXpT4YH7hWhltMLInrpt+Pl3OTWSL 4viVxDZunpDDNZ0PBpunoni02DkX91yozOuuKeLWpA28RGqTaj/4JqKlu7IeUf3N0cnJ gUeV3fDALaleWZ9x+v4mzjx83djmMIO8QIvWleon+KY6kniszhuhHRR0w7fx4M5cU/TD j7ZZFYnMlhsGfj7KvgzAPwrv0hbUeIp2H+9PztZbCcGwazqbRCBVrmoBpf8bpHkkzje9 6bxg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=GwpDrpLxHphzM2y1BmvfF0fkc6vmYO6P4//roMJ5Xlo=; b=65DXJWl5ApjwAd83N14riMcyN3nbzW5HGOZOd67fojXXhVuUUKQAdSyQ4gnx9k9HZ3 Z+jyuoiexOJT4D2WfDs6T4BXk1VGY7mV9TjiH9OnCQQ61gnGHXi7OdRc/bcvkov2LbEk 4QLXVQ+aHV+UsHbIZ85z99b5v7kJOMm4eixD5XBVKq/+Kne25p06y8EYmZZAgtbm/mOV /nDY7jAU4pzJKIj+zbbH5yPCO9cmGMt7VH9o17zaLhzpDFppMU3GNskXBx8sYf9o+fci e4BLC9Arzj8WH95fcgG8Ie40cAXUIhylTbX1UiOgsV6a6d98LcFbyoVZJMGqO9M2p1Cf fZNQ== X-Gm-Message-State: ACgBeo3or2cowuC4B7Qz1i7i/2MvbLo18Kb4Eir2eL8RWs0pkEkOb50B 2S+JiRZpou41Vzu3EkjyYDo8Hgl5eEkLUmCY0fxpwyF+Xno= X-Google-Smtp-Source: AA6agR50jQ30lfgMirgOaQB2+evO/PjdGtaha4+mW/zYPZaVoLc8nSkoHPVHYr+HqAT4LMRSE0IJpeYK5hBfL8sUnuM= X-Received: by 2002:ac8:7f92:0:b0:344:8cd8:59a1 with SMTP id z18-20020ac87f92000000b003448cd859a1mr25829693qtj.384.1662060412032; Thu, 01 Sep 2022 12:26:52 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Andy Shevchenko Date: Thu, 1 Sep 2022 22:26:15 +0300 Message-ID: Subject: Re: [PATCH 4/6] platform/apple: Add new Apple Mac SMC driver To: Russell King Cc: Arnd Bergmann , Lee Jones , Linus Walleij , Alyssa Rosenzweig , asahi@lists.linux.dev, Bartosz Golaszewski , Hector Martin , linux-arm Mailing List , "open list:GPIO SUBSYSTEM" , Sven Peter X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220901_122653_234812_10DAC32D X-CRM114-Status: GOOD ( 29.51 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 1, 2022 at 5:18 PM Russell King wrote: > > From: Hector Martin > > This driver implements support for the SMC (System Management > Controller) in Apple Macs. In contrast to the existing applesmc driver, > it uses pluggable backends that allow it to support different SMC > implementations, and uses the MFD subsystem to expose the core SMC > functionality so that specific features (gpio, hwmon, battery, etc.) can > be implemented by separate drivers in their respective downstream > subsystems. > > The initial RTKit backend adds support for Apple Silicon Macs (M1 et > al). We hope a backend for T2 Macs will be written in the future > (since those are not supported by applesmc), and eventually an x86 > backend would allow us to fully deprecate applesmc in favor of this > driver. ... > drivers/platform/Kconfig | 2 + > drivers/platform/Makefile | 1 + > drivers/platform/apple/Kconfig | 49 ++++ > drivers/platform/apple/Makefile | 11 + Are you going to collect the code from, e.g., PDx86 which supports some apple devices here? ... > +EXPORT_SYMBOL(apple_smc_read); Can you from day 1 make it a namespaced variant? Ditto for the rest of the exported stuff. ... > +#include Usually we put asm/* after linux/*. Missed bits.h. > +#include > +#include > +#include > +#include > +#include > +#include > +#include ... > + smc->msg_id = (smc->msg_id + 1) & 0xf; % 16 will tell much cleaner of the purpose, no? ... > + while (smc->atomic_pending) { > + ret = apple_rtkit_poll(smc->rtk); > + if (ret < 0) { > + dev_err(smc->dev, "RTKit poll failed (%llx)", msg); > + return ret; > + } > + udelay(100); > + } Something from iopoll.h to be utilised? ... > + if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) { > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); Why casting? > + return -EIO; > + } ... > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; And this is in Linux error numbering space?! ... > + smc->msg_id = (smc->msg_id + 1) & 0xf; See above. Perhaps you need a macro / inline helper for this to avoid dups. ... > + do { > + if (wait_for_completion_timeout(&smc->cmd_done, > + msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) { > + dev_err(smc->dev, "Command timed out (%llx)", msg); > + return -ETIMEDOUT; > + } > + if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id) > + break; > + dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n", > + smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret)); Guaranteed to flood the logs... > + } while(1); ...with such a conditional. ... > + result = FIELD_GET(SMC_RESULT, smc->cmd_ret); > + if (result != 0) > + return -result; Linux error numbering space? ... > + if (size <= 4) > + memcpy(buf, &rdata, size); > + else > + memcpy_fromio(buf, smc->shmem.iomem, size); This is unclear why plain memcpy() for the small size and what are the side effects of the memory. Maybe you wanted memremap() instead of ioremap() to begin with? ... > + *key = swab32(*key); swab32s() ... > + if (res.end < res.start || !resource_contains(smc->sram, &res)) { Is it a reimplementation of something like resource_intersect() and Co? > + dev_err(smc->dev, > + "RTKit buffer request outside SRAM region: %pR", &res); > + return -EFAULT; > + } ... > + bfr->iomem = smc->sram_base + (res.start - smc->sram->start); Isn't it better to write as res.start + (base - start) ? ... > + if (smc->atomic_pending) { > + smc->atomic_pending = false; > + } else { > + complete(&smc->cmd_done); > + } Redundant {} in both cases. ... > + smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram"); > + if (!smc->sram) > + return dev_err_probe(dev, EIO, > + "No SRAM region"); Dup, the below does this message for you. > + smc->sram_base = devm_ioremap_resource(dev, smc->sram); > + if (IS_ERR(smc->sram_base)) > + return dev_err_probe(dev, PTR_ERR(smc->sram_base), > + "Failed to map SRAM region"); Don't we have devm_platform_ioremap_resource_byname() ? ... > + ret = apple_rtkit_wake(smc->rtk); > + if (ret != 0) Drop all these ' != 0' > + return dev_err_probe(dev, ret, > + "Failed to wake up SMC"); Why not on one line? ... > +static const struct of_device_id apple_smc_rtkit_of_match[] = { > + { .compatible = "apple,smc" }, > + {}, No comma for the terminator entry. > +}; ... > +static struct platform_driver apple_smc_rtkit_driver = { > + .driver = { > + .name = "macsmc-rtkit", > + .owner = THIS_MODULE, Unneeded dup. > + .of_match_table = apple_smc_rtkit_of_match, > + }, > + .probe = apple_smc_rtkit_probe, > + .remove = apple_smc_rtkit_remove, > +}; ... > +typedef u32 smc_key; Why?! ... > +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3]) If s is a byte buffer, the above is NIH get_unaligned_be32(). Or in case of alignment be32_to_cpu() with respective type (__be32) to be used. ... > +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key) > +{ > + u8 val; > + int ret = apple_smc_read_u8(smc, key, &val); Split assignment and definition. > + if (ret < 0) > + return ret; > + return val ? 1 : 0; > +} ... > +#define apple_smc_write_flag apple_smc_write_u8 Why is it needed? -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel