Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48583 )
Change subject: soc/intel: hook up new gpio device in the soc chips ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/chi... File src/soc/intel/alderlake/chip.c:
https://review.coreboot.org/c/coreboot/+/48583/7/src/soc/intel/alderlake/chi... PS7, Line 202: else if (dev->path.type == DEVICE_PATH_GPIO) : dev->ops = &soc_gpio_ops;
It also relieves each SoC from duplicating the same code to set up GPIO device ops.
Nico's solution does, too
You have to add this to every SoC chip.c as done in the latest patchset which wouldn't be required if the GPIO block had its own chip:
else if (dev->path.type == DEVICE_PATH_GPIO) block_gpio_enable(dev);
Well, a chip would then require such construct:
chip soc/outel/whateverlake
chip soc/intel/common/block/gpio device generic 0 alias soc_gpio on end end
chip drivers/iamachip chip drivers/iamachip/gpio device generic 0 alias whereever_gpio on end end end
chip drivers/ipmi use whereever_gpio as gpio end
chip drivers/sample use soc_gpio as gpio end end
... instead of:
chip soc/outel/whateverlake
device gpio 0 alias soc_gpio on end
chip drivers/iamachip device gpio 0 alias whereever_gpio on end end
chip drivers/ipmi use whereever_gpio as gpio end
chip drivers/sample use soc_gpio as gpio end end
Huh? block gpio is common code without any soc specific options, isn't it?
Yes, it is. What I meant is if in the future, the common code required any configs to be set by mainboard or specific SoC, then it can have its own soc_intel_common_block_gpio_config {} rather than requiring each SoC config to add these.
Then maybe let's revisit that, when it's actually required?