Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sylwester, On Thu, Sep 20, 2018 at 11:01:26PM +0200, Sylwester Nawrocki wrote: > Hi Sakari, > > On 09/16/2018 12:52 AM, Sakari Ailus wrote: > > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > > entities) to what is fairly certainly known to be unique in the system, > > even if there were more devices of the same kind. > > > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > > the name to the name of the driver or the module while omitting the > > device's I²C address and bus, leaving the devices with a static name and > > effectively limiting the number of such devices in a media device to 1. > > > > Address this by using the name set by the V4L2 framework. > > > > Signed-off-by: Sakari Ailus > > Reviewed-by: Akinobu Mita (ov9650) > > I'm not against this patch but please don't expect an ack from me as this > patch breaks existing user space code, scripts using media-ctl, etc. will > need to be updated after kernel upgrade. I'm mostly concerned about ov9650 > as other drivers are likely only used by Samsung or are obsoleted. That is a fair point and also why I originally sent the patch out as RFC... I checked that OV9650 is around 14 years (!) old now, so it's unlikely to appear in modern hardware in dual configurations. I think this patch thus probably causes more trouble than it has chances of fixing anything. I'll submit one that adds a note that the convention is not to be used in new drivers instead --- unless someone strongly feels this patch really should be merged. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com
Re: [RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sakari, On 09/16/2018 12:52 AM, Sakari Ailus wrote: > v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as > entities) to what is fairly certainly known to be unique in the system, > even if there were more devices of the same kind. > > These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set > the name to the name of the driver or the module while omitting the > device's I²C address and bus, leaving the devices with a static name and > effectively limiting the number of such devices in a media device to 1. > > Address this by using the name set by the V4L2 framework. > > Signed-off-by: Sakari Ailus > Reviewed-by: Akinobu Mita (ov9650) I'm not against this patch but please don't expect an ack from me as this patch breaks existing user space code, scripts using media-ctl, etc. will need to be updated after kernel upgrade. I'm mostly concerned about ov9650 as other drivers are likely only used by Samsung or are obsoleted. -- Thanks, Sylwester
Re: [PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
Hi Sakari,
I love your patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20180913]
[cannot apply to v4.19-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Sakari-Ailus/v4l-samsung-ov9650-Rely-on-V4L2-set-sub-device-names/20180916-232558
base: git://linuxtv.org/media_tree.git master
config: i386-randconfig-x0-09171846 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/media/i2c/s5c73m3/s5c73m3-core.c: In function 's5c73m3_probe':
>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:1686:2: error: implicit declaration
>> of function 'v4l2_i2c_subdev_set_name'
>> [-Werror=implicit-function-declaration]
v4l2_i2c_subdev_set_name(sd, client, NULL, NULL);
^
cc1: some warnings being treated as errors
vim +/v4l2_i2c_subdev_set_name +1686 drivers/media/i2c/s5c73m3/s5c73m3-core.c
1660
1661 static int s5c73m3_probe(struct i2c_client *client,
1662 const struct i2c_device_id *id)
1663 {
1664 struct device *dev = >dev;
1665 struct v4l2_subdev *sd;
1666 struct v4l2_subdev *oif_sd;
1667 struct s5c73m3 *state;
1668 int ret, i;
1669
1670 state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
1671 if (!state)
1672 return -ENOMEM;
1673
1674 state->i2c_client = client;
1675 ret = s5c73m3_get_platform_data(state);
1676 if (ret < 0)
1677 return ret;
1678
1679 mutex_init(>lock);
1680 sd = >sensor_sd;
1681 oif_sd = >oif_sd;
1682
1683 v4l2_subdev_init(sd, _subdev_ops);
1684 sd->owner = client->dev.driver->owner;
1685 v4l2_set_subdevdata(sd, state);
> 1686 v4l2_i2c_subdev_set_name(sd, client, NULL, NULL);
1687
1688 sd->internal_ops = _internal_ops;
1689 sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
1690
1691 state->sensor_pads[S5C73M3_JPEG_PAD].flags =
MEDIA_PAD_FL_SOURCE;
1692 state->sensor_pads[S5C73M3_ISP_PAD].flags = MEDIA_PAD_FL_SOURCE;
1693 sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
1694
1695 ret = media_entity_pads_init(>entity, S5C73M3_NUM_PADS,
1696
state->sensor_pads);
1697 if (ret < 0)
1698 return ret;
1699
1700 v4l2_i2c_subdev_init(oif_sd, client, _subdev_ops);
1701 v4l2_i2c_subdev_set_name(sd, client, NULL, "-oif");
1702
1703 oif_sd->internal_ops = _internal_ops;
1704 oif_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
1705
1706 state->oif_pads[OIF_ISP_PAD].flags = MEDIA_PAD_FL_SINK;
1707 state->oif_pads[OIF_JPEG_PAD].flags = MEDIA_PAD_FL_SINK;
1708 state->oif_pads[OIF_SOURCE_PAD].flags = MEDIA_PAD_FL_SOURCE;
1709 oif_sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_SCALER;
1710
1711 ret = media_entity_pads_init(_sd->entity, OIF_NUM_PADS,
1712
state->oif_pads);
1713 if (ret < 0)
1714 return ret;
1715
1716 ret = s5c73m3_configure_gpios(state);
1717 if (ret)
1718 goto out_err;
1719
1720 for (i = 0; i < S5C73M3_MAX_SUPPLIES; i++)
1721 state->supplies[i].supply = s5c73m3_supply_names[i];
1722
1723 ret = devm_regulator_bulk_get(dev, S5C73M3_MAX_SUPPLIES,
1724 state->supplies);
1725 if (ret) {
1726 dev_err(dev, "failed to get regulators\n");
1727 goto out_err;
1728 }
1729
1730 ret = s5c73m3_init_controls(state);
1731 if (ret)
1732 goto out_err;
1733
1734 state->sensor_pix_size[RES_ISP] = _isp_resolutions[1];
1735 state->sensor_pix_size[RES_JPEG] = _jpeg_resolutions[1];
1736 state->oif_pix_size[RES_ISP] = state->sensor_pix_size[RES_ISP];
1737 state->oif_pix_size[RES_JPEG] =
state->sensor_pix_size[RES_JPEG];
1738
1739 state->mbus_code = S5C73M3_ISP_FMT;
1740
1741 state->fiv = _intervals[S5C73M3_DEFAULT_FRAME_INTERVAL];
1742
1743 state->fw_file_version[0] = 'G';
1744 state->fw_file_version[1] = 'C';
1745
1746 ret = s5c73m3_register_spi_driver(state);
1747 if (ret < 0)
1748 goto out_err;
1749
1750 oif_sd->dev = dev;
1751
1752 ret =
[RESEND PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as entities) to what is fairly certainly known to be unique in the system, even if there were more devices of the same kind. These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set the name to the name of the driver or the module while omitting the device's I²C address and bus, leaving the devices with a static name and effectively limiting the number of such devices in a media device to 1. Address this by using the name set by the V4L2 framework. Signed-off-by: Sakari Ailus Reviewed-by: Akinobu Mita (ov9650) --- (Resending with appropriate folks cc'd.) since RFC v1: - Use "-oif" instead of "-OIF" postfix for s5c73m3 OIF bit. (Suggested by Sylwester.) drivers/media/i2c/m5mols/m5mols_core.c | 1 - drivers/media/i2c/noon010pc30.c | 1 - drivers/media/i2c/ov9650.c | 1 - drivers/media/i2c/s5c73m3/s5c73m3-core.c | 4 ++-- drivers/media/i2c/s5k4ecgx.c | 1 - drivers/media/i2c/s5k6aa.c | 1 - 6 files changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c index 155424a43d4c..320e79b63555 100644 --- a/drivers/media/i2c/m5mols/m5mols_core.c +++ b/drivers/media/i2c/m5mols/m5mols_core.c @@ -987,7 +987,6 @@ static int m5mols_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, MODULE_NAME, sizeof(sd->name)); sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; sd->internal_ops = _subdev_internal_ops; diff --git a/drivers/media/i2c/noon010pc30.c b/drivers/media/i2c/noon010pc30.c index 4698e40fedd2..0629bc138fbc 100644 --- a/drivers/media/i2c/noon010pc30.c +++ b/drivers/media/i2c/noon010pc30.c @@ -720,7 +720,6 @@ static int noon010_probe(struct i2c_client *client, mutex_init(>lock); sd = >sd; v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, MODULE_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c index 3c9e6798d14b..9f1ed79d2a99 100644 --- a/drivers/media/i2c/ov9650.c +++ b/drivers/media/i2c/ov9650.c @@ -1539,7 +1539,6 @@ static int ov965x_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _subdev_ops); - strscpy(sd->name, DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _sd_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c index 21ca5186f9ed..69967346f787 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c @@ -1683,7 +1683,7 @@ static int s5c73m3_probe(struct i2c_client *client, v4l2_subdev_init(sd, _subdev_ops); sd->owner = client->dev.driver->owner; v4l2_set_subdevdata(sd, state); - strscpy(sd->name, "S5C73M3", sizeof(sd->name)); + v4l2_i2c_subdev_set_name(sd, client, NULL, NULL); sd->internal_ops = _internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; @@ -1698,7 +1698,7 @@ static int s5c73m3_probe(struct i2c_client *client, return ret; v4l2_i2c_subdev_init(oif_sd, client, _subdev_ops); - strscpy(oif_sd->name, "S5C73M3-OIF", sizeof(oif_sd->name)); + v4l2_i2c_subdev_set_name(sd, client, NULL, "-oif"); oif_sd->internal_ops = _internal_ops; oif_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c index 8c0dca6cb20c..8aaf5ad26826 100644 --- a/drivers/media/i2c/s5k4ecgx.c +++ b/drivers/media/i2c/s5k4ecgx.c @@ -954,7 +954,6 @@ static int s5k4ecgx_probe(struct i2c_client *client, sd = >sd; /* Registering subdev */ v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; /* Support v4l2 sub-device user space API */ diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c index 52ca033f7069..9536316e2d80 100644 --- a/drivers/media/i2c/s5k6aa.c +++ b/drivers/media/i2c/s5k6aa.c @@ -1576,7 +1576,6 @@ static int s5k6aa_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _subdev_ops); - strscpy(sd->name, DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; -- 2.11.0
[PATCH 1/1] v4l: samsung, ov9650: Rely on V4L2-set sub-device names
v4l2_i2c_subdev_init() sets the name of the sub-devices (as well as entities) to what is fairly certainly known to be unique in the system, even if there were more devices of the same kind. These drivers (m5mols, noon010pc30, ov9650, s5c73m3, s5k4ecgx, s5k6aa) set the name to the name of the driver or the module while omitting the device's I²C address and bus, leaving the devices with a static name and effectively limiting the number of such devices in a media device to 1. Address this by using the name set by the V4L2 framework. Signed-off-by: Sakari Ailus Reviewed-by: Akinobu Mita (ov9650) --- since RFC v1: - Use "-oif" instead of "-OIF" postfix for s5c73m3 OIF bit. (Suggested by Sylwester.) drivers/media/i2c/m5mols/m5mols_core.c | 1 - drivers/media/i2c/noon010pc30.c | 1 - drivers/media/i2c/ov9650.c | 1 - drivers/media/i2c/s5c73m3/s5c73m3-core.c | 4 ++-- drivers/media/i2c/s5k4ecgx.c | 1 - drivers/media/i2c/s5k6aa.c | 1 - 6 files changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/media/i2c/m5mols/m5mols_core.c b/drivers/media/i2c/m5mols/m5mols_core.c index 155424a43d4c..320e79b63555 100644 --- a/drivers/media/i2c/m5mols/m5mols_core.c +++ b/drivers/media/i2c/m5mols/m5mols_core.c @@ -987,7 +987,6 @@ static int m5mols_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, MODULE_NAME, sizeof(sd->name)); sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; sd->internal_ops = _subdev_internal_ops; diff --git a/drivers/media/i2c/noon010pc30.c b/drivers/media/i2c/noon010pc30.c index 4698e40fedd2..0629bc138fbc 100644 --- a/drivers/media/i2c/noon010pc30.c +++ b/drivers/media/i2c/noon010pc30.c @@ -720,7 +720,6 @@ static int noon010_probe(struct i2c_client *client, mutex_init(>lock); sd = >sd; v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, MODULE_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; diff --git a/drivers/media/i2c/ov9650.c b/drivers/media/i2c/ov9650.c index 3c9e6798d14b..9f1ed79d2a99 100644 --- a/drivers/media/i2c/ov9650.c +++ b/drivers/media/i2c/ov9650.c @@ -1539,7 +1539,6 @@ static int ov965x_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _subdev_ops); - strscpy(sd->name, DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _sd_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c b/drivers/media/i2c/s5c73m3/s5c73m3-core.c index 21ca5186f9ed..69967346f787 100644 --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c @@ -1683,7 +1683,7 @@ static int s5c73m3_probe(struct i2c_client *client, v4l2_subdev_init(sd, _subdev_ops); sd->owner = client->dev.driver->owner; v4l2_set_subdevdata(sd, state); - strscpy(sd->name, "S5C73M3", sizeof(sd->name)); + v4l2_i2c_subdev_set_name(sd, client, NULL, NULL); sd->internal_ops = _internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; @@ -1698,7 +1698,7 @@ static int s5c73m3_probe(struct i2c_client *client, return ret; v4l2_i2c_subdev_init(oif_sd, client, _subdev_ops); - strscpy(oif_sd->name, "S5C73M3-OIF", sizeof(oif_sd->name)); + v4l2_i2c_subdev_set_name(sd, client, NULL, "-oif"); oif_sd->internal_ops = _internal_ops; oif_sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; diff --git a/drivers/media/i2c/s5k4ecgx.c b/drivers/media/i2c/s5k4ecgx.c index 8c0dca6cb20c..8aaf5ad26826 100644 --- a/drivers/media/i2c/s5k4ecgx.c +++ b/drivers/media/i2c/s5k4ecgx.c @@ -954,7 +954,6 @@ static int s5k4ecgx_probe(struct i2c_client *client, sd = >sd; /* Registering subdev */ v4l2_i2c_subdev_init(sd, client, _ops); - strscpy(sd->name, S5K4ECGX_DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; /* Support v4l2 sub-device user space API */ diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c index 52ca033f7069..9536316e2d80 100644 --- a/drivers/media/i2c/s5k6aa.c +++ b/drivers/media/i2c/s5k6aa.c @@ -1576,7 +1576,6 @@ static int s5k6aa_probe(struct i2c_client *client, sd = >sd; v4l2_i2c_subdev_init(sd, client, _subdev_ops); - strscpy(sd->name, DRIVER_NAME, sizeof(sd->name)); sd->internal_ops = _subdev_internal_ops; sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; -- 2.11.0
