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.
> > +static DEVICE_ATTR_RO(prop)
> > +
> > +DEV_ATTR_PLD_PROP(revision);
> > +DEV_ATTR_PLD_PROP(group_token);
> > +DEV_ATTR_PLD_PROP(group_position);
> > +DEV_ATTR_PLD_PROP(user_visible);
> > +DEV_ATTR_PLD_PROP(dock);
> > +DEV_ATTR_PLD_PROP(bay);
> > +DEV_ATTR_PLD_PROP(lid);
> > +DEV_ATTR_PLD_PROP(panel);
> > +DEV_ATTR_PLD_PROP(vertical_position);
> > +DEV_ATTR_PLD_PROP(horizontal_position);
> > +DEV_ATTR_PLD_PROP(shape);
> > +
> > +static struct attribute *dev_attr_pld[] = {
> > + &dev_attr_revision.attr,
> > + &dev_attr_group_token.attr,
> > + &dev_attr_group_position.attr,
> > + &dev_attr_user_visible.attr,
> > + &dev_attr_dock.attr,
> > + &dev_attr_bay.attr,
> > + &dev_attr_lid.attr,
> > + &dev_attr_panel.attr,
> > + &dev_attr_vertical_position.attr,
> > + &dev_attr_horizontal_position.attr,
> > + &dev_attr_shape.attr,
> > + NULL,
> > +};
> > +
> > +static struct attribute_group dev_attr_pld_group = {
> > + .name = "pld",
> > + .attrs = dev_attr_pld,
> > +};
> > +
> > /**
> > * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> > * @dev: ACPI device object.
> > @@ -595,6 +638,16 @@ int acpi_device_setup_files(struct acpi_device *dev)
> > &dev_attr_real_power_state);
> > }
> >
> > + if (acpi_has_method(dev->handle, "_PLD")) {
> > + status = acpi_get_physical_device_location(dev->handle,
> > + &dev->pld);
> > + if (ACPI_FAILURE(status))
> > + goto end;
> > + result = device_add_group(&dev->dev, &dev_attr_pld_group);
> > + if (result)
> > + goto end;
> > + }
>
> You probable want to store the pld in acpi_store_pld_crc(). Perhaps
> also rename that function to just acpi_store_pld() at the same time.
>
> Here you just want to create the sysfs group.
>
Got it. I will send v5 with this change.
> > acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
> >
> > end:
> > @@ -645,4 +698,6 @@ void acpi_device_remove_files(struct acpi_device *dev)
> > device_remove_file(&dev->dev, &dev_attr_status);
> > if (dev->handle)
> > device_remove_file(&dev->dev, &dev_attr_path);
> > + if (acpi_has_method(dev->handle, "_PLD"))
> > + device_remove_group(&dev->dev, &dev_attr_pld_group);
> > }
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index ca88c4706f2b..929e726a666b 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -381,6 +381,7 @@ struct acpi_device {
> > struct acpi_hotplug_context *hp;
> > struct acpi_driver *driver;
> > const struct acpi_gpio_mapping *driver_gpios;
> > + struct acpi_pld_info *pld;
> > void *driver_data;
> > struct device dev;
> > unsigned int physical_node_count;
> > --
>
> thanks,
>
> --
> heikki
Thank you,
Won
