lkml.org 
[lkml]   [2020]   [Oct]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v6 2/3] pinctrl: pinctrl-microchip-sgpio: Add pinctrl driver for Microsemi Serial GPIO
    Date

    Hi Andy!

    Andy Shevchenko writes:

    > On Wed, Oct 14, 2020 at 6:25 PM Lars Povlsen <lars.povlsen@microchip.com> wrote:
    >>
    >> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
    >> (SGPIO) device used in various SoC's.
    >
    > ...
    >
    >> +#define PIN_NAM_SZ (sizeof("SGPIO_D_pXXbY")+1)
    >
    > +1 for what?
    >

    Reverse fencepost :-). I'll remove it.

    > ...
    >
    >> +#define __shf(x) (__builtin_ffsll(x) - 1)
    >> +#define __BF_PREP(bf, x) (bf & ((x) << __shf(bf)))
    >> +#define __BF_GET(bf, x) (((x & bf) >> __shf(bf)))
    >
    > This smells like bitfield.h.
    >

    Yes, and I would use it if I could, just bitfield.h don't like anything
    but constexpr. The driver support 3 SoC variants which (unfortunately)
    have different register layouts in some areas.

    > ...
    >
    >> +static int sgpio_input_get(struct sgpio_priv *priv,
    >> + struct sgpio_port_addr *addr)
    >> +{
    >
    >> + int ret;
    >> +
    >> + ret = !!(sgpio_readl(priv, REG_INPUT_DATA, addr->bit) &
    >> + BIT(addr->port));
    >> +
    >> + return ret;
    >
    > Sounds like one line.
    >

    Yes, I'll change that.

    >> +}
    >
    >> +static int sgpio_get_functions_count(struct pinctrl_dev *pctldev)
    >> +{
    >
    >> + return 1;
    >
    > I didn't get why it's not a pure GPIO driver?
    > It has only one function (no pinmux).
    > I didn't find any pin control features either.
    >
    > What did I miss?

    The hardware has more functions, which are planned to be added
    later. This has already been agreed with Linux Walleij.

    >
    > ...
    >
    >> +static int microchip_sgpio_get_value(struct gpio_chip *gc, unsigned int gpio)
    >> +{
    >> + struct sgpio_bank *bank = gpiochip_get_data(gc);
    >> + struct sgpio_priv *priv = bank->priv;
    >> + struct sgpio_port_addr addr;
    >
    >> + int ret;
    >
    > No need.

    Ok, I'll trim it.

    >
    >> +
    >> + sgpio_pin_to_addr(priv, gpio, &addr);
    >> +
    >> + if (bank->is_input)
    >> + ret = sgpio_input_get(priv, &addr);
    >> + else
    >> + ret = sgpio_output_get(priv, &addr);
    >> +
    >> + return ret;
    >> +}
    >
    >
    > ...
    >
    >
    >> + ret = devm_gpiochip_add_data(dev, gc, bank);
    >> + if (ret == 0)
    >
    >> + dev_info(dev, "Registered %d GPIOs\n", ngpios);
    >
    > No noise.
    >

    OK, gone.

    >> + else
    >> + dev_err(dev, "Failed to register: ret %d\n", ret);
    >> +
    >
    > ...
    >
    >> + /* Get register map */
    >> + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    >> + priv->regs = devm_ioremap_resource(dev, regs);
    >
    > devm_platform_ioremap_resource();

    Yes, I'll replace with that.

    >
    >> + if (IS_ERR(priv->regs))
    >> + return PTR_ERR(priv->regs);
    >
    >> + priv->properties = of_device_get_match_data(dev);
    >
    > It's interesting you have a mix between OF APIs and device property
    > APIs. Choose one. If you stick with OF, use of_property_ and so,
    > otherwise replace of_*() by corresponding device_*() or generic calls.

    Sure. I will change the device_property_read_u32() with
    of_property_read_u32().

    >
    > Can you use gpio-regmap APIs?

    No, I think the sgpio hardware is a little too odd for that
    (of_gpio_n_cells == 3). And then there's alternate pinctrl functions.

    Thank you for your comments, they are very much appreciated. Let me know
    if I missed anything.

    I will refresh the series shortly (on v5.10-rc1).

    ---Lars

    --
    Lars Povlsen,
    Microchip

    \
     
     \ /
      Last update: 2020-10-26 15:42    [W:4.055 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site