* [PATCH 1/9] media: atomisp: Drop MRFLD_PORT_NUM define
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 16:05 ` Andy Shevchenko
2023-05-18 15:37 ` [PATCH 2/9] media: atomisp: Remove unused fields from struct atomisp_input_subdev Hans de Goede
` (8 subsequent siblings)
9 siblings, 1 reply; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
The info in the MRFLD_PORT_NUM define is duplicate with
the ATOMISP_CAMERA_NR_PORTS define. Drop MRFLD_PORT_NUM.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/media/atomisp/pci/atomisp-regs.h | 1 -
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 8 ++++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp-regs.h b/drivers/staging/media/atomisp/pci/atomisp-regs.h
index 022997f47121..a7b0196686be 100644
--- a/drivers/staging/media/atomisp/pci/atomisp-regs.h
+++ b/drivers/staging/media/atomisp/pci/atomisp-regs.h
@@ -112,7 +112,6 @@
/* MRFLD CSI lane configuration related */
#define MRFLD_PORT_CONFIG_NUM 8
-#define MRFLD_PORT_NUM 3
#define MRFLD_PORT1_ENABLE_SHIFT 0
#define MRFLD_PORT2_ENABLE_SHIFT 1
#define MRFLD_PORT3_ENABLE_SHIFT 2
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index a76b60f8b411..93998fdc836d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -701,7 +701,7 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
struct pci_dev *pdev = to_pci_dev(isp->dev);
static const struct {
u8 code;
- u8 lanes[MRFLD_PORT_NUM];
+ u8 lanes[ATOMISP_CAMERA_NR_PORTS];
} portconfigs[] = {
/* Tangier/Merrifield available lane configurations */
{ 0x00, { 4, 1, 0 } }, /* 00000 */
@@ -725,7 +725,7 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
};
unsigned int i, j;
- u8 sensor_lanes[MRFLD_PORT_NUM] = { 0 };
+ u8 sensor_lanes[ATOMISP_CAMERA_NR_PORTS] = { 0 };
u32 csi_control;
int nportconfigs;
u32 port_config_mask;
@@ -782,12 +782,12 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
}
for (i = 0; i < nportconfigs; i++) {
- for (j = 0; j < MRFLD_PORT_NUM; j++)
+ for (j = 0; j < ATOMISP_CAMERA_NR_PORTS; j++)
if (sensor_lanes[j] &&
sensor_lanes[j] != portconfigs[i].lanes[j])
break;
- if (j == MRFLD_PORT_NUM)
+ if (j == ATOMISP_CAMERA_NR_PORTS)
break; /* Found matching setting */
}
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 1/9] media: atomisp: Drop MRFLD_PORT_NUM define
2023-05-18 15:37 ` [PATCH 1/9] media: atomisp: Drop MRFLD_PORT_NUM define Hans de Goede
@ 2023-05-18 16:05 ` Andy Shevchenko
0 siblings, 0 replies; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-18 16:05 UTC (permalink / raw)
To: Hans de Goede
Cc: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko, Kate Hsuan,
Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
On Thu, May 18, 2023 at 6:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The info in the MRFLD_PORT_NUM define is duplicate with
> the ATOMISP_CAMERA_NR_PORTS define. Drop MRFLD_PORT_NUM.
JFYI: Since the long term plan of mine is to eventually do something
useful on Intel MID platforms, I would like to keep the Merrifield
(Tangier) / Moorefield (Annidale) support in this driver. That said,
this patch doesn't do any harm to that, so I'm happy.
...
> - u8 sensor_lanes[MRFLD_PORT_NUM] = { 0 };
> + u8 sensor_lanes[ATOMISP_CAMERA_NR_PORTS] = { 0 };
You may drop 0 as well.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/9] media: atomisp: Remove unused fields from struct atomisp_input_subdev
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
2023-05-18 15:37 ` [PATCH 1/9] media: atomisp: Drop MRFLD_PORT_NUM define Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 3/9] media: atomisp: Remove atomisp_video_init() parametrization Hans de Goede
` (7 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
Remove unused fields from struct atomisp_input_subdev:
1. frame_size is never used at all
2. sensor_index is always 0, just directly pass 0 in the single user.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/media/atomisp/pci/atomisp_internal.h | 3 ---
drivers/staging/media/atomisp/pci/atomisp_ioctl.c | 2 +-
drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 7 -------
3 files changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index feaf4037a389..ee0dd5eb4711 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -126,15 +126,12 @@ struct atomisp_input_subdev {
enum atomisp_camera_port port;
struct v4l2_subdev *camera;
struct v4l2_subdev *motor;
- struct v4l2_frmsizeenum frame_size;
/*
* To show this resource is used by
* which stream, in ISP multiple stream mode
*/
struct atomisp_sub_device *asd;
-
- int sensor_index;
};
enum atomisp_dfs_mode {
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 900e4c79cd78..2cde1af77a2d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -673,7 +673,7 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
/* select operating sensor */
ret = v4l2_subdev_call(isp->inputs[input].camera, video, s_routing,
- 0, isp->inputs[input].sensor_index, 0);
+ 0, 0, 0);
if (ret && (ret != -ENOIOCTLCMD)) {
dev_err(isp->dev, "Failed to select sensor\n");
return ret;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 93998fdc836d..a2440f270ffe 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -866,13 +866,6 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
isp->inputs[isp->input_cnt].type = subdevs->type;
isp->inputs[isp->input_cnt].port = subdevs->port;
isp->inputs[isp->input_cnt].camera = subdevs->subdev;
- isp->inputs[isp->input_cnt].sensor_index = 0;
- /*
- * initialize the subdev frame size, then next we can
- * judge whether frame_size store effective value via
- * pixel_format.
- */
- isp->inputs[isp->input_cnt].frame_size.pixel_format = 0;
isp->input_cnt++;
break;
case CAMERA_MOTOR:
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 3/9] media: atomisp: Remove atomisp_video_init() parametrization
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
2023-05-18 15:37 ` [PATCH 1/9] media: atomisp: Drop MRFLD_PORT_NUM define Hans de Goede
2023-05-18 15:37 ` [PATCH 2/9] media: atomisp: Remove unused fields from struct atomisp_input_subdev Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 4/9] media: atomisp: Rename __get_mipi_port() to atomisp_port_to_mipi_port() Hans de Goede
` (6 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
Now that we only have a single /dev/video# node it is no longer
necessary for atomisp_video_init() to be parametrized.
Remove its parameters and while at it also change the name
from the single /dev/video# node from "ATOMISP ISP PREVIEW output"
to "ATOMISP video output".
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../staging/media/atomisp/pci/atomisp_fops.c | 2 +-
.../media/atomisp/pci/atomisp_subdev.c | 2 +-
.../media/atomisp/pci/atomisp_subdev.h | 2 --
.../staging/media/atomisp/pci/atomisp_v4l2.c | 24 +++++--------------
.../staging/media/atomisp/pci/atomisp_v4l2.h | 3 +--
5 files changed, 9 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index fb42c2710795..36e441dce7d5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -537,7 +537,7 @@ static int atomisp_open(struct file *file)
atomisp_subdev_init_struct(asd);
/* Ensure that a mode is set */
- v4l2_ctrl_s_ctrl(asd->run_mode, pipe->default_run_mode);
+ v4l2_ctrl_s_ctrl(asd->run_mode, ATOMISP_RUN_MODE_PREVIEW);
pipe->users++;
mutex_unlock(&isp->mutex);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 8b99805bcc77..c2ae77cd77a7 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -898,7 +898,7 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
if (ret)
return ret;
- ret = atomisp_video_init(&asd->video_out, "PREVIEW", ATOMISP_RUN_MODE_PREVIEW);
+ ret = atomisp_video_init(&asd->video_out);
if (ret < 0)
return ret;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index dc6970b48633..cd82554d5f65 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -67,8 +67,6 @@ struct atomisp_video_pipe {
/* Filled through atomisp_get_css_frame_info() on queue setup */
struct ia_css_frame_info frame_info;
- /* Store here the initial run mode */
- unsigned int default_run_mode;
/* Set from streamoff to disallow queuing further buffers in CSS */
bool stopping;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index a2440f270ffe..94b9908139ab 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -333,34 +333,22 @@ const struct atomisp_dfs_config dfs_config_cht_soc = {
.dfs_table_size = ARRAY_SIZE(dfs_rules_cht_soc),
};
-int atomisp_video_init(struct atomisp_video_pipe *video, const char *name,
- unsigned int run_mode)
+int atomisp_video_init(struct atomisp_video_pipe *video)
{
int ret;
- const char *direction;
-
- switch (video->type) {
- case V4L2_BUF_TYPE_VIDEO_CAPTURE:
- direction = "output";
- video->pad.flags = MEDIA_PAD_FL_SINK;
- video->vdev.fops = &atomisp_fops;
- video->vdev.ioctl_ops = &atomisp_ioctl_ops;
- video->vdev.lock = &video->isp->mutex;
- break;
- default:
- return -EINVAL;
- }
+ video->pad.flags = MEDIA_PAD_FL_SINK;
ret = media_entity_pads_init(&video->vdev.entity, 1, &video->pad);
if (ret < 0)
return ret;
/* Initialize the video device. */
- snprintf(video->vdev.name, sizeof(video->vdev.name),
- "ATOMISP ISP %s %s", name, direction);
+ strscpy(video->vdev.name, "ATOMISP video output", sizeof(video->vdev.name));
+ video->vdev.fops = &atomisp_fops;
+ video->vdev.ioctl_ops = &atomisp_ioctl_ops;
+ video->vdev.lock = &video->isp->mutex;
video->vdev.release = video_device_release_empty;
video_set_drvdata(&video->vdev, video->isp);
- video->default_run_mode = run_mode;
return 0;
}
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
index ccf1c0ac17b2..c8ee3ad83320 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
@@ -26,8 +26,7 @@ struct v4l2_device;
struct atomisp_device;
struct firmware;
-int atomisp_video_init(struct atomisp_video_pipe *video, const char *name,
- unsigned int run_mode);
+int atomisp_video_init(struct atomisp_video_pipe *video);
void atomisp_video_unregister(struct atomisp_video_pipe *video);
const struct firmware *atomisp_load_firmware(struct atomisp_device *isp);
int atomisp_csi_lane_config(struct atomisp_device *isp);
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 4/9] media: atomisp: Rename __get_mipi_port() to atomisp_port_to_mipi_port()
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (2 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 3/9] media: atomisp: Remove atomisp_video_init() parametrization Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 5/9] media: atomisp: Store number of sensor lanes per port in struct atomisp_device Hans de Goede
` (5 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
Rename __get_mipi_port() to atomisp_port_to_mipi_port(), this is not a
private (not static) function so its name should be properly prefixed.
While at is also cleanup the weird handling of ATOMISP_CAMERA_PORT_TERTIARY
this seems to be a left over from when the driver also supported CSI
receivers with only 2 ports, but those are not supported by the current
code base, so this can be cleaned up now.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/media/atomisp/pci/atomisp_cmd.c | 10 ++++------
drivers/staging/media/atomisp/pci/atomisp_cmd.h | 4 ++--
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 9c44ffba2828..f4a0341d1f8d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3893,8 +3893,8 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
return 0;
}
-enum mipi_port_id __get_mipi_port(struct atomisp_device *isp,
- enum atomisp_camera_port port)
+enum mipi_port_id atomisp_port_to_mipi_port(struct atomisp_device *isp,
+ enum atomisp_camera_port port)
{
switch (port) {
case ATOMISP_CAMERA_PORT_PRIMARY:
@@ -3902,9 +3902,7 @@ enum mipi_port_id __get_mipi_port(struct atomisp_device *isp,
case ATOMISP_CAMERA_PORT_SECONDARY:
return MIPI_PORT1_ID;
case ATOMISP_CAMERA_PORT_TERTIARY:
- if (MIPI_PORT1_ID + 1 != N_MIPI_PORT_ID)
- return MIPI_PORT1_ID + 1;
- fallthrough;
+ return MIPI_PORT2_ID;
default:
dev_err(isp->dev, "unsupported port: %d\n", port);
return MIPI_PORT0_ID;
@@ -3980,7 +3978,7 @@ static inline int atomisp_set_sensor_mipi_to_isp(
return -EINVAL;
input_format = fc->atomisp_in_fmt;
atomisp_css_input_configure_port(asd,
- __get_mipi_port(asd->isp, mipi_info->port),
+ atomisp_port_to_mipi_port(asd->isp, mipi_info->port),
mipi_info->num_lanes,
0xffff4, mipi_freq,
input_format,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index 783fb1e6f4f9..5270c370e463 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -285,8 +285,8 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
/* Events. Only one event has to be exported for now. */
void atomisp_eof_event(struct atomisp_sub_device *asd, uint8_t exp_id);
-enum mipi_port_id __get_mipi_port(struct atomisp_device *isp,
- enum atomisp_camera_port port);
+enum mipi_port_id atomisp_port_to_mipi_port(struct atomisp_device *isp,
+ enum atomisp_camera_port port);
void atomisp_apply_css_parameters(
struct atomisp_sub_device *asd,
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 5/9] media: atomisp: Store number of sensor lanes per port in struct atomisp_device
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (3 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 4/9] media: atomisp: Rename __get_mipi_port() to atomisp_port_to_mipi_port() Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 6/9] media: atomisp: Delay mapping sensors to inputs till atomisp_register_device_nodes() Hans de Goede
` (4 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
Store number of sensor lanes per port in struct atomisp_device.
This is a preparation patch for adding v4l2-async sensor probing support.
With async probing the inputs will get registered later, but we can
already fill the sensor_lanes array when parsing the fwnodes.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../atomisp/include/linux/atomisp_platform.h | 1 +
.../media/atomisp/pci/atomisp_gmin_platform.c | 2 +
.../media/atomisp/pci/atomisp_internal.h | 5 ++
.../staging/media/atomisp/pci/atomisp_v4l2.c | 56 ++++---------------
4 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
index e8e965f73fc8..487ef5846c24 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
@@ -125,6 +125,7 @@ struct intel_v4l2_subdev_id {
struct intel_v4l2_subdev_table {
enum intel_v4l2_subdev_type type;
enum atomisp_camera_port port;
+ unsigned int lanes;
struct v4l2_subdev *subdev;
};
diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index ae45c1d8c50c..bc2dd96176d0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -189,6 +189,7 @@ int atomisp_register_i2c_module(struct v4l2_subdev *subdev,
pdata.subdevs[i].type = type;
pdata.subdevs[i].port = gs->csi_port;
+ pdata.subdevs[i].lanes = gs->csi_lanes;
pdata.subdevs[i].subdev = subdev;
return 0;
}
@@ -1150,6 +1151,7 @@ int atomisp_register_sensor_no_gmin(struct v4l2_subdev *subdev, u32 lanes,
pdata.subdevs[i].type = RAW_CAMERA;
pdata.subdevs[i].port = port;
+ pdata.subdevs[i].lanes = lanes;
pdata.subdevs[i].subdev = subdev;
return 0;
}
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index ee0dd5eb4711..b8d643c9df8f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -189,6 +189,11 @@ struct atomisp_device {
* structures and css API calls. */
struct mutex mutex;
+ /*
+ * Number of lanes used by each sensor per port.
+ * Note this is indexed by mipi_port_id not atomisp_camera_port.
+ */
+ int sensor_lanes[N_MIPI_PORT_ID];
unsigned int input_cnt;
struct atomisp_input_subdev inputs[ATOM_ISP_MAX_INPUTS];
struct v4l2_subdev *flash;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 94b9908139ab..b5a070f69120 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -713,7 +713,6 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
};
unsigned int i, j;
- u8 sensor_lanes[ATOMISP_CAMERA_NR_PORTS] = { 0 };
u32 csi_control;
int nportconfigs;
u32 port_config_mask;
@@ -741,38 +740,10 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
nportconfigs = ARRAY_SIZE(portconfigs);
}
- for (i = 0; i < isp->input_cnt; i++) {
- struct camera_mipi_info *mipi_info;
-
- if (isp->inputs[i].type != RAW_CAMERA)
- continue;
-
- mipi_info = atomisp_to_sensor_mipi_info(isp->inputs[i].camera);
- if (!mipi_info)
- continue;
-
- switch (mipi_info->port) {
- case ATOMISP_CAMERA_PORT_PRIMARY:
- sensor_lanes[0] = mipi_info->num_lanes;
- break;
- case ATOMISP_CAMERA_PORT_SECONDARY:
- sensor_lanes[1] = mipi_info->num_lanes;
- break;
- case ATOMISP_CAMERA_PORT_TERTIARY:
- sensor_lanes[2] = mipi_info->num_lanes;
- break;
- default:
- dev_err(isp->dev,
- "%s: invalid port: %d for the %dth sensor\n",
- __func__, mipi_info->port, i);
- return -EINVAL;
- }
- }
-
for (i = 0; i < nportconfigs; i++) {
for (j = 0; j < ATOMISP_CAMERA_NR_PORTS; j++)
- if (sensor_lanes[j] &&
- sensor_lanes[j] != portconfigs[i].lanes[j])
+ if (isp->sensor_lanes[j] &&
+ isp->sensor_lanes[j] != portconfigs[i].lanes[j])
break;
if (j == ATOMISP_CAMERA_NR_PORTS)
@@ -783,7 +754,7 @@ int atomisp_csi_lane_config(struct atomisp_device *isp)
dev_err(isp->dev,
"%s: could not find the CSI port setting for %d-%d-%d\n",
__func__,
- sensor_lanes[0], sensor_lanes[1], sensor_lanes[2]);
+ isp->sensor_lanes[0], isp->sensor_lanes[1], isp->sensor_lanes[2]);
return -EINVAL;
}
@@ -811,7 +782,7 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
{
const struct atomisp_platform_data *pdata;
struct intel_v4l2_subdev_table *subdevs;
- int ret, raw_index = -1, count;
+ int ret, mipi_port, raw_index = -1, count;
pdata = atomisp_get_platform_data();
if (!pdata) {
@@ -851,10 +822,18 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
break;
}
+ if (subdevs->port >= ATOMISP_CAMERA_NR_PORTS) {
+ dev_err(isp->dev, "port %d not supported\n", subdevs->port);
+ break;
+ }
+
isp->inputs[isp->input_cnt].type = subdevs->type;
isp->inputs[isp->input_cnt].port = subdevs->port;
isp->inputs[isp->input_cnt].camera = subdevs->subdev;
isp->input_cnt++;
+
+ mipi_port = atomisp_port_to_mipi_port(isp, subdevs->port);
+ isp->sensor_lanes[mipi_port] = subdevs->lanes;
break;
case CAMERA_MOTOR:
if (isp->motor) {
@@ -964,15 +943,6 @@ static int atomisp_register_entities(struct atomisp_device *isp)
goto subdev_register_failed;
}
- for (i = 0; i < isp->input_cnt; i++) {
- if (isp->inputs[i].port >= ATOMISP_CAMERA_NR_PORTS) {
- dev_err(isp->dev, "isp->inputs port %d not supported\n",
- isp->inputs[i].port);
- ret = -EINVAL;
- goto link_failed;
- }
- }
-
if (isp->input_cnt < ATOM_ISP_MAX_INPUTS) {
dev_dbg(isp->dev,
"TPG detected, camera_cnt: %d\n", isp->input_cnt);
@@ -985,8 +955,6 @@ static int atomisp_register_entities(struct atomisp_device *isp)
return 0;
-link_failed:
- atomisp_subdev_unregister_entities(&isp->asd);
subdev_register_failed:
atomisp_tpg_unregister_entities(&isp->tpg);
tpg_register_failed:
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 6/9] media: atomisp: Delay mapping sensors to inputs till atomisp_register_device_nodes()
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (4 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 5/9] media: atomisp: Store number of sensor lanes per port in struct atomisp_device Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 7/9] media: atomisp: Move pad linking to atomisp_register_device_nodes() Hans de Goede
` (3 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
Delay mapping sensors to inputs till atomisp_register_device_nodes()
time. There are 2 reasons for this:
1. This guarantees a stable input order independent of the sensor
probe order.
2. This is a preparation patch for v4l2-async sensor probing support.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../media/atomisp/pci/atomisp_internal.h | 1 +
.../staging/media/atomisp/pci/atomisp_v4l2.c | 83 ++++++++++---------
2 files changed, 45 insertions(+), 39 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index b8d643c9df8f..514c360d4d03 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -194,6 +194,7 @@ struct atomisp_device {
* Note this is indexed by mipi_port_id not atomisp_camera_port.
*/
int sensor_lanes[N_MIPI_PORT_ID];
+ struct v4l2_subdev *sensor_subdevs[ATOMISP_CAMERA_NR_PORTS];
unsigned int input_cnt;
struct atomisp_input_subdev inputs[ATOM_ISP_MAX_INPUTS];
struct v4l2_subdev *flash;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index b5a070f69120..c668375e04ae 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -782,7 +782,7 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
{
const struct atomisp_platform_data *pdata;
struct intel_v4l2_subdev_table *subdevs;
- int ret, mipi_port, raw_index = -1, count;
+ int ret, mipi_port, count;
pdata = atomisp_get_platform_data();
if (!pdata) {
@@ -814,26 +814,20 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
switch (subdevs->type) {
case RAW_CAMERA:
- dev_dbg(isp->dev, "raw_index: %d\n", raw_index);
- raw_index = isp->input_cnt;
- if (isp->input_cnt >= ATOM_ISP_MAX_INPUTS) {
- dev_warn(isp->dev,
- "too many atomisp inputs, ignored\n");
- break;
- }
-
if (subdevs->port >= ATOMISP_CAMERA_NR_PORTS) {
dev_err(isp->dev, "port %d not supported\n", subdevs->port);
break;
}
- isp->inputs[isp->input_cnt].type = subdevs->type;
- isp->inputs[isp->input_cnt].port = subdevs->port;
- isp->inputs[isp->input_cnt].camera = subdevs->subdev;
- isp->input_cnt++;
+ if (isp->sensor_subdevs[subdevs->port]) {
+ dev_err(isp->dev, "port %d already has a sensor attached\n",
+ subdevs->port);
+ break;
+ }
mipi_port = atomisp_port_to_mipi_port(isp, subdevs->port);
isp->sensor_lanes[mipi_port] = subdevs->lanes;
+ isp->sensor_subdevs[subdevs->port] = subdevs->subdev;
break;
case CAMERA_MOTOR:
if (isp->motor) {
@@ -855,21 +849,6 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
}
}
- /*
- * HACK: Currently VCM belongs to primary sensor only, but correct
- * approach must be to acquire from platform code which sensor
- * owns it.
- */
- if (isp->motor && raw_index >= 0)
- isp->inputs[raw_index].motor = isp->motor;
-
- /* Proceed even if no modules detected. For COS mode and no modules. */
- if (!isp->input_cnt)
- dev_warn(isp->dev, "no camera attached or fail to detect\n");
- else
- dev_info(isp->dev, "detected %d camera sensors\n",
- isp->input_cnt);
-
return atomisp_csi_lane_config(isp);
}
@@ -943,16 +922,6 @@ static int atomisp_register_entities(struct atomisp_device *isp)
goto subdev_register_failed;
}
- if (isp->input_cnt < ATOM_ISP_MAX_INPUTS) {
- dev_dbg(isp->dev,
- "TPG detected, camera_cnt: %d\n", isp->input_cnt);
- isp->inputs[isp->input_cnt].type = TEST_PATTERN;
- isp->inputs[isp->input_cnt].port = -1;
- isp->inputs[isp->input_cnt++].camera = &isp->tpg.sd;
- } else {
- dev_warn(isp->dev, "too many atomisp inputs, TPG ignored.\n");
- }
-
return 0;
subdev_register_failed:
@@ -970,7 +939,43 @@ static int atomisp_register_entities(struct atomisp_device *isp)
static int atomisp_register_device_nodes(struct atomisp_device *isp)
{
- int err;
+ struct atomisp_input_subdev *input;
+ int i, err;
+
+ for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
+ if (!isp->sensor_subdevs[i])
+ continue;
+
+ input = &isp->inputs[isp->input_cnt];
+
+ input->type = RAW_CAMERA;
+ input->port = i;
+ input->camera = isp->sensor_subdevs[i];
+
+ /*
+ * HACK: Currently VCM belongs to primary sensor only, but correct
+ * approach must be to acquire from platform code which sensor
+ * owns it.
+ */
+ if (i == ATOMISP_CAMERA_PORT_PRIMARY)
+ input->motor = isp->motor;
+
+ isp->input_cnt++;
+ }
+
+ if (!isp->input_cnt)
+ dev_warn(isp->dev, "no camera attached or fail to detect\n");
+ else
+ dev_info(isp->dev, "detected %d camera sensors\n", isp->input_cnt);
+
+ if (isp->input_cnt < ATOM_ISP_MAX_INPUTS) {
+ dev_dbg(isp->dev, "TPG detected, camera_cnt: %d\n", isp->input_cnt);
+ isp->inputs[isp->input_cnt].type = TEST_PATTERN;
+ isp->inputs[isp->input_cnt].port = -1;
+ isp->inputs[isp->input_cnt++].camera = &isp->tpg.sd;
+ } else {
+ dev_warn(isp->dev, "too many atomisp inputs, TPG ignored.\n");
+ }
isp->asd.video_out.vdev.v4l2_dev = &isp->v4l2_dev;
isp->asd.video_out.vdev.device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 7/9] media: atomisp: Move pad linking to atomisp_register_device_nodes()
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (5 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 6/9] media: atomisp: Delay mapping sensors to inputs till atomisp_register_device_nodes() Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 8/9] media: atomisp: Allow camera_mipi_info to be NULL Hans de Goede
` (2 subsequent siblings)
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
atomisp_register_device_nodes() already iterates over the ports/sensors
in a loop and that loop already does not include the TPG input.
So we can simply setup the CSI2-port <-> ISP and sensor <-> CSI2-port
mediactl-pad links there instead of repeating the loop in
atomisp_create_pads_links(), which atomisp_register_device_nodes()
used to call later on.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../media/atomisp/pci/atomisp_subdev.c | 35 -------------------
.../media/atomisp/pci/atomisp_subdev.h | 1 -
.../staging/media/atomisp/pci/atomisp_v4l2.c | 16 ++++++++-
3 files changed, 15 insertions(+), 37 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index c2ae77cd77a7..7985a0319a39 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -933,41 +933,6 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
return asd->ctrl_handler.error;
}
-int atomisp_create_pads_links(struct atomisp_device *isp)
-{
- int i, ret;
-
- for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
- ret = media_create_pad_link(&isp->csi2_port[i].subdev.entity,
- CSI2_PAD_SOURCE, &isp->asd.subdev.entity,
- ATOMISP_SUBDEV_PAD_SINK, 0);
- if (ret < 0)
- return ret;
- }
-
- for (i = 0; i < isp->input_cnt; i++) {
- /* Don't create links for the test-pattern-generator */
- if (isp->inputs[i].type == TEST_PATTERN)
- continue;
-
- ret = media_create_pad_link(&isp->inputs[i].camera->entity, 0,
- &isp->csi2_port[isp->inputs[i].
- port].subdev.entity,
- CSI2_PAD_SINK,
- MEDIA_LNK_FL_ENABLED |
- MEDIA_LNK_FL_IMMUTABLE);
- if (ret < 0)
- return ret;
- }
-
- ret = media_create_pad_link(&isp->asd.subdev.entity, ATOMISP_SUBDEV_PAD_SOURCE,
- &isp->asd.video_out.vdev.entity, 0, 0);
- if (ret < 0)
- return ret;
-
- return 0;
-}
-
static void atomisp_subdev_cleanup_entities(struct atomisp_sub_device *asd)
{
v4l2_ctrl_handler_free(&asd->ctrl_handler);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index cd82554d5f65..c9f6561dbcb6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -366,6 +366,5 @@ int atomisp_subdev_register_subdev(struct atomisp_sub_device *asd,
struct v4l2_device *vdev);
int atomisp_subdev_init(struct atomisp_device *isp);
void atomisp_subdev_cleanup(struct atomisp_device *isp);
-int atomisp_create_pads_links(struct atomisp_device *isp);
#endif /* __ATOMISP_SUBDEV_H__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index c668375e04ae..ef04effc66bf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -943,6 +943,12 @@ static int atomisp_register_device_nodes(struct atomisp_device *isp)
int i, err;
for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
+ err = media_create_pad_link(&isp->csi2_port[i].subdev.entity,
+ CSI2_PAD_SOURCE, &isp->asd.subdev.entity,
+ ATOMISP_SUBDEV_PAD_SINK, 0);
+ if (err)
+ return err;
+
if (!isp->sensor_subdevs[i])
continue;
@@ -960,6 +966,13 @@ static int atomisp_register_device_nodes(struct atomisp_device *isp)
if (i == ATOMISP_CAMERA_PORT_PRIMARY)
input->motor = isp->motor;
+ err = media_create_pad_link(&input->camera->entity, 0,
+ &isp->csi2_port[i].subdev.entity,
+ CSI2_PAD_SINK,
+ MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
+ if (err)
+ return err;
+
isp->input_cnt++;
}
@@ -983,7 +996,8 @@ static int atomisp_register_device_nodes(struct atomisp_device *isp)
if (err)
return err;
- err = atomisp_create_pads_links(isp);
+ err = media_create_pad_link(&isp->asd.subdev.entity, ATOMISP_SUBDEV_PAD_SOURCE,
+ &isp->asd.video_out.vdev.entity, 0, 0);
if (err)
return err;
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 8/9] media: atomisp: Allow camera_mipi_info to be NULL
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (6 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 7/9] media: atomisp: Move pad linking to atomisp_register_device_nodes() Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 15:37 ` [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
2023-05-18 16:19 ` [PATCH 0/9] " Andy Shevchenko
9 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
camera_mipi_info is an atomisp / atomisp_gmin_platform specific struct,
allow mipi_info pointers to be NULL.
This is a preparation patch for making atomisp work with
standard v4l2 sensor drivers.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
.../staging/media/atomisp/pci/atomisp_cmd.c | 33 ++++++++++---------
.../staging/media/atomisp/pci/atomisp_csi2.c | 6 +---
2 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index f4a0341d1f8d..5b244d173b9a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3919,6 +3919,8 @@ static inline int atomisp_set_sensor_mipi_to_isp(
const struct atomisp_in_fmt_conv *fc;
int mipi_freq = 0;
unsigned int input_format, bayer_order;
+ enum atomisp_input_format metadata_format = ATOMISP_INPUT_FORMAT_EMBEDDED;
+ u32 mipi_port, metadata_width = 0, metadata_height = 0;
ctrl.id = V4L2_CID_LINK_FREQ;
if (v4l2_g_ctrl
@@ -3946,7 +3948,7 @@ static inline int atomisp_set_sensor_mipi_to_isp(
/* Compatibility for sensors which provide no media bus code
* in s_mbus_framefmt() nor support pad formats. */
- if (mipi_info->input_format != -1) {
+ if (mipi_info && mipi_info->input_format != -1) {
bayer_order = mipi_info->raw_bayer_order;
/* Input stream config is still needs configured */
@@ -3956,6 +3958,9 @@ static inline int atomisp_set_sensor_mipi_to_isp(
if (!fc)
return -EINVAL;
input_format = fc->atomisp_in_fmt;
+ metadata_format = mipi_info->metadata_format;
+ metadata_width = mipi_info->metadata_width;
+ metadata_height = mipi_info->metadata_height;
} else {
struct v4l2_mbus_framefmt *sink;
@@ -3972,18 +3977,17 @@ static inline int atomisp_set_sensor_mipi_to_isp(
atomisp_css_input_set_format(asd, stream_id, input_format);
atomisp_css_input_set_bayer_order(asd, stream_id, bayer_order);
- fc = atomisp_find_in_fmt_conv_by_atomisp_in_fmt(
- mipi_info->metadata_format);
+ fc = atomisp_find_in_fmt_conv_by_atomisp_in_fmt(metadata_format);
if (!fc)
return -EINVAL;
+
input_format = fc->atomisp_in_fmt;
- atomisp_css_input_configure_port(asd,
- atomisp_port_to_mipi_port(asd->isp, mipi_info->port),
- mipi_info->num_lanes,
+ mipi_port = atomisp_port_to_mipi_port(isp, isp->inputs[asd->input_curr].port);
+ atomisp_css_input_configure_port(asd, mipi_port,
+ isp->sensor_lanes[mipi_port],
0xffff4, mipi_freq,
input_format,
- mipi_info->metadata_width,
- mipi_info->metadata_height);
+ metadata_width, metadata_height);
return 0;
}
@@ -4071,7 +4075,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
int (*configure_pp_input)(struct atomisp_sub_device *asd,
unsigned int width, unsigned int height) =
configure_pp_input_nop;
- const struct atomisp_in_fmt_conv *fc;
+ const struct atomisp_in_fmt_conv *fc = NULL;
int ret, i;
if (!asd) {
@@ -4093,15 +4097,14 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
if (isp->inputs[asd->input_curr].type != TEST_PATTERN) {
mipi_info = atomisp_to_sensor_mipi_info(
isp->inputs[asd->input_curr].camera);
- if (!mipi_info) {
- dev_err(isp->dev, "mipi_info is NULL\n");
- return -EINVAL;
- }
+
if (atomisp_set_sensor_mipi_to_isp(asd, ATOMISP_INPUT_STREAM_GENERAL,
mipi_info))
return -EINVAL;
- fc = atomisp_find_in_fmt_conv_by_atomisp_in_fmt(
- mipi_info->input_format);
+
+ if (mipi_info)
+ fc = atomisp_find_in_fmt_conv_by_atomisp_in_fmt(mipi_info->input_format);
+
if (!fc)
fc = atomisp_find_in_fmt_conv(
atomisp_subdev_get_ffmt(&asd->subdev,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.c b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
index b00bc0b7aaad..0045c4d3a7f6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
@@ -322,15 +322,11 @@ static void atomisp_csi2_configure_isp2401(struct atomisp_sub_device *asd)
struct v4l2_control ctrl;
struct atomisp_device *isp = asd->isp;
- struct camera_mipi_info *mipi_info;
int mipi_freq = 0;
enum atomisp_camera_port port;
-
int n;
- mipi_info = atomisp_to_sensor_mipi_info(
- isp->inputs[asd->input_curr].camera);
- port = mipi_info->port;
+ port = isp->inputs[asd->input_curr].port;
ctrl.id = V4L2_CID_LINK_FREQ;
if (v4l2_g_ctrl
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (7 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 8/9] media: atomisp: Allow camera_mipi_info to be NULL Hans de Goede
@ 2023-05-18 15:37 ` Hans de Goede
2023-05-18 17:36 ` Hans de Goede
2023-05-19 12:45 ` Andy Shevchenko
2023-05-18 16:19 ` [PATCH 0/9] " Andy Shevchenko
9 siblings, 2 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 15:37 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, linux-media, linux-staging
WIP
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/media/atomisp/Makefile | 1 +
.../staging/media/atomisp/pci/atomisp_csi2.c | 4 +
.../staging/media/atomisp/pci/atomisp_csi2.h | 64 ++
.../media/atomisp/pci/atomisp_csi2_bridge.c | 592 ++++++++++++++++++
.../media/atomisp/pci/atomisp_internal.h | 2 +
.../staging/media/atomisp/pci/atomisp_v4l2.c | 38 +-
.../staging/media/atomisp/pci/atomisp_v4l2.h | 1 +
7 files changed, 681 insertions(+), 21 deletions(-)
create mode 100644 drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
diff --git a/drivers/staging/media/atomisp/Makefile b/drivers/staging/media/atomisp/Makefile
index 532e12ed72e6..38b370124109 100644
--- a/drivers/staging/media/atomisp/Makefile
+++ b/drivers/staging/media/atomisp/Makefile
@@ -16,6 +16,7 @@ atomisp-objs += \
pci/atomisp_cmd.o \
pci/atomisp_compat_css20.o \
pci/atomisp_csi2.o \
+ pci/atomisp_csi2_bridge.o \
pci/atomisp_drvfs.o \
pci/atomisp_fops.o \
pci/atomisp_ioctl.o \
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.c b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
index 0045c4d3a7f6..abf55a86f795 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
@@ -371,6 +371,10 @@ int atomisp_mipi_csi2_init(struct atomisp_device *isp)
unsigned int i;
int ret;
+ ret = atomisp_csi2_bridge_init(isp);
+ if (ret < 0)
+ return ret;
+
for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
csi2_port = &isp->csi2_port[i];
csi2_port->isp = isp;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
index b245b2f5ce99..c714202db52a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
@@ -21,14 +21,76 @@
#include <media/v4l2-subdev.h>
#include <media/v4l2-ctrls.h>
+#include "../../include/linux/atomisp.h"
+
#define CSI2_PAD_SINK 0
#define CSI2_PAD_SOURCE 1
#define CSI2_PADS_NUM 2
+#define CSI2_MAX_LANES 4
+
struct atomisp_device;
struct v4l2_device;
struct atomisp_sub_device;
+enum atomisp_csi2_sensor_swnodes {
+ SWNODE_SENSOR,
+ SWNODE_SENSOR_PORT,
+ SWNODE_SENSOR_ENDPOINT,
+ SWNODE_CSI2_PORT,
+ SWNODE_CSI2_ENDPOINT,
+ SWNODE_COUNT
+};
+
+struct atomisp_csi2_property_names {
+ char rotation[9];
+ char bus_type[9];
+ char data_lanes[11];
+ char remote_endpoint[16];
+};
+
+struct atomisp_csi2_node_names {
+ char port[7];
+ char endpoint[11];
+ char remote_port[7];
+};
+
+struct atomisp_csi2_sensor_config {
+ const char *hid;
+ int lanes;
+};
+
+struct atomisp_csi2_sensor {
+ /* Append port in "-%u" format as suffix of HID */
+ char name[ACPI_ID_LEN + 4];
+ struct acpi_device *adev;
+ int port;
+ int lanes;
+
+ /* SWNODE_COUNT + 1 for terminating NULL */
+ const struct software_node *group[SWNODE_COUNT + 1];
+ struct software_node swnodes[SWNODE_COUNT];
+ struct atomisp_csi2_node_names node_names;
+ struct atomisp_csi2_property_names prop_names;
+ /* "rotation" + terminating entry */
+ struct property_entry dev_properties[2];
+ /* "bus-type", "data-lanes", "remote-endpoint" + terminating entry */
+ struct property_entry ep_properties[4];
+ /* "data-lanes", "remote-endpoint" + terminating entry */
+ struct property_entry csi2_properties[3];
+ struct software_node_ref_args local_ref[1];
+ struct software_node_ref_args remote_ref[1];
+ struct software_node_ref_args vcm_ref[1];
+};
+
+struct atomisp_csi2_bridge {
+ char csi2_node_name[14];
+ struct software_node csi2_node;
+ u32 data_lanes[CSI2_MAX_LANES];
+ unsigned int n_sensors;
+ struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
+};
+
struct atomisp_mipi_csi2_device {
struct v4l2_subdev subdev;
struct media_pad pads[CSI2_PADS_NUM];
@@ -48,6 +110,8 @@ void atomisp_mipi_csi2_unregister_entities(
struct atomisp_mipi_csi2_device *csi2);
int atomisp_mipi_csi2_register_entities(struct atomisp_mipi_csi2_device *csi2,
struct v4l2_device *vdev);
+int atomisp_csi2_bridge_init(struct atomisp_device *isp);
+int atomisp_csi2_bridge_parse_firmware(struct atomisp_device *isp);
void atomisp_csi2_configure(struct atomisp_sub_device *asd);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
new file mode 100644
index 000000000000..8539d7f354ae
--- /dev/null
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -0,0 +1,592 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Code to build software firmware node graph for atomisp2 connected sensors
+ * from ACPI tables.
+ *
+ * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
+ *
+ * Based on drivers/media/pci/intel/ipu3/cio2-bridge.c written by:
+ * Dan Scally <djrscally@gmail.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/dmi.h>
+#include <linux/property.h>
+#include <media/v4l2-fwnode.h>
+
+#include "atomisp_cmd.h"
+#include "atomisp_csi2.h"
+#include "atomisp_internal.h"
+
+#define NODE_SENSOR(_HID, _PROPS) \
+ ((const struct software_node) { \
+ .name = _HID, \
+ .properties = _PROPS, \
+ })
+
+#define NODE_PORT(_PORT, _SENSOR_NODE) \
+ ((const struct software_node) { \
+ .name = _PORT, \
+ .parent = _SENSOR_NODE, \
+ })
+
+#define NODE_ENDPOINT(_EP, _PORT, _PROPS) \
+ ((const struct software_node) { \
+ .name = _EP, \
+ .parent = _PORT, \
+ .properties = _PROPS, \
+ })
+
+/*
+ * Extend this array with ACPI Hardware IDs of sensors known to be working
+ * plus the number of links expected by their drivers.
+ *
+ * Do not add an entry for a sensor that is not actually supported,
+ * or which have not yet been converted to work without atomisp_gmin
+ * power-management and with v4l2-async probing.
+ */
+static const struct atomisp_csi2_sensor_config supported_sensors[] = {
+ /* GalaxyCore GC0310 */
+ { "INT0310", 1 },
+ /* Omnivision OV2680 */
+ { "OVTI2680", 1 },
+};
+
+/*
+ * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
+ * code from atomisp_gmin_platform.c.
+ * Once all sensors are moved to v4l2-async probing atomisp_gmin_platform.c can
+ * be removed and the duplication of this code goes away.
+ */
+struct gmin_cfg_var {
+ const char *acpi_dev_name;
+ const char *key;
+ const char *val;
+};
+
+static struct gmin_cfg_var lenovo_ideapad_miix_310_vars[] = {
+ /* _DSM contains the wrong CsiPort! */
+ { "OVTI2680:01", "CsiPort", "0" },
+ {}
+};
+
+static const struct dmi_system_id gmin_cfg_dmi_overrides[] = {
+ {
+ /* Lenovo Ideapad Miix 310 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+ DMI_MATCH(DMI_PRODUCT_VERSION, "MIIX 310-10"),
+ },
+ .driver_data = lenovo_ideapad_miix_310_vars,
+ },
+ {}
+};
+
+static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
+ 0x97, 0xb9, 0x88, 0x2a,
+ 0x68, 0x60, 0xa4, 0xbe);
+
+static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
+{
+ union acpi_object *obj;
+ char *val = NULL;
+ int i;
+
+ obj = acpi_evaluate_dsm_typed(adev->handle, &atomisp_dsm_guid, 0, 0,
+ NULL, ACPI_TYPE_PACKAGE);
+ if (!obj)
+ return NULL;
+
+ for (i = 0; i < obj->package.count - 1; i += 2) {
+ /* The package should only contain strings */
+ if (obj->package.elements[i].type != ACPI_TYPE_STRING ||
+ obj->package.elements[i + 1].type != ACPI_TYPE_STRING)
+ break;
+
+ if (!strcmp(obj->package.elements[i].string.pointer, key)) {
+ val = kstrdup(obj->package.elements[i + 1].string.pointer, GFP_KERNEL);
+ dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
+ break;
+ }
+ }
+
+ ACPI_FREE(obj);
+ return val;
+}
+
+static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key)
+{
+ const struct dmi_system_id *id;
+ struct gmin_cfg_var *gv;
+
+ id = dmi_first_match(gmin_cfg_dmi_overrides);
+ if (!id)
+ return NULL;
+
+ for (gv = id->driver_data; gv->acpi_dev_name; gv++) {
+ if (strcmp(gv->acpi_dev_name, acpi_dev_name(adev)))
+ continue;
+
+ if (strcmp(key, gv->key))
+ continue;
+
+ dev_info(&adev->dev, "Using DMI entry %s=%s\n", key, gv->val);
+ return kstrdup(gv->val, GFP_KERNEL);
+ }
+
+ return NULL;
+}
+
+static char *gmin_cfg_get(struct acpi_device *adev, const char *key)
+{
+ char *val;
+
+ val = gmin_cfg_get_dmi_override(adev, key);
+ if (val)
+ return val;
+
+ return gmin_cfg_get_dsm(adev, key);
+}
+
+static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int default_val)
+{
+ char *str_val;
+ long int_val;
+ int ret;
+
+ str_val = gmin_cfg_get(adev, key);
+ if (!str_val)
+ goto use_default;
+
+ ret = kstrtoul(str_val, 0, &int_val);
+ kfree(str_val);
+ if (ret)
+ goto use_default;
+
+ return int_val;
+
+use_default:
+ dev_info(&adev->dev, "Using default %s=%d\n", key, default_val);
+ return default_val;
+}
+
+static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
+{
+ char name[5];
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct acpi_buffer b_name = { sizeof(name), name };
+ union acpi_object *package, *element;
+ int clock_num = -ENOENT;
+ acpi_handle rhandle;
+ acpi_status status;
+ int i;
+
+ status = acpi_evaluate_object(adev->handle, "_PR0", NULL, &buffer);
+ if (!ACPI_SUCCESS(status))
+ return -ENOENT;
+
+ package = buffer.pointer;
+
+ if (!buffer.length || !package || package->type != ACPI_TYPE_PACKAGE)
+ goto fail;
+
+ for (i = 0; i < package->package.count; i++) {
+ element = &package->package.elements[i];
+
+ if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
+ continue;
+
+ rhandle = element->reference.handle;
+ if (!rhandle)
+ continue;
+
+ acpi_get_name(rhandle, ACPI_SINGLE_NAME, &b_name);
+
+ if (strlen(name) == 4 && !strncmp(name, "CLK", 3) &&
+ name[3] >= '0' && name[3] <= '4') {
+ clock_num = name[3] - '0';
+ break;
+ }
+ }
+fail:
+ ACPI_FREE(buffer.pointer);
+
+ return clock_num;
+}
+
+static int atomisp_csi2_get_port(struct acpi_device *adev)
+{
+ int clock_num, port;
+
+ /*
+ * Get pmc-clock number from ACPI _PR0 method and compare this to
+ * the CsiPort 1 pmc-clock used in the CHT/BYT reference designs.
+ */
+ clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
+ if (IS_ISP2401)
+ port = clock_num == 4 ? 1 : 0;
+ else
+ port = clock_num == 0 ? 1 : 0;
+
+ /* Intel DSM or DMI quirk overrides PR0 derived default */
+ port = gmin_cfg_get_int(adev, "CsiPort", port);
+
+ return port;
+}
+
+static const struct atomisp_csi2_property_names prop_names = {
+ .rotation = "rotation",
+ .bus_type = "bus-type",
+ .data_lanes = "data-lanes",
+ .remote_endpoint = "remote-endpoint",
+};
+
+static void atomisp_csi2_create_fwnode_properties(struct atomisp_csi2_sensor *sensor,
+ struct atomisp_csi2_bridge *bridge,
+ const struct atomisp_csi2_sensor_config *cfg)
+{
+ sensor->prop_names = prop_names;
+
+ sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
+ sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
+
+ sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.rotation, 0);
+
+ sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
+ V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
+ sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+ bridge->data_lanes,
+ sensor->lanes);
+ sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+ sensor->local_ref);
+
+ sensor->csi2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
+ bridge->data_lanes,
+ sensor->lanes);
+ sensor->csi2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
+ sensor->remote_ref);
+}
+
+static void atomisp_csi2_init_swnode_names(struct atomisp_csi2_sensor *sensor)
+{
+ snprintf(sensor->node_names.remote_port,
+ sizeof(sensor->node_names.remote_port),
+ SWNODE_GRAPH_PORT_NAME_FMT, sensor->port);
+ snprintf(sensor->node_names.port,
+ sizeof(sensor->node_names.port),
+ SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
+ snprintf(sensor->node_names.endpoint,
+ sizeof(sensor->node_names.endpoint),
+ SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
+}
+
+static void atomisp_csi2_init_swnode_group(struct atomisp_csi2_sensor *sensor)
+{
+ struct software_node *nodes = sensor->swnodes;
+
+ sensor->group[SWNODE_SENSOR] = &nodes[SWNODE_SENSOR];
+ sensor->group[SWNODE_SENSOR_PORT] = &nodes[SWNODE_SENSOR_PORT];
+ sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
+ sensor->group[SWNODE_CSI2_PORT] = &nodes[SWNODE_CSI2_PORT];
+ sensor->group[SWNODE_CSI2_ENDPOINT] = &nodes[SWNODE_CSI2_ENDPOINT];
+}
+
+static void atomisp_csi2_create_connection_swnodes(struct atomisp_csi2_bridge *bridge,
+ struct atomisp_csi2_sensor *sensor)
+{
+ struct software_node *nodes = sensor->swnodes;
+
+ atomisp_csi2_init_swnode_names(sensor);
+
+ nodes[SWNODE_SENSOR] = NODE_SENSOR(sensor->name,
+ sensor->dev_properties);
+ nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
+ &nodes[SWNODE_SENSOR]);
+ nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+ &nodes[SWNODE_SENSOR_PORT],
+ sensor->ep_properties);
+ nodes[SWNODE_CSI2_PORT] = NODE_PORT(sensor->node_names.remote_port,
+ &bridge->csi2_node);
+ nodes[SWNODE_CSI2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
+ &nodes[SWNODE_CSI2_PORT],
+ sensor->csi2_properties);
+
+ atomisp_csi2_init_swnode_group(sensor);
+}
+
+static void atomisp_csi2_unregister_sensors(struct atomisp_csi2_bridge *bridge)
+{
+ struct atomisp_csi2_sensor *sensor;
+ unsigned int i;
+
+ for (i = 0; i < bridge->n_sensors; i++) {
+ sensor = &bridge->sensors[i];
+ software_node_unregister_node_group(sensor->group);
+ acpi_dev_put(sensor->adev);
+ }
+}
+
+static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *cfg,
+ struct atomisp_csi2_bridge *bridge,
+ struct atomisp_device *isp)
+{
+ struct fwnode_handle *fwnode, *primary;
+ struct atomisp_csi2_sensor *sensor;
+ struct acpi_device *adev;
+ int ret;
+
+ for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
+ if (!adev->status.enabled)
+ continue;
+
+ if (bridge->n_sensors >= ATOMISP_CAMERA_NR_PORTS) {
+ dev_err(isp->dev, "Exceeded available CSI2 ports\n");
+ ret = -EINVAL;
+ goto err_put_adev;
+ }
+
+ sensor = &bridge->sensors[bridge->n_sensors];
+
+ sensor->port = atomisp_csi2_get_port(adev);
+ if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
+ dev_err(&adev->dev, "Invalid port: %d\n", sensor->port);
+ ret = -EINVAL;
+ goto err_put_adev;
+ }
+
+ sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", cfg->lanes);
+ if (sensor->lanes > CSI2_MAX_LANES) {
+ dev_err(&adev->dev, "Invalid number of lanes: %d\n", sensor->lanes);
+ ret = -EINVAL;
+ goto err_put_adev;
+ }
+
+ snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
+ cfg->hid, sensor->port);
+
+ atomisp_csi2_create_fwnode_properties(sensor, bridge, cfg);
+ atomisp_csi2_create_connection_swnodes(bridge, sensor);
+
+ ret = software_node_register_node_group(sensor->group);
+ if (ret)
+ goto err_put_adev;
+
+ fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR]);
+ if (!fwnode) {
+ ret = -ENODEV;
+ goto err_free_swnodes;
+ }
+
+ sensor->adev = acpi_dev_get(adev);
+
+ primary = acpi_fwnode_handle(adev);
+ primary->secondary = fwnode;
+
+ dev_info(isp->dev, "Found supported sensor %s\n", acpi_dev_name(adev));
+
+ bridge->n_sensors++;
+ }
+
+ return 0;
+
+err_free_swnodes:
+ software_node_unregister_node_group(sensor->group);
+err_put_adev:
+ acpi_dev_put(adev);
+ return ret;
+}
+
+static int atomisp_csi2_connect_sensors(struct atomisp_csi2_bridge *bridge,
+ struct atomisp_device *isp)
+{
+ unsigned int i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(supported_sensors); i++) {
+ const struct atomisp_csi2_sensor_config *cfg = &supported_sensors[i];
+
+ ret = atomisp_csi2_connect_sensor(cfg, bridge, isp);
+ if (ret)
+ goto err_unregister_sensors;
+ }
+
+ return 0;
+
+err_unregister_sensors:
+ atomisp_csi2_unregister_sensors(bridge);
+ return ret;
+}
+
+int atomisp_csi2_bridge_init(struct atomisp_device *isp)
+{
+ struct atomisp_csi2_bridge *bridge;
+ struct device *dev = isp->dev;
+ struct fwnode_handle *fwnode;
+ int i, ret;
+
+ /*
+ * This function is intended to run only once and then leave
+ * the created nodes attached even after a rmmod, therefor:
+ * 1. The bridge memory is leaked deliberately on success
+ * 2. If a secondary fwnode is already set exit early.
+ */
+ if (dev->fwnode && dev->fwnode->secondary)
+ return 0;
+
+ bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+ if (!bridge)
+ return -ENOMEM;
+
+ strscpy(bridge->csi2_node_name, "atomisp-csi2", sizeof(bridge->csi2_node_name));
+ bridge->csi2_node.name = bridge->csi2_node_name;
+
+ ret = software_node_register(&bridge->csi2_node);
+ if (ret < 0) {
+ dev_err(dev, "Failed to register the CSI2 HID node\n");
+ goto err_free_bridge;
+ }
+
+ /*
+ * Map the lane arrangement, which is fixed for the ISP2 (meaning we
+ * only need one, rather than one per sensor). We include it as a
+ * member of the bridge struct rather than a global variable so
+ * that it survives if the module is unloaded along with the rest of
+ * the struct.
+ */
+ for (i = 0; i < CSI2_MAX_LANES; i++)
+ bridge->data_lanes[i] = i + 1;
+
+ ret = atomisp_csi2_connect_sensors(bridge, isp);
+ if (ret || bridge->n_sensors == 0)
+ goto err_unregister_csi2;
+
+ fwnode = software_node_fwnode(&bridge->csi2_node);
+ if (!fwnode) {
+ dev_err(dev, "Error getting fwnode from csi2 software_node\n");
+ ret = -ENODEV;
+ goto err_unregister_sensors;
+ }
+
+ set_secondary_fwnode(dev, fwnode);
+
+ return 0;
+
+err_unregister_sensors:
+ atomisp_csi2_unregister_sensors(bridge);
+err_unregister_csi2:
+ software_node_unregister(&bridge->csi2_node);
+err_free_bridge:
+ kfree(bridge);
+
+ return ret;
+}
+
+/******* V4L2 sub-device asynchronous registration callbacks***********/
+
+struct sensor_async_subdev {
+ struct v4l2_async_subdev asd;
+ int port;
+};
+
+#define to_sensor_asd(a) container_of(a, struct sensor_async_subdev, asd)
+#define notifier_to_atomisp(n) container_of(n, struct atomisp_device, notifier)
+
+/* .bound() notifier callback when a match is found */
+static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *sd,
+ struct v4l2_async_subdev *asd)
+{
+ struct atomisp_device *isp = notifier_to_atomisp(notifier);
+ struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
+
+ if (s_asd->port >= ATOMISP_CAMERA_NR_PORTS) {
+ dev_err(isp->dev, "port %d not supported\n", s_asd->port);
+ return -EINVAL;
+ }
+
+ if (isp->sensor_subdevs[s_asd->port]) {
+ dev_err(isp->dev, "port %d already has a sensor attached\n", s_asd->port);
+ return -EBUSY;
+ }
+
+ isp->sensor_subdevs[s_asd->port] = sd;
+ return 0;
+}
+
+/* The .unbind callback */
+static void atomisp_notifier_unbind(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *sd,
+ struct v4l2_async_subdev *asd)
+{
+ struct atomisp_device *isp = notifier_to_atomisp(notifier);
+ struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
+
+ isp->sensor_subdevs[s_asd->port] = NULL;
+}
+
+/* .complete() is called after all subdevices have been located */
+static int atomisp_notifier_complete(struct v4l2_async_notifier *notifier)
+{
+ struct atomisp_device *isp = notifier_to_atomisp(notifier);
+
+ return atomisp_register_device_nodes(isp);
+}
+
+static const struct v4l2_async_notifier_operations atomisp_async_ops = {
+ .bound = atomisp_notifier_bound,
+ .unbind = atomisp_notifier_unbind,
+ .complete = atomisp_notifier_complete,
+};
+
+int atomisp_csi2_bridge_parse_firmware(struct atomisp_device *isp)
+{
+ int i, mipi_port, ret;
+
+ v4l2_async_nf_init(&isp->notifier);
+ isp->notifier.ops = &atomisp_async_ops;
+
+ for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
+ struct v4l2_fwnode_endpoint vep = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
+ struct sensor_async_subdev *s_asd;
+ struct fwnode_handle *ep;
+
+ ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev), i, 0,
+ FWNODE_GRAPH_ENDPOINT_NEXT);
+ if (!ep)
+ continue;
+
+ ret = v4l2_fwnode_endpoint_parse(ep, &vep);
+ if (ret)
+ goto err_parse;
+
+ if (vep.base.port >= ATOMISP_CAMERA_NR_PORTS) {
+ dev_err(isp->dev, "port %d not supported\n", vep.base.port);
+ ret = -EINVAL;
+ goto err_parse;
+ }
+
+ mipi_port = atomisp_port_to_mipi_port(isp, vep.base.port);
+ isp->sensor_lanes[mipi_port] = vep.bus.mipi_csi2.num_data_lanes;
+
+ s_asd = v4l2_async_nf_add_fwnode_remote(&isp->notifier, ep,
+ struct sensor_async_subdev);
+ if (IS_ERR(s_asd)) {
+ ret = PTR_ERR(s_asd);
+ goto err_parse;
+ }
+
+ s_asd->port = vep.base.port;
+
+ fwnode_handle_put(ep);
+ continue;
+
+err_parse:
+ fwnode_handle_put(ep);
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index 514c360d4d03..e59c0f1e7f53 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -27,6 +27,7 @@
#include <linux/idr.h>
#include <media/media-device.h>
+#include <media/v4l2-async.h>
#include <media/v4l2-subdev.h>
/* ISP2400*/
@@ -173,6 +174,7 @@ struct atomisp_device {
struct v4l2_device v4l2_dev;
struct media_device media_dev;
struct atomisp_sub_device asd;
+ struct v4l2_async_notifier notifier;
struct atomisp_platform_data *pdata;
void *mmu_l1_base;
void __iomem *base;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index ef04effc66bf..802c16697ca6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -27,6 +27,7 @@
#include <linux/dmi.h>
#include <linux/interrupt.h>
#include <linux/bits.h>
+#include <media/v4l2-fwnode.h>
#include <asm/iosf_mbi.h>
@@ -782,7 +783,11 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
{
const struct atomisp_platform_data *pdata;
struct intel_v4l2_subdev_table *subdevs;
- int ret, mipi_port, count;
+ int ret, mipi_port;
+
+ ret = atomisp_csi2_bridge_parse_firmware(isp);
+ if (ret)
+ return ret;
pdata = atomisp_get_platform_data();
if (!pdata) {
@@ -790,23 +795,12 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
return 0;
}
- /* FIXME: should return -EPROBE_DEFER if not all subdevs were probed */
- for (count = 0; count < SUBDEV_WAIT_TIMEOUT_MAX_COUNT; count++) {
- int camera_count = 0;
-
- for (subdevs = pdata->subdevs; subdevs->type; ++subdevs) {
- if (subdevs->type == RAW_CAMERA)
- camera_count++;
- }
- if (camera_count)
- break;
- msleep(SUBDEV_WAIT_TIMEOUT);
- }
- /* Wait more time to give more time for subdev init code to finish */
- msleep(5 * SUBDEV_WAIT_TIMEOUT);
-
- /* FIXME: should, instead, use I2C probe */
-
+ /*
+ * TODO: this is left here for now to allow testing atomisp-sensor
+ * drivers which are still using the atomisp_gmin_platform infra before
+ * converting them to standard v4l2 sensor drivers using runtime-pm +
+ * ACPI for pm and v4l2_async_register_subdev_sensor() registration.
+ */
for (subdevs = pdata->subdevs; subdevs->type; ++subdevs) {
ret = v4l2_device_register_subdev(&isp->v4l2_dev, subdevs->subdev);
if (ret)
@@ -937,7 +931,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
return ret;
}
-static int atomisp_register_device_nodes(struct atomisp_device *isp)
+int atomisp_register_device_nodes(struct atomisp_device *isp)
{
struct atomisp_input_subdev *input;
int i, err;
@@ -1429,9 +1423,11 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
isp->firmware = NULL;
isp->css_env.isp_css_fw.data = NULL;
- err = atomisp_register_device_nodes(isp);
- if (err)
+ err = v4l2_async_nf_register(&isp->v4l2_dev, &isp->notifier);
+ if (err) {
+ dev_err(isp->dev, "failed to register async notifier : %d\n", err);
goto css_init_fail;
+ }
atomisp_drvfs_init(isp);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
index c8ee3ad83320..fad9573374b3 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
@@ -30,5 +30,6 @@ int atomisp_video_init(struct atomisp_video_pipe *video);
void atomisp_video_unregister(struct atomisp_video_pipe *video);
const struct firmware *atomisp_load_firmware(struct atomisp_device *isp);
int atomisp_csi_lane_config(struct atomisp_device *isp);
+int atomisp_register_device_nodes(struct atomisp_device *isp);
#endif /* __ATOMISP_V4L2_H__ */
--
2.40.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-18 15:37 ` [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
@ 2023-05-18 17:36 ` Hans de Goede
2023-05-19 12:45 ` Andy Shevchenko
1 sibling, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 17:36 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko
Cc: Kate Hsuan, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
Hi,
On 5/18/23 17:37, Hans de Goede wrote:
> WIP
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
My bad I forgot to update the commit message after finishing my work on this.
Here is the new commit msg now in my local tree:
"""
media: atomisp: Add support for v4l2-async sensor registration
Add support for using v4l2-async sensor registration.
This has been tested with both the gc0310 and the ov2680 sensor drivers.
Drivers should add the ACPI HIDs they match on to the supported_sensors[]
array in the same commit as that they are convered to
v4l2_async_register_subdev_sensor().
For now it also is still possible to use the old atomisp_gmin_platform
based sensor drivers. This is mainly intended for testing while moving
"""
I have also made the supported_sensors[] array empty in my tree and
moved the addition of the INT0310 and OVTI2680 HIDs to the commits
converting the 2 sensor drivers to v4l2_async_register_subdev_sensor(),
so that there is no intermittend state where the sensors will not work.
Regards,
Hans
> ---
> drivers/staging/media/atomisp/Makefile | 1 +
> .../staging/media/atomisp/pci/atomisp_csi2.c | 4 +
> .../staging/media/atomisp/pci/atomisp_csi2.h | 64 ++
> .../media/atomisp/pci/atomisp_csi2_bridge.c | 592 ++++++++++++++++++
> .../media/atomisp/pci/atomisp_internal.h | 2 +
> .../staging/media/atomisp/pci/atomisp_v4l2.c | 38 +-
> .../staging/media/atomisp/pci/atomisp_v4l2.h | 1 +
> 7 files changed, 681 insertions(+), 21 deletions(-)
> create mode 100644 drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
>
> diff --git a/drivers/staging/media/atomisp/Makefile b/drivers/staging/media/atomisp/Makefile
> index 532e12ed72e6..38b370124109 100644
> --- a/drivers/staging/media/atomisp/Makefile
> +++ b/drivers/staging/media/atomisp/Makefile
> @@ -16,6 +16,7 @@ atomisp-objs += \
> pci/atomisp_cmd.o \
> pci/atomisp_compat_css20.o \
> pci/atomisp_csi2.o \
> + pci/atomisp_csi2_bridge.o \
> pci/atomisp_drvfs.o \
> pci/atomisp_fops.o \
> pci/atomisp_ioctl.o \
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.c b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
> index 0045c4d3a7f6..abf55a86f795 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.c
> @@ -371,6 +371,10 @@ int atomisp_mipi_csi2_init(struct atomisp_device *isp)
> unsigned int i;
> int ret;
>
> + ret = atomisp_csi2_bridge_init(isp);
> + if (ret < 0)
> + return ret;
> +
> for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
> csi2_port = &isp->csi2_port[i];
> csi2_port->isp = isp;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> index b245b2f5ce99..c714202db52a 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> @@ -21,14 +21,76 @@
> #include <media/v4l2-subdev.h>
> #include <media/v4l2-ctrls.h>
>
> +#include "../../include/linux/atomisp.h"
> +
> #define CSI2_PAD_SINK 0
> #define CSI2_PAD_SOURCE 1
> #define CSI2_PADS_NUM 2
>
> +#define CSI2_MAX_LANES 4
> +
> struct atomisp_device;
> struct v4l2_device;
> struct atomisp_sub_device;
>
> +enum atomisp_csi2_sensor_swnodes {
> + SWNODE_SENSOR,
> + SWNODE_SENSOR_PORT,
> + SWNODE_SENSOR_ENDPOINT,
> + SWNODE_CSI2_PORT,
> + SWNODE_CSI2_ENDPOINT,
> + SWNODE_COUNT
> +};
> +
> +struct atomisp_csi2_property_names {
> + char rotation[9];
> + char bus_type[9];
> + char data_lanes[11];
> + char remote_endpoint[16];
> +};
> +
> +struct atomisp_csi2_node_names {
> + char port[7];
> + char endpoint[11];
> + char remote_port[7];
> +};
> +
> +struct atomisp_csi2_sensor_config {
> + const char *hid;
> + int lanes;
> +};
> +
> +struct atomisp_csi2_sensor {
> + /* Append port in "-%u" format as suffix of HID */
> + char name[ACPI_ID_LEN + 4];
> + struct acpi_device *adev;
> + int port;
> + int lanes;
> +
> + /* SWNODE_COUNT + 1 for terminating NULL */
> + const struct software_node *group[SWNODE_COUNT + 1];
> + struct software_node swnodes[SWNODE_COUNT];
> + struct atomisp_csi2_node_names node_names;
> + struct atomisp_csi2_property_names prop_names;
> + /* "rotation" + terminating entry */
> + struct property_entry dev_properties[2];
> + /* "bus-type", "data-lanes", "remote-endpoint" + terminating entry */
> + struct property_entry ep_properties[4];
> + /* "data-lanes", "remote-endpoint" + terminating entry */
> + struct property_entry csi2_properties[3];
> + struct software_node_ref_args local_ref[1];
> + struct software_node_ref_args remote_ref[1];
> + struct software_node_ref_args vcm_ref[1];
> +};
> +
> +struct atomisp_csi2_bridge {
> + char csi2_node_name[14];
> + struct software_node csi2_node;
> + u32 data_lanes[CSI2_MAX_LANES];
> + unsigned int n_sensors;
> + struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
> +};
> +
> struct atomisp_mipi_csi2_device {
> struct v4l2_subdev subdev;
> struct media_pad pads[CSI2_PADS_NUM];
> @@ -48,6 +110,8 @@ void atomisp_mipi_csi2_unregister_entities(
> struct atomisp_mipi_csi2_device *csi2);
> int atomisp_mipi_csi2_register_entities(struct atomisp_mipi_csi2_device *csi2,
> struct v4l2_device *vdev);
> +int atomisp_csi2_bridge_init(struct atomisp_device *isp);
> +int atomisp_csi2_bridge_parse_firmware(struct atomisp_device *isp);
>
> void atomisp_csi2_configure(struct atomisp_sub_device *asd);
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> new file mode 100644
> index 000000000000..8539d7f354ae
> --- /dev/null
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> @@ -0,0 +1,592 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Code to build software firmware node graph for atomisp2 connected sensors
> + * from ACPI tables.
> + *
> + * Copyright (C) 2023 Hans de Goede <hdegoede@redhat.com>
> + *
> + * Based on drivers/media/pci/intel/ipu3/cio2-bridge.c written by:
> + * Dan Scally <djrscally@gmail.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/dmi.h>
> +#include <linux/property.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "atomisp_cmd.h"
> +#include "atomisp_csi2.h"
> +#include "atomisp_internal.h"
> +
> +#define NODE_SENSOR(_HID, _PROPS) \
> + ((const struct software_node) { \
> + .name = _HID, \
> + .properties = _PROPS, \
> + })
> +
> +#define NODE_PORT(_PORT, _SENSOR_NODE) \
> + ((const struct software_node) { \
> + .name = _PORT, \
> + .parent = _SENSOR_NODE, \
> + })
> +
> +#define NODE_ENDPOINT(_EP, _PORT, _PROPS) \
> + ((const struct software_node) { \
> + .name = _EP, \
> + .parent = _PORT, \
> + .properties = _PROPS, \
> + })
> +
> +/*
> + * Extend this array with ACPI Hardware IDs of sensors known to be working
> + * plus the number of links expected by their drivers.
> + *
> + * Do not add an entry for a sensor that is not actually supported,
> + * or which have not yet been converted to work without atomisp_gmin
> + * power-management and with v4l2-async probing.
> + */
> +static const struct atomisp_csi2_sensor_config supported_sensors[] = {
> + /* GalaxyCore GC0310 */
> + { "INT0310", 1 },
> + /* Omnivision OV2680 */
> + { "OVTI2680", 1 },
> +};
> +
> +/*
> + * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
> + * code from atomisp_gmin_platform.c.
> + * Once all sensors are moved to v4l2-async probing atomisp_gmin_platform.c can
> + * be removed and the duplication of this code goes away.
> + */
> +struct gmin_cfg_var {
> + const char *acpi_dev_name;
> + const char *key;
> + const char *val;
> +};
> +
> +static struct gmin_cfg_var lenovo_ideapad_miix_310_vars[] = {
> + /* _DSM contains the wrong CsiPort! */
> + { "OVTI2680:01", "CsiPort", "0" },
> + {}
> +};
> +
> +static const struct dmi_system_id gmin_cfg_dmi_overrides[] = {
> + {
> + /* Lenovo Ideapad Miix 310 */
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_VERSION, "MIIX 310-10"),
> + },
> + .driver_data = lenovo_ideapad_miix_310_vars,
> + },
> + {}
> +};
> +
> +static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
> + 0x97, 0xb9, 0x88, 0x2a,
> + 0x68, 0x60, 0xa4, 0xbe);
> +
> +static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
> +{
> + union acpi_object *obj;
> + char *val = NULL;
> + int i;
> +
> + obj = acpi_evaluate_dsm_typed(adev->handle, &atomisp_dsm_guid, 0, 0,
> + NULL, ACPI_TYPE_PACKAGE);
> + if (!obj)
> + return NULL;
> +
> + for (i = 0; i < obj->package.count - 1; i += 2) {
> + /* The package should only contain strings */
> + if (obj->package.elements[i].type != ACPI_TYPE_STRING ||
> + obj->package.elements[i + 1].type != ACPI_TYPE_STRING)
> + break;
> +
> + if (!strcmp(obj->package.elements[i].string.pointer, key)) {
> + val = kstrdup(obj->package.elements[i + 1].string.pointer, GFP_KERNEL);
> + dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
> + break;
> + }
> + }
> +
> + ACPI_FREE(obj);
> + return val;
> +}
> +
> +static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key)
> +{
> + const struct dmi_system_id *id;
> + struct gmin_cfg_var *gv;
> +
> + id = dmi_first_match(gmin_cfg_dmi_overrides);
> + if (!id)
> + return NULL;
> +
> + for (gv = id->driver_data; gv->acpi_dev_name; gv++) {
> + if (strcmp(gv->acpi_dev_name, acpi_dev_name(adev)))
> + continue;
> +
> + if (strcmp(key, gv->key))
> + continue;
> +
> + dev_info(&adev->dev, "Using DMI entry %s=%s\n", key, gv->val);
> + return kstrdup(gv->val, GFP_KERNEL);
> + }
> +
> + return NULL;
> +}
> +
> +static char *gmin_cfg_get(struct acpi_device *adev, const char *key)
> +{
> + char *val;
> +
> + val = gmin_cfg_get_dmi_override(adev, key);
> + if (val)
> + return val;
> +
> + return gmin_cfg_get_dsm(adev, key);
> +}
> +
> +static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int default_val)
> +{
> + char *str_val;
> + long int_val;
> + int ret;
> +
> + str_val = gmin_cfg_get(adev, key);
> + if (!str_val)
> + goto use_default;
> +
> + ret = kstrtoul(str_val, 0, &int_val);
> + kfree(str_val);
> + if (ret)
> + goto use_default;
> +
> + return int_val;
> +
> +use_default:
> + dev_info(&adev->dev, "Using default %s=%d\n", key, default_val);
> + return default_val;
> +}
> +
> +static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
> +{
> + char name[5];
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + struct acpi_buffer b_name = { sizeof(name), name };
> + union acpi_object *package, *element;
> + int clock_num = -ENOENT;
> + acpi_handle rhandle;
> + acpi_status status;
> + int i;
> +
> + status = acpi_evaluate_object(adev->handle, "_PR0", NULL, &buffer);
> + if (!ACPI_SUCCESS(status))
> + return -ENOENT;
> +
> + package = buffer.pointer;
> +
> + if (!buffer.length || !package || package->type != ACPI_TYPE_PACKAGE)
> + goto fail;
> +
> + for (i = 0; i < package->package.count; i++) {
> + element = &package->package.elements[i];
> +
> + if (element->type != ACPI_TYPE_LOCAL_REFERENCE)
> + continue;
> +
> + rhandle = element->reference.handle;
> + if (!rhandle)
> + continue;
> +
> + acpi_get_name(rhandle, ACPI_SINGLE_NAME, &b_name);
> +
> + if (strlen(name) == 4 && !strncmp(name, "CLK", 3) &&
> + name[3] >= '0' && name[3] <= '4') {
> + clock_num = name[3] - '0';
> + break;
> + }
> + }
> +fail:
> + ACPI_FREE(buffer.pointer);
> +
> + return clock_num;
> +}
> +
> +static int atomisp_csi2_get_port(struct acpi_device *adev)
> +{
> + int clock_num, port;
> +
> + /*
> + * Get pmc-clock number from ACPI _PR0 method and compare this to
> + * the CsiPort 1 pmc-clock used in the CHT/BYT reference designs.
> + */
> + clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
> + if (IS_ISP2401)
> + port = clock_num == 4 ? 1 : 0;
> + else
> + port = clock_num == 0 ? 1 : 0;
> +
> + /* Intel DSM or DMI quirk overrides PR0 derived default */
> + port = gmin_cfg_get_int(adev, "CsiPort", port);
> +
> + return port;
> +}
> +
> +static const struct atomisp_csi2_property_names prop_names = {
> + .rotation = "rotation",
> + .bus_type = "bus-type",
> + .data_lanes = "data-lanes",
> + .remote_endpoint = "remote-endpoint",
> +};
> +
> +static void atomisp_csi2_create_fwnode_properties(struct atomisp_csi2_sensor *sensor,
> + struct atomisp_csi2_bridge *bridge,
> + const struct atomisp_csi2_sensor_config *cfg)
> +{
> + sensor->prop_names = prop_names;
> +
> + sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
> + sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
> +
> + sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.rotation, 0);
> +
> + sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
> + V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> + sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> + bridge->data_lanes,
> + sensor->lanes);
> + sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> + sensor->local_ref);
> +
> + sensor->csi2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> + bridge->data_lanes,
> + sensor->lanes);
> + sensor->csi2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> + sensor->remote_ref);
> +}
> +
> +static void atomisp_csi2_init_swnode_names(struct atomisp_csi2_sensor *sensor)
> +{
> + snprintf(sensor->node_names.remote_port,
> + sizeof(sensor->node_names.remote_port),
> + SWNODE_GRAPH_PORT_NAME_FMT, sensor->port);
> + snprintf(sensor->node_names.port,
> + sizeof(sensor->node_names.port),
> + SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
> + snprintf(sensor->node_names.endpoint,
> + sizeof(sensor->node_names.endpoint),
> + SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
> +}
> +
> +static void atomisp_csi2_init_swnode_group(struct atomisp_csi2_sensor *sensor)
> +{
> + struct software_node *nodes = sensor->swnodes;
> +
> + sensor->group[SWNODE_SENSOR] = &nodes[SWNODE_SENSOR];
> + sensor->group[SWNODE_SENSOR_PORT] = &nodes[SWNODE_SENSOR_PORT];
> + sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
> + sensor->group[SWNODE_CSI2_PORT] = &nodes[SWNODE_CSI2_PORT];
> + sensor->group[SWNODE_CSI2_ENDPOINT] = &nodes[SWNODE_CSI2_ENDPOINT];
> +}
> +
> +static void atomisp_csi2_create_connection_swnodes(struct atomisp_csi2_bridge *bridge,
> + struct atomisp_csi2_sensor *sensor)
> +{
> + struct software_node *nodes = sensor->swnodes;
> +
> + atomisp_csi2_init_swnode_names(sensor);
> +
> + nodes[SWNODE_SENSOR] = NODE_SENSOR(sensor->name,
> + sensor->dev_properties);
> + nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> + &nodes[SWNODE_SENSOR]);
> + nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> + &nodes[SWNODE_SENSOR_PORT],
> + sensor->ep_properties);
> + nodes[SWNODE_CSI2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> + &bridge->csi2_node);
> + nodes[SWNODE_CSI2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> + &nodes[SWNODE_CSI2_PORT],
> + sensor->csi2_properties);
> +
> + atomisp_csi2_init_swnode_group(sensor);
> +}
> +
> +static void atomisp_csi2_unregister_sensors(struct atomisp_csi2_bridge *bridge)
> +{
> + struct atomisp_csi2_sensor *sensor;
> + unsigned int i;
> +
> + for (i = 0; i < bridge->n_sensors; i++) {
> + sensor = &bridge->sensors[i];
> + software_node_unregister_node_group(sensor->group);
> + acpi_dev_put(sensor->adev);
> + }
> +}
> +
> +static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *cfg,
> + struct atomisp_csi2_bridge *bridge,
> + struct atomisp_device *isp)
> +{
> + struct fwnode_handle *fwnode, *primary;
> + struct atomisp_csi2_sensor *sensor;
> + struct acpi_device *adev;
> + int ret;
> +
> + for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> + if (!adev->status.enabled)
> + continue;
> +
> + if (bridge->n_sensors >= ATOMISP_CAMERA_NR_PORTS) {
> + dev_err(isp->dev, "Exceeded available CSI2 ports\n");
> + ret = -EINVAL;
> + goto err_put_adev;
> + }
> +
> + sensor = &bridge->sensors[bridge->n_sensors];
> +
> + sensor->port = atomisp_csi2_get_port(adev);
> + if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
> + dev_err(&adev->dev, "Invalid port: %d\n", sensor->port);
> + ret = -EINVAL;
> + goto err_put_adev;
> + }
> +
> + sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", cfg->lanes);
> + if (sensor->lanes > CSI2_MAX_LANES) {
> + dev_err(&adev->dev, "Invalid number of lanes: %d\n", sensor->lanes);
> + ret = -EINVAL;
> + goto err_put_adev;
> + }
> +
> + snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> + cfg->hid, sensor->port);
> +
> + atomisp_csi2_create_fwnode_properties(sensor, bridge, cfg);
> + atomisp_csi2_create_connection_swnodes(bridge, sensor);
> +
> + ret = software_node_register_node_group(sensor->group);
> + if (ret)
> + goto err_put_adev;
> +
> + fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR]);
> + if (!fwnode) {
> + ret = -ENODEV;
> + goto err_free_swnodes;
> + }
> +
> + sensor->adev = acpi_dev_get(adev);
> +
> + primary = acpi_fwnode_handle(adev);
> + primary->secondary = fwnode;
> +
> + dev_info(isp->dev, "Found supported sensor %s\n", acpi_dev_name(adev));
> +
> + bridge->n_sensors++;
> + }
> +
> + return 0;
> +
> +err_free_swnodes:
> + software_node_unregister_node_group(sensor->group);
> +err_put_adev:
> + acpi_dev_put(adev);
> + return ret;
> +}
> +
> +static int atomisp_csi2_connect_sensors(struct atomisp_csi2_bridge *bridge,
> + struct atomisp_device *isp)
> +{
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(supported_sensors); i++) {
> + const struct atomisp_csi2_sensor_config *cfg = &supported_sensors[i];
> +
> + ret = atomisp_csi2_connect_sensor(cfg, bridge, isp);
> + if (ret)
> + goto err_unregister_sensors;
> + }
> +
> + return 0;
> +
> +err_unregister_sensors:
> + atomisp_csi2_unregister_sensors(bridge);
> + return ret;
> +}
> +
> +int atomisp_csi2_bridge_init(struct atomisp_device *isp)
> +{
> + struct atomisp_csi2_bridge *bridge;
> + struct device *dev = isp->dev;
> + struct fwnode_handle *fwnode;
> + int i, ret;
> +
> + /*
> + * This function is intended to run only once and then leave
> + * the created nodes attached even after a rmmod, therefor:
> + * 1. The bridge memory is leaked deliberately on success
> + * 2. If a secondary fwnode is already set exit early.
> + */
> + if (dev->fwnode && dev->fwnode->secondary)
> + return 0;
> +
> + bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> + if (!bridge)
> + return -ENOMEM;
> +
> + strscpy(bridge->csi2_node_name, "atomisp-csi2", sizeof(bridge->csi2_node_name));
> + bridge->csi2_node.name = bridge->csi2_node_name;
> +
> + ret = software_node_register(&bridge->csi2_node);
> + if (ret < 0) {
> + dev_err(dev, "Failed to register the CSI2 HID node\n");
> + goto err_free_bridge;
> + }
> +
> + /*
> + * Map the lane arrangement, which is fixed for the ISP2 (meaning we
> + * only need one, rather than one per sensor). We include it as a
> + * member of the bridge struct rather than a global variable so
> + * that it survives if the module is unloaded along with the rest of
> + * the struct.
> + */
> + for (i = 0; i < CSI2_MAX_LANES; i++)
> + bridge->data_lanes[i] = i + 1;
> +
> + ret = atomisp_csi2_connect_sensors(bridge, isp);
> + if (ret || bridge->n_sensors == 0)
> + goto err_unregister_csi2;
> +
> + fwnode = software_node_fwnode(&bridge->csi2_node);
> + if (!fwnode) {
> + dev_err(dev, "Error getting fwnode from csi2 software_node\n");
> + ret = -ENODEV;
> + goto err_unregister_sensors;
> + }
> +
> + set_secondary_fwnode(dev, fwnode);
> +
> + return 0;
> +
> +err_unregister_sensors:
> + atomisp_csi2_unregister_sensors(bridge);
> +err_unregister_csi2:
> + software_node_unregister(&bridge->csi2_node);
> +err_free_bridge:
> + kfree(bridge);
> +
> + return ret;
> +}
> +
> +/******* V4L2 sub-device asynchronous registration callbacks***********/
> +
> +struct sensor_async_subdev {
> + struct v4l2_async_subdev asd;
> + int port;
> +};
> +
> +#define to_sensor_asd(a) container_of(a, struct sensor_async_subdev, asd)
> +#define notifier_to_atomisp(n) container_of(n, struct atomisp_device, notifier)
> +
> +/* .bound() notifier callback when a match is found */
> +static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct atomisp_device *isp = notifier_to_atomisp(notifier);
> + struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
> +
> + if (s_asd->port >= ATOMISP_CAMERA_NR_PORTS) {
> + dev_err(isp->dev, "port %d not supported\n", s_asd->port);
> + return -EINVAL;
> + }
> +
> + if (isp->sensor_subdevs[s_asd->port]) {
> + dev_err(isp->dev, "port %d already has a sensor attached\n", s_asd->port);
> + return -EBUSY;
> + }
> +
> + isp->sensor_subdevs[s_asd->port] = sd;
> + return 0;
> +}
> +
> +/* The .unbind callback */
> +static void atomisp_notifier_unbind(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *sd,
> + struct v4l2_async_subdev *asd)
> +{
> + struct atomisp_device *isp = notifier_to_atomisp(notifier);
> + struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
> +
> + isp->sensor_subdevs[s_asd->port] = NULL;
> +}
> +
> +/* .complete() is called after all subdevices have been located */
> +static int atomisp_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct atomisp_device *isp = notifier_to_atomisp(notifier);
> +
> + return atomisp_register_device_nodes(isp);
> +}
> +
> +static const struct v4l2_async_notifier_operations atomisp_async_ops = {
> + .bound = atomisp_notifier_bound,
> + .unbind = atomisp_notifier_unbind,
> + .complete = atomisp_notifier_complete,
> +};
> +
> +int atomisp_csi2_bridge_parse_firmware(struct atomisp_device *isp)
> +{
> + int i, mipi_port, ret;
> +
> + v4l2_async_nf_init(&isp->notifier);
> + isp->notifier.ops = &atomisp_async_ops;
> +
> + for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + struct sensor_async_subdev *s_asd;
> + struct fwnode_handle *ep;
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(isp->dev), i, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + continue;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret)
> + goto err_parse;
> +
> + if (vep.base.port >= ATOMISP_CAMERA_NR_PORTS) {
> + dev_err(isp->dev, "port %d not supported\n", vep.base.port);
> + ret = -EINVAL;
> + goto err_parse;
> + }
> +
> + mipi_port = atomisp_port_to_mipi_port(isp, vep.base.port);
> + isp->sensor_lanes[mipi_port] = vep.bus.mipi_csi2.num_data_lanes;
> +
> + s_asd = v4l2_async_nf_add_fwnode_remote(&isp->notifier, ep,
> + struct sensor_async_subdev);
> + if (IS_ERR(s_asd)) {
> + ret = PTR_ERR(s_asd);
> + goto err_parse;
> + }
> +
> + s_asd->port = vep.base.port;
> +
> + fwnode_handle_put(ep);
> + continue;
> +
> +err_parse:
> + fwnode_handle_put(ep);
> + return ret;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> index 514c360d4d03..e59c0f1e7f53 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> @@ -27,6 +27,7 @@
> #include <linux/idr.h>
>
> #include <media/media-device.h>
> +#include <media/v4l2-async.h>
> #include <media/v4l2-subdev.h>
>
> /* ISP2400*/
> @@ -173,6 +174,7 @@ struct atomisp_device {
> struct v4l2_device v4l2_dev;
> struct media_device media_dev;
> struct atomisp_sub_device asd;
> + struct v4l2_async_notifier notifier;
> struct atomisp_platform_data *pdata;
> void *mmu_l1_base;
> void __iomem *base;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index ef04effc66bf..802c16697ca6 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -27,6 +27,7 @@
> #include <linux/dmi.h>
> #include <linux/interrupt.h>
> #include <linux/bits.h>
> +#include <media/v4l2-fwnode.h>
>
> #include <asm/iosf_mbi.h>
>
> @@ -782,7 +783,11 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
> {
> const struct atomisp_platform_data *pdata;
> struct intel_v4l2_subdev_table *subdevs;
> - int ret, mipi_port, count;
> + int ret, mipi_port;
> +
> + ret = atomisp_csi2_bridge_parse_firmware(isp);
> + if (ret)
> + return ret;
>
> pdata = atomisp_get_platform_data();
> if (!pdata) {
> @@ -790,23 +795,12 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
> return 0;
> }
>
> - /* FIXME: should return -EPROBE_DEFER if not all subdevs were probed */
> - for (count = 0; count < SUBDEV_WAIT_TIMEOUT_MAX_COUNT; count++) {
> - int camera_count = 0;
> -
> - for (subdevs = pdata->subdevs; subdevs->type; ++subdevs) {
> - if (subdevs->type == RAW_CAMERA)
> - camera_count++;
> - }
> - if (camera_count)
> - break;
> - msleep(SUBDEV_WAIT_TIMEOUT);
> - }
> - /* Wait more time to give more time for subdev init code to finish */
> - msleep(5 * SUBDEV_WAIT_TIMEOUT);
> -
> - /* FIXME: should, instead, use I2C probe */
> -
> + /*
> + * TODO: this is left here for now to allow testing atomisp-sensor
> + * drivers which are still using the atomisp_gmin_platform infra before
> + * converting them to standard v4l2 sensor drivers using runtime-pm +
> + * ACPI for pm and v4l2_async_register_subdev_sensor() registration.
> + */
> for (subdevs = pdata->subdevs; subdevs->type; ++subdevs) {
> ret = v4l2_device_register_subdev(&isp->v4l2_dev, subdevs->subdev);
> if (ret)
> @@ -937,7 +931,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
> return ret;
> }
>
> -static int atomisp_register_device_nodes(struct atomisp_device *isp)
> +int atomisp_register_device_nodes(struct atomisp_device *isp)
> {
> struct atomisp_input_subdev *input;
> int i, err;
> @@ -1429,9 +1423,11 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
> isp->firmware = NULL;
> isp->css_env.isp_css_fw.data = NULL;
>
> - err = atomisp_register_device_nodes(isp);
> - if (err)
> + err = v4l2_async_nf_register(&isp->v4l2_dev, &isp->notifier);
> + if (err) {
> + dev_err(isp->dev, "failed to register async notifier : %d\n", err);
> goto css_init_fail;
> + }
>
> atomisp_drvfs_init(isp);
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
> index c8ee3ad83320..fad9573374b3 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.h
> @@ -30,5 +30,6 @@ int atomisp_video_init(struct atomisp_video_pipe *video);
> void atomisp_video_unregister(struct atomisp_video_pipe *video);
> const struct firmware *atomisp_load_firmware(struct atomisp_device *isp);
> int atomisp_csi_lane_config(struct atomisp_device *isp);
> +int atomisp_register_device_nodes(struct atomisp_device *isp);
>
> #endif /* __ATOMISP_V4L2_H__ */
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-18 15:37 ` [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
2023-05-18 17:36 ` Hans de Goede
@ 2023-05-19 12:45 ` Andy Shevchenko
2023-05-19 13:19 ` Hans de Goede
1 sibling, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-19 12:45 UTC (permalink / raw)
To: Hans de Goede
Cc: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko, Kate Hsuan,
Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
On Thu, May 18, 2023 at 6:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
...
> +static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
> + 0x97, 0xb9, 0x88, 0x2a,
> + 0x68, 0x60, 0xa4, 0xbe);
Can we use the de facto pattern for this kind of assignments?
... guid_t foo =
<TAB>GUID_INIT(...first 3 parameters...
<TAB><TAB>[spaces if needed)...last 8 parameters...);
?
Also would be nice to have a comment where the GUID is represented in
text format so it can be easily googled/searched for in
internet/documentation.
...
> + for (i = 0; i < obj->package.count - 1; i += 2) {
> + /* The package should only contain strings */
> + if (obj->package.elements[i].type != ACPI_TYPE_STRING ||
i + 0 ?
> + obj->package.elements[i + 1].type != ACPI_TYPE_STRING)
> + break;
> +
> + if (!strcmp(obj->package.elements[i].string.pointer, key)) {
Ditto?
> + val = kstrdup(obj->package.elements[i + 1].string.pointer, GFP_KERNEL);
> + dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
> + break;
> + }
I would even go for temporary for element pointer
... *elem0 = &[i + 0];
... *elem1 = &[i + 1];
> + }
...
> +use_default:
out_use_default:
...
> + status = acpi_evaluate_object(adev->handle, "_PR0", NULL, &buffer);
acpi_evaluate_object_typed()
> + if (!ACPI_SUCCESS(status))
> + return -ENOENT;
...
> + if (!buffer.length || !package || package->type != ACPI_TYPE_PACKAGE)
See above.
> + goto fail;
...
> + if (strlen(name) == 4 && !strncmp(name, "CLK", 3) &&
strlen() assumes that name is NUL-terminated, hence it can be simply
replaced with name[5] == '\0' check which can go at the end of
conditional, so that it's also implied in strncmp() for the start of
the string, but why not using str_has_prefix()?
> + name[3] >= '0' && name[3] <= '4') {
It's also possible to have it done via kstrtou8() that does almost all
checks along with conversion. You will only need to check for > 4.
> + clock_num = name[3] - '0';
> + break;
> + }
> + }
Altogether
if (str_has_prefix()) {
ret = kstrto...(&clock_num);
if (ret)
...
check for clock_num range if needed.
}
Yes it's longer in code.
...
> +fail:
err_free_pointer:
(It will be also in align with the rest of the code AFAICS)
> + ACPI_FREE(buffer.pointer);
> +
> + return clock_num;
...
> + /* Intel DSM or DMI quirk overrides PR0 derived default */
> + port = gmin_cfg_get_int(adev, "CsiPort", port);
> +
> + return port;
return gmin_...;
...
> + if (dev->fwnode && dev->fwnode->secondary)
Please, use dev_fwnode() instead of direct access to the fwnode in
struct device.
> + return 0;
...
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
I would add a trailing comma here.
> + };
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-19 12:45 ` Andy Shevchenko
@ 2023-05-19 13:19 ` Hans de Goede
0 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-19 13:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko, Kate Hsuan,
Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
Hi Andy,
As always thank you for the review. I know you spend a lot of time
on reviews and it is much appreciated!
I agree with all your comments and I'll address them as suggested
for the next version.
Regards,
Hans
On 5/19/23 14:45, Andy Shevchenko wrote:
> On Thu, May 18, 2023 at 6:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> ...
>
>> +static const guid_t atomisp_dsm_guid = GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
>> + 0x97, 0xb9, 0x88, 0x2a,
>> + 0x68, 0x60, 0xa4, 0xbe);
>
> Can we use the de facto pattern for this kind of assignments?
>
> ... guid_t foo =
> <TAB>GUID_INIT(...first 3 parameters...
> <TAB><TAB>[spaces if needed)...last 8 parameters...);
> ?
>
> Also would be nice to have a comment where the GUID is represented in
> text format so it can be easily googled/searched for in
> internet/documentation.
>
> ...
>
>> + for (i = 0; i < obj->package.count - 1; i += 2) {
>> + /* The package should only contain strings */
>
>> + if (obj->package.elements[i].type != ACPI_TYPE_STRING ||
>
> i + 0 ?
>
>> + obj->package.elements[i + 1].type != ACPI_TYPE_STRING)
>> + break;
>> +
>> + if (!strcmp(obj->package.elements[i].string.pointer, key)) {
>
> Ditto?
>
>> + val = kstrdup(obj->package.elements[i + 1].string.pointer, GFP_KERNEL);
>> + dev_info(&adev->dev, "Using DSM entry %s=%s\n", key, val);
>> + break;
>> + }
>
> I would even go for temporary for element pointer
>
> ... *elem0 = &[i + 0];
> ... *elem1 = &[i + 1];
>
>> + }
>
> ...
>
>> +use_default:
>
> out_use_default:
>
> ...
>
>> + status = acpi_evaluate_object(adev->handle, "_PR0", NULL, &buffer);
>
> acpi_evaluate_object_typed()
>
>> + if (!ACPI_SUCCESS(status))
>> + return -ENOENT;
>
> ...
>
>> + if (!buffer.length || !package || package->type != ACPI_TYPE_PACKAGE)
>
> See above.
>
>> + goto fail;
>
> ...
>
>> + if (strlen(name) == 4 && !strncmp(name, "CLK", 3) &&
>
> strlen() assumes that name is NUL-terminated, hence it can be simply
> replaced with name[5] == '\0' check which can go at the end of
> conditional, so that it's also implied in strncmp() for the start of
> the string, but why not using str_has_prefix()?
>
>> + name[3] >= '0' && name[3] <= '4') {
>
> It's also possible to have it done via kstrtou8() that does almost all
> checks along with conversion. You will only need to check for > 4.
>
>> + clock_num = name[3] - '0';
>> + break;
>> + }
>> + }
>
> Altogether
>
> if (str_has_prefix()) {
> ret = kstrto...(&clock_num);
> if (ret)
> ...
> check for clock_num range if needed.
> }
>
> Yes it's longer in code.
>
> ...
>
>> +fail:
>
> err_free_pointer:
> (It will be also in align with the rest of the code AFAICS)
>
>> + ACPI_FREE(buffer.pointer);
>> +
>> + return clock_num;
>
> ...
>
>> + /* Intel DSM or DMI quirk overrides PR0 derived default */
>> + port = gmin_cfg_get_int(adev, "CsiPort", port);
>> +
>> + return port;
>
> return gmin_...;
>
> ...
>
>> + if (dev->fwnode && dev->fwnode->secondary)
>
> Please, use dev_fwnode() instead of direct access to the fwnode in
> struct device.
>
>> + return 0;
>
> ...
>
>> + struct v4l2_fwnode_endpoint vep = {
>> + .bus_type = V4L2_MBUS_CSI2_DPHY
>
> I would add a trailing comma here.
>
>> + };
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-18 15:37 [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
` (8 preceding siblings ...)
2023-05-18 15:37 ` [PATCH 9/9] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
@ 2023-05-18 16:19 ` Andy Shevchenko
2023-05-18 16:36 ` Hans de Goede
9 siblings, 1 reply; 16+ messages in thread
From: Andy Shevchenko @ 2023-05-18 16:19 UTC (permalink / raw)
To: Hans de Goede
Cc: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko, Kate Hsuan,
Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
On Thu, May 18, 2023 at 6:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> I'm quite happy to present this patch series which makes it possible
> to use v4l2-async sensor registration together with the atomisp code :)
>
> This has been tested with both the gc0310 and the ov2680 sensor drivers.
>
> For now it also is still possible to use the old atomisp_gmin_platform
> based sensor drivers. This is mainly intended for testing while moving
> other sensor drivers over to runtime-pm + v4l2-async.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
for non-commented ones or in case you resolve remarks my way.
Definitely this does not apply to patch 9/9 which is WIP and requires
more review, I'm still planning to go through it (not done yet).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 0/9] media: atomisp: Add support for v4l2-async sensor registration
2023-05-18 16:19 ` [PATCH 0/9] " Andy Shevchenko
@ 2023-05-18 16:36 ` Hans de Goede
0 siblings, 0 replies; 16+ messages in thread
From: Hans de Goede @ 2023-05-18 16:36 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Mauro Carvalho Chehab, Sakari Ailus, Andy Shevchenko, Kate Hsuan,
Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov,
Fabio Aiuto, linux-media, linux-staging
Hi,
On 5/18/23 18:19, Andy Shevchenko wrote:
> On Thu, May 18, 2023 at 6:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> I'm quite happy to present this patch series which makes it possible
>> to use v4l2-async sensor registration together with the atomisp code :)
>>
>> This has been tested with both the gc0310 and the ov2680 sensor drivers.
>>
>> For now it also is still possible to use the old atomisp_gmin_platform
>> based sensor drivers. This is mainly intended for testing while moving
>> other sensor drivers over to runtime-pm + v4l2-async.
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> for non-commented ones or in case you resolve remarks my way.
>
> Definitely this does not apply to patch 9/9 which is WIP and requires
> more review, I'm still planning to go through it (not done yet).
Ugh, my bad I forgot to change the commit message of 9/9 from
my pov other then the commit message 9/9 is ready for merging
I have tested 9/9 with both the gc0310 and ov2680 drivers with
all mixes of 0 / 1 / both sensors using async (and the others
"classic" atomisp) enumeration and that all works well.
Regards,
Hans
^ permalink raw reply [flat|nested] 16+ messages in thread