Re: [RFC PATCH 2/2] platform/surface: Add Surface Go 2 board file

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On Thu, Jul 08, 2021 at 11:42:26PM +0100, Daniel Scally wrote:

> The DSDT tables on the Surface Go 2 contain no power control methods for
> the world facing camera, so the regulator, gpio and clk frameworks have no
> way to discover the appropriate connections and settings.

Does anything actually need these connections or settings?

> +static const struct property_entry core_properties[] = {
> +	PROPERTY_ENTRY_STRING("regulator-name", "CORE"),
> +	PROPERTY_ENTRY_U32("regulator-min-microvolt", 1200000),
> +	PROPERTY_ENTRY_U32("regulator-max-microvolt", 1200000),
> +	{ }
> +};

Does anything actually care about the voltages here - why specify them?
As far as I can tell from grepping all the supplies you're adding are
digital.

> +static const struct software_node regulator_nodes[] = {
> +	{"ANA", &tps68470_node, ana_properties},
> +	{"VSIO", &tps68470_node, vsio_properties},
> +	{"CORE", &tps68470_node, core_properties},
> +};

> +static const struct property_entry sensor_properties[] = {
> +	PROPERTY_ENTRY_REF("avdd-supply", &regulator_nodes[0]),
> +	PROPERTY_ENTRY_REF("dovdd-supply", &regulator_nodes[1]),
> +	PROPERTY_ENTRY_REF("dvdd-supply", &regulator_nodes[2]),
> +	{ }

I would strongly caution against this idiom of referencing elements in
other arrays using magic numbers, it is both hard to read and error
prone when someone goes in making changes.  If they need to be in arrays
then constants for the array indexes would help a lot.

> +static struct software_node sensor_node = {
> +	.name		= "INT347A",
> +	.properties	= sensor_properties,
> +};
> +
> +static struct gpiod_lookup_table surface_go_2_gpios = {
> +	.dev_id = "i2c-INT347A:00",
> +	.table = {
> +		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
> +		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW)
> +	}
> +};

This appears to be using regular board file stuff for GPIOs, if this
swnode stuff is somehow needed for regulators why is it not also needed
for GPIOs?  I think this is going back to the thing I was saying earlier
about not really understanding the problem being solved here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]