mbox series

[v3,0/2] gpio: loongson: add firmware offset parse support

Message ID 20230807074043.31288-1-zhuyinbo@loongson.cn
Headers show
Series gpio: loongson: add firmware offset parse support | expand

Message

Yinbo Zhu Aug. 7, 2023, 7:40 a.m. UTC 
Add some GPIO register offset value parse support for device properties
allowing to specify them in ACPI or DT.

Change in v3:
		1. Reword the dt-bindings patch commit log information.
		2. Add "loongson,ls2k1000-gpio" compatible.
Change in v2:
		1. Reword the patch commit log information.
		2. Add some GPIO register offset description in yaml.

Yinbo Zhu (2):
  gpio: dt-bindings: add parsing of loongson gpio offset
  gpio: loongson: add firmware offset parse support

 .../bindings/gpio/loongson,ls-gpio.yaml       | 40 +++++++++-
 drivers/gpio/gpio-loongson-64bit.c            | 74 +++++++++++++++++--
 2 files changed, 106 insertions(+), 8 deletions(-)

Comments

Linus Walleij Aug. 10, 2023, 8:27 a.m. UTC | #1 
Hi Yinbo,

thanks for your patch!

On Mon, Aug 7, 2023 at 9:41 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:

> Loongson GPIO controllers come in multiple variants that are compatible
> except for certain register offset values.  Add support for device
> properties allowing to specify them in ACPI or DT.
>
> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>

