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 99EB3ECAAD1 for ; Thu, 1 Sep 2022 18:57:07 +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=lreSmwEKZCDKYcvY6iekywjyMsn1AUb05cpVprmxt0U=; b=Aeo9XXKbrXdLMs qJ1uaIiiXNNVfjj25C6sGd0arTwggX4OFbAG9VUvglYZwb4Z1xp8OT0zY5hu/JJDB7NvErfR5IU4X k1P9N8hWn+biazqDBC0Y6EAqBcGznqI6SWc1BZcBvfGfMe8Wjiwmdq8VLH7939Tm5CBAz8PmJ8DYD 38lWWf9/sxjIZgg3ah0gz91Oai/4VXLXgSFJYte6gpI82UNYRr9hDljxrc6V6AsT+YMWN82jYJYlB JpkacjwwViCAjgrlpjimvsUk5WoQ2AbDzXl08U2o4MF+f7L4C5m6ryCaGOSXacppC4sMqIu5lFqTP IVAA24RL7PD3qXiFV/ig==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTpMC-00E625-1J; Thu, 01 Sep 2022 18:56:04 +0000 Received: from mail-qk1-x732.google.com ([2607:f8b0:4864:20::732]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTpM8-00E60S-Un for linux-arm-kernel@lists.infradead.org; Thu, 01 Sep 2022 18:56:02 +0000 Received: by mail-qk1-x732.google.com with SMTP id a10so12719qkl.13 for ; Thu, 01 Sep 2022 11:56:00 -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=1VvKQrI+SSW3GbbR3EcVWpVRmYCp6SaG8dIjhzsYhOM=; b=Bh36Zm/09czxvJIp6FKPj/gpDE2cQv5AUbtCEww7iLv77eW+xe70dYVjKj8o0DKgkp wX5sNd2NIxQZZHMIouEgDUx2HVgoi+JhXP16roRVEPLgjneAOLJo0badD8V14/T6aAIc BYEWg1IuGeQi5Dh4nHFvq2eFQpOaYBufXZ7qCy88SMhJevvMR1CEENesqlyR3exklmHx hOcHxLhJtfupJ3PgfRe/mvZmU1k+2jQ9Oi1NfHRNNF8pBFKczaONc6hb7Pa3yQHtU9LJ MYgFIKZHOfhRt5NWgRpuHQvaKcK5oJCDjKoeY0Ad4kfCRracjbRMMCSPKXRqlBYXywSW cEzA== 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=1VvKQrI+SSW3GbbR3EcVWpVRmYCp6SaG8dIjhzsYhOM=; b=KFKK1JvZOShGE4MzmjpNBgJzwvADv22jHZppCctZlvKYYIcWGSq7yrSvFiasR/apTB 0zHSQpfuNe4Da7uOpml1Q76YZp29t2HwX8Tl0+UfIJGnib7JmyYa45qJCaHHmEbkuT7d N9K4Y+2Gxbq+8C+2HxpVQZvtGRbFOdwTdgMUqrBwyTwyKypZg4w6R/tAmLvRpgTD10hp ShXngakUxIHjxq10laKBwXnq0trhRoTci0HmeinZCNQKo14Ztkdg5OBZB0SsbQU7EHUl 9QNzE03sxhr6l8Ry1oqobzwE8820QorsMAFJyGmf7Bv4MJrN8FhBCTEcZ3cCq0ifN6eV 674A== X-Gm-Message-State: ACgBeo0ZJid+LZ67fqXiNevjL/duswSXFw9bwwMCYxgNf/d7UJql2fmu 8w5+0tFOfzqSVVkUTnsk1kL7YJT7QGV0V2gufBM= X-Google-Smtp-Source: AA6agR6yVph8B9MEmz/Yn3Kga+Chvz+h6RQ4roBohfY8Ki1bWSSoJdC3BLtoybu3oxfMWiUVAIYlbYqnS2VKv/VV7oI= X-Received: by 2002:a05:620a:254d:b0:6ab:84b8:25eb with SMTP id s13-20020a05620a254d00b006ab84b825ebmr20567527qko.383.1662058559429; Thu, 01 Sep 2022 11:55:59 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Andy Shevchenko Date: Thu, 1 Sep 2022 21:55:23 +0300 Message-ID: Subject: Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs 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_115601_028383_F1D3226D X-CRM114-Status: GOOD ( 34.54 ) 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:17 PM Russell King wrote: > > From: Hector Martin > > This driver implements the GPIO service on top of the SMC framework > on Apple Mac machines. In particular, these are the GPIOs present in the > PMU IC which are used to control power to certain on-board devices. > > Although the underlying hardware supports various pin config settings > (input/output, open drain, etc.), this driver does not implement that > functionality and leaves it up to the firmware to configure things > properly. We also don't yet support interrupts/events. This is > sufficient for device power control, which is the only thing we need to > support at this point. More features will be implemented when needed. > > To our knowledge, only Apple Silicon Macs implement this SMC feature. ... > +/* > + * Commands 0-6 are, presumably, the intended API. > + * Command 0xff lets you get/set the pin configuration in detail directly, > + * but the bit meanings seem not to be stable between devices/PMU hardware > + * versions. Probably for debugging purposes... > + * > + * We're going to try to make do with the low commands for now. > + * We don't implement pin mode changes at this time. > + */ ... > +/* > + * output modes seem to differ depending on the PMU in use... ? Output > + * j274 / M1 (Sera PMU): > + * 0 = input > + * 1 = output > + * 2 = open drain > + * 3 = disable > + * j314 / M1Pro (Maverick PMU): > + * 0 = input > + * 1 = open drain > + * 2 = output > + * 3 = ? > + */ ... > +struct macsmc_gpio { > + struct device *dev; > + struct apple_smc *smc; > + struct gpio_chip gc; You might save some CPU cycles / code by shuffling members around. Usually we put gpio_chip as a first one, so pointer arithmetics to get it becomes a bit simpler, but it needs to be checked by the tool and also paying attention to what is used in critical paths (so performance-wise). > + int first_index; > +}; ... > +static int macsmc_gpio_nr(smc_key key) > +{ > + int low = hex_to_bin(key & 0xff); > + int high = hex_to_bin((key >> 8) & 0xff); > + > + if (low < 0 || high < 0) > + return -1; > + > + return low | (high << 4); > +} NIH hex2bin(). > +static int macsmc_gpio_key(unsigned int offset) > +{ > + return _SMC_KEY("gP\0\0") | (hex_asc_hi(offset) << 8) | hex_asc_lo(offset); > +} NIH hex_byte_pack(). ... > + /* First try reading the explicit pin mode register */ > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val); > + if (!ret) > + return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; > + > + /* > + * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode. > + * Fall back to reading IRQ mode, which will only succeed for inputs. > + */ > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val); > + return (!ret) ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT; What is the meaning of val in this case? ... > + if (ret == GPIO_LINE_DIRECTION_OUT) > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_OUTPUT, &val); > + else > + ret = apple_smc_rw_u32(smcgp->smc, key, CMD_INPUT, &val); > + Unnecessary blank line. > + if (ret < 0) > + return ret; ... > + ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value); > + if (ret < 0) > + dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value); Strange specifier... It seems like a hashed pointer with added (or skipped? I don't remember) '4ch'. Perhaps you meant one of '%pE', '%p4cc'? Ditto for other cases. ... > + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); > + int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index; I would split this assignment and move it closer to the first user. > + int i; > + > + if (count > MAX_GPIO) > + count = MAX_GPIO; Hmm... When can it be the case? > + bitmap_zero(valid_mask, ngpios); > + > + for (i = 0; i < count; i++) { > + smc_key key; > + int gpio_nr; > + int ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key); Ditto. > + > + if (ret < 0) > + return ret; > + > + if (key > SMC_KEY(gPff)) > + break; > + > + gpio_nr = macsmc_gpio_nr(key); > + if (gpio_nr < 0 || gpio_nr > MAX_GPIO) { > + dev_err(smcgp->dev, "Bad GPIO key %p4ch\n", &key); Yeah, according to the code it will print something you didn't want. > + continue; > + } > + > + set_bit(gpio_nr, valid_mask); > + } > + > + return 0; > +} ... > + pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio"); Can we use fwnode APIs instead? Or do you really need this? -- 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