On Wed, Feb 2, 2022 at 9:43 PM Won Chung <wonchung@xxxxxxxxxx> wrote:
>
> Hi Heikki,
>
> Thank you for the review!
>
> On Wed, Feb 2, 2022 at 12:19 AM Heikki Krogerus
> <heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
> >
> > Hi Won,
> >
> > On Tue, Feb 01, 2022 at 01:55:18AM +0000, Won Chung wrote:
> > > When ACPI table includes _PLD fields for a device, create a new
> > > directory (pld) in sysfs to share _PLD fields.
> >
> > I think you need to explain what needs this information in user space.
> >
> > > Signed-off-by: Won Chung <wonchung@xxxxxxxxxx>
> > > ---
> > > Documentation/ABI/testing/sysfs-bus-acpi | 107 +++++++++++++++++++++++
> > > drivers/acpi/device_sysfs.c | 55 ++++++++++++
> > > include/acpi/acpi_bus.h | 1 +
> > > 3 files changed, 163 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-acpi b/Documentation/ABI/testing/sysfs-bus-acpi
> > > index 58abacf59b2a..b8b71c8f3cfd 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-acpi
> > > +++ b/Documentation/ABI/testing/sysfs-bus-acpi
> > > @@ -96,3 +96,110 @@ 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: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + This directory contains the output of the device object's _PLD
> > > + control method, if present. This information provides details
> > > + on physical location of a device.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/revision
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + The current revision is 0x2.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/group_token
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Unique numerical value identifying a group.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/group_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Identifies this device connection point’s position in the group.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/user_visible
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Set if the device connection point can be seen by the user
> > > + without disassembly.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/dock
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Set if the device connection point resides in a docking station
> > > + or port replicator.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/bay
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Set if describing a device in a bay or if device connection
> > > + point is a bay.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/lid
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + Set if this device connection point resides on the lid of
> > > + laptop system.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/panel
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + 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)
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/vertical_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + 0 - Upper
> > > + 1 - Center
> > > + 2 - Lower
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/horizontal_position
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + ACPI specification does not define horizontal position field.
> > > + Can be used as either
> > > + 0 - Left
> > > + 1 - Center
> > > + 2 - Right
> > > + or
> > > + 0 - Leftmost
> > > + and higher numbers going toward the right.
> > > +
> > > +What: /sys/bus/acpi/devices/.../pld/shape
> > > +Date: Feb, 2022
> > > +Contact: Won Chung <wonchung@xxxxxxxxxx>
> > > +Description:
> > > + 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..610be93635a0 100644
> > > --- a/drivers/acpi/device_sysfs.c
> > > +++ b/drivers/acpi/device_sysfs.c
> > > @@ -509,6 +509,49 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > > }
> > > static DEVICE_ATTR_RO(status);
> > >
> > > +#define DEV_ATTR_PLD_PROP(prop) \
> > > + static ssize_t prop##_show(struct device *dev, struct device_attribute *attr, \
> > > + char *buf) \
> > > +{ \
> > > + struct acpi_device *acpi_dev = to_acpi_device(dev); \
> > > + if (acpi_dev->pld == NULL) \
> > > + return -EIO; \
> > > + return sprintf(buf, "%u\n", acpi_dev->pld->prop); \
> > > +}; \
> >
> > Ah, you are storing the _PLD below. Before there were concerns about
> > the memory that the cached _PLD information would consume. Another way
> > of doing this would be to just always evaluate the _PLD here.
> >
> > Rafael needs to comment on this. My personal opinion is that let's
> > just store the thing.
> >
>
> By "always evaluate the _PLD here", you mean something like
> acpi_get_physical_device_location(dev->handle, &pld)
> for every _PLD field, right?
>
> I will wait for Rafael's comment on this.
So I would like to get back to the very beginning: Do you need full
_PLD output to address the issue at hand?
If so, do you really need it for all devices that have _PLD?
If not, then why waste memory for all that stuff?
