| Message ID | E1oTkeb-003t9e-Iy@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Apple Mac System Management Controller GPIOs | expand |
On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > From: Hector Martin <marcan@marcan.st> > > 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
> On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: >> + 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. As was pointed out by Sven elsewhere in the thread, this is an unupstreamed specifier (that was missed as a dependency of this code). https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code Martin
On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > > >> + 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. > > As was pointed out by Sven elsewhere in the thread, this is an > unupstreamed specifier (that was missed as a dependency of this > code). > > https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code I don't see why we need that. The %.4s (0x%08x) is repeating that with the existing codebase. (I do understand why v4l2/drm have it). Ideally the first should use %4pE, but it might not be suitable in some cases.
On Thu, Sep 1, 2022 at 3:54 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: > From: Hector Martin <marcan@marcan.st> > > 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. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Overall this looks very good provided the SMC API is fine with the platform/MFD maintainers, I like the usage of .init_valid_mask which is used just as intended. Andy's detailed review points should be addressed reasonably after that it's: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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? 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. > > +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]; 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: char buf[5]; snprintf(buf, sizeof(buf), "gP%02x", offset); return _SMC_KEY(buf); 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. > > + 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; > > + 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? 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. > > + 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. > > + 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: 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. Thanks for the review.
On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) <linux@armlinux.org.uk> 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.
Pardon, I lost the CC list in my earlier reply. Adding it back now. > On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: >>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: >>>>> On 1. 9. 2022, at 20:55, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>> On Thu, Sep 1, 2022 at 5:17 PM Russell King <rmk+kernel@armlinux.org.uk> wrote: >>>> >>>>>> + 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. >>>> >>>> As was pointed out by Sven elsewhere in the thread, this is an >>>> unupstreamed specifier (that was missed as a dependency of this >>>> code). >>>> >>>> https://github.com/AsahiLinux/linux/blob/f8c0d18173a7b649999ee27515393f7aae40310c/Documentation/core-api/printk-formats.rst#generic-fourcc-code >>> >>> I don't see why we need that. The %.4s (0x%08x) is repeating that with >>> the existing codebase. (I do understand why v4l2/drm have it). Ideally >>> the first should use %4pE, but it might not be suitable in some cases. >> >> Just from a superficial understanding of things: %p4ch on little-endian >> will print in a reversed order to %.4s. As I see it the handling of >> endianness is the value proposition of the new specifiers. > > So, what prevents you from adding this to %pE? > The preferred way is not adding a specifier for a single user with a > particular case, esp. when it's covered by the existing ones. Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? If you think that would be accepted... I guess this was added on the assumption that keys like this will be a common occurrence in interaction with Apple firmware. Though greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the SMC code (9 times): ./drivers/power/reset/macsmc-reboot.c ./drivers/platform/apple/smc_core.c ./drivers/gpio/gpio-macsmc.c >> So >> >> %p4ch - interpret as an u32, print the character in most significant byte first > > %.4s + be32_to_cpu() Well, AIUI instead of printk(“%p4ch = ...\n”, &key); you need to do u32 key_be = cpu_to_be32(key); printk(“%.4s = ...\n”, &key_be); in at least 9 places now, the number of which will probably grow. Just to make the case for *some* printk helper. > >> %p4cb - the same as %.4s > >> %p4cl - reversed to %.4s > > %.4s + swab32() Sure, these two are uninteresting, probably added for completeness. > > So? Well, so nothing. I am primarily explaining how that strange specifier came to be. Martin > -- > With Best Regards, > Andy Shevchenko
On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > <linux@armlinux.org.uk> 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(). Sorry, I don't understand what you're suggesting, because it doesn't make sense to me. The byteorder helpers do not give a char array, which is what hex2bin() wants, so we end up with something like: __le16 foo = cpu_to_le16(key); u8 result; ret = hex2bin(&result, (char *)&foo, 1); if (ret < 0) return ret; return result; This to me looks like yucky code, It still results in "foo" having to be on the stack, because the out-of-line hex2bin() requires a pointer to be passed as the second argument. Maybe you could provide an example of what you're thinking of, because I'm at a loss to understand what you're thinking this should look like. > > > > + /* 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. Already done, thanks. > > > > + 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) I agree, I'd prefer it not to be there. > > 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. Yes, that's what I meant in my preceeding paragraph above, but it needs involvement and decisions from the Asahi maintainers.
On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: > > On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: > >>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: ... > >>> I don't see why we need that. The %.4s (0x%08x) is repeating that with > >>> the existing codebase. (I do understand why v4l2/drm have it). Ideally > >>> the first should use %4pE, but it might not be suitable in some cases. > >> > >> Just from a superficial understanding of things: %p4ch on little-endian > >> will print in a reversed order to %.4s. As I see it the handling of > >> endianness is the value proposition of the new specifiers. > > > > So, what prevents you from adding this to %pE? > > The preferred way is not adding a specifier for a single user with a > > particular case, esp. when it's covered by the existing ones. > > Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? > If you think that would be accepted... > > I guess this was added on the assumption that keys like this will > be a common occurrence in interaction with Apple firmware. Though > greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the > SMC code (9 times): > > ./drivers/power/reset/macsmc-reboot.c > ./drivers/platform/apple/smc_core.c > ./drivers/gpio/gpio-macsmc.c > >> %p4ch - interpret as an u32, print the character in most significant byte first > > > > %.4s + be32_to_cpu() > > Well, AIUI instead of > > printk(“%p4ch = ...\n”, &key); > > you need to do > > u32 key_be = cpu_to_be32(key); > printk(“%.4s = ...\n”, &key_be); > > in at least 9 places now, the number of which will probably grow. > Just to make the case for *some* printk helper. Wouldn't this be one line printk(“%.4s = ...\n”, &cpu_to_be32(key)); ? So I don't see disadvantages right now. Later on we can consider a new specifier _separately_, otherwise this series would be dragging for no sense.
On Fri, Sep 2, 2022 at 4:33 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: ... > > in at least 9 places now, the number of which will probably grow. > > Just to make the case for *some* printk helper. > > Wouldn't this be one line > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > ? > > So I don't see disadvantages right now. Later on we can consider a new > specifier _separately_, otherwise this series would be dragging for no > sense. Just to make my point clear. The single user or small subset of users does not justify the new specifier. New specifier is a lot of burden to printf() code. Find 3+ independent users and we can talk again. That's why I suggest to either create a local (to apple whatever stuff) helper or use existing specifiers in _this_ series.
> On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: >>> On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: >>>>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > ... > >>>>> I don't see why we need that. The %.4s (0x%08x) is repeating that with >>>>> the existing codebase. (I do understand why v4l2/drm have it). Ideally >>>>> the first should use %4pE, but it might not be suitable in some cases. >>>> >>>> Just from a superficial understanding of things: %p4ch on little-endian >>>> will print in a reversed order to %.4s. As I see it the handling of >>>> endianness is the value proposition of the new specifiers. >>> >>> So, what prevents you from adding this to %pE? >>> The preferred way is not adding a specifier for a single user with a >>> particular case, esp. when it's covered by the existing ones. >> >> Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? >> If you think that would be accepted... >> >> I guess this was added on the assumption that keys like this will >> be a common occurrence in interaction with Apple firmware. Though >> greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the >> SMC code (9 times): >> >> ./drivers/power/reset/macsmc-reboot.c >> ./drivers/platform/apple/smc_core.c >> ./drivers/gpio/gpio-macsmc.c > >>>> %p4ch - interpret as an u32, print the character in most significant byte first >>> >>> %.4s + be32_to_cpu() >> >> Well, AIUI instead of >> >> printk(“%p4ch = ...\n”, &key); >> >> you need to do >> >> u32 key_be = cpu_to_be32(key); >> printk(“%.4s = ...\n”, &key_be); >> >> in at least 9 places now, the number of which will probably grow. >> Just to make the case for *some* printk helper. > > Wouldn't this be one line > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > ? That would compile? I thought that’s not valid C, taking an address of function’s return value. > > So I don't see disadvantages right now. Later on we can consider a new > specifier _separately_, otherwise this series would be dragging for no > sense. Absolutely agreed on the latter point. Martin
On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > <linux@armlinux.org.uk> 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(). > > Sorry, I don't understand what you're suggesting, because it doesn't > make sense to me. The byteorder helpers do not give a char array, which > is what hex2bin() wants, so we end up with something like: > > __le16 foo = cpu_to_le16(key); > u8 result; > > ret = hex2bin(&result, (char *)&foo, 1); > if (ret < 0) > return ret; > > return result; > > This to me looks like yucky code, It still results in "foo" having to > be on the stack, because the out-of-line hex2bin() requires a pointer > to be passed as the second argument. > > Maybe you could provide an example of what you're thinking of, because > I'm at a loss to understand what you're thinking this should look like. So, let's look into the real callers to see, oh wait, it's a single caller! Why can't you simply do ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); if (ret < 0) return ret; in-place there?
On Fri, Sep 2, 2022 at 4:37 PM Martin Povišer <povik@cutebit.org> wrote: > > On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: ... > >> you need to do > >> > >> u32 key_be = cpu_to_be32(key); > >> printk(“%.4s = ...\n”, &key_be); > >> > >> in at least 9 places now, the number of which will probably grow. > >> Just to make the case for *some* printk helper. > > > > Wouldn't this be one line > > > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > > > ? > > That would compile? I thought that’s not valid C, taking an > address of function’s return value. Ah, you are right. My bad.
On Fri, Sep 02, 2022 at 03:37:27PM +0200, Martin Povišer wrote: > > > On 2. 9. 2022, at 15:33, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Sep 2, 2022 at 2:12 PM Martin Povišer <povik@cutebit.org> wrote: > >>> On 2. 9. 2022, at 12:23, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>> On Fri, Sep 2, 2022 at 12:47 PM Martin Povišer <povik@cutebit.org> wrote: > >>>>> On 2. 9. 2022, at 8:31, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >>>>> On Fri, Sep 2, 2022 at 12:52 AM Martin Povišer <povik@cutebit.org> wrote: > > > > ... > > > >>>>> I don't see why we need that. The %.4s (0x%08x) is repeating that with > >>>>> the existing codebase. (I do understand why v4l2/drm have it). Ideally > >>>>> the first should use %4pE, but it might not be suitable in some cases. > >>>> > >>>> Just from a superficial understanding of things: %p4ch on little-endian > >>>> will print in a reversed order to %.4s. As I see it the handling of > >>>> endianness is the value proposition of the new specifiers. > >>> > >>> So, what prevents you from adding this to %pE? > >>> The preferred way is not adding a specifier for a single user with a > >>> particular case, esp. when it's covered by the existing ones. > >> > >> Adding the endianness conversion into %pE as, ehm, an ‘escaping flag’? > >> If you think that would be accepted... > >> > >> I guess this was added on the assumption that keys like this will > >> be a common occurrence in interaction with Apple firmware. Though > >> greping the ‘asahi’ staging tree for ‘%p4ch’ I only see it in the > >> SMC code (9 times): > >> > >> ./drivers/power/reset/macsmc-reboot.c > >> ./drivers/platform/apple/smc_core.c > >> ./drivers/gpio/gpio-macsmc.c > > > >>>> %p4ch - interpret as an u32, print the character in most significant byte first > >>> > >>> %.4s + be32_to_cpu() > >> > >> Well, AIUI instead of > >> > >> printk(“%p4ch = ...\n”, &key); > >> > >> you need to do > >> > >> u32 key_be = cpu_to_be32(key); > >> printk(“%.4s = ...\n”, &key_be); > >> > >> in at least 9 places now, the number of which will probably grow. > >> Just to make the case for *some* printk helper. > > > > Wouldn't this be one line > > > > printk(“%.4s = ...\n”, &cpu_to_be32(key)); > > > > ? > > That would compile? I thought that’s not valid C, taking an > address of function’s return value. It isn't legal C. int foo(int bar); int blah(int *v); int test(int v) { return blah(&foo(v)); } t.c: In function ‘test’: t.c:7:14: error: lvalue required as unary ‘&’ operand And just to make sure that it's not just my test that is wrong, and there's something magical about cpu_to_be32()... In file included from include/linux/device.h:15, from drivers/gpio/gpio-macsmc.c:11: drivers/gpio/gpio-macsmc.c: In function 'macsmc_gpio_probe': drivers/gpio/gpio-macsmc.c:356:49: error: lvalue required as unary '&' operand 356 | dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key)); | ^ include/linux/dev_printk.h:110:23: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ drivers/gpio/gpio-macsmc.c:356:2: note: in expansion of macro 'dev_info' 356 | dev_info(smcgp->dev, "First GPIO key: %.4s\n", &cpu_to_be32(key)); | ^~~~~~~~ make[3]: *** [scripts/Makefile.build:249: drivers/gpio/gpio-macsmc.o] Error 1 make[2]: *** [scripts/Makefile.build:466: drivers/gpio] Error 2 make[1]: *** [Makefile:1843: drivers] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:219: __sub-make] Error 2 So, sorry Andy, but this suggestion does not appear to be legal C. This also applies to your suggestion in the other sub-thread of: ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); As we've now discovered that this is not legal C, can we back up *both* discussions and start again on these points.
On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> 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(). > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > make sense to me. The byteorder helpers do not give a char array, which > > is what hex2bin() wants, so we end up with something like: > > > > __le16 foo = cpu_to_le16(key); > > u8 result; > > > > ret = hex2bin(&result, (char *)&foo, 1); > > if (ret < 0) > > return ret; > > > > return result; > > > > This to me looks like yucky code, It still results in "foo" having to > > be on the stack, because the out-of-line hex2bin() requires a pointer > > to be passed as the second argument. > > > > Maybe you could provide an example of what you're thinking of, because > > I'm at a loss to understand what you're thinking this should look like. > > So, let's look into the real callers to see, oh wait, it's a single caller! > Why can't you simply do > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > if (ret < 0) > return ret; > > in-place there? This is not legal C. Please can we back up this discussion, and start over with legal C suggestions. Thanks.
On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > <linux@armlinux.org.uk> 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(). > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > make sense to me. The byteorder helpers do not give a char array, which > > > is what hex2bin() wants, so we end up with something like: > > > > > > __le16 foo = cpu_to_le16(key); > > > u8 result; > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > if (ret < 0) > > > return ret; > > > > > > return result; > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > to be passed as the second argument. > > > > > > Maybe you could provide an example of what you're thinking of, because > > > I'm at a loss to understand what you're thinking this should look like. > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > Why can't you simply do > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > if (ret < 0) > > return ret; > > > > in-place there? > > This is not legal C. I acknowledged this, sorry. > Please can we back up this discussion, and start > over with legal C suggestions. Thanks. Suggestion was given as well, let's create a helper used by apple stuff and later on we will consider the separate submission for the (new) specifier. Would it work for you?
On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > > <linux@armlinux.org.uk> 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(). > > > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > > make sense to me. The byteorder helpers do not give a char array, which > > > > is what hex2bin() wants, so we end up with something like: > > > > > > > > __le16 foo = cpu_to_le16(key); > > > > u8 result; > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > if (ret < 0) > > > > return ret; > > > > > > > > return result; > > > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > > to be passed as the second argument. > > > > > > > > Maybe you could provide an example of what you're thinking of, because > > > > I'm at a loss to understand what you're thinking this should look like. > > > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > > Why can't you simply do > > > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > > if (ret < 0) > > > return ret; > > > > > > in-place there? > > > > This is not legal C. > > I acknowledged this, sorry. > > > Please can we back up this discussion, and start > > over with legal C suggestions. Thanks. > > Suggestion was given as well, let's create a helper used by apple > stuff and later on we will consider the separate submission for the > (new) specifier. Would it work for you? This sub-thread isn't about the %p4ch specifier. It's about a reasonable implementation of macsmc_gpio_nr(). Extracting from the context above, the original code was: 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); } I suggested: 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; } You didn't like that, so I then suggested: static int macsmc_gpio_nr(smc_key key) { __le16 foo = cpu_to_le16(key); u8 result; int ret; ret = hex2bin(&result, (char *)&foo, 1); if (ret < 0) return ret; return result; } which you also didn't like, and then you suggested something that isn't legal C. So, I then asked you to backup this discussion... As I've made a number of suggestions, and you've essentially rejected them all, I still need to know what you would find acceptable for this, because I'm out of ideas. (I haven't bothered to check whether my last suggestion even works - I am hoping to find out what general style of code you would accept here.
On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > > > <linux@armlinux.org.uk> wrote: > > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > > > <linux@armlinux.org.uk> 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(). > > > > > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > > > make sense to me. The byteorder helpers do not give a char array, which > > > > > is what hex2bin() wants, so we end up with something like: > > > > > > > > > > __le16 foo = cpu_to_le16(key); > > > > > u8 result; > > > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > return result; > > > > > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > > > to be passed as the second argument. > > > > > > > > > > Maybe you could provide an example of what you're thinking of, because > > > > > I'm at a loss to understand what you're thinking this should look like. > > > > > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > > > Why can't you simply do > > > > > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > > > if (ret < 0) > > > > return ret; > > > > > > > > in-place there? > > > > > > This is not legal C. > > > > I acknowledged this, sorry. > > > > > Please can we back up this discussion, and start > > > over with legal C suggestions. Thanks. > > > > Suggestion was given as well, let's create a helper used by apple > > stuff and later on we will consider the separate submission for the > > (new) specifier. Would it work for you? > > This sub-thread isn't about the %p4ch specifier. It's about a > reasonable implementation of macsmc_gpio_nr(). > > Extracting from the context above, the original code was: > > 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); > } > > I suggested: > > 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; > } > > You didn't like that, so I then suggested: > > static int macsmc_gpio_nr(smc_key key) > { > __le16 foo = cpu_to_le16(key); > u8 result; > int ret; > > ret = hex2bin(&result, (char *)&foo, 1); > if (ret < 0) > return ret; > > return result; > } > > which you also didn't like, ...based on the wrong suggestion below. That said, the above is fine to me. > and then you suggested something that isn't > legal C. So, I then asked you to backup this discussion... > > As I've made a number of suggestions, and you've essentially rejected > them all, I still need to know what you would find acceptable for this, > because I'm out of ideas.
On Fri, Sep 02, 2022 at 06:43:36PM +0300, Andy Shevchenko wrote: > On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 5:46 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > On Fri, Sep 02, 2022 at 04:39:16PM +0300, Andy Shevchenko wrote: > > > > > On Fri, Sep 2, 2022 at 2:33 PM Russell King (Oracle) > > > > > <linux@armlinux.org.uk> wrote: > > > > > > On Fri, Sep 02, 2022 at 01:37:14PM +0300, Andy Shevchenko wrote: > > > > > > > On Fri, Sep 2, 2022 at 1:05 PM Russell King (Oracle) > > > > > > > <linux@armlinux.org.uk> 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(). > > > > > > > > > > > > Sorry, I don't understand what you're suggesting, because it doesn't > > > > > > make sense to me. The byteorder helpers do not give a char array, which > > > > > > is what hex2bin() wants, so we end up with something like: > > > > > > > > > > > > __le16 foo = cpu_to_le16(key); > > > > > > u8 result; > > > > > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > > > > > > > return result; > > > > > > > > > > > > This to me looks like yucky code, It still results in "foo" having to > > > > > > be on the stack, because the out-of-line hex2bin() requires a pointer > > > > > > to be passed as the second argument. > > > > > > > > > > > > Maybe you could provide an example of what you're thinking of, because > > > > > > I'm at a loss to understand what you're thinking this should look like. > > > > > > > > > > So, let's look into the real callers to see, oh wait, it's a single caller! > > > > > Why can't you simply do > > > > > > > > > > ret = hex2bin(&result, (char *)&cpu_to_le16(key), 1); > > > > > if (ret < 0) > > > > > return ret; > > > > > > > > > > in-place there? > > > > > > > > This is not legal C. > > > > > > I acknowledged this, sorry. > > > > > > > Please can we back up this discussion, and start > > > > over with legal C suggestions. Thanks. > > > > > > Suggestion was given as well, let's create a helper used by apple > > > stuff and later on we will consider the separate submission for the > > > (new) specifier. Would it work for you? > > > > This sub-thread isn't about the %p4ch specifier. It's about a > > reasonable implementation of macsmc_gpio_nr(). > > > > Extracting from the context above, the original code was: > > > > 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); > > } > > > > I suggested: > > > > 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; > > } > > > > You didn't like that, so I then suggested: > > > > static int macsmc_gpio_nr(smc_key key) > > { > > __le16 foo = cpu_to_le16(key); > > u8 result; > > int ret; > > > > ret = hex2bin(&result, (char *)&foo, 1); > > if (ret < 0) > > return ret; > > > > return result; > > } > > > > which you also didn't like, > > ...based on the wrong suggestion below. That said, the above is fine to me. To be honest, using the endian conversion macro there doesn't feel right and is more prone to programming errors. I can't tell just by looking at it that either cpu_to_le16() or cpu_to_le32() would be the right thing here - and if it's not obvious then it's a bug waiting to happen. As if to prove the point, the above suggestions turn out to *all* be buggy. The initial suggestion gets the k[0] and k[1] assignment round the wrong way. The second, le16() is definitely not the right conversion. If we start using the endian conversion macros, then this is going to screw up if someone runs a BE kernel against the SMC (since the _SMC_KEY() macro will still be doing its conversion.) This seems utterly counter-productive, and I've spent quite a long time trying to work out what would be correct. At this point, I'm not sure that changing what has already been established in the Asahi Linux tree for something entirely different in mainline is going to be practical - it's a recipe for repeated mistakes converting keys from the Asahi kernel to the mainline kernel. It's not _just_ the GPIO driver. There are multiple other drivers that will be impacted by changing the scheme here. Any change to the scheme for these SMC keys needs to happen in the Asahi kernel tree by the Asahi Linux maintainers, not by someone pushing the code upstream - doing so would be a recipe for repeated trainwrecks. So, I'm going with my first suggestion for the hex2bin() conversion above, and adding a comment thusly: /* * The most significant nibble comes from k[0] and key bits 15..8 * The least significant nibble comes from k[1] and key bits 7..0 */ k[0] = key >> 8; k[1] = key; because I needed the comment to prove to myself that I wasn't breaking this code. Maybe it's obvious to you, but it isn't obvious to everyone.
On Mon, Sep 5, 2022 at 1:20 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Fri, Sep 02, 2022 at 06:43:36PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: ... > > > static int macsmc_gpio_nr(smc_key key) > > > { > > > __le16 foo = cpu_to_le16(key); > > > u8 result; > > > int ret; > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > if (ret < 0) > > > return ret; > > > > > > return result; > > > } > > > > > > which you also didn't like, > > > > ...based on the wrong suggestion below. That said, the above is fine to me. > > To be honest, using the endian conversion macro there doesn't feel > right and is more prone to programming errors. I can't tell just by > looking at it that either cpu_to_le16() or cpu_to_le32() would be the > right thing here - and if it's not obvious then it's a bug waiting to > happen. > > As if to prove the point, the above suggestions turn out to *all* be > buggy. > > The initial suggestion gets the k[0] and k[1] assignment round the > wrong way. The second, le16() is definitely not the right conversion. > If we start using the endian conversion macros, then this is going to > screw up if someone runs a BE kernel against the SMC (since the > _SMC_KEY() macro will still be doing its conversion.) > > This seems utterly counter-productive, and I've spent quite a long > time trying to work out what would be correct. > > At this point, I'm not sure that changing what has already been > established in the Asahi Linux tree for something entirely different > in mainline is going to be practical - it's a recipe for repeated > mistakes converting keys from the Asahi kernel to the mainline > kernel. > > It's not _just_ the GPIO driver. There are multiple other drivers > that will be impacted by changing the scheme here. > > Any change to the scheme for these SMC keys needs to happen in the > Asahi kernel tree by the Asahi Linux maintainers, not by someone > pushing the code upstream - doing so would be a recipe for repeated > trainwrecks. > > So, I'm going with my first suggestion for the hex2bin() conversion > above, and adding a comment thusly: > > /* > * The most significant nibble comes from k[0] and key bits 15..8 > * The least significant nibble comes from k[1] and key bits 7..0 > */ > k[0] = key >> 8; > k[1] = key; > > because I needed the comment to prove to myself that I wasn't breaking > this code. Maybe it's obvious to you, but it isn't obvious to everyone. And how is it different to the key being __be16 and all operations against it be correct with the endianness helpers? Adding redundant comments when the bitwise type exists seems just like being afraid of the unknown. Ah, I see that in one of your long letters the proposal somehow switched from (implicit) be16 to (explicit) le16... Still to me it's not enough justification for the comment, but since it has no effect on the code generation, add it if you think it would be better.
On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 1:20 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Fri, Sep 02, 2022 at 06:43:36PM +0300, Andy Shevchenko wrote: > > > On Fri, Sep 2, 2022 at 6:34 PM Russell King (Oracle) > > > <linux@armlinux.org.uk> wrote: > > > > On Fri, Sep 02, 2022 at 05:53:25PM +0300, Andy Shevchenko wrote: > > ... > > > > > static int macsmc_gpio_nr(smc_key key) > > > > { > > > > __le16 foo = cpu_to_le16(key); > > > > u8 result; > > > > int ret; > > > > > > > > ret = hex2bin(&result, (char *)&foo, 1); > > > > if (ret < 0) > > > > return ret; > > > > > > > > return result; > > > > } > > > > > > > > which you also didn't like, > > > > > > ...based on the wrong suggestion below. That said, the above is fine to me. > > > > To be honest, using the endian conversion macro there doesn't feel > > right and is more prone to programming errors. I can't tell just by > > looking at it that either cpu_to_le16() or cpu_to_le32() would be the > > right thing here - and if it's not obvious then it's a bug waiting to > > happen. > > > > As if to prove the point, the above suggestions turn out to *all* be > > buggy. > > > > The initial suggestion gets the k[0] and k[1] assignment round the > > wrong way. The second, le16() is definitely not the right conversion. > > If we start using the endian conversion macros, then this is going to > > screw up if someone runs a BE kernel against the SMC (since the > > _SMC_KEY() macro will still be doing its conversion.) > > > > This seems utterly counter-productive, and I've spent quite a long > > time trying to work out what would be correct. > > > > At this point, I'm not sure that changing what has already been > > established in the Asahi Linux tree for something entirely different > > in mainline is going to be practical - it's a recipe for repeated > > mistakes converting keys from the Asahi kernel to the mainline > > kernel. > > > > It's not _just_ the GPIO driver. There are multiple other drivers > > that will be impacted by changing the scheme here. > > > > Any change to the scheme for these SMC keys needs to happen in the > > Asahi kernel tree by the Asahi Linux maintainers, not by someone > > pushing the code upstream - doing so would be a recipe for repeated > > trainwrecks. > > > > So, I'm going with my first suggestion for the hex2bin() conversion > > above, and adding a comment thusly: > > > > /* > > * The most significant nibble comes from k[0] and key bits 15..8 > > * The least significant nibble comes from k[1] and key bits 7..0 > > */ > > k[0] = key >> 8; > > k[1] = key; > > > > because I needed the comment to prove to myself that I wasn't breaking > > this code. Maybe it's obvious to you, but it isn't obvious to everyone. > > And how is it different to the key being __be16 and all operations > against it be correct with the endianness helpers? First, the key is not 16-bit, it's 32-bit. Secondly, the "key" returned from the SMC is always swab()'d before we use it - and before we pass it back to the SMC. There's a big open question right now about whether it's the Asahi developers choice to arrange the four character key in big-endian form on LE platforms, and whether this is application processor endian dependent or not. It's packed into a 64-bit integer: msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) | FIELD_PREP(SMC_SIZE, size) | FIELD_PREP(SMC_ID, smc->msg_id) | FIELD_PREP(SMC_DATA, key)); in bits 32..63, which is then written using writeq_relaxed() to the mailbox registers. However, the keys returned from the SMC are the opposite endian-ness: static int apple_smc_rtkit_get_key_by_index(void *cookie, int index, smc_key *key) { struct apple_smc_rtkit *smc = cookie; int ret; ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_BY_INDEX, index, 0, 0, key); *key = swab32(*key); return ret; } where apple_smc_cmd() does this to get the returned data: if (ret_data) *ret_data = FIELD_GET(SMC_DATA, smc->cmd_ret); which comes from apple_smc_rtkit_recv_early(..., u64 message): smc->cmd_ret = message; which comes from apple_rtkit_rx(): if (ep >= APPLE_RTKIT_APP_ENDPOINT_START && rtk->ops->recv_message_early && rtk->ops->recv_message_early(rtk->cookie, ep, msg->msg0)) return; which ultimately comes from apple_mbox_hw_recv(); msg->msg0 = readq_relaxed(apple_mbox->regs + apple_mbox->hw->i2a_recv0); So what _is_ the right endian for the keys? I've no idea. > Adding redundant > comments when the bitwise type exists seems just like being afraid of > the unknown. Ah, I see that in one of your long letters the proposal > somehow switched from (implicit) be16 to (explicit) le16... Still to > me it's not enough justification for the comment, but since it has no > effect on the code generation, add it if you think it would be better. If you have a clear picture what this should be throughout multiple drivers (it seems you do) then please explain it to me, and make proposals to the Asahi Linux people how the SMC key stuff can be more understandable, because quite honestly, I don't think I'm qualified to touch what they have without introducing a shit-load of bugs. And in that circumstance, simple obviously correct code is better than a pile of steaming crap that no longer works. Even worse is when there's a fundamental incompatibility between what we have in mainline and what Asahi folk are using. Even worse is if this breaks on other Apple application-processor architectures. Let me say again: I am not changing this. That's for Asahi people to do if they wish. I am the just middle-man here. And yes, we need to do something about the %p4ch stuff. I have _no_ idea how to properly solve that right now - and how to properly solve that depends on what comes out of the discussion about what endianness this "smc_key" thing should be. Unless I get some input from the Asahi folk, I won't be posting a v2, because I can't address stuff like %p4ch without that. And I'm not going to mess about with endian conversions in silly places when it's not obvious that it's the right thing to be doing.
On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: ... > Let me say again: I am not changing this. That's for Asahi people to > do if they wish. I am the just middle-man here. While I agree on technical aspects, this mythical "they" is frustrating me. They haven't participated in this discussion (yet?) so they do not care, why should we (as a community of upstream)? P.S. Do you have a platform to test all these?
On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: > > ... > > > Let me say again: I am not changing this. That's for Asahi people to > > do if they wish. I am the just middle-man here. > > While I agree on technical aspects, this mythical "they" is > frustrating me. They haven't participated in this discussion (yet?) so > they do not care, why should we (as a community of upstream)? That's strange. I wonder. If they don't exist, then I wonder how Linus is running Linux on aarch64 apple hardware. Maybe it's not me with a problem here? Hector has been promising to get involved in this discussion for a few days now, his latest comment on IRC yesterday: 16:23 <@marcan> I'm going to allocate tuesday to playing the merge game (rmk: haven't forgotten about you either, IRQs today, but I'll get to it before tuesday): So he is aware that he needs to respond - but like any central project lead developer, he's probably exceedingly busy with other issues. > P.S. Do you have a platform to test all these? Yes, but that doesn't mean I can do testing sufficient to ensure that the modifications are correct. As I understand things, the SMC is not limited to just aarch64 hardware. It doesn't mean that "if it boots it must be okay" is sufficient. So, how about you stop insisting on changes until Hector can respond to some of the questions raised; as I've said many times, you are asking for stuff to be changed that is quite clearly in the realm of decisions that the Asahi developer(s) have taken, and I have no right to change them without reference to them - because I do not know this platform well enough to make the decisions you're asking of me. I'm not going to say that again; I'm going to start ignoring you if you persist in demanding that I make these kinds of decisions, because *you* leave me no other option but to do that... because *you* just don't seem to be willing to accept that I need others to be involved in these decisions.
On Mon, Sep 05, 2022 at 03:01:09PM +0100, Russell King (Oracle) wrote: > On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: > > > > ... > > > > > Let me say again: I am not changing this. That's for Asahi people to > > > do if they wish. I am the just middle-man here. > > > > While I agree on technical aspects, this mythical "they" is > > frustrating me. They haven't participated in this discussion (yet?) so > > they do not care, why should we (as a community of upstream)? > > That's strange. I wonder. If they don't exist, then I wonder how Linus > is running Linux on aarch64 apple hardware. Maybe it's not me with a > problem here? > > Hector has been promising to get involved in this discussion for a few > days now, his latest comment on IRC yesterday: > > 16:23 <@marcan> I'm going to allocate tuesday to playing the merge game (rmk: > haven't forgotten about you either, IRQs today, but I'll get to > it before tuesday): > > So he is aware that he needs to respond - but like any central project > lead developer, he's probably exceedingly busy with other issues. > > > P.S. Do you have a platform to test all these? > > Yes, but that doesn't mean I can do testing sufficient to ensure that > the modifications are correct. As I understand things, the SMC is not > limited to just aarch64 hardware. > > It doesn't mean that "if it boots it must be okay" is sufficient. > > So, how about you stop insisting on changes until Hector can respond > to some of the questions raised; as I've said many times, you are > asking for stuff to be changed that is quite clearly in the realm of > decisions that the Asahi developer(s) have taken, and I have no right > to change them without reference to them - because I do not know this > platform well enough to make the decisions you're asking of me. > > I'm not going to say that again; I'm going to start ignoring you if > you persist in demanding that I make these kinds of decisions, because > *you* leave me no other option but to do that... because *you* just > don't seem to be willing to accept that I need others to be involved > in these decisions. Oh, and another thing. Your behaviour on this is making me regret trying to get involved in improving the upstream support for this platform.
On Mon, Sep 5, 2022 at 5:02 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Mon, Sep 05, 2022 at 03:01:09PM +0100, Russell King (Oracle) wrote: ... > Oh, and another thing. Your behaviour on this is making me regret > trying to get involved in improving the upstream support for this > platform. Taking into account that technical aspects quite likely are not the cause, what did you expect to be different?
On Mon, Sep 5, 2022 at 5:01 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) > > <linux@armlinux.org.uk> wrote: > > > On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: ... > > > Let me say again: I am not changing this. That's for Asahi people to > > > do if they wish. I am the just middle-man here. > > > > While I agree on technical aspects, this mythical "they" is > > frustrating me. They haven't participated in this discussion (yet?) so > > they do not care, why should we (as a community of upstream)? > > That's strange. I wonder. If they don't exist, then I wonder how Linus > is running Linux on aarch64 apple hardware. Maybe it's not me with a > problem here? You mentioned them several times in the thread and no response from them. > Hector has been promising to get involved in this discussion for a few > days now, his latest comment on IRC yesterday: IRC is not an email discussion and if he is not alone in the team, who else can answer the queries? But it's good that there is an interest from their side. > 16:23 <@marcan> I'm going to allocate tuesday to playing the merge game (rmk: > haven't forgotten about you either, IRQs today, but I'll get to > it before tuesday): > > So he is aware that he needs to respond - but like any central project > lead developer, he's probably exceedingly busy with other issues. > So, how about you stop insisting on changes until Hector can respond > to some of the questions raised; as I've said many times, you are > asking for stuff to be changed that is quite clearly in the realm of > decisions that the Asahi developer(s) have taken, and I have no right > to change them without reference to them - because I do not know this > platform well enough to make the decisions you're asking of me. > > I'm not going to say that again; I'm going to start ignoring you if > you persist in demanding that I make these kinds of decisions, because > *you* leave me no other option but to do that... because *you* just > don't seem to be willing to accept that I need others to be involved > in these decisions. If I spoke like this for all contributions, from our side to the upstream, I would become a crazy guy, because upstream almost always insists on some changes here and there in the patches which are longer than a couple of dozens of LoCs and more than a single in a series. It's normal practice to follow the latest available APIs in the kernel. Nevertheless, I told *you*, that technically I have no more issues. Let's wait for the technical response from them.
On Mon, Sep 05, 2022 at 05:42:46PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 5:02 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Mon, Sep 05, 2022 at 03:01:09PM +0100, Russell King (Oracle) wrote: > > ... > > > Oh, and another thing. Your behaviour on this is making me regret > > trying to get involved in improving the upstream support for this > > platform. > > Taking into account that technical aspects quite likely are not the > cause, what did you expect to be different? How about the decency to wait for the Asahi developers to respond, which they now have - but I haven't read it yet. How about some understanding that I might not have all the answers. How about some understanding that the issue with the SMC Keys is bigger than just the one driver, and that cpu_to_whatever may not be appropriate. How about some understanding and patience on your part?
On 02/09/2022 03.55, Andy Shevchenko wrote: >> +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). This is a GPIO chip accessed via a remote CPU. Saving two cycles on pointer arithmetic is the very definition of premature optimization. > >> + 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(). No. hex2bin() works on string buffers. This works on an integer containing packed characters. They are not the same, and do not have the same semantics endian-wise. Integer represent numbers abstractly, byte buffers represent bytes in memory in sequence. >> +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(). Same comment as above. >> + /* 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? Have you tried reading the comment above the code? When I write code doing something unintuitive and put a comment on top, I expect reviewers to *read* it. If you're not going to do that, I might as well stop writing comments. > 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. As mentioned in the other thread, there was a missed dependency that added this specifier. > >> + 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. It is one line away from the first user. > >> + int i; >> + >> + if (count > MAX_GPIO) >> + count = MAX_GPIO; > > Hmm... When can it be the case? Let's find out! Two lines above: + int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index; So I get the toal SMC key count, which is probably 1000 or so, then subtract the index of the first GPIO key, to get an upper bound on the last GPIO key just to make sure we don't run off the end of the key list. In other words, pretty much always. But you didn't read two lines prior, did you. > >> + 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. This is zero lines away from the first user. > >> + >> + 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. What? >> + 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? This is a producer, not a consumer. It needs to set the of_node so there is something for consumers to target in the device tree. The consumers may well use fwnode APIs. - Hector
On 02/09/2022 19.05, Russell King (Oracle) wrote: > 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. I'm fine with adding compatibles if this makes the of/fwnode handling simpler. However, keep in mind these aren't hardware, they're effectively software services implemented in a coprocessor. The idea behind not having compatibles was that the SMC stuff should be self-discovering enough that it can decide what's available and not at runtime, and that the SMC core needs to probe before the child devices anyway, so it's not like this can be a simple-bus. But I'm new to the MFD subsystem, so if compatibles make life easier, sure. Personally, I'd defer to Rob's opinon on this (CC'ed), since he has the last word on DT bindings :). - Hector
On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: > > ... > > > Let me say again: I am not changing this. That's for Asahi people to > > do if they wish. I am the just middle-man here. > > While I agree on technical aspects, this mythical "they" is > frustrating me. They haven't participated in this discussion (yet?) so > they do not care, why should we (as a community of upstream)? > > P.S. Do you have a platform to test all these? Right, having addressed as many review comments as I possibly can, I've rebuilt and booted it on the platform - and the good news is, it still works _but_ I don't have enough support in the "mainline" kernel that I'm testing for anything to make use of any of the interrupt support in this patch set - so I can't actually test those changes. I'm going to drop the interrupt patch temporarily as it's unnecessary for what I want to do, which will be one less patch to worry about. I still need a resolution between you and Hector over the smc_key issue - specifically, do I pick up the patch that adds support for %p4ch, or do we re-architect the smc_key thing and also in doing so get rid of the need for your "endian conversion" thing. Given that Hector has rejected some of your comments, I now need to back out those changes that resulted from your NIH comments.
On 05/09/2022 19.20, Russell King (Oracle) wrote: > So, I'm going with my first suggestion for the hex2bin() conversion > above, and adding a comment thusly: > > /* > * The most significant nibble comes from k[0] and key bits 15..8 > * The least significant nibble comes from k[1] and key bits 7..0 > */ > k[0] = key >> 8; > k[1] = key; > > because I needed the comment to prove to myself that I wasn't breaking > this code. Maybe it's obvious to you, but it isn't obvious to everyone. Honestly, I don't see what this buys us over the original code. It's longer, no more readable, makes you think about the order that characters are stored in the string as an extra indirection step, which as as you found out with having to write the comment, is easy to get backwards. But I will say it is at least *semantically* correct, if awkward. Let's back up and talk a bit about SMC keys for a second. First, SMC is a legacy mess and plays around with endianness wrong in several places - there are values which are in wrong-endian for no reason, etc. So any discussion over "what would happen on a big-endian platform" is ultimately speculation. If this ever ends up running on some ancient PowerPC Mac (did any of those ever ship with an SMC that followed these semantics?) then we'll have to deal with the endianness issues then and correct any incorrect assumptions, because right now we just don't have the information on what Apple's *intent* was when designing this whole thing, if there was an intent at all. That said. When I designed this driver, and the way I understand the hardware, I consider SMC keys to be 32-bit integers containing packed ASCII characters in natural integer order, that is, 0xAABBCCDD representing the fourcc ABCD in that order. The SMC backend is designed with this in mind, and puts things in the right endian in the right contexts when it comes to the actual interface with the SMC coprocessor (which is, itself, a mix of shared memory - which is a bag of bytes - and 64-bit mailbox messages - which are fundamentally integers and merely represented in little-endian at the hardware level - so I'm sure how you can see how this gets interesting). In other words, at the driver level, *SMC keys are not character strings, nor integers stored in some byte order*. They are integers. Integers do not have a byte order until they are stored to memory. Therefore, using functions that operate on strings on SMC keys is wrong, and requires you to make a trip through endian-land to get it right (as you found out). Making the representation of SMC keys in the driver 32-bit integers makes manipulating them easier and ergonomic in C, and allows for things like comparisons (look at how the GPIO code uses < to compare SMC keys, which maps to ASCIIbetical sort the way the keys are naturally encoded), while basically relegating all the endian issues to the SMC core. For comparison, if the data structure were a char[4] in reading order, there would be no ergonomic way to do comparisons without some helper function/macro. And comparisons are used quite a bit as part of the self-discovery aspects of SMC (there's that binary search function to find key indices, which also took like 4 tries to get right... please don't break it! :). This is why I added a printk specifier, because V4L/etc already had a very special-purpose specifier with fancy rules just for them, and I think a generic FOURCC style format specifier that works in any context is useful (this isn't the only driver dealing with this kind of FOURCC-style construct). The printk patch in particular adds 4 variations to the existing v4l specifier that that interpret endianness differently, so it can be used in any context (in this context, the specifier is 'h' which means 'host endian' and is the correct specifier for abstract integers, which are passed by reference in this case and therefore inherit the host endianness). - Hector
> On 5. 9. 2022, at 17:32, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > I still need a resolution between you and Hector over the smc_key > issue - specifically, do I pick up the patch that adds support for > %p4ch, or do we re-architect the smc_key thing and also in doing so > get rid of the need for your "endian conversion" thing. Idea about the %p4ch thing: We will leave the keys true to their nature (i.e. 32-bit integer), and at least initially for the prints we will employ macros #define SMC_KEYFMT “%c%c%c%c” #define SMC_KEYFMT_VAL(val) (val)>>24,(val)>>16,(val)>>8,(val) used like printk(“blah blah” SMC_KEYFMT “ blah\n”, SMC_KEYFMT_VAL(key)); This has the nice property that it is pretty much like the specifier, and later can be easily replaced with the real thing. -- Martin
On 05/09/2022 22.16, Andy Shevchenko wrote: > On Mon, Sep 5, 2022 at 4:10 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: >> On Mon, Sep 05, 2022 at 01:32:29PM +0300, Andy Shevchenko wrote: > > ... > >> Let me say again: I am not changing this. That's for Asahi people to >> do if they wish. I am the just middle-man here. > > While I agree on technical aspects, this mythical "they" is > frustrating me. They haven't participated in this discussion (yet?) so > they do not care, why should we (as a community of upstream)? Hi Andy, This was submitted 4 days before your comment, 2 of which were the weekend. Not all of us spend our weekends reviewing patches. Russell graciously volunteered to help upstream this patchset and I hadn't had a chance to reply yet. I would appreciate it if you don't accuse us of "not caring", or I might have to start pointing out upstreaming attempts by us that were ignored by the respective maintainers for *months*, not 4 days. Glass houses and throwing stones and all that. - Hector
On 05/09/2022 23.01, Russell King (Oracle) wrote: > On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: >> P.S. Do you have a platform to test all these? > > Yes, but that doesn't mean I can do testing sufficient to ensure that > the modifications are correct. As I understand things, the SMC is not > limited to just aarch64 hardware. FWIW, the RTKit backend is limited to aarch64 hardware, and that's also why I kept all the endian-munging there. T2 and legacy x86 backends (which don't exist yet, so whether things need changing for those platforms is an open question anyway) would respectively do whatever endian-munging is appropriate for them. So at this point, only Apple Mx AArch64 SoCs matter, though I *tried* to write the code in the way that I thought was most likely to cleanly transfer over to other SMC platforms by just changing the backend code. - Hector
On Tue, Sep 06, 2022 at 12:52:48AM +0900, Hector Martin wrote: > On 05/09/2022 23.01, Russell King (Oracle) wrote: > > On Mon, Sep 05, 2022 at 04:16:27PM +0300, Andy Shevchenko wrote: > >> P.S. Do you have a platform to test all these? > > > > Yes, but that doesn't mean I can do testing sufficient to ensure that > > the modifications are correct. As I understand things, the SMC is not > > limited to just aarch64 hardware. > > FWIW, the RTKit backend is limited to aarch64 hardware, and that's also > why I kept all the endian-munging there. T2 and legacy x86 backends > (which don't exist yet, so whether things need changing for those > platforms is an open question anyway) would respectively do whatever > endian-munging is appropriate for them. > > So at this point, only Apple Mx AArch64 SoCs matter, though I *tried* to > write the code in the way that I thought was most likely to cleanly > transfer over to other SMC platforms by just changing the backend code. Right, and that's a good argument not to go and change the smc_key layout... that invites one hell of a big headache (also for the reasons you've set out in one of your previous replies.) I'm going to back out the hex2bin conversion; you've made a good argument why the code is the way it is. Thanks.
On 06/09/2022 00.44, Martin Povišer wrote: > >> On 5. 9. 2022, at 17:32, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > >> I still need a resolution between you and Hector over the smc_key >> issue - specifically, do I pick up the patch that adds support for >> %p4ch, or do we re-architect the smc_key thing and also in doing so >> get rid of the need for your "endian conversion" thing. > > Idea about the %p4ch thing: We will leave the keys true > to their nature (i.e. 32-bit integer), and at least initially > for the prints we will employ macros > > #define SMC_KEYFMT “%c%c%c%c” > #define SMC_KEYFMT_VAL(val) (val)>>24,(val)>>16,(val)>>8,(val) > > used like > > printk(“blah blah” SMC_KEYFMT “ blah\n”, SMC_KEYFMT_VAL(key)); > > This has the nice property that it is pretty much like the specifier, > and later can be easily replaced with the real thing. Not the prettiest, but I'll take this over trying to mess around with string buffer conversions or anything involving non-native endianness if the printk specifier patch is going to be controversial. I'd prefer shorter macro names though, like SMC_KFMT/SMC_KVAL(), to avoid further lengthening already-long printk lines. - Hector
On Tue, Sep 06, 2022 at 12:58:48AM +0900, Hector Martin wrote: > On 06/09/2022 00.44, Martin Povišer wrote: > > > >> On 5. 9. 2022, at 17:32, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > > >> I still need a resolution between you and Hector over the smc_key > >> issue - specifically, do I pick up the patch that adds support for > >> %p4ch, or do we re-architect the smc_key thing and also in doing so > >> get rid of the need for your "endian conversion" thing. > > > > Idea about the %p4ch thing: We will leave the keys true > > to their nature (i.e. 32-bit integer), and at least initially > > for the prints we will employ macros > > > > #define SMC_KEYFMT “%c%c%c%c” > > #define SMC_KEYFMT_VAL(val) (val)>>24,(val)>>16,(val)>>8,(val) > > > > used like > > > > printk(“blah blah” SMC_KEYFMT “ blah\n”, SMC_KEYFMT_VAL(key)); > > > > This has the nice property that it is pretty much like the specifier, > > and later can be easily replaced with the real thing. > > Not the prettiest, but I'll take this over trying to mess around with > string buffer conversions or anything involving non-native endianness if > the printk specifier patch is going to be controversial. > > I'd prefer shorter macro names though, like SMC_KFMT/SMC_KVAL(), to > avoid further lengthening already-long printk lines. I suggest that I try resubmitting the series with IRQ support dropped, and with the %p4ch support in it and we'll see what happens. If %p4ch gets accepted, then changing it would be adding extra work. In any case, these %p... format extensions are supposed to avoid yucky stuff such as the above. Andy's objection to %p4ch was predicated on using the illegal C of &cpu_to_be32(key) which has been shown to have been a waste of time. For reference for those reading this, %p4ch doesn't print only print the key as characters, it prints the hex value as well. For example: macsmc-rtkit 23e400000.smc: Initialized (922 keys #KEY (0x234b4559)..zETM (0x7a45544d)) ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ macsmc-gpio macsmc-gpio: First GPIO key: gP01 (0x67503031) ^^^^^^^^^^^^^^^^^ The underlined strings is the output from %p4ch. So, even if Andy's cpu_to_be32() idea was legal C, it wouldn't be functionally the same without adding extra code to every place that one of these keys is printed.
On Mon, Sep 5, 2022 at 6:13 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > I suggest that I try resubmitting the series with IRQ support dropped, > and with the %p4ch support in it and we'll see what happens. You can add my Reviewed-by: Linus Walleij <linus.walleij@linaro.org> on the result, and the code should go in when Hector & you are happy with it. I surely trust you to fix the final polish. I don't mind the IRQ patch either, but I understand it's a bit annoying if you can't test it on anything. Yours, Linus Walleij
On 06/09/2022 04.10, Linus Walleij wrote: > On Mon, Sep 5, 2022 at 6:13 PM Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > >> I suggest that I try resubmitting the series with IRQ support dropped, >> and with the %p4ch support in it and we'll see what happens. > > You can add my > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > on the result, and the code should go in when Hector & you are > happy with it. I surely trust you to fix the final polish. > > I don't mind the IRQ patch either, but I understand it's a bit > annoying if you can't test it on anything. Thank you! FWIW, I wrote that commit as part of working on PCIe power saving and the SD card reader specifically, where the SMC GPIO card detect pin can replace the internal card detect, and this allows us to completely power down the SD card reader when there is no card inserted. But that PCIe power management code isn't anywhere near done yet, I just tested that using the SMC card detect IRQ works as expected with sdhci-pci. So it's perfectly fine to hold off on that patch until later, and submit it once we've actually been using it downstream for a bit and it gets some testing. - Hector
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 0642f579196f..9b87f5ebe1b9 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1292,6 +1292,17 @@ config GPIO_LP87565 This driver can also be built as a module. If so, the module will be called gpio-lp87565. +config GPIO_MACSMC + tristate "Apple Mac SMC GPIO" + depends on APPLE_SMC + default ARCH_APPLE + help + Support for GPIOs controlled by the SMC microcontroller on Apple Mac + systems. + + This driver can also be built as a module. If so, the module will be + called gpio-macsmc. + config GPIO_MADERA tristate "Cirrus Logic Madera class codecs" depends on PINCTRL_MADERA diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index a0985d30f51b..a401a467c6f4 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_LP873X) += gpio-lp873x.o obj-$(CONFIG_GPIO_LP87565) += gpio-lp87565.o obj-$(CONFIG_GPIO_LPC18XX) += gpio-lpc18xx.o obj-$(CONFIG_GPIO_LPC32XX) += gpio-lpc32xx.o +obj-$(CONFIG_GPIO_MACSMC) += gpio-macsmc.o obj-$(CONFIG_GPIO_MADERA) += gpio-madera.o obj-$(CONFIG_GPIO_MAX3191X) += gpio-max3191x.o obj-$(CONFIG_GPIO_MAX7300) += gpio-max7300.o diff --git a/drivers/gpio/gpio-macsmc.c b/drivers/gpio/gpio-macsmc.c new file mode 100644 index 000000000000..ff9950afb69a --- /dev/null +++ b/drivers/gpio/gpio-macsmc.c @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0-only OR MIT +/* + * Apple SMC GPIO driver + * Copyright The Asahi Linux Contributors + * + * This driver implements basic SMC PMU GPIO support that can read inputs + * and write outputs. Mode changes and IRQ config are not yet implemented. + */ + +#include <linux/bitmap.h> +#include <linux/device.h> +#include <linux/gpio/driver.h> +#include <linux/mfd/core.h> +#include <linux/mfd/macsmc.h> + +#define MAX_GPIO 64 + +/* + * 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. + * + * We're going to try to make do with the low commands for now. + * We don't implement pin mode changes at this time. + */ + +#define CMD_ACTION (0 << 24) +#define CMD_OUTPUT (1 << 24) +#define CMD_INPUT (2 << 24) +#define CMD_PINMODE (3 << 24) +#define CMD_IRQ_ENABLE (4 << 24) +#define CMD_IRQ_ACK (5 << 24) +#define CMD_IRQ_MODE (6 << 24) +#define CMD_CONFIG (0xff << 24) + +#define MODE_INPUT 0 +#define MODE_OUTPUT 1 +#define MODE_VALUE_0 0 +#define MODE_VALUE_1 2 + +#define IRQ_MODE_HIGH 0 +#define IRQ_MODE_LOW 1 +#define IRQ_MODE_RISING 2 +#define IRQ_MODE_FALLING 3 +#define IRQ_MODE_BOTH 4 + +#define CONFIG_MASK GENMASK(23, 16) +#define CONFIG_VAL GENMASK(7, 0) + +#define CONFIG_OUTMODE GENMASK(7, 6) +#define CONFIG_IRQMODE GENMASK(5, 3) +#define CONFIG_PULLDOWN BIT(2) +#define CONFIG_PULLUP BIT(1) +#define CONFIG_OUTVAL BIT(0) + +/* + * output modes seem to differ depending on the PMU in use... ? + * 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; + + 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); +} + +static int macsmc_gpio_key(unsigned int offset) +{ + return _SMC_KEY("gP\0\0") | (hex_asc_hi(offset) << 8) | hex_asc_lo(offset); +} + +static int macsmc_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) +{ + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + smc_key key = macsmc_gpio_key(offset); + u32 val; + int ret; + + /* 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; +} + +static int macsmc_gpio_get(struct gpio_chip *gc, unsigned int offset) +{ + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + smc_key key = macsmc_gpio_key(offset); + u32 val; + int ret; + + ret = macsmc_gpio_get_direction(gc, offset); + if (ret < 0) + return 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); + + if (ret < 0) + return ret; + + return val ? 1 : 0; +} + +static void macsmc_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) +{ + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + smc_key key = macsmc_gpio_key(offset); + int ret; + + value |= CMD_OUTPUT; + 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); +} + +static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc, + unsigned long *valid_mask, unsigned int ngpios) +{ + struct macsmc_gpio *smcgp = gpiochip_get_data(gc); + int count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index; + int i; + + if (count > MAX_GPIO) + count = MAX_GPIO; + + 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); + + 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); + continue; + } + + set_bit(gpio_nr, valid_mask); + } + + return 0; +} + +static int macsmc_gpio_probe(struct platform_device *pdev) +{ + struct macsmc_gpio *smcgp; + struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent); + smc_key key; + int ret; + + smcgp = devm_kzalloc(&pdev->dev, sizeof(*smcgp), GFP_KERNEL); + if (!smcgp) + return -ENOMEM; + + pdev->dev.of_node = of_get_child_by_name(pdev->dev.parent->of_node, "gpio"); + + smcgp->dev = &pdev->dev; + smcgp->smc = smc; + smcgp->first_index = apple_smc_find_first_key_index(smc, SMC_KEY(gP00)); + + if (smcgp->first_index >= apple_smc_get_key_count(smc)) + return -ENODEV; + + ret = apple_smc_get_key_by_index(smc, smcgp->first_index, &key); + if (ret < 0) + return ret; + + if (key > macsmc_gpio_key(MAX_GPIO - 1)) + return -ENODEV; + + dev_info(smcgp->dev, "First GPIO key: %p4ch\n", &key); + + smcgp->gc.label = "macsmc-pmu-gpio"; + smcgp->gc.owner = THIS_MODULE; + smcgp->gc.get = macsmc_gpio_get; + smcgp->gc.set = macsmc_gpio_set; + smcgp->gc.get_direction = macsmc_gpio_get_direction; + smcgp->gc.init_valid_mask = macsmc_gpio_init_valid_mask; + smcgp->gc.can_sleep = true; + smcgp->gc.ngpio = MAX_GPIO; + smcgp->gc.base = -1; + smcgp->gc.parent = &pdev->dev; + + return devm_gpiochip_add_data(&pdev->dev, &smcgp->gc, smcgp); +} + +static struct platform_driver macsmc_gpio_driver = { + .driver = { + .name = "macsmc-gpio", + }, + .probe = macsmc_gpio_probe, +}; +module_platform_driver(macsmc_gpio_driver); + +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>"); +MODULE_LICENSE("Dual MIT/GPL"); +MODULE_DESCRIPTION("Apple SMC GPIO driver"); +MODULE_ALIAS("platform:macsmc-gpio");