Re: [PATCH v5 2/3] gpio: loongson: add gpio driver support

[Yinbo Zhu <zhuyinbo@loongson.cn>]

在 2022/11/24 上午6:05, Linus Walleij 写道:
> On Wed, Nov 23, 2022 at 9:02 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
>> 在 2022/11/21 下午9:24, Linus Walleij 写道:
> 
>>>> +static int loongson_gpio_request(
>>>> +                       struct gpio_chip *chip, unsigned int pin)
>>>> +{
>>>> +       if (pin >= chip->ngpio)
>>>> +               return -EINVAL;
>>>
>>> This is not needed, the gpiolib core already checks this. Drop it.
>> I check gpio_request in gpilib, I notice gpio_is_valid is not equal to
>> this condition, so I still kept it for byte mode.
> 
> This is because descriptors can only be obtained from gpiod_get() and
> similar and gpiod_get() falls to gpiod_get_index() which will not
> return a valid descriptor from either HW backend. gpiod_get()
> will call gpiod_request() for if and only if the descriptor is valid.
> 
> The only reason to implement something like this is because of
> using the legacy GPIO numberspace which we are getting rid
> of so it is irrelevant, the consumers of your driver will only be
> using gpio descriptors, will only come in through gpiod_get_index()
> and will have desc validity check done before calling gpiod_request().
> 
> So drop this.
> 
>>> I am bit suspicious that your IRQchip implementation expects consumers
>>> to call gpiod_to_irq() first and this is not legal.
>>
>> okay, I got it, and other driver use gpio interrupt doesn't rely on
>> gpiod_to_irq, but can use gpiod_to_irq.
> 
> Yes it can be used to look up the irq corresponding to a GPIO
> but it is not mandatory to do that.
> 
>> The reason is that gpio interrupt wasn't an independent module,  The
>> loongson interrupt controller liointc include lots of interrupt was
>> route to perpherial, such as i2c/spi .. gpio, so gpio interrupt as
>> normal perpherial interrupt, It is unnecessary and redundant to
>> implement a gpio irq chip. The liointc controller driver had cover all
>> interrupt.
> 
> This is fine, and it is common for GPIO drivers to implement
> their own IRQchips.
> 
> But these drivers can not rely on the .gpio_to_irq() callback
> to be called before an IRQ is requested and used.
> 
>> in addition,  I don't like to use the dynamically allocated gpio base,
>> so I set the gpio base after call bgpio_init.
> 
> Don't do that because the GPIO maintainers love the
> dynamic base and hate hardcoded bases. Set the base to -1
> If you wonder why, read drivers/gpio/TODO.
Hi Linus,

I recenly verfied other peripheral on upstream, some peripheral driver
need use gpio number, but if use dynamic base that gpio number will be
meaningless.  in additon I notice that many gpio driver don't use
dynamic base, although bgpio_int was called.

so I think the gpio number should be keep consistent with datasheet for
some platform that need use gpio number.

Yinbo.

> 
> Yours,
> Linus Walleij
>