Hi Dan, Thanks for the review. On 22.03.21 01:19, Daniel Scally wrote: > Hi Fabian, thanks for doing this > > On 21/03/2021 19:11, 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. >> >> Signed-off-by: Fabian Wüthrich <me@xxxxxxxx> > > Couple of comments below, but after addressing those: > > > Reviewed-by: Daniel Scally <djrscally@xxxxxxxxx> > >> --- >> 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; >> + } >> +} >> + >> 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 */ > > > Educated guesses? > Yes. >> +#define CIO2_SENSOR_ROTATION_NORMAL 0 >> +#define CIO2_SENSOR_ROTATION_INVERTED 1 >> + > > > I think these are good here but... > >> +/* 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 >> + > > > ...I wonder if these ought to go somewhere in the include/acpi headers. > You might be the only person to refer to pld->panel in driver code (at > least a quick grep doesn't show me another one, and only another couple > of uses of pld at all) so it's probably not a big deal, but it just > feels slightly the wrong place. What do you think? > I agree. What about include/acpi/acbuffer.h? But I don't know if this hinders the acceptance of this patch. >> #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]; > > > You should extend dev_properties to 4 members; there needs to be an > empty entry as a terminator or it'll be a problem in the event someone > tries to access a property that isn't there. > Good catch. Thanks I missed that.
