From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f181.google.com (mail-qt1-f181.google.com [209.85.160.181]) (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 71840185C for ; Fri, 2 Sep 2022 10:37:51 +0000 (UTC) Received: by mail-qt1-f181.google.com with SMTP id l5so1114459qtv.4 for ; Fri, 02 Sep 2022 03:37:51 -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=dhxY1XN9LqDd4p8jCsOrBrLU/6pXWataZOAm6mzXO/0=; b=i1ti6U87GQmvuzYoKKQtiUc9u7ziwtznBwkS0Ib/RwIbJM+TJ0Iy+/driqB5WPMz97 PjBOfjbmfxlKWQqcXD+CCHfievlDoLDkZR5AVkL/FUzdyT4oX9nOaxIfgiGJxJBd5k5v GVkF2VEGkBB8buU5hMISCa3oTeuZT9w60gacplfyytUvau38t0iWvNgbBl7Z3vSLbLTl ncfdIhTAYTi04F15JEBttyZ7bynhoo69CGRi+8qcD3fFRvdKhOs9IdTtQ+tFw5zh6+We y/k669r8oe0FK4lypWpxV+uSpLXWq8mrI/ETmGZeSgn9v/8HqDrZMTsEe7Ytz9hxdXGz om+A== 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=dhxY1XN9LqDd4p8jCsOrBrLU/6pXWataZOAm6mzXO/0=; b=nmxhDBuFa4Np8ixLaLujkRbmcgXQQszU2y89WGRZG0BOJlVzIEovUOzTGkgKdIwzjt Oh9jztehddRgTfA7cvY8n2Ht1CAG9GUn/MRQEGMYlMJdcDRY5mATDFMka/hXsSvorUx6 2cTeyFDuqMEYJhaN4/Ethfey8VdbbBbQrFauC7FCR6NWBHTYDFuJ+9TOf6o9c8f+LJp2 IGEgMUkSXbAnnxJiByP4gzrrDs+BnYQ/OzM4GDwTzwc4hiHS5qsKpQDDwU7dNi5L0uHh 6kbcXE+6x6uWxAd7WrOqMlVmL9JGZ9j/qaoSK5PDUK3lgCf/3+U7EWd4rXkGr6P0hdI8 atBg== X-Gm-Message-State: ACgBeo1g9gHJoPcsDxYAXdpoczdrgcQF4/fO9LIOkLwJZVC5G4FyKK/g 0TPCVbl7IKR46Jhxo2sk1RNbxOcfMFh5wHSdSwE= X-Google-Smtp-Source: AA6agR6DmZf5PoN+MCkvXtJaqlV67oH/Ez4z9v5Fo7VjrPQL57s7MFmEv1tM4k0nYROeGI7vu96Ftx9+29on6qxDNAQ= X-Received: by 2002:ac8:7f92:0:b0:344:8cd8:59a1 with SMTP id z18-20020ac87f92000000b003448cd859a1mr28231282qtj.384.1662115070372; Fri, 02 Sep 2022 03:37:50 -0700 (PDT) Precedence: bulk X-Mailing-List: asahi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: In-Reply-To: From: Andy Shevchenko Date: Fri, 2 Sep 2022 13:37:14 +0300 Message-ID: Subject: Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs To: "Russell King (Oracle)" 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 Content-Type: text/plain; charset="UTF-8" On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) wrote: > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: ... > > > +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(). > > Is using hex2bin really better? Yes. > static int macsmc_gpio_nr(smc_key key) > { > char k[2]; > u8 result; > int ret; > > k[0] = key; > k[1] = key >> 8; > > ret = hex2bin(&result, k, 2); > if (ret < 0) > return ret; > > return result; > } > > This looks to me like it consumes more CPU cycles - because we have to > write each "character" to the stack, then call a function, only to then > call the hex_to_bin() function. One can't just pass "key" into hex2bin > because that will bring with it endian issues. With one detail missed, why do you need all that if you can use byteorder helpers()? What's the stack? Just replace this entire function with the respectful calls to 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(). > > This would become: > > char buf[2]; > > hex_byte_pack(buf, offset); > > return _SMC_KEY("gP\0\0") | buf[0] << 8 | buf[1]; You have a point here. > to avoid the endian issues. It just seems to be a more complex way to > do the conversion. One could then argue that this is just a NIH > sprintf(), so it could then be written: No, no. snprintf() is too much here. > which looks nicer, but involves a lot more code. > > Since this is called for every GPIO operation, and you were worred above > about the layout of the macsmc_gpio structure (which is a micro- > optimisation), it seems weird to be concerned about the efficiency of > the structure layout and then suggest less efficient code in each of the > functional paths of the driver. There seems to be a contradiction. ... > > > + /* 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? > > Reading the comment, it seems that "val" is irrelevant. I'm not sure that > needs explaining given there's a comment that's already explaining what > is going on here. OK. Just convert then (!ret) --> ret. ... > > > + 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. > > I think that's personal style preference, it isn't mentioned in the coding > style. However, the following is much nicer and likely produces better > code: > > if (ret == GPIO_LINE_DIRECTION_OUT) > cmd = CMD_OUTPUT; > else > cmd = CMD_INPUT; > > ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val); > if (ret < 0) > return ret; Go for it! ... > > > + if (count > MAX_GPIO) > > > + count = MAX_GPIO; > > > > Hmm... When can it be the case? > > That's a question for the Asahi folk - it's not obvious whether it could > or could not be from the code. I think it depends on firmware. If it's the case, why does the code not support higher GPIOs? Needs at least a comment. ... > > > + 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. > > What does "ditto" here mean, because I don't think you mean "Hmm... > When can it be the case?" and "I would split this assignment and move > it closer to the first user." doesn't seem to be relevant either. Split int ret = foo(); to int ret; ret = foo(); ... > > > + 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? > > Ouch, that's not nice. I can change this to: (Some background on why my eye caught this. We as GPIO SIG in the kernel want to move the library to be fwnode one without looking into the underneath property provider. This kind of lines makes driver look a bit ugly from that perspective) > fwnode = device_get_named_child_node(pdev->dev.parent, "gpio"); > device_set_node(&pdev->dev, fwnode); > > but even that isn't _that_ nice. I'd like to hear comments from the Asahi > folk about whether these sub-blocks of the SMC can have compatibles, so > that the MFD layer can automatically fill in the firmware nodes on the > struct device before the probe function gets called. > If not, then I think it would be reasonable to have a discussion with > Lee about extending MFD to be able to have mfd cells name a child, so > that MFD can do the above instead of having it littered amongst drivers. MFD cells can be matched by compatible strings. -- With Best Regards, Andy Shevchenko 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 7B65BECAAD5 for ; Fri, 2 Sep 2022 10:39: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=qgA1m3m9GSh+hPgSY5IRv5T9seHYZXTrZZXTRMl2Z3g=; b=WQdzelTPjLwHbZ 5mxs93MlsnaF5wkrA+yr2E3iS+b4nvZ0jt0IOKMEV1PU9G5K+C8cr2FchQgDqo/TgB5p52t3aKPD8 a4jKNbnbu1H2F7crkHrbeSTczPTTOCV1HsnK8rj9unDOyw1YPsaXijx9c2vMx/Fut/6N/TJPFYHb7 jXEHIrFl3bxvjShw00GEnywIa7cZmB2h7jmbn+LlhRb3yksGCFTcnHAQg0d1i2lb4QKntaqR3whIk TcqZLBPgFWAqvKG1YO7cvSYxzjAQwuofH1D7WMIFFZKFjfzCoJZTPz0UoO383rcL8BAtxI2ibTzf6 2rBtTM+QYbFWg/+8bi/g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oU43g-003WNb-Kl; Fri, 02 Sep 2022 10:37: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 1oU43d-003WHi-PP for linux-arm-kernel@lists.infradead.org; Fri, 02 Sep 2022 10:37:55 +0000 Received: by mail-qt1-x82b.google.com with SMTP id a22so1092204qtw.10 for ; Fri, 02 Sep 2022 03:37:51 -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=dhxY1XN9LqDd4p8jCsOrBrLU/6pXWataZOAm6mzXO/0=; b=i1ti6U87GQmvuzYoKKQtiUc9u7ziwtznBwkS0Ib/RwIbJM+TJ0Iy+/driqB5WPMz97 PjBOfjbmfxlKWQqcXD+CCHfievlDoLDkZR5AVkL/FUzdyT4oX9nOaxIfgiGJxJBd5k5v GVkF2VEGkBB8buU5hMISCa3oTeuZT9w60gacplfyytUvau38t0iWvNgbBl7Z3vSLbLTl ncfdIhTAYTi04F15JEBttyZ7bynhoo69CGRi+8qcD3fFRvdKhOs9IdTtQ+tFw5zh6+We y/k669r8oe0FK4lypWpxV+uSpLXWq8mrI/ETmGZeSgn9v/8HqDrZMTsEe7Ytz9hxdXGz om+A== 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=dhxY1XN9LqDd4p8jCsOrBrLU/6pXWataZOAm6mzXO/0=; b=gZVtWSNiCwLBvBRCKUlbYMrhbvOGewwItML/DHRBpBoDivcJ+9BD5QA4jDlZTmwJ4B auQ7T5jBql2R4Yz9xDdgUDMe+T6NzYoR4EmUHS/9Hh3II4pMid4NfkpN9CMsJtRiVs/l pJAFurodvKOWv17Doh30GX3+uRFV+lIbDFU5eKMSlXVqUARmuOt2SVH5ZpZMZ1K27yMd db6wv3ErrvHHjzWcMHsxq6KJOTt7eqtET6sqYSq6kti2Qqrl0UA2E/wS2bZCSpcmjmLh nT9Obtz/4MouPttoqWD0TxSzlnhJJAGPren7j5Xbfxegxc2KBWFocKYQxVdmHNZwd8O1 cuBA== X-Gm-Message-State: ACgBeo24+T7Q1AWqkVfw9xlFBSWTOnQLjWdCg9SNCPMlUPJwuEgSa5v0 ayeHH8/abt7P/nAK2fpPSwD5mKxCEqoDM3R2Zu0= X-Google-Smtp-Source: AA6agR6DmZf5PoN+MCkvXtJaqlV67oH/Ez4z9v5Fo7VjrPQL57s7MFmEv1tM4k0nYROeGI7vu96Ftx9+29on6qxDNAQ= X-Received: by 2002:ac8:7f92:0:b0:344:8cd8:59a1 with SMTP id z18-20020ac87f92000000b003448cd859a1mr28231282qtj.384.1662115070372; Fri, 02 Sep 2022 03:37:50 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Andy Shevchenko Date: Fri, 2 Sep 2022 13:37:14 +0300 Message-ID: Subject: Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs To: "Russell King (Oracle)" 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-20220902_033753_872990_D425A94F X-CRM114-Status: GOOD ( 54.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: , 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 Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) wrote: > On Thu, Sep 01, 2022 at 09:55:23PM +0300, Andy Shevchenko wrote: ... > > > +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(). > > Is using hex2bin really better? Yes. > static int macsmc_gpio_nr(smc_key key) > { > char k[2]; > u8 result; > int ret; > > k[0] = key; > k[1] = key >> 8; > > ret = hex2bin(&result, k, 2); > if (ret < 0) > return ret; > > return result; > } > > This looks to me like it consumes more CPU cycles - because we have to > write each "character" to the stack, then call a function, only to then > call the hex_to_bin() function. One can't just pass "key" into hex2bin > because that will bring with it endian issues. With one detail missed, why do you need all that if you can use byteorder helpers()? What's the stack? Just replace this entire function with the respectful calls to 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(). > > This would become: > > char buf[2]; > > hex_byte_pack(buf, offset); > > return _SMC_KEY("gP\0\0") | buf[0] << 8 | buf[1]; You have a point here. > to avoid the endian issues. It just seems to be a more complex way to > do the conversion. One could then argue that this is just a NIH > sprintf(), so it could then be written: No, no. snprintf() is too much here. > which looks nicer, but involves a lot more code. > > Since this is called for every GPIO operation, and you were worred above > about the layout of the macsmc_gpio structure (which is a micro- > optimisation), it seems weird to be concerned about the efficiency of > the structure layout and then suggest less efficient code in each of the > functional paths of the driver. There seems to be a contradiction. ... > > > + /* 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? > > Reading the comment, it seems that "val" is irrelevant. I'm not sure that > needs explaining given there's a comment that's already explaining what > is going on here. OK. Just convert then (!ret) --> ret. ... > > > + 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. > > I think that's personal style preference, it isn't mentioned in the coding > style. However, the following is much nicer and likely produces better > code: > > if (ret == GPIO_LINE_DIRECTION_OUT) > cmd = CMD_OUTPUT; > else > cmd = CMD_INPUT; > > ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val); > if (ret < 0) > return ret; Go for it! ... > > > + if (count > MAX_GPIO) > > > + count = MAX_GPIO; > > > > Hmm... When can it be the case? > > That's a question for the Asahi folk - it's not obvious whether it could > or could not be from the code. I think it depends on firmware. If it's the case, why does the code not support higher GPIOs? Needs at least a comment. ... > > > + 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. > > What does "ditto" here mean, because I don't think you mean "Hmm... > When can it be the case?" and "I would split this assignment and move > it closer to the first user." doesn't seem to be relevant either. Split int ret = foo(); to int ret; ret = foo(); ... > > > + 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? > > Ouch, that's not nice. I can change this to: (Some background on why my eye caught this. We as GPIO SIG in the kernel want to move the library to be fwnode one without looking into the underneath property provider. This kind of lines makes driver look a bit ugly from that perspective) > fwnode = device_get_named_child_node(pdev->dev.parent, "gpio"); > device_set_node(&pdev->dev, fwnode); > > but even that isn't _that_ nice. I'd like to hear comments from the Asahi > folk about whether these sub-blocks of the SMC can have compatibles, so > that the MFD layer can automatically fill in the firmware nodes on the > struct device before the probe function gets called. > If not, then I think it would be reasonable to have a discussion with > Lee about extending MFD to be able to have mfd cells name a child, so > that MFD can do the above instead of having it littered amongst drivers. MFD cells can be matched by compatible strings. -- 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