Re: [PATCH v2 1/2] pinctrl: qcom: Add sm8250 lpass lpi pinctrl driver

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Srinivas,

thanks for your patch!

On Thu, Nov 5, 2020 at 1:04 PM Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:

> Add initial pinctrl driver to support pin configuration for
> LPASS (Low Power Audio SubSystem) LPI (Low Power Island) pinctrl
> on SM8250.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>

So this is in essence a completely new pin controller that shares
nothing with the previous Qcom SoC pin control hardware?

I'd still like Bjorn to review it of course, but if you are going to
maintain this driver an entry to the MAINTAINERS file would
be nice.

I'd like some more talk in the commit message about how this
driver is engineered so I point those things out below.

> +config PINCTRL_LPASS_LPI
> +       tristate "Qualcomm Technologies Inc LPASS LPI pin controller driver"
> +       depends on GPIOLIB && OF

These days you can actually just
select GPIOLIB
but no big deal.

> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>

Do not use this legacy header for new GPIO drivers.
#include <linux/gpio/driver.h>
should work.

> +#define LPI_GPIO_REG_VAL_CTL             0x00
> +#define LPI_GPIO_REG_DIR_CTL             0x04
> +#define LPI_SLEW_REG_VAL_CTL             0x00
> +#define LPI_SLEW_RATE_MAX                0x03
> +#define LPI_SLEW_BITS_SIZE               0x02
> +#define LPI_GPIO_REG_PULL_SHIFT                0x0
> +#define LPI_GPIO_REG_PULL_MASK         GENMASK(1, 0)
> +#define LPI_GPIO_REG_FUNCTION_SHIFT    0x2
> +#define LPI_GPIO_REG_FUNCTION_MASK     GENMASK(5, 2)
> +#define LPI_GPIO_REG_OUT_STRENGTH_SHIFT        0x6
> +#define LPI_GPIO_REG_OUT_STRENGTH_MASK GENMASK(8, 6)
> +#define LPI_GPIO_REG_OE_SHIFT          0x9
> +#define LPI_GPIO_REG_OE_MASK           BIT(9)
> +#define LPI_GPIO_REG_DIR_SHIFT         0x1
> +#define LPI_GPIO_REG_DIR_MASK          0x2
> +#define LPI_GPIO_BIAS_DISABLE          0x0
> +#define LPI_GPIO_PULL_DOWN             0x1
> +#define LPI_GPIO_KEEPER                        0x2
> +#define LPI_GPIO_PULL_UP               0x3

So the way I understand it, the GPIO lines have one register each and then the
functionality of each line is handled by different bits in that register, like
output is driven in bit 9.

This would be nice to have mentioned in the commit message.

> +static const unsigned int gpio0_pins[] = { 0 };
> +static const unsigned int gpio1_pins[] = { 1 };
> +static const unsigned int gpio2_pins[] = { 2 };
> +static const unsigned int gpio3_pins[] = { 3 };
> +static const unsigned int gpio4_pins[] = { 4 };
> +static const unsigned int gpio5_pins[] = { 5 };
> +static const unsigned int gpio6_pins[] = { 6 };
> +static const unsigned int gpio7_pins[] = { 7 };
> +static const unsigned int gpio8_pins[] = { 8 };
> +static const unsigned int gpio9_pins[] = { 9 };
> +static const unsigned int gpio10_pins[] = { 10 };
> +static const unsigned int gpio11_pins[] = { 11 };
> +static const unsigned int gpio12_pins[] = { 12 };
> +static const unsigned int gpio13_pins[] = { 13 };
> +static const char * const swr_tx_clk_groups[] = { "gpio0" };
> +static const char * const swr_tx_data1_groups[] = { "gpio1" };
> +static const char * const swr_tx_data2_groups[] = { "gpio2" };
> +static const char * const swr_rx_clk_groups[] = { "gpio3" };
> +static const char * const swr_rx_data1_groups[] = { "gpio4" };
> +static const char * const swr_tx_data3_groups[] = { "gpio5" };
> +static const char * const dmic1_clk_groups[] = { "gpio6" };
> +static const char * const dmic1_data_groups[] = { "gpio7" };
> +static const char * const dmic2_clk_groups[] = { "gpio8" };
> +static const char * const dmic2_data_groups[] = { "gpio9" };
> +static const char * const i2s2_clk_groups[] = { "gpio10" };
> +static const char * const i2s2_ws_groups[] = { "gpio11" };
> +static const char * const dmic3_clk_groups[] = { "gpio12" };
> +static const char * const dmic3_data_groups[] = { "gpio13" };
> +static const char * const qua_mi2s_sclk_groups[] = { "gpio0" };
> +static const char * const qua_mi2s_ws_groups[] = { "gpio1" };
> +static const char * const qua_mi2s_data0_groups[] = { "gpio2" };
> +static const char * const qua_mi2s_data1_groups[] = { "gpio3" };
> +static const char * const qua_mi2s_data2_groups[] = { "gpio4" };
> +static const char * const swr_rx_data2_groups[] = { "gpio5" };
> +static const char * const i2s1_clk_groups[] = { "gpio6" };
> +static const char * const i2s1_ws_groups[] = { "gpio7" };
> +static const char * const i2s1_data0_groups[] = { "gpio8" };
> +static const char * const i2s1_data1_groups[] = { "gpio9" };
> +static const char * const wsa_swr_clk_groups[] = { "gpio10" };
> +static const char * const wsa_swr_data_groups[] = { "gpio11" };
> +static const char * const i2s2_data0_groups[] = { "gpio12" };
> +static const char * const i2s2_data1_groups[] = { "gpio13" };

The driver appears to follow other qualcomm pin controllers in using
the "one group is one pin" approach. This is idiomatic and should be
mentioned in the commit message.

> +static int sm8250_slew_reg_offsets[] = {
> +               0x0, 0x2, 0x4, 0x8, 0xa,
> +               0xc, 0x0, 0x0, 0x0, 0x0,
> +               0x10, 0x12, 0x0, 0x0,
> +};

Maybe it is obvious to everyone what this array is about, but why so
many zeroes? I think it warrants a comment in the code if that
means for example that some pins do not support slew rate setting.

Overall this is a nice and self-contained driver that uses the abstractions
the right way and very straight-forward, so I think we can merge it
soon.

(Look into Andy's comments as well.)

Yours,
Linus Walleij