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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 9 Jul 2021 18:40:00 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Daniel Scally <djrscally@...il.com>
Cc:     linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        hdegoede@...hat.com, mgross@...ux.intel.com,
        luzmaximilian@...il.com, lgirdwood@...il.com,
        andy.shevchenko@...il.com, laurent.pinchart@...asonboard.com,
        kieran.bingham@...asonboard.com
Subject: Re: [RFC PATCH 2/2] platform/surface: Add Surface Go 2 board file

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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)