Hi Fabian, Jacopo On 07/04/2021 14:19, Fabian Wüthrich wrote: > That's a good point but I'm also not experienced enough to answer this question. > > It looks like that these SSDB buffers are device specific and every vendor has its > own memory layout. So parsing these in the core could be difficult. > > Maybe Dan knows more... I think the cio2-bridge is the right place to handle it; as far as we know that buffer only appears on Intel skl/kbl devices that have an IPU3 for the cameras to connect to. To say it's all a bit outside the normal ACPI specifications would be an understatement, so this isn't a solution for ACPI generally but rather just for a few specific product ranges. > On 07.04.21 14:41, Jacopo Mondi wrote: >> Hi Fabian, >> >> On Wed, Apr 07, 2021 at 01:52:31PM +0200, Fabian Wüthrich wrote: >>> >>> On 07.04.21 10:22, Jacopo Mondi wrote: >>>> Hi Fabian, >>>> >>>> On Sun, Mar 21, 2021 at 08:11:56PM +0100, Fabian Wüthrich wrote: >>>>> The sensor orientation is read from the _PLC ACPI buffer and converted >>>>> to a v4l2 format. >>>>> >>>>> See https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf >>>>> page 351 for a definition of the Panel property. >>>>> >>>>> The sensor rotation is read from the SSDB ACPI buffer and converted into >>>>> degrees. >>>>> >>>> The framework has v4l2_fwnode_device_parse() in v4l2-fwnode.c which >>>> works for DT based systems. Could support for retreiving those >>>> properties from the SSDB block be added there ? >>>> >>>> Thanks >>>> j >>>> >>> This is exactly the purpose of this patch. I've should have been more precise >>> in the commit message but basically this patch converts the properties from >>> the SSDB block into fwnodes which are then picked up by v4l2_fwnode_device_parse(). >> Ah, right! The cio2_bridge_create_fwnode_properties() should have >> hinted me... My question was more on the lines of: "Instead of parsing >> the SSDB block in the CIO2 driver to translate into v4l2-fwnode >> consumable properties, can the parsing be done directly into the >> core" due to my poor understanding of the swnode infrastructure. >> >> Thanks for clarifying >> >>> P.S. I have a quick question below >>> >>>>> Signed-off-by: Fabian Wüthrich <me@xxxxxxxx> >>>>> --- >>>>> drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++-- >>>>> drivers/media/pci/intel/ipu3/cio2-bridge.h | 16 ++++++ >>>>> 2 files changed, 72 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c >>>>> index c2199042d3db..503809907b92 100644 >>>>> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.c >>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c >>>>> @@ -29,6 +29,7 @@ static const struct cio2_sensor_config cio2_supported_sensors[] = { >>>>> static const struct cio2_property_names prop_names = { >>>>> .clock_frequency = "clock-frequency", >>>>> .rotation = "rotation", >>>>> + .orientation = "orientation", >>>>> .bus_type = "bus-type", >>>>> .data_lanes = "data-lanes", >>>>> .remote_endpoint = "remote-endpoint", >>>>> @@ -72,11 +73,51 @@ static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id, >>>>> return ret; >>>>> } >>>>> >>>>> +static u32 cio2_bridge_parse_rotation(struct cio2_sensor *sensor) >>>>> +{ >>>>> + switch (sensor->ssdb.degree) { >>>>> + case CIO2_SENSOR_ROTATION_NORMAL: >>>>> + return 0; >>>>> + case CIO2_SENSOR_ROTATION_INVERTED: >>>>> + return 180; >>>>> + default: >>>>> + dev_warn(&sensor->adev->dev, >>>>> + "Unknown rotation %d. Assume 0 degree rotation\n", >>>>> + sensor->ssdb.degree); >>>>> + return 0; >>>>> + } >>>>> +} >>>>> + >>>>> +static enum v4l2_fwnode_orientation cio2_bridge_parse_orientation(struct cio2_sensor *sensor) >>>>> +{ >>>>> + switch (sensor->pld->panel) { >>>>> + case CIO2_PLD_PANEL_FRONT: >>>>> + return V4L2_FWNODE_ORIENTATION_FRONT; >>>>> + case CIO2_PLD_PANEL_BACK: >>>>> + return V4L2_FWNODE_ORIENTATION_BACK; >>>>> + case CIO2_PLD_PANEL_TOP: >>>>> + case CIO2_PLD_PANEL_LEFT: >>>>> + case CIO2_PLD_PANEL_RIGHT: >>>>> + case CIO2_PLD_PANEL_UNKNOWN: >>>>> + return V4L2_FWNODE_ORIENTATION_EXTERNAL; >>>>> + default: >>>>> + dev_warn(&sensor->adev->dev, "Unknown _PLD panel value %d\n", >>>>> + sensor->pld->panel); >>>>> + return V4L2_FWNODE_ORIENTATION_EXTERNAL; >>>>> + } >>>>> +} >>> These constants are ACPI related and shouldn't be here. Should I move them to e.g. >>> include/acpi/acbuffer.h or do you know a better place? >>> >> I know very few things about ACPI, don't think I'm the right person to >> answer that question :) >> >> Although as I read the definitions of ACPI_PLD_GET_ROTATION() and >> ACPI_PLD_GET_PANEL() and see they match the bit offset as reported by >> the ACPI specs you linked here above, I would say your suggestion >> makes sense, but please confirm with someone that knows better :) >> >> Thanks >> j >> >>>>> + >>>>> static void cio2_bridge_create_fwnode_properties( >>>>> struct cio2_sensor *sensor, >>>>> struct cio2_bridge *bridge, >>>>> const struct cio2_sensor_config *cfg) >>>>> { >>>>> + u32 rotation; >>>>> + enum v4l2_fwnode_orientation orientation; >>>>> + >>>>> + rotation = cio2_bridge_parse_rotation(sensor); >>>>> + orientation = cio2_bridge_parse_orientation(sensor); >>>>> + >>>>> sensor->prop_names = prop_names; >>>>> >>>>> sensor->local_ref[0].node = &sensor->swnodes[SWNODE_CIO2_ENDPOINT]; >>>>> @@ -85,9 +126,12 @@ static void cio2_bridge_create_fwnode_properties( >>>>> sensor->dev_properties[0] = PROPERTY_ENTRY_U32( >>>>> sensor->prop_names.clock_frequency, >>>>> sensor->ssdb.mclkspeed); >>>>> - sensor->dev_properties[1] = PROPERTY_ENTRY_U8( >>>>> + sensor->dev_properties[1] = PROPERTY_ENTRY_U32( >>>>> sensor->prop_names.rotation, >>>>> - sensor->ssdb.degree); >>>>> + rotation); >>>>> + sensor->dev_properties[2] = PROPERTY_ENTRY_U32( >>>>> + sensor->prop_names.orientation, >>>>> + orientation); >>>>> >>>>> sensor->ep_properties[0] = PROPERTY_ENTRY_U32( >>>>> sensor->prop_names.bus_type, >>>>> @@ -159,6 +203,7 @@ static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge) >>>>> for (i = 0; i < bridge->n_sensors; i++) { >>>>> sensor = &bridge->sensors[i]; >>>>> software_node_unregister_nodes(sensor->swnodes); >>>>> + ACPI_FREE(sensor->pld); >>>>> acpi_dev_put(sensor->adev); >>>>> } >>>>> } >>>>> @@ -170,6 +215,7 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg, >>>>> struct fwnode_handle *fwnode; >>>>> struct cio2_sensor *sensor; >>>>> struct acpi_device *adev; >>>>> + acpi_status status; >>>>> int ret; >>>>> >>>>> for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) { >>>>> @@ -193,11 +239,15 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg, >>>>> if (ret) >>>>> goto err_put_adev; >>>>> >>>>> + status = acpi_get_physical_device_location(adev->handle, &sensor->pld); >>>>> + if (ACPI_FAILURE(status)) >>>>> + goto err_put_adev; >>>>> + >>>>> if (sensor->ssdb.lanes > CIO2_MAX_LANES) { >>>>> dev_err(&adev->dev, >>>>> "Number of lanes in SSDB is invalid\n"); >>>>> ret = -EINVAL; >>>>> - goto err_put_adev; >>>>> + goto err_free_pld; >>>>> } >>>>> >>>>> cio2_bridge_create_fwnode_properties(sensor, bridge, cfg); >>>>> @@ -205,7 +255,7 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg, >>>>> >>>>> ret = software_node_register_nodes(sensor->swnodes); >>>>> if (ret) >>>>> - goto err_put_adev; >>>>> + goto err_free_pld; >>>>> >>>>> fwnode = software_node_fwnode(&sensor->swnodes[ >>>>> SWNODE_SENSOR_HID]); >>>>> @@ -226,6 +276,8 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg, >>>>> >>>>> err_free_swnodes: >>>>> software_node_unregister_nodes(sensor->swnodes); >>>>> +err_free_pld: >>>>> + ACPI_FREE(sensor->pld); >>>>> err_put_adev: >>>>> acpi_dev_put(sensor->adev); >>>>> err_out: >>>>> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h >>>>> index dd0ffcafa489..e1e388cc9f45 100644 >>>>> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.h >>>>> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h >>>>> @@ -12,6 +12,19 @@ >>>>> #define CIO2_MAX_LANES 4 >>>>> #define MAX_NUM_LINK_FREQS 3 >>>>> >>>>> +/* Values are estimated guesses as we don't have a spec */ >>>>> +#define CIO2_SENSOR_ROTATION_NORMAL 0 >>>>> +#define CIO2_SENSOR_ROTATION_INVERTED 1 >>>>> + >>>>> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */ >>>>> +#define CIO2_PLD_PANEL_TOP 0 >>>>> +#define CIO2_PLD_PANEL_BOTTOM 1 >>>>> +#define CIO2_PLD_PANEL_LEFT 2 >>>>> +#define CIO2_PLD_PANEL_RIGHT 3 >>>>> +#define CIO2_PLD_PANEL_FRONT 4 >>>>> +#define CIO2_PLD_PANEL_BACK 5 >>>>> +#define CIO2_PLD_PANEL_UNKNOWN 6 >>>>> + >>>>> #define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \ >>>>> (const struct cio2_sensor_config) { \ >>>>> .hid = _HID, \ >>>>> @@ -80,6 +93,7 @@ struct cio2_sensor_ssdb { >>>>> struct cio2_property_names { >>>>> char clock_frequency[16]; >>>>> char rotation[9]; >>>>> + char orientation[12]; >>>>> char bus_type[9]; >>>>> char data_lanes[11]; >>>>> char remote_endpoint[16]; >>>>> @@ -106,6 +120,8 @@ struct cio2_sensor { >>>>> struct cio2_node_names node_names; >>>>> >>>>> struct cio2_sensor_ssdb ssdb; >>>>> + struct acpi_pld_info *pld; >>>>> + >>>>> struct cio2_property_names prop_names; >>>>> struct property_entry ep_properties[5]; >>>>> struct property_entry dev_properties[3]; >>>>> -- >>>>> 2.31.0 >>>>>
