Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 Jan 2022 09:33:25 +0200
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Won Chung <wonchung@google.com>
Cc:     "Rafael J . Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Benson Leung <bleung@chromium.org>,
        Prashant Malani <pmalani@chromium.org>,
        linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] ACPI: device_sysfs: Add sysfs support for _PLD
Message-ID: <YfeQxYNzWltRQ7mq@kuha.fi.intel.com>
References: <20220128180832.2329149-1-wonchung@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220128180832.2329149-1-wonchung@google.com>
Precedence: bulk

On Fri, Jan 28, 2022 at 06:08:32PM +0000, Won Chung wrote:
> When ACPI table includes _PLD fields for a device, create a new file
> (pld) in sysfs to share _PLD fields.
> 
> Signed-off-by: Won Chung <wonchung@google.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-acpi | 53 ++++++++++++++++++++++++
>  drivers/acpi/device_sysfs.c              | 42 +++++++++++++++++++
>  2 files changed, 95 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> index 58abacf59b2a..3a9c6a4f4603 100644
> --- a/Documentation/ABI/testing/sysfs-bus-acpi
> +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> @@ -96,3 +96,56 @@ Description:
>  		hardware, if the _HRV control method is present.  It is mostly
>  		useful for non-PCI devices because lspci can list the hardware
>  		version for PCI devices.
> +
> +What:		/sys/bus/acpi/devices/.../pld
> +Date:		Jan, 2022
> +Contact:	Won Chung <wonchung@google.com>
> +Description:
> +		This attribute contains the output of the device object's
> +		_PLD control method, if present. This information provides
> +		details on physical location of a port.
> +
> +		Description on each _PLD field from ACPI specification:
> +
> +		===============	============================================
> +		GROUP_TOKEN	Unique numerical value identifying a group.
> +		GROUP_POSITION	Identifies this device connection point’s
> +				position in the group.
> +		USER_VISIBLE	Set if the device connection point can be
> +				seen by the user without disassembly.
> +		DOCK		Set if the device connection point resides
> +				in a docking station or port replicator.
> +		BAY		Set if describing a device in a bay or if
> +				device connection point is a bay.
> +		LID		Set if this device connection point resides
> +				on the lid of laptop system.
> +		PANEL		Describes which panel surface of the system’s
> +				housing the device connection point resides on:
> +				0 - Top
> +				1 - Bottom
> +				2 - Left
> +				3 - Right
> +				4 - Front
> +				5 - Back
> +				6 - Unknown (Vertical Position and Horizontal
> +				Position will be ignored)
> +		HORIZONTAL_	0 - Left
> +		POSITION	1 - Center
> +				2 - Right
> +		VERTICAL_	0 - Upper
> +		POSITION	1 - Center
> +				2 - Lower
> +		SHAPE		Describes the shape of the device connection
> +				point.
> +				0 - Round
> +				1 - Oval
> +				2 - Square
> +				3 - Vertical Rectangle
> +				4 - Horizontal Rectangle
> +				5 - Vertical Trapezoid
> +				6 - Horizontal Trapezoid
> +				7 - Unknown - Shape rendered as a Rectangle
> +				with dotted lines
> +				8 - Chamfered
> +				15:9 - Reserved
> +		===============	===============================================
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index d5d6403ba07b..8d4df5fb1c45 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -509,6 +509,40 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(status);
>  
> +static ssize_t pld_show(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct acpi_device *acpi_dev = to_acpi_device(dev);
> +	acpi_status status;
> +	struct acpi_pld_info *pld;
> +
> +	status = acpi_get_physical_device_location(acpi_dev->handle, &pld);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	return sprintf(buf, "GROUP_TOKEN=%u\n"
> +		"GROUP_POSITION=%u\n"
> +		"USER_VISIBLE=%u\n"
> +		"DOCK=%u\n"
> +		"BAY=%u\n"
> +		"LID=%u\n"
> +		"PANEL=%u\n"
> +		"HORIZONTAL_POSITION=%u\n"
> +		"VERTICAL_POSITION=%u\n"
> +		"SHAPE=%u\n",
> +		pld->group_token,
> +		pld->group_position,
> +		pld->user_visible,
> +		pld->dock,
> +		pld->bay,
> +		pld->lid,
> +		pld->panel,
> +		pld->horizontal_position,
> +		pld->vertical_position,
> +		pld->shape);
> +}
> +static DEVICE_ATTR_RO(pld);

Why not have a pld group (directory) and a separate attribute file for
each field?


thanks,

-- 
heikki