(...)
> @@ -26,6 +26,7 @@ struct loongson_gpio_chip_data {
>         unsigned int            conf_offset;
>         unsigned int            out_offset;
>         unsigned int            in_offset;
> +       unsigned int            inten_offset;

Consider just changing all of these from unsigned int to u32.

(...)
> +       if (device_property_read_u32(dev, "loongson,gpio-conf-offset", (u32 *)&d->conf_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-in-offset", (u32 *)&d->in_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-out-offset", (u32 *)&d->out_offset)
> +           || device_property_read_u32(dev, "loongson,gpio-ctrl-mode", (u32 *)&d->mode))

Because then you can get rid of this annoying forest of cast.

I'm fine with doing this change in this patch without a need for a separate
refactoring, as it's just a contained driver and clearly just about typing.

Yours,
Linus Walleij
Yinbo Zhu Aug. 11, 2023, 3:50 a.m. UTC | #2 
在 2023/8/10 下午4:27, Linus Walleij 写道:
> Hi Yinbo,
> 
> thanks for your patch!
> 
> On Mon, Aug 7, 2023 at 9:41 AM Yinbo Zhu <zhuyinbo@loongson.cn> wrote:
> 
>> Loongson GPIO controllers come in multiple variants that are compatible
>> except for certain register offset values.  Add support for device
>> properties allowing to specify them in ACPI or DT.
>>
>> Signed-off-by: Yinbo Zhu <zhuyinbo@loongson.cn>
> 
> (...)
>> @@ -26,6 +26,7 @@ struct loongson_gpio_chip_data {
>>          unsigned int            conf_offset;
>>          unsigned int            out_offset;
>>          unsigned int            in_offset;
>> +       unsigned int            inten_offset;
> 
> Consider just changing all of these from unsigned int to u32.


okay, I got it.

> 
> (...)
>> +       if (device_property_read_u32(dev, "loongson,gpio-conf-offset", (u32 *)&d->conf_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-in-offset", (u32 *)&d->in_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-out-offset", (u32 *)&d->out_offset)
>> +           || device_property_read_u32(dev, "loongson,gpio-ctrl-mode", (u32 *)&d->mode))
> 
> Because then you can get rid of this annoying forest of cast.


Change offset to u32 and here still need use a (u32 *) cast, because the
chip_data is const type so &chip_data->offset will be (const u32 *) type
and need a (u32 *) cast.

> 
> I'm fine with doing this change in this patch without a need for a separate
> refactoring, as it's just a contained driver and clearly just about typing.


okay, I got it.

Thanks,
Yinbo.
Andy Shevchenko Aug. 14, 2023, 9:48 p.m. UTC | #3 
Mon, Aug 07, 2023 at 03:40:43PM +0800, Yinbo Zhu kirjoitti:
> Loongson GPIO controllers come in multiple variants that are compatible
> except for certain register offset values.  Add support for device
> properties allowing to specify them in ACPI or DT.

> +	if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
> +		return -EINVAL;
> +
> +	ret = DIV_ROUND_UP(ngpios, 8);
> +	switch (ret) {
> +	case 1 ... 2:
> +		io_width = ret;
> +		break;
> +	case 3 ... 4:
> +		io_width = 0x4;
> +		break;
> +	case 5 ... 8:
> +		io_width = 0x8;
> +		break;
> +	default:
> +		dev_err(dev, "unsupported io width\n");
> +		return -EINVAL;
> +	}

Why? We have bgpio_init() handle this.
https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=55b2395e4e92adc492c6b30ac109eb78250dcd9d

...

> +	lgpio->chip.can_sleep = 0;

It's boolean, use boolean initializer.

...

> +	if (lgpio->chip_data->label)
> +		lgpio->chip.label = lgpio->chip_data->label;
> +	else
> +		lgpio->chip.label = kstrdup(to_platform_device(dev)->name, GFP_KERNEL);

No error check? Not a devm_*() variant, so leaking memory?

...

> +	{
> +		.id = "LOON0007",
> +	},

How does DSDT excerpt for this device look like?
Yinbo Zhu Aug. 23, 2023, 7:36 a.m. UTC | #4 
Hi andy,

Sorry, I lost this email due to issues with my company's email server. I
will adopt your suggestion in v5 version.

在 2023/8/15 上午5:48, andy.shevchenko@gmail.com 写道:
> Mon, Aug 07, 2023 at 03:40:43PM +0800, Yinbo Zhu kirjoitti:
>> Loongson GPIO controllers come in multiple variants that are compatible
>> except for certain register offset values.  Add support for device
>> properties allowing to specify them in ACPI or DT.
> 
>> +	if (device_property_read_u32(dev, "ngpios", &ngpios) || !ngpios)
>> +		return -EINVAL;
>> +
>> +	ret = DIV_ROUND_UP(ngpios, 8);
>> +	switch (ret) {
>> +	case 1 ... 2:
>> +		io_width = ret;
>> +		break;
>> +	case 3 ... 4:
>> +		io_width = 0x4;
>> +		break;
>> +	case 5 ... 8:
>> +		io_width = 0x8;
>> +		break;
>> +	default:
>> +		dev_err(dev, "unsupported io width\n");
>> +		return -EINVAL;
>> +	}
> 
> Why? We have bgpio_init() handle this.
> https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/commit/?h=gpio/for-next&id=55b2395e4e92adc492c6b30ac109eb78250dcd9d


okay, I got it.

> 
> ...
> 
>> +	lgpio->chip.can_sleep = 0;
> 
> It's boolean, use boolean initializer.


okay, I got it.

> 
> ...
> 
>> +	if (lgpio->chip_data->label)
>> +		lgpio->chip.label = lgpio->chip_data->label;
>> +	else
>> +		lgpio->chip.label = kstrdup(to_platform_device(dev)->name, GFP_KERNEL);
> 
> No error check? Not a devm_*() variant, so leaking memory?


This code had been removed in v4.

> 
> ...
> 
>> +	{
>> +		.id = "LOON0007",
>> +	},
> 
> How does DSDT excerpt for this device look like?


LOON0007 and LOON000A are similar, LOON000A is for 2k2000 gpio0.

      Device (GPO0)
         {
             Name (_HID, "LOON000A")  // _HID: Hardware ID
             Name (_ADR, Zero)  // _ADR: Address
             Name (_UID, One)  // _UID: Unique ID
             Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource 
Settings
             {
                 QWordMemory (ResourceConsumer, PosDecode, MinFixed, 
MaxFixed, NonCacheable, ReadWrite,
                     0x0000000000000000, // Granularity
                     0x000000001FE00500, // Range Minimum
                     0x000000001FE00520, // Range Maximum
                     0x0000000000000000, // Translation Offset
                     0x0000000000000021, // Length
                     ,, , AddressRangeMemory, TypeStatic)
             })
             Name (_DSD, Package (0x02)  // _DSD: Device-Specific Data
             {
                 ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301") /* 
Device Properties for _DSD */,
                 Package (0x01)
                 {
                     Package (0x02)
                     {
                         "ngpios",
                         0x20
                     }
                 }
             })
         }


Thanks,
Yinbo