Re: [PATCH 5/6] gpio: Add new gpio-macsmc driver for Apple Macs

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx> 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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!