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 > >>
