Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1378567 > unrolled thread

Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for Broadcom NS2 SoC

Back to article view | Back to linux.kernel

Contents

  Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for  Broadcom NS2 SoC Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com> - 2016-04-14 10:00 +0200
    Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for  Broadcom NS2 SoC Linus Walleij <linus.walleij@linaro.org> - 2016-04-14 11:50 +0200

#1378567 — Re: [PATCH RESEND 1/2] pinctrl: ns2: add pinmux driver support for Broadcom NS2 SoC

Hi Linus,

On Wed, Apr 13, 2016 at 6:49 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Mar 29, 2016 at 5:22 PM, Yendapally Reddy Dhananjaya Reddy
> <yendapally.reddy@broadcom.com> wrote:
>
>> This adds the initial support of the Broadcom NS2 pinmux driver
>>
>> Signed-off-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@broadcom.com>
>> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
>
> Sorry for slow replies. :(
>
> This mostly looks good. Some small things needs fixing:
>
>> +config PINCTRL_NS2_MUX
>> +       bool "Broadcom Northstar2 pinmux driver"
>
> This is a bool driver, yet it uses MODULE* macros at the end of the file.
> Check the recent commits from Paul Gortmaker cleaning this up.
> Just a git log -p --author=Gortmaker in the main kernel tree will quickly
> give you an idea of what you have to do.
>

Sure. I will address this in the v2 patch set.

>
>> +static const unsigned int gpio_0_1_pins[] = {24, 25};
>> +static const unsigned int pwm_0_pins[] = {24};
>> +static const unsigned int pwm_1_pins[] = {25};
>
> So either the same pins are used for GPIO or PWM.
> And this pattern persists.
>
> Do you have a brewing GPIO driver for this block as well?
> Is it a separate front-end calling to pinctrl using the
> pinctrl_gpio_* calls or do you plan to patch it into this
> driver?
>
> This is more of a question.
>

This SoC supports group based configuration and there is a top level register
to select groups. Once the gpio_0_1_pins group is selected, there is one more
register to select between gpio_0_1 and pwm (only four pins). The pins
24 and 25 are shared between nor pins and gpio at the top group level. Once
gpio group is selected, then we can select to be either gpio or pwm. I missed
these two pins to be added to nor_data_pins and will add in the next version.

static const unsigned nor_data_pins[] =  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
        10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};

NS2_PIN_GROUP(nand, 0, 0, 31, 1, 0),
NS2_PIN_GROUP(nor_data, 0, 0, 31, 1, 1),
NS2_PIN_GROUP(gpio_0_1, 0, 0, 31, 1, 0),

To select PWM, we need to select gpio and pwm as well.

gpio: gpio {
function = "gpio";
groups = "gpio_0_1_grp";

pwm: pwm {
function = "pwm";
groups = "pwm0_grp", "pwm1_grp";
};

Already available gpio driver "pinctrl-iproc-gpio.c" will be the gpio driver
for this soc as well without pin request.

> Apart from this it looks good.
>
> Yours,
> Linus Walleij

Thanks & Regards
Dhananjay

[toc] | [next] | [standalone]

#1378661

On Thu, Apr 14, 2016 at 9:53 AM, Yendapally Reddy Dhananjaya Reddy
<yendapally.reddy@broadcom.com> wrote:
> On Wed, Apr 13, 2016 at 6:49 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Tue, Mar 29, 2016 at 5:22 PM, Yendapally Reddy Dhananjaya Reddy
>> <yendapally.reddy@broadcom.com> wrote:

>>> +static const unsigned int gpio_0_1_pins[] = {24, 25};
>>> +static const unsigned int pwm_0_pins[] = {24};
>>> +static const unsigned int pwm_1_pins[] = {25};
>>
>> So either the same pins are used for GPIO or PWM.
>> And this pattern persists.
>>
>> Do you have a brewing GPIO driver for this block as well?
>> Is it a separate front-end calling to pinctrl using the
>> pinctrl_gpio_* calls or do you plan to patch it into this
>> driver?
>>
>> This is more of a question.
>>
>
> This SoC supports group based configuration and there is a top level register
> to select groups. Once the gpio_0_1_pins group is selected, there is one more
> register to select between gpio_0_1 and pwm (only four pins). The pins
> 24 and 25 are shared between nor pins and gpio at the top group level. Once
> gpio group is selected, then we can select to be either gpio or pwm. I missed
> these two pins to be added to nor_data_pins and will add in the next version.
>
> static const unsigned nor_data_pins[] =  {0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
>         10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25};
>
> NS2_PIN_GROUP(nand, 0, 0, 31, 1, 0),
> NS2_PIN_GROUP(nor_data, 0, 0, 31, 1, 1),
> NS2_PIN_GROUP(gpio_0_1, 0, 0, 31, 1, 0),
>
> To select PWM, we need to select gpio and pwm as well.
>
> gpio: gpio {
> function = "gpio";
> groups = "gpio_0_1_grp";
>
> pwm: pwm {
> function = "pwm";
> groups = "pwm0_grp", "pwm1_grp";
> };
>
> Already available gpio driver "pinctrl-iproc-gpio.c" will be the gpio driver
> for this soc as well without pin request.

Then you are doing something wrong. Look in pinctrl-iproc-gpio.c:

/*
 * Request the Iproc IOMUX pinmux controller to mux individual pins to GPIO
 */
static int iproc_gpio_request(struct gpio_chip *gc, unsigned offset)
{
        struct iproc_gpio *chip = gpiochip_get_data(gc);
        unsigned gpio = gc->base + offset;

        /* not all Iproc GPIO pins can be muxed individually */
        if (!chip->pinmux_is_supported)
                return 0;

        return pinctrl_request_gpio(gpio);
}

static void iproc_gpio_free(struct gpio_chip *gc, unsigned offset)
{
        struct iproc_gpio *chip = gpiochip_get_data(gc);
        unsigned gpio = gc->base + offset;

        if (!chip->pinmux_is_supported)
                return;

        pinctrl_free_gpio(gpio);
}

So as you see pinctrl_request_gpio() and pinctrl_free_gpio()
are being called.

These will in turn call pinmux_request_gpio() and
pinmux_free_gpio() to make the backing pin controller
mux in the pin as GPIO.

pinmux_request_gpio() will end up in pin_request()
and at this point:

if (gpio_range && ops->gpio_request_enable)
                /* This requests and enables a single GPIO pin */
                status = ops->gpio_request_enable(pctldev, gpio_range, pin);

As you can see: it will attempt to call the .gpio_request_enable()
method of your struct pinmux_ops.

But your pinmux ops look like this:

+static struct pinmux_ops ns2_pinmux_ops = {
+       .get_functions_count = ns2_get_functions_count,
+       .get_function_name = ns2_get_function_name,
+       .get_function_groups = ns2_get_function_groups,
+       .set_mux = ns2_pinmux_enable,
+};

I.e. there is no way that GPIO can be set up as a GPIO line,
and you're relying on some other pin control entries in the
device tree to do that, which is unnecessarily complicated.

Please consider implementing the .gpio_request_enable callback
for this pin multiplexer.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web