2023-09-14 08:48:20

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry

On 14/09/2023 04:20, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> i.MX8ULP supports two interrupts, while i.MX7ULP supports one interrupt.
> So from hardware perspective, they are not compatible.
>
> So add entry for i.MX8ULP.
>
> Signed-off-by: Peng Fan <[email protected]>
> ---
> drivers/gpio/gpio-vf610.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> index dbc7ba0ee72c..88f7215cdf4b 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -67,6 +67,7 @@ static const struct fsl_gpio_soc_data imx_data = {
> static const struct of_device_id vf610_gpio_dt_ids[] = {
> { .compatible = "fsl,vf610-gpio", .data = NULL, },
> { .compatible = "fsl,imx7ulp-gpio", .data = &imx_data, },
> + { .compatible = "fsl,imx8ulp-gpio", .data = &imx_data, },

Why? It is the same as imx7. No need.

Best regards,
Krzysztof


2023-09-14 15:18:45

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry

On Thu, Sep 14, 2023 at 7:48 AM Krzysztof Kozlowski
<[email protected]> wrote:
> On 14/09/2023 04:20, Peng Fan (OSS) wrote:
> > From: Peng Fan <[email protected]>
> >
> > i.MX8ULP supports two interrupts, while i.MX7ULP supports one interrupt.
> > So from hardware perspective, they are not compatible.
> >
> > So add entry for i.MX8ULP.
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/gpio/gpio-vf610.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> > index dbc7ba0ee72c..88f7215cdf4b 100644
> > --- a/drivers/gpio/gpio-vf610.c
> > +++ b/drivers/gpio/gpio-vf610.c
> > @@ -67,6 +67,7 @@ static const struct fsl_gpio_soc_data imx_data = {
> > static const struct of_device_id vf610_gpio_dt_ids[] = {
> > { .compatible = "fsl,vf610-gpio", .data = NULL, },
> > { .compatible = "fsl,imx7ulp-gpio", .data = &imx_data, },
> > + { .compatible = "fsl,imx8ulp-gpio", .data = &imx_data, },
>
> Why? It is the same as imx7. No need.

Because compatible = "fsl,imx7ulp-gpio" is not what is going to be in the
device tree, but compatible = "fsl,imx8ulp-gpio"?

What am I missing here? Maybe the commit message is weird.

Yours,
Linus Walleij

2023-09-14 15:31:31

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry

On 14/09/2023 10:48, Linus Walleij wrote:
> On Thu, Sep 14, 2023 at 7:48 AM Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 14/09/2023 04:20, Peng Fan (OSS) wrote:
>>> From: Peng Fan <[email protected]>
>>>
>>> i.MX8ULP supports two interrupts, while i.MX7ULP supports one interrupt.
>>> So from hardware perspective, they are not compatible.
>>>
>>> So add entry for i.MX8ULP.
>>>
>>> Signed-off-by: Peng Fan <[email protected]>
>>> ---
>>> drivers/gpio/gpio-vf610.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
>>> index dbc7ba0ee72c..88f7215cdf4b 100644
>>> --- a/drivers/gpio/gpio-vf610.c
>>> +++ b/drivers/gpio/gpio-vf610.c
>>> @@ -67,6 +67,7 @@ static const struct fsl_gpio_soc_data imx_data = {
>>> static const struct of_device_id vf610_gpio_dt_ids[] = {
>>> { .compatible = "fsl,vf610-gpio", .data = NULL, },
>>> { .compatible = "fsl,imx7ulp-gpio", .data = &imx_data, },
>>> + { .compatible = "fsl,imx8ulp-gpio", .data = &imx_data, },
>>
>> Why? It is the same as imx7. No need.
>
> Because compatible = "fsl,imx7ulp-gpio" is not what is going to be in the
> device tree, but compatible = "fsl,imx8ulp-gpio"?
>
> What am I missing here? Maybe the commit message is weird.
>

If the devices used before and are still going to use same driver data,
they look compatible from OS point of view. Therefore usually we express
such compatibility and do not add unneeded device_id entries.

Now whether the devices are truly compatible or not, I don't know and
with some recent emails I am bit confused.

Best regards,
Krzysztof

2023-09-14 17:13:18

by Peng Fan

[permalink] [raw]
Subject: RE: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry

> Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry
>
> On 14/09/2023 10:48, Linus Walleij wrote:
> > On Thu, Sep 14, 2023 at 7:48 AM Krzysztof Kozlowski
> > <[email protected]> wrote:
> >> On 14/09/2023 04:20, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <[email protected]>
> >>>
> >>> i.MX8ULP supports two interrupts, while i.MX7ULP supports one
> interrupt.
> >>> So from hardware perspective, they are not compatible.
> >>>
> >>> So add entry for i.MX8ULP.
> >>>
> >>> Signed-off-by: Peng Fan <[email protected]>
> >>> ---
> >>> drivers/gpio/gpio-vf610.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
> >>> index dbc7ba0ee72c..88f7215cdf4b 100644
> >>> --- a/drivers/gpio/gpio-vf610.c
> >>> +++ b/drivers/gpio/gpio-vf610.c
> >>> @@ -67,6 +67,7 @@ static const struct fsl_gpio_soc_data imx_data = {
> >>> static const struct of_device_id vf610_gpio_dt_ids[] = {
> >>> { .compatible = "fsl,vf610-gpio", .data = NULL, },
> >>> { .compatible = "fsl,imx7ulp-gpio", .data = &imx_data, },
> >>> + { .compatible = "fsl,imx8ulp-gpio", .data = &imx_data, },
> >>
> >> Why? It is the same as imx7. No need.
> >
> > Because compatible = "fsl,imx7ulp-gpio" is not what is going to be in
> > the device tree, but compatible = "fsl,imx8ulp-gpio"?
> >
> > What am I missing here? Maybe the commit message is weird.
> >
>
> If the devices used before and are still going to use same driver data, they
> look compatible from OS point of view. Therefore usually we express such
> compatibility and do not add unneeded device_id entries.
>
> Now whether the devices are truly compatible or not, I don't know and with
> some recent emails I am bit confused.

Some tricks in dtb are made to make the driver could work for both i.MX7ULP,
i.MX8ULP, i.MX93 with fsl,imx7ulp-gpio.

Such as i.MX8ULP:
reg = <0x2d000080 0x1000>, <0x2d000040 0x40>;
Actually the two regs are belong to one physical continuous space,
<0x2d000000 0x1000>
Just to i.MX8ULP could reuse the vf610 gpio driver, the regs are partitioned
into two with some offset added

So from hw, I think they are not compatible, just some sw tricks to
make the linux driver could work for both platform.

Thanks,
Peng.

>
> Best regards,
> Krzysztof

2023-09-15 05:41:02

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry

On 14/09/2023 11:08, Peng Fan wrote:
>> Subject: Re: [PATCH 3/5] gpio: vf610: add i.MX8ULP of_device_id entry
>>
>> On 14/09/2023 10:48, Linus Walleij wrote:
>>> On Thu, Sep 14, 2023 at 7:48 AM Krzysztof Kozlowski
>>> <[email protected]> wrote:
>>>> On 14/09/2023 04:20, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <[email protected]>
>>>>>
>>>>> i.MX8ULP supports two interrupts, while i.MX7ULP supports one
>> interrupt.
>>>>> So from hardware perspective, they are not compatible.
>>>>>
>>>>> So add entry for i.MX8ULP.
>>>>>
>>>>> Signed-off-by: Peng Fan <[email protected]>
>>>>> ---
>>>>> drivers/gpio/gpio-vf610.c | 1 +
>>>>> 1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
>>>>> index dbc7ba0ee72c..88f7215cdf4b 100644
>>>>> --- a/drivers/gpio/gpio-vf610.c
>>>>> +++ b/drivers/gpio/gpio-vf610.c
>>>>> @@ -67,6 +67,7 @@ static const struct fsl_gpio_soc_data imx_data = {
>>>>> static const struct of_device_id vf610_gpio_dt_ids[] = {
>>>>> { .compatible = "fsl,vf610-gpio", .data = NULL, },
>>>>> { .compatible = "fsl,imx7ulp-gpio", .data = &imx_data, },
>>>>> + { .compatible = "fsl,imx8ulp-gpio", .data = &imx_data, },
>>>>
>>>> Why? It is the same as imx7. No need.
>>>
>>> Because compatible = "fsl,imx7ulp-gpio" is not what is going to be in
>>> the device tree, but compatible = "fsl,imx8ulp-gpio"?
>>>
>>> What am I missing here? Maybe the commit message is weird.
>>>
>>
>> If the devices used before and are still going to use same driver data, they
>> look compatible from OS point of view. Therefore usually we express such
>> compatibility and do not add unneeded device_id entries.
>>
>> Now whether the devices are truly compatible or not, I don't know and with
>> some recent emails I am bit confused.
>
> Some tricks in dtb are made to make the driver could work for both i.MX7ULP,
> i.MX8ULP, i.MX93 with fsl,imx7ulp-gpio.
>
> Such as i.MX8ULP:
> reg = <0x2d000080 0x1000>, <0x2d000040 0x40>;
> Actually the two regs are belong to one physical continuous space,
> <0x2d000000 0x1000>
> Just to i.MX8ULP could reuse the vf610 gpio driver, the regs are partitioned
> into two with some offset added
>
> So from hw, I think they are not compatible, just some sw tricks to
> make the linux driver could work for both platform.

OK, that sounds like reason to clean this up - drivers, bindings and
finally DTS - all with proper explanation justifying affected DTS users.

Best regards,
Krzysztof