* [PATCH] ipu3-cio2: Parse sensor orientation and rotation
@ 2021-03-21 19:11 Fabian Wüthrich
2021-03-22 0:19 ` Daniel Scally
2021-04-07 8:22 ` Jacopo Mondi
0 siblings, 2 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-03-21 19:11 UTC (permalink / raw)
To: linux-media
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally, Tianshu Qiu,
Mauro Carvalho Chehab, Fabian Wüthrich
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@fabwu.ch>
---
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 */
+#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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-03-21 19:11 [PATCH] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
@ 2021-03-22 0:19 ` Daniel Scally
2021-03-22 15:16 ` Fabian Wüthrich
2021-04-07 8:22 ` Jacopo Mondi
1 sibling, 1 reply; 26+ messages in thread
From: Daniel Scally @ 2021-03-22 0:19 UTC (permalink / raw)
To: Fabian Wüthrich, linux-media
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu, Mauro Carvalho Chehab
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@fabwu.ch>
Couple of comments below, but after addressing those:
Reviewed-by: Daniel Scally <djrscally@gmail.com>
> ---
> 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?
> +#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?
> #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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-03-22 0:19 ` Daniel Scally
@ 2021-03-22 15:16 ` Fabian Wüthrich
2021-03-29 20:47 ` Daniel Scally
0 siblings, 1 reply; 26+ messages in thread
From: Fabian Wüthrich @ 2021-03-22 15:16 UTC (permalink / raw)
To: Daniel Scally, linux-media
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu, Mauro Carvalho Chehab
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@fabwu.ch>
>
> Couple of comments below, but after addressing those:
>
>
> Reviewed-by: Daniel Scally <djrscally@gmail.com>
>
>> ---
>> 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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-03-22 15:16 ` Fabian Wüthrich
@ 2021-03-29 20:47 ` Daniel Scally
0 siblings, 0 replies; 26+ messages in thread
From: Daniel Scally @ 2021-03-29 20:47 UTC (permalink / raw)
To: Fabian Wüthrich, linux-media
Cc: Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu, Mauro Carvalho Chehab
Hi Fabian - sorry for the late reply.
On 22/03/2021 15:16, Fabian Wüthrich wrote:
>>> +#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.
Yeah I think that's probably the right place. I don't think this is a
blocker no, but you'll need to do a v2 anyway to extend the array below,
so you could include another patch then.
>
>>> #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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-03-21 19:11 [PATCH] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
2021-03-22 0:19 ` Daniel Scally
@ 2021-04-07 8:22 ` Jacopo Mondi
2021-04-07 11:52 ` Fabian Wüthrich
1 sibling, 1 reply; 26+ messages in thread
From: Jacopo Mondi @ 2021-04-07 8:22 UTC (permalink / raw)
To: Fabian Wüthrich
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab
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
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> ---
> 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 */
> +#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
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 8:22 ` Jacopo Mondi
@ 2021-04-07 11:52 ` Fabian Wüthrich
2021-04-07 12:41 ` Jacopo Mondi
0 siblings, 1 reply; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-07 11:52 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab
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().
P.S. I have a quick question below
>> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
>> ---
>> 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?
>> +
>> 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
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 11:52 ` Fabian Wüthrich
@ 2021-04-07 12:41 ` Jacopo Mondi
2021-04-07 13:19 ` Fabian Wüthrich
0 siblings, 1 reply; 26+ messages in thread
From: Jacopo Mondi @ 2021-04-07 12:41 UTC (permalink / raw)
To: Fabian Wüthrich
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab
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@fabwu.ch>
> >> ---
> >> 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
> >>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 12:41 ` Jacopo Mondi
@ 2021-04-07 13:19 ` Fabian Wüthrich
2021-04-07 13:51 ` Daniel Scally
2021-04-13 6:34 ` [PATCH v2] " Fabian Wüthrich
0 siblings, 2 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-07 13:19 UTC (permalink / raw)
To: Jacopo Mondi, Dan Scally
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab
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...
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@fabwu.ch>
>>>> ---
>>>> 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
>>>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 13:19 ` Fabian Wüthrich
@ 2021-04-07 13:51 ` Daniel Scally
2021-04-07 15:27 ` Fabian Wüthrich
2021-04-13 6:34 ` [PATCH v2] " Fabian Wüthrich
1 sibling, 1 reply; 26+ messages in thread
From: Daniel Scally @ 2021-04-07 13:51 UTC (permalink / raw)
To: Fabian Wüthrich, Jacopo Mondi
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab
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@fabwu.ch>
>>>>> ---
>>>>> 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
>>>>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 13:51 ` Daniel Scally
@ 2021-04-07 15:27 ` Fabian Wüthrich
0 siblings, 0 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-07 15:27 UTC (permalink / raw)
To: Daniel Scally, Jacopo Mondi
Cc: linux-media, Yong Zhi, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
Mauro Carvalho Chehab
Hi Dan,
On 07.04.21 15:51, Daniel Scally wrote:
> 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.
>
I agree. I'll post a v2 soon with your comments addressed.
>> 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@fabwu.ch>
>>>>>> ---
>>>>>> 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
>>>>>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2] ipu3-cio2: Parse sensor orientation and rotation
2021-04-07 13:19 ` Fabian Wüthrich
2021-04-07 13:51 ` Daniel Scally
@ 2021-04-13 6:34 ` Fabian Wüthrich
2021-04-13 15:15 ` [Devel] " Andy Shevchenko
2021-04-14 8:30 ` [PATCH v3 0/2] " Fabian Wüthrich
1 sibling, 2 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-13 6:34 UTC (permalink / raw)
To: linux-media, linux-acpi, devel
Cc: Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown, Fabian Wüthrich
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@fabwu.ch>
Reviewed-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v2:
- Move ACPI PLD constants to ACPI headers
- Fix dev_properties size
drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
include/acpi/acbuffer.h | 9 ++++
3 files changed, 73 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index c2199042d3db..926141e9a516 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 ACPI_PLD_PANEL_FRONT:
+ return V4L2_FWNODE_ORIENTATION_FRONT;
+ case ACPI_PLD_PANEL_BACK:
+ return V4L2_FWNODE_ORIENTATION_BACK;
+ case ACPI_PLD_PANEL_TOP:
+ case ACPI_PLD_PANEL_LEFT:
+ case ACPI_PLD_PANEL_RIGHT:
+ case ACPI_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..202c7d494f7a 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -12,6 +12,10 @@
#define CIO2_MAX_LANES 4
#define MAX_NUM_LINK_FREQS 3
+/* Values are educated guesses as we don't have a spec */
+#define CIO2_SENSOR_ROTATION_NORMAL 0
+#define CIO2_SENSOR_ROTATION_INVERTED 1
+
#define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
(const struct cio2_sensor_config) { \
.hid = _HID, \
@@ -80,6 +84,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,9 +111,11 @@ 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];
+ struct property_entry dev_properties[4];
struct property_entry cio2_properties[3];
struct software_node_ref_args local_ref[1];
struct software_node_ref_args remote_ref[1];
diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
index 18197c16149f..d42e82a82852 100644
--- a/include/acpi/acbuffer.h
+++ b/include/acpi/acbuffer.h
@@ -207,4 +207,13 @@ struct acpi_pld_info {
#define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
#define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
+/* Panel position defined in _PLD section of ACPI Specification 6.3 */
+#define ACPI_PLD_PANEL_TOP 0
+#define ACPI_PLD_PANEL_BOTTOM 1
+#define ACPI_PLD_PANEL_LEFT 2
+#define ACPI_PLD_PANEL_RIGHT 3
+#define ACPI_PLD_PANEL_FRONT 4
+#define ACPI_PLD_PANEL_BACK 5
+#define ACPI_PLD_PANEL_UNKNOWN 6
+
#endif /* ACBUFFER_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2] ipu3-cio2: Parse sensor orientation and rotation
@ 2021-04-13 15:15 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2021-04-13 15:15 UTC (permalink / raw)
To: Fabian Wüthrich
Cc: Linux Media Mailing List, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown
On Tue, Apr 13, 2021 at 12:02 PM Fabian Wüthrich <me@fabwu.ch> 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.
Better to refer to the version, chapter number and title, like]
ACPI specification v6.3 defines that in the chapter 6.1.8 "_PLD
(Physical Location of Device)"
> The sensor rotation is read from the SSDB ACPI buffer and converted into
> degrees.
After splitting to two patches, take my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v2:
> - Move ACPI PLD constants to ACPI headers
> - Fix dev_properties size
>
> drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
> drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
> include/acpi/acbuffer.h | 9 ++++
> 3 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> index c2199042d3db..926141e9a516 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 ACPI_PLD_PANEL_FRONT:
> + return V4L2_FWNODE_ORIENTATION_FRONT;
> + case ACPI_PLD_PANEL_BACK:
> + return V4L2_FWNODE_ORIENTATION_BACK;
> + case ACPI_PLD_PANEL_TOP:
> + case ACPI_PLD_PANEL_LEFT:
> + case ACPI_PLD_PANEL_RIGHT:
> + case ACPI_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..202c7d494f7a 100644
> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -12,6 +12,10 @@
> #define CIO2_MAX_LANES 4
> #define MAX_NUM_LINK_FREQS 3
>
> +/* Values are educated guesses as we don't have a spec */
> +#define CIO2_SENSOR_ROTATION_NORMAL 0
> +#define CIO2_SENSOR_ROTATION_INVERTED 1
> +
> #define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
> (const struct cio2_sensor_config) { \
> .hid = _HID, \
> @@ -80,6 +84,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,9 +111,11 @@ 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];
> + struct property_entry dev_properties[4];
> struct property_entry cio2_properties[3];
> struct software_node_ref_args local_ref[1];
> struct software_node_ref_args remote_ref[1];
...
> --- a/include/acpi/acbuffer.h
> +++ b/include/acpi/acbuffer.h
> @@ -207,4 +207,13 @@ struct acpi_pld_info {
> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
>
> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
> +#define ACPI_PLD_PANEL_TOP 0
> +#define ACPI_PLD_PANEL_BOTTOM 1
> +#define ACPI_PLD_PANEL_LEFT 2
> +#define ACPI_PLD_PANEL_RIGHT 3
> +#define ACPI_PLD_PANEL_FRONT 4
> +#define ACPI_PLD_PANEL_BACK 5
> +#define ACPI_PLD_PANEL_UNKNOWN 6
> +
> #endif /* ACBUFFER_H */
I assume this is a separate patch against ACPICA.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Devel] Re: [PATCH v2] ipu3-cio2: Parse sensor orientation and rotation
@ 2021-04-13 15:15 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2021-04-13 15:15 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 9372 bytes --]
On Tue, Apr 13, 2021 at 12:02 PM Fabian Wüthrich <me(a)fabwu.ch> 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.
Better to refer to the version, chapter number and title, like]
ACPI specification v6.3 defines that in the chapter 6.1.8 "_PLD
(Physical Location of Device)"
> The sensor rotation is read from the SSDB ACPI buffer and converted into
> degrees.
After splitting to two patches, take my
Reviewed-by: Andy Shevchenko <andy.shevchenko(a)gmail.com>
> Signed-off-by: Fabian Wüthrich <me(a)fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally(a)gmail.com>
> ---
> Changes in v2:
> - Move ACPI PLD constants to ACPI headers
> - Fix dev_properties size
>
> drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
> drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
> include/acpi/acbuffer.h | 9 ++++
> 3 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> index c2199042d3db..926141e9a516 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 ACPI_PLD_PANEL_FRONT:
> + return V4L2_FWNODE_ORIENTATION_FRONT;
> + case ACPI_PLD_PANEL_BACK:
> + return V4L2_FWNODE_ORIENTATION_BACK;
> + case ACPI_PLD_PANEL_TOP:
> + case ACPI_PLD_PANEL_LEFT:
> + case ACPI_PLD_PANEL_RIGHT:
> + case ACPI_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..202c7d494f7a 100644
> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -12,6 +12,10 @@
> #define CIO2_MAX_LANES 4
> #define MAX_NUM_LINK_FREQS 3
>
> +/* Values are educated guesses as we don't have a spec */
> +#define CIO2_SENSOR_ROTATION_NORMAL 0
> +#define CIO2_SENSOR_ROTATION_INVERTED 1
> +
> #define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
> (const struct cio2_sensor_config) { \
> .hid = _HID, \
> @@ -80,6 +84,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,9 +111,11 @@ 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];
> + struct property_entry dev_properties[4];
> struct property_entry cio2_properties[3];
> struct software_node_ref_args local_ref[1];
> struct software_node_ref_args remote_ref[1];
...
> --- a/include/acpi/acbuffer.h
> +++ b/include/acpi/acbuffer.h
> @@ -207,4 +207,13 @@ struct acpi_pld_info {
> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
>
> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
> +#define ACPI_PLD_PANEL_TOP 0
> +#define ACPI_PLD_PANEL_BOTTOM 1
> +#define ACPI_PLD_PANEL_LEFT 2
> +#define ACPI_PLD_PANEL_RIGHT 3
> +#define ACPI_PLD_PANEL_FRONT 4
> +#define ACPI_PLD_PANEL_BACK 5
> +#define ACPI_PLD_PANEL_UNKNOWN 6
> +
> #endif /* ACBUFFER_H */
I assume this is a separate patch against ACPICA.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 0/2] ipu3-cio2: Parse sensor orientation and rotation
2021-04-13 6:34 ` [PATCH v2] " Fabian Wüthrich
2021-04-13 15:15 ` [Devel] " Andy Shevchenko
@ 2021-04-14 8:30 ` Fabian Wüthrich
2021-04-14 8:30 ` [PATCH v3 1/2] ACPI: Add _PLD panel positions Fabian Wüthrich
2021-04-14 8:30 ` [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
1 sibling, 2 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-14 8:30 UTC (permalink / raw)
To: linux-media, linux-acpi, devel
Cc: Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown, Andy Shevchenko,
Fabian Wüthrich
This set exposes the orientation and rotation of a camera sensor
attached to ipu3-cio2 as fwnodes properties. Individual drivers can use
v4l2_fwnode_device_parse() to pick up these properties and expose them
as v4l2 controls using v4l2_ctrl_new_fwnode_properties().
Changes in v2:
- Move ACPI PLD constants to ACPI headers
- Fix dev_properties size
Changes in v3:
- Split patch into patch for ipu3 and patch for ACPICA
Fabian Wüthrich (2):
ACPI: Add _PLD panel positions
ipu3-cio2: Parse sensor orientation and rotation
drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
include/acpi/acbuffer.h | 9 ++++
3 files changed, 73 insertions(+), 5 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/2] ACPI: Add _PLD panel positions
2021-04-14 8:30 ` [PATCH v3 0/2] " Fabian Wüthrich
@ 2021-04-14 8:30 ` Fabian Wüthrich
2021-04-14 13:50 ` [Devel] " Rafael J. Wysocki
2021-04-14 8:30 ` [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
1 sibling, 1 reply; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-14 8:30 UTC (permalink / raw)
To: linux-media, linux-acpi, devel
Cc: Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown, Andy Shevchenko,
Fabian Wüthrich
The ACPI specification v6.3 defines the panel positions in chapter 6.1.8
"_PLD (Physical Location of Device)"
Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
Reviewed-by: Daniel Scally <djrscally@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
include/acpi/acbuffer.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
index 18197c16149f..d42e82a82852 100644
--- a/include/acpi/acbuffer.h
+++ b/include/acpi/acbuffer.h
@@ -207,4 +207,13 @@ struct acpi_pld_info {
#define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
#define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
+/* Panel position defined in _PLD section of ACPI Specification 6.3 */
+#define ACPI_PLD_PANEL_TOP 0
+#define ACPI_PLD_PANEL_BOTTOM 1
+#define ACPI_PLD_PANEL_LEFT 2
+#define ACPI_PLD_PANEL_RIGHT 3
+#define ACPI_PLD_PANEL_FRONT 4
+#define ACPI_PLD_PANEL_BACK 5
+#define ACPI_PLD_PANEL_UNKNOWN 6
+
#endif /* ACBUFFER_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation
2021-04-14 8:30 ` [PATCH v3 0/2] " Fabian Wüthrich
2021-04-14 8:30 ` [PATCH v3 1/2] ACPI: Add _PLD panel positions Fabian Wüthrich
@ 2021-04-14 8:30 ` Fabian Wüthrich
2021-05-09 16:30 ` Fabian Wüthrich
2021-07-12 9:03 ` [PATCH v4] " Fabian Wüthrich
1 sibling, 2 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-04-14 8:30 UTC (permalink / raw)
To: linux-media, linux-acpi, devel
Cc: Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown, Andy Shevchenko,
Fabian Wüthrich
The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
format.
The sensor rotation is read from the SSDB ACPI buffer and converted into
degrees.
Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
Reviewed-by: Daniel Scally <djrscally@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
2 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index c2199042d3db..926141e9a516 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 ACPI_PLD_PANEL_FRONT:
+ return V4L2_FWNODE_ORIENTATION_FRONT;
+ case ACPI_PLD_PANEL_BACK:
+ return V4L2_FWNODE_ORIENTATION_BACK;
+ case ACPI_PLD_PANEL_TOP:
+ case ACPI_PLD_PANEL_LEFT:
+ case ACPI_PLD_PANEL_RIGHT:
+ case ACPI_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..202c7d494f7a 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -12,6 +12,10 @@
#define CIO2_MAX_LANES 4
#define MAX_NUM_LINK_FREQS 3
+/* Values are educated guesses as we don't have a spec */
+#define CIO2_SENSOR_ROTATION_NORMAL 0
+#define CIO2_SENSOR_ROTATION_INVERTED 1
+
#define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
(const struct cio2_sensor_config) { \
.hid = _HID, \
@@ -80,6 +84,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,9 +111,11 @@ 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];
+ struct property_entry dev_properties[4];
struct property_entry cio2_properties[3];
struct software_node_ref_args local_ref[1];
struct software_node_ref_args remote_ref[1];
--
2.31.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/2] ACPI: Add _PLD panel positions
@ 2021-04-14 13:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-04-14 13:50 UTC (permalink / raw)
To: Fabian Wüthrich, Erik Kaneda
Cc: Linux Media Mailing List, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore,
Rafael J . Wysocki, Len Brown, Andy Shevchenko
On Wed, Apr 14, 2021 at 10:30 AM Fabian Wüthrich <me@fabwu.ch> wrote:
>
> The ACPI specification v6.3 defines the panel positions in chapter 6.1.8
> "_PLD (Physical Location of Device)"
>
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
This is ACPICA material.
Erik, can you pick up this one, please?
> ---
> include/acpi/acbuffer.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
> index 18197c16149f..d42e82a82852 100644
> --- a/include/acpi/acbuffer.h
> +++ b/include/acpi/acbuffer.h
> @@ -207,4 +207,13 @@ struct acpi_pld_info {
> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
>
> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
> +#define ACPI_PLD_PANEL_TOP 0
> +#define ACPI_PLD_PANEL_BOTTOM 1
> +#define ACPI_PLD_PANEL_LEFT 2
> +#define ACPI_PLD_PANEL_RIGHT 3
> +#define ACPI_PLD_PANEL_FRONT 4
> +#define ACPI_PLD_PANEL_BACK 5
> +#define ACPI_PLD_PANEL_UNKNOWN 6
> +
> #endif /* ACBUFFER_H */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Devel] Re: [PATCH v3 1/2] ACPI: Add _PLD panel positions
@ 2021-04-14 13:50 ` Rafael J. Wysocki
0 siblings, 0 replies; 26+ messages in thread
From: Rafael J. Wysocki @ 2021-04-14 13:50 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]
On Wed, Apr 14, 2021 at 10:30 AM Fabian Wüthrich <me(a)fabwu.ch> wrote:
>
> The ACPI specification v6.3 defines the panel positions in chapter 6.1.8
> "_PLD (Physical Location of Device)"
>
> Signed-off-by: Fabian Wüthrich <me(a)fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally(a)gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko(a)gmail.com>
This is ACPICA material.
Erik, can you pick up this one, please?
> ---
> include/acpi/acbuffer.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
> index 18197c16149f..d42e82a82852 100644
> --- a/include/acpi/acbuffer.h
> +++ b/include/acpi/acbuffer.h
> @@ -207,4 +207,13 @@ struct acpi_pld_info {
> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
>
> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
> +#define ACPI_PLD_PANEL_TOP 0
> +#define ACPI_PLD_PANEL_BOTTOM 1
> +#define ACPI_PLD_PANEL_LEFT 2
> +#define ACPI_PLD_PANEL_RIGHT 3
> +#define ACPI_PLD_PANEL_FRONT 4
> +#define ACPI_PLD_PANEL_BACK 5
> +#define ACPI_PLD_PANEL_UNKNOWN 6
> +
> #endif /* ACBUFFER_H */
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/2] ACPI: Add _PLD panel positions
2021-04-14 13:50 ` [Devel] " Rafael J. Wysocki
(?)
@ 2021-05-09 16:29 ` Fabian Wüthrich
2021-05-14 17:32 ` Kaneda, Erik
-1 siblings, 1 reply; 26+ messages in thread
From: Fabian Wüthrich @ 2021-05-09 16:29 UTC (permalink / raw)
To: Erik Kaneda
Cc: Linux Media Mailing List, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore,
Rafael J . Wysocki, Len Brown, Andy Shevchenko,
Rafael J. Wysocki
Hi Erik,
Do I need to add anything to this patch or is it fine like that?
Thanks,
Fabian
On 14.04.21 15:50, Rafael J. Wysocki wrote:
> On Wed, Apr 14, 2021 at 10:30 AM Fabian Wüthrich <me@fabwu.ch> wrote:
>>
>> The ACPI specification v6.3 defines the panel positions in chapter 6.1.8
>> "_PLD (Physical Location of Device)"
>>
>> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
>> Reviewed-by: Daniel Scally <djrscally@gmail.com>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> This is ACPICA material.
>
> Erik, can you pick up this one, please?
>
>> ---
>> include/acpi/acbuffer.h | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
>> index 18197c16149f..d42e82a82852 100644
>> --- a/include/acpi/acbuffer.h
>> +++ b/include/acpi/acbuffer.h
>> @@ -207,4 +207,13 @@ struct acpi_pld_info {
>> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS (dword, 16, ACPI_16BIT_MASK)
>> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
>>
>> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
>> +#define ACPI_PLD_PANEL_TOP 0
>> +#define ACPI_PLD_PANEL_BOTTOM 1
>> +#define ACPI_PLD_PANEL_LEFT 2
>> +#define ACPI_PLD_PANEL_RIGHT 3
>> +#define ACPI_PLD_PANEL_FRONT 4
>> +#define ACPI_PLD_PANEL_BACK 5
>> +#define ACPI_PLD_PANEL_UNKNOWN 6
>> +
>> #endif /* ACBUFFER_H */
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation
2021-04-14 8:30 ` [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
@ 2021-05-09 16:30 ` Fabian Wüthrich
2021-07-12 9:03 ` [PATCH v4] " Fabian Wüthrich
1 sibling, 0 replies; 26+ messages in thread
From: Fabian Wüthrich @ 2021-05-09 16:30 UTC (permalink / raw)
To: linux-media
Cc: Jacopo Mondi, Yong Zhi, Sakari Ailus, Bingbu Cao, Dan Scally,
Tianshu Qiu, Mauro Carvalho Chehab, Robert Moore, Erik Kaneda,
Rafael J . Wysocki, Len Brown, Andy Shevchenko
Any other comments about this patch or is it ready to be merged?
Thanks,
Fabian
On 14.04.21 10:30, Fabian Wüthrich wrote:
> The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
> format.
>
> The sensor rotation is read from the SSDB ACPI buffer and converted into
> degrees.
>
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
> drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
> drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
> 2 files changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
> index c2199042d3db..926141e9a516 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 ACPI_PLD_PANEL_FRONT:
> + return V4L2_FWNODE_ORIENTATION_FRONT;
> + case ACPI_PLD_PANEL_BACK:
> + return V4L2_FWNODE_ORIENTATION_BACK;
> + case ACPI_PLD_PANEL_TOP:
> + case ACPI_PLD_PANEL_LEFT:
> + case ACPI_PLD_PANEL_RIGHT:
> + case ACPI_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..202c7d494f7a 100644
> --- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
> +++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
> @@ -12,6 +12,10 @@
> #define CIO2_MAX_LANES 4
> #define MAX_NUM_LINK_FREQS 3
>
> +/* Values are educated guesses as we don't have a spec */
> +#define CIO2_SENSOR_ROTATION_NORMAL 0
> +#define CIO2_SENSOR_ROTATION_INVERTED 1
> +
> #define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
> (const struct cio2_sensor_config) { \
> .hid = _HID, \
> @@ -80,6 +84,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,9 +111,11 @@ 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];
> + struct property_entry dev_properties[4];
> struct property_entry cio2_properties[3];
> struct software_node_ref_args local_ref[1];
> struct software_node_ref_args remote_ref[1];
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH v3 1/2] ACPI: Add _PLD panel positions
2021-05-09 16:29 ` Fabian Wüthrich
@ 2021-05-14 17:32 ` Kaneda, Erik
0 siblings, 0 replies; 26+ messages in thread
From: Kaneda, Erik @ 2021-05-14 17:32 UTC (permalink / raw)
To: Fabian Wüthrich
Cc: Linux Media Mailing List, ACPI Devel Maling List,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Jacopo Mondi, Zhi, Yong, Sakari Ailus, Cao, Bingbu, Dan Scally,
Qiu, Tian Shu, Mauro Carvalho Chehab, Moore, Robert, Wysocki,
Rafael J, Len Brown, Andy Shevchenko, Rafael J. Wysocki
> -----Original Message-----
> From: Fabian Wüthrich <me@fabwu.ch>
> Sent: Sunday, May 9, 2021 9:29 AM
> To: Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; ACPI Devel
> Maling List <linux-acpi@vger.kernel.org>; open list:ACPI COMPONENT
> ARCHITECTURE (ACPICA) <devel@acpica.org>; Jacopo Mondi
> <jacopo@jmondi.org>; Zhi, Yong <yong.zhi@intel.com>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Dan
> Scally <djrscally@gmail.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;
> Mauro Carvalho Chehab <mchehab@kernel.org>; Moore, Robert
> <robert.moore@intel.com>; Wysocki, Rafael J <rafael.j.wysocki@intel.com>;
> Len Brown <lenb@kernel.org>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Rafael J. Wysocki <rafael@kernel.org>
> Subject: Re: [PATCH v3 1/2] ACPI: Add _PLD panel positions
>
> Hi Erik,
>
Hi Fabian,
> Do I need to add anything to this patch or is it fine like that?
Sorry about the late response. I submitted a pull request on your behalf for ACPICA upstream here: https://github.com/acpica/acpica/pull/689
I'll port it to Linux and circulate on this mailing list after ACPICA does a release (usually about once per month).
Thanks,
Erik
>
> Thanks,
> Fabian
>
> On 14.04.21 15:50, Rafael J. Wysocki wrote:
> > On Wed, Apr 14, 2021 at 10:30 AM Fabian Wüthrich <me@fabwu.ch>
> wrote:
> >>
> >> The ACPI specification v6.3 defines the panel positions in chapter 6.1.8
> >> "_PLD (Physical Location of Device)"
> >>
> >> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> >> Reviewed-by: Daniel Scally <djrscally@gmail.com>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > This is ACPICA material.
> >
> > Erik, can you pick up this one, please?
> >
> >> ---
> >> include/acpi/acbuffer.h | 9 +++++++++
> >> 1 file changed, 9 insertions(+)
> >>
> >> diff --git a/include/acpi/acbuffer.h b/include/acpi/acbuffer.h
> >> index 18197c16149f..d42e82a82852 100644
> >> --- a/include/acpi/acbuffer.h
> >> +++ b/include/acpi/acbuffer.h
> >> @@ -207,4 +207,13 @@ struct acpi_pld_info {
> >> #define ACPI_PLD_GET_HORIZ_OFFSET(dword) ACPI_GET_BITS
> (dword, 16, ACPI_16BIT_MASK)
> >> #define ACPI_PLD_SET_HORIZ_OFFSET(dword,value) ACPI_SET_BITS
> (dword, 16, ACPI_16BIT_MASK, value) /* Offset 128+16=144, Len 16 */
> >>
> >> +/* Panel position defined in _PLD section of ACPI Specification 6.3 */
> >> +#define ACPI_PLD_PANEL_TOP 0
> >> +#define ACPI_PLD_PANEL_BOTTOM 1
> >> +#define ACPI_PLD_PANEL_LEFT 2
> >> +#define ACPI_PLD_PANEL_RIGHT 3
> >> +#define ACPI_PLD_PANEL_FRONT 4
> >> +#define ACPI_PLD_PANEL_BACK 5
> >> +#define ACPI_PLD_PANEL_UNKNOWN 6
> >> +
> >> #endif /* ACBUFFER_H */
> >> --
> >> 2.31.1
> >>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4] ipu3-cio2: Parse sensor orientation and rotation
2021-04-14 8:30 ` [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
2021-05-09 16:30 ` Fabian Wüthrich
@ 2021-07-12 9:03 ` Fabian Wüthrich
2021-08-20 13:12 ` Sakari Ailus
1 sibling, 1 reply; 26+ messages in thread
From: Fabian Wüthrich @ 2021-07-12 9:03 UTC (permalink / raw)
To: linux-media
Cc: Sakari Ailus, Jacopo Mondi, Andy Shevchenko, Dan Scally,
Fabian Wüthrich
The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
format.
The sensor rotation is read from the SSDB ACPI buffer and converted into
degrees.
Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
Reviewed-by: Daniel Scally <djrscally@gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
Changes in v2:
- Move ACPI PLD constants to ACPI headers
- Fix dev_properties size
Changes in v3:
- Split patch into patch for ipu3 and patch for ACPICA
Changes in v4:
- same patch as v2 but no ACPICA changes are merged
drivers/media/pci/intel/ipu3/cio2-bridge.c | 60 ++++++++++++++++++++--
drivers/media/pci/intel/ipu3/cio2-bridge.h | 9 +++-
2 files changed, 64 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 4657e99df033..f0f148222039 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 ACPI_PLD_PANEL_FRONT:
+ return V4L2_FWNODE_ORIENTATION_FRONT;
+ case ACPI_PLD_PANEL_BACK:
+ return V4L2_FWNODE_ORIENTATION_BACK;
+ case ACPI_PLD_PANEL_TOP:
+ case ACPI_PLD_PANEL_LEFT:
+ case ACPI_PLD_PANEL_RIGHT:
+ case ACPI_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] = SOFTWARE_NODE_REFERENCE(&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) {
@@ -194,11 +240,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);
@@ -206,7 +256,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]);
@@ -227,6 +277,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);
return ret;
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
index dd0ffcafa489..202c7d494f7a 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -12,6 +12,10 @@
#define CIO2_MAX_LANES 4
#define MAX_NUM_LINK_FREQS 3
+/* Values are educated guesses as we don't have a spec */
+#define CIO2_SENSOR_ROTATION_NORMAL 0
+#define CIO2_SENSOR_ROTATION_INVERTED 1
+
#define CIO2_SENSOR_CONFIG(_HID, _NR, ...) \
(const struct cio2_sensor_config) { \
.hid = _HID, \
@@ -80,6 +84,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,9 +111,11 @@ 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];
+ struct property_entry dev_properties[4];
struct property_entry cio2_properties[3];
struct software_node_ref_args local_ref[1];
struct software_node_ref_args remote_ref[1];
--
2.32.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4] ipu3-cio2: Parse sensor orientation and rotation
2021-07-12 9:03 ` [PATCH v4] " Fabian Wüthrich
@ 2021-08-20 13:12 ` Sakari Ailus
2021-08-20 13:25 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Sakari Ailus @ 2021-08-20 13:12 UTC (permalink / raw)
To: Fabian Wüthrich
Cc: linux-media, Jacopo Mondi, Andy Shevchenko, Dan Scally
On Mon, Jul 12, 2021 at 11:03:26AM +0200, Fabian Wüthrich wrote:
> The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
> format.
>
> The sensor rotation is read from the SSDB ACPI buffer and converted into
> degrees.
>
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> Reviewed-by: Daniel Scally <djrscally@gmail.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
It's in my tree now.
--
Sakari Ailus
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] ipu3-cio2: Parse sensor orientation and rotation
2021-08-20 13:12 ` Sakari Ailus
@ 2021-08-20 13:25 ` Andy Shevchenko
2021-08-20 15:02 ` Sakari Ailus
0 siblings, 1 reply; 26+ messages in thread
From: Andy Shevchenko @ 2021-08-20 13:25 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Fabian Wüthrich, linux-media, Jacopo Mondi, Dan Scally
On Fri, Aug 20, 2021 at 4:12 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Mon, Jul 12, 2021 at 11:03:26AM +0200, Fabian Wüthrich wrote:
> > The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
> > format.
> >
> > The sensor rotation is read from the SSDB ACPI buffer and converted into
> > degrees.
> >
> > Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> > Reviewed-by: Daniel Scally <djrscally@gmail.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> It's in my tree now.
Do you know what's going on with
https://lore.kernel.org/linux-media/20210726084055.54887-1-andriy.shevchenko@linux.intel.com/
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] ipu3-cio2: Parse sensor orientation and rotation
2021-08-20 13:25 ` Andy Shevchenko
@ 2021-08-20 15:02 ` Sakari Ailus
2021-08-20 15:12 ` Andy Shevchenko
0 siblings, 1 reply; 26+ messages in thread
From: Sakari Ailus @ 2021-08-20 15:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Fabian Wüthrich, linux-media, Jacopo Mondi, Dan Scally
Hi Andy,
On Fri, Aug 20, 2021 at 04:25:15PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 20, 2021 at 4:12 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On Mon, Jul 12, 2021 at 11:03:26AM +0200, Fabian Wüthrich wrote:
> > > The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
> > > format.
> > >
> > > The sensor rotation is read from the SSDB ACPI buffer and converted into
> > > degrees.
> > >
> > > Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> > > Reviewed-by: Daniel Scally <djrscally@gmail.com>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > It's in my tree now.
>
> Do you know what's going on with
> https://lore.kernel.org/linux-media/20210726084055.54887-1-andriy.shevchenko@linux.intel.com/
> ?
I thought you'd be merging that through the other tree where the other
patch was merged.
I can also take it through the media tree. I guess by now it'll be the next
version in any case.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4] ipu3-cio2: Parse sensor orientation and rotation
2021-08-20 15:02 ` Sakari Ailus
@ 2021-08-20 15:12 ` Andy Shevchenko
0 siblings, 0 replies; 26+ messages in thread
From: Andy Shevchenko @ 2021-08-20 15:12 UTC (permalink / raw)
To: Sakari Ailus, Rafael J. Wysocki, linux-acpi
Cc: Fabian Wüthrich, linux-media, Jacopo Mondi, Dan Scally
On Fri, Aug 20, 2021 at 6:02 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Fri, Aug 20, 2021 at 04:25:15PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 20, 2021 at 4:12 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > > On Mon, Jul 12, 2021 at 11:03:26AM +0200, Fabian Wüthrich wrote:
> > > > The sensor orientation is read from the _PLC ACPI buffer and converted to a v4l2
> > > > format.
> > > >
> > > > The sensor rotation is read from the SSDB ACPI buffer and converted into
> > > > degrees.
> > > >
> > > > Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> > > > Reviewed-by: Daniel Scally <djrscally@gmail.com>
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > >
> > > It's in my tree now.
> >
> > Do you know what's going on with
> > https://lore.kernel.org/linux-media/20210726084055.54887-1-andriy.shevchenko@linux.intel.com/
> > ?
>
> I thought you'd be merging that through the other tree where the other
> patch was merged.
Ah, okay, let's ask Rafael if he can take this for v5.14 cycle.
Rafael, I can resubmit a new version with Sakari's ACK. Or you may
retrieve this by message ID from lore. What do you prefer?
> I can also take it through the media tree. I guess by now it'll be the next
> version in any case.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-08-20 15:13 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-21 19:11 [PATCH] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
2021-03-22 0:19 ` Daniel Scally
2021-03-22 15:16 ` Fabian Wüthrich
2021-03-29 20:47 ` Daniel Scally
2021-04-07 8:22 ` Jacopo Mondi
2021-04-07 11:52 ` Fabian Wüthrich
2021-04-07 12:41 ` Jacopo Mondi
2021-04-07 13:19 ` Fabian Wüthrich
2021-04-07 13:51 ` Daniel Scally
2021-04-07 15:27 ` Fabian Wüthrich
2021-04-13 6:34 ` [PATCH v2] " Fabian Wüthrich
2021-04-13 15:15 ` Andy Shevchenko
2021-04-13 15:15 ` [Devel] " Andy Shevchenko
2021-04-14 8:30 ` [PATCH v3 0/2] " Fabian Wüthrich
2021-04-14 8:30 ` [PATCH v3 1/2] ACPI: Add _PLD panel positions Fabian Wüthrich
2021-04-14 13:50 ` Rafael J. Wysocki
2021-04-14 13:50 ` [Devel] " Rafael J. Wysocki
2021-05-09 16:29 ` Fabian Wüthrich
2021-05-14 17:32 ` Kaneda, Erik
2021-04-14 8:30 ` [PATCH v3 2/2] ipu3-cio2: Parse sensor orientation and rotation Fabian Wüthrich
2021-05-09 16:30 ` Fabian Wüthrich
2021-07-12 9:03 ` [PATCH v4] " Fabian Wüthrich
2021-08-20 13:12 ` Sakari Ailus
2021-08-20 13:25 ` Andy Shevchenko
2021-08-20 15:02 ` Sakari Ailus
2021-08-20 15:12 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.