| Message ID | 20180824163525.12694-4-luca@lucaceresoli.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: imx274: miscellaneous improvements | expand |
Hi Luca, On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > it non-unique and less informative. > > Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > --- > drivers/media/i2c/imx274.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index 9b524de08470..570706695ca7 100644 > --- a/drivers/media/i2c/imx274.c > +++ b/drivers/media/i2c/imx274.c > @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, > imx274->client = client; > sd = &imx274->sd; > v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); > - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > /* initialize subdev media pad */ This ends up changing the entity as well as the sub-device name which may well break applications. On the other hand, you currently can't have more than one of these devices on a media device complex due to the name being specific to a driver, not the device. An option avoiding that would be to let the user choose by e.g. through a Kconfig option would avoid having to address that, but I really hate adding such options. I wonder what others think. If anyone ever needs to add another on a board so that it ends up being the part of the same media device complex (likely), then changing the name now rather than later would be the least pain. In this case I'd be leaning (slightly) towards accepting the patch and hoping there wouldn't be any fallout... I don't see any board (DT) containing imx274, at least not in the upstream kernel. Cc Hans and Laurent.
Hi Sakari, On 25/08/2018 16:49, Sakari Ailus wrote: > Hi Luca, > > On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: >> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes >> it non-unique and less informative. >> >> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". >> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >> --- >> drivers/media/i2c/imx274.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c >> index 9b524de08470..570706695ca7 100644 >> --- a/drivers/media/i2c/imx274.c >> +++ b/drivers/media/i2c/imx274.c >> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, >> imx274->client = client; >> sd = &imx274->sd; >> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); >> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); >> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; >> >> /* initialize subdev media pad */ > > This ends up changing the entity as well as the sub-device name which may > well break applications. Right, unfortunately. > On the other hand, you currently can't have more > than one of these devices on a media device complex due to the name being > specific to a driver, not the device. > > An option avoiding that would be to let the user choose by e.g. through a > Kconfig option would avoid having to address that, but I really hate adding > such options. I agree adding a Kconfig option just for this would be very annoying. However I think the issue affects a few other drivers (sr030pc30.c and s5c73m3-core.c apparently), thus maybe one option could serve them all. > I wonder what others think. If anyone ever needs to add another on a board > so that it ends up being the part of the same media device complex > (likely), then changing the name now rather than later would be the least > pain. In this case I'd be leaning (slightly) towards accepting the patch > and hoping there wouldn't be any fallout... I don't see any board (DT) > containing imx274, at least not in the upstream kernel. I'll be OK with either decision. Should we keep it as is, then I think a comment before that line would be appropriate to clarify it's not correct but it is kept for backward userspace compatibility. This would help avoid new driver writers doing the same mistake, and prevent other people to send another patch like mine. Regards,
Hi Sakari and Luca, On Sun, Aug 26, 2018 at 10:41:13PM +0200, Luca Ceresoli wrote: > Hi Sakari, > > On 25/08/2018 16:49, Sakari Ailus wrote: > > Hi Luca, > > > > On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > >> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > >> it non-unique and less informative. > >> > >> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > >> ... > > > > This ends up changing the entity as well as the sub-device name which may > > well break applications. > > Right, unfortunately. > > > On the other hand, you currently can't have more > > than one of these devices on a media device complex due to the name being > > specific to a driver, not the device. > > > > An option avoiding that would be to let the user choose by e.g. through a > > Kconfig option would avoid having to address that, but I really hate adding > > such options. > > I agree adding a Kconfig option just for this would be very annoying. > However I think the issue affects a few other drivers (sr030pc30.c and > s5c73m3-core.c apparently), thus maybe one option could serve them all. > > > I wonder what others think. If anyone ever needs to add another on a board > > so that it ends up being the part of the same media device complex > > (likely), then changing the name now rather than later would be the least > > pain. In this case I'd be leaning (slightly) towards accepting the patch > > and hoping there wouldn't be any fallout... I don't see any board (DT) > > containing imx274, at least not in the upstream kernel. > > I'll be OK with either decision. Should we keep it as is, then I think a > comment before that line would be appropriate to clarify it's not > correct but it is kept for backward userspace compatibility. This would > help avoid new driver writers doing the same mistake, and prevent other > people to send another patch like mine. Would it be acceptable to accept Luca's patch but add a dev_info message indicating the old and the new name, so that at least if the user notices a problem he'll find an informative message helping him to fix his config ? This dev_info message could even be standardized to be usable for other drivers with only the names changed. Philippe
On 26/08/18 22:41, Luca Ceresoli wrote: > Hi Sakari, > > On 25/08/2018 16:49, Sakari Ailus wrote: >> Hi Luca, >> >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes >>> it non-unique and less informative. >>> >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". >>> >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> >>> --- >>> drivers/media/i2c/imx274.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c >>> index 9b524de08470..570706695ca7 100644 >>> --- a/drivers/media/i2c/imx274.c >>> +++ b/drivers/media/i2c/imx274.c >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, >>> imx274->client = client; >>> sd = &imx274->sd; >>> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); >>> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); >>> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; >>> >>> /* initialize subdev media pad */ >> >> This ends up changing the entity as well as the sub-device name which may >> well break applications. > > Right, unfortunately. > >> On the other hand, you currently can't have more >> than one of these devices on a media device complex due to the name being >> specific to a driver, not the device. >> >> An option avoiding that would be to let the user choose by e.g. through a >> Kconfig option would avoid having to address that, but I really hate adding >> such options. > > I agree adding a Kconfig option just for this would be very annoying. > However I think the issue affects a few other drivers (sr030pc30.c and > s5c73m3-core.c apparently), thus maybe one option could serve them all. > >> I wonder what others think. If anyone ever needs to add another on a board >> so that it ends up being the part of the same media device complex >> (likely), then changing the name now rather than later would be the least >> pain. In this case I'd be leaning (slightly) towards accepting the patch >> and hoping there wouldn't be any fallout... I don't see any board (DT) >> containing imx274, at least not in the upstream kernel. > > I'll be OK with either decision. Should we keep it as is, then I think a > comment before that line would be appropriate to clarify it's not > correct but it is kept for backward userspace compatibility. This would > help avoid new driver writers doing the same mistake, and prevent other > people to send another patch like mine. In this end, this is a driver bug. I would just fix this, but add a comment that states the old name and why it was changed. No need for a dev_info IMHO. It would be nice if you can check if the same mistake is made in other drivers, and update those as well. It's easier if this is all done at the same time. Regards, Hans
Hi Hans, Sakari and Luca On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote: > On 26/08/18 22:41, Luca Ceresoli wrote: > > Hi Sakari, > > > > On 25/08/2018 16:49, Sakari Ailus wrote: > >> Hi Luca, > >> > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > >>> it non-unique and less informative. > >>> > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > >>> > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > >>> --- > >>> drivers/media/i2c/imx274.c | 1 - > >>> 1 file changed, 1 deletion(-) > >>> > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > >>> index 9b524de08470..570706695ca7 100644 > >>> --- a/drivers/media/i2c/imx274.c > >>> +++ b/drivers/media/i2c/imx274.c > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, > >>> imx274->client = client; > >>> sd = &imx274->sd; > >>> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); > >>> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > >>> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > >>> > >>> /* initialize subdev media pad */ > >> > >> This ends up changing the entity as well as the sub-device name which may > >> well break applications. > > > > Right, unfortunately. > > > >> On the other hand, you currently can't have more > >> than one of these devices on a media device complex due to the name being > >> specific to a driver, not the device. > >> > >> An option avoiding that would be to let the user choose by e.g. through a > >> Kconfig option would avoid having to address that, but I really hate adding > >> such options. > > > > I agree adding a Kconfig option just for this would be very annoying. > > However I think the issue affects a few other drivers (sr030pc30.c and > > s5c73m3-core.c apparently), thus maybe one option could serve them all. > > > >> I wonder what others think. If anyone ever needs to add another on a board > >> so that it ends up being the part of the same media device complex > >> (likely), then changing the name now rather than later would be the least > >> pain. In this case I'd be leaning (slightly) towards accepting the patch > >> and hoping there wouldn't be any fallout... I don't see any board (DT) > >> containing imx274, at least not in the upstream kernel. > > > > I'll be OK with either decision. Should we keep it as is, then I think a > > comment before that line would be appropriate to clarify it's not > > correct but it is kept for backward userspace compatibility. This would > > help avoid new driver writers doing the same mistake, and prevent other > > people to send another patch like mine. > > In this end, this is a driver bug. I would just fix this, but add a comment > that states the old name and why it was changed. No need for a dev_info > IMHO. > > It would be nice if you can check if the same mistake is made in other drivers, > and update those as well. It's easier if this is all done at the same time. > Then we should probably also apply the following patch I submitted : "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" https://patchwork.kernel.org/patch/10553035/ and perhaps "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" https://patchwork.kernel.org/patch/10553037/ Philippe
Hi Philippe, On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: > Hi Hans, Sakari and Luca > > On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote: > > On 26/08/18 22:41, Luca Ceresoli wrote: > > > Hi Sakari, > > > > > > On 25/08/2018 16:49, Sakari Ailus wrote: > > >> Hi Luca, > > >> > > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > > >>> it non-unique and less informative. > > >>> > > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > > >>> > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > >>> --- > > >>> drivers/media/i2c/imx274.c | 1 - > > >>> 1 file changed, 1 deletion(-) > > >>> > > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > > >>> index 9b524de08470..570706695ca7 100644 > > >>> --- a/drivers/media/i2c/imx274.c > > >>> +++ b/drivers/media/i2c/imx274.c > > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, > > >>> imx274->client = client; > > >>> sd = &imx274->sd; > > >>> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); > > >>> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > > >>> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > >>> > > >>> /* initialize subdev media pad */ > > >> > > >> This ends up changing the entity as well as the sub-device name which may > > >> well break applications. > > > > > > Right, unfortunately. > > > > > >> On the other hand, you currently can't have more > > >> than one of these devices on a media device complex due to the name being > > >> specific to a driver, not the device. > > >> > > >> An option avoiding that would be to let the user choose by e.g. through a > > >> Kconfig option would avoid having to address that, but I really hate adding > > >> such options. > > > > > > I agree adding a Kconfig option just for this would be very annoying. > > > However I think the issue affects a few other drivers (sr030pc30.c and > > > s5c73m3-core.c apparently), thus maybe one option could serve them all. > > > > > >> I wonder what others think. If anyone ever needs to add another on a board > > >> so that it ends up being the part of the same media device complex > > >> (likely), then changing the name now rather than later would be the least > > >> pain. In this case I'd be leaning (slightly) towards accepting the patch > > >> and hoping there wouldn't be any fallout... I don't see any board (DT) > > >> containing imx274, at least not in the upstream kernel. > > > > > > I'll be OK with either decision. Should we keep it as is, then I think a > > > comment before that line would be appropriate to clarify it's not > > > correct but it is kept for backward userspace compatibility. This would > > > help avoid new driver writers doing the same mistake, and prevent other > > > people to send another patch like mine. > > > > In this end, this is a driver bug. I would just fix this, but add a comment > > that states the old name and why it was changed. No need for a dev_info > > IMHO. > > > > It would be nice if you can check if the same mistake is made in other drivers, > > and update those as well. It's easier if this is all done at the same time. > > > > Then we should probably also apply the following patch I submitted : > > "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > https://patchwork.kernel.org/patch/10553035/ > > and perhaps > > "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > https://patchwork.kernel.org/patch/10553037/ The problem with this patch is that the existing naming scheme is very similar while the new one offers no tangible benefits apart from being in line with the rest of the kernel. That's still not a benefit for uAPI: changing the name is certain to break user space applications. I posted another patch on which I'm somewhat uncertain applying it would be only a good thing: <URL:https://patchwork.linuxtv.org/patch/51791/> But it would fix an actual bug. I maintain that the information (device name as it's seen by the rest of the system) would be good to have in device properties; Hans posted a patchset on that some time ago.
Hi Sakari, On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote: > Hi Philippe, > > On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: > > Hi Hans, Sakari and Luca > > > > On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote: > > > On 26/08/18 22:41, Luca Ceresoli wrote: > > > > Hi Sakari, > > > > > > > > On 25/08/2018 16:49, Sakari Ailus wrote: > > > >> Hi Luca, > > > >> > > > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > > > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > > > >>> it non-unique and less informative. > > > >>> > > > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > > > >>> > > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > > >>> --- > > > >>> drivers/media/i2c/imx274.c | 1 - > > > >>> 1 file changed, 1 deletion(-) > > > >>> > > > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > > > >>> index 9b524de08470..570706695ca7 100644 > > > >>> --- a/drivers/media/i2c/imx274.c > > > >>> +++ b/drivers/media/i2c/imx274.c > > > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, > > > >>> imx274->client = client; > > > >>> sd = &imx274->sd; > > > >>> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); > > > >>> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > > > >>> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > > >>> > > > >>> /* initialize subdev media pad */ > > > >> > > > >> This ends up changing the entity as well as the sub-device name which may > > > >> well break applications. > > > > > > > > Right, unfortunately. > > > > > > > >> On the other hand, you currently can't have more > > > >> than one of these devices on a media device complex due to the name being > > > >> specific to a driver, not the device. > > > >> > > > >> An option avoiding that would be to let the user choose by e.g. through a > > > >> Kconfig option would avoid having to address that, but I really hate adding > > > >> such options. > > > > > > > > I agree adding a Kconfig option just for this would be very annoying. > > > > However I think the issue affects a few other drivers (sr030pc30.c and > > > > s5c73m3-core.c apparently), thus maybe one option could serve them all. > > > > > > > >> I wonder what others think. If anyone ever needs to add another on a board > > > >> so that it ends up being the part of the same media device complex > > > >> (likely), then changing the name now rather than later would be the least > > > >> pain. In this case I'd be leaning (slightly) towards accepting the patch > > > >> and hoping there wouldn't be any fallout... I don't see any board (DT) > > > >> containing imx274, at least not in the upstream kernel. > > > > > > > > I'll be OK with either decision. Should we keep it as is, then I think a > > > > comment before that line would be appropriate to clarify it's not > > > > correct but it is kept for backward userspace compatibility. This would > > > > help avoid new driver writers doing the same mistake, and prevent other > > > > people to send another patch like mine. > > > > > > In this end, this is a driver bug. I would just fix this, but add a comment > > > that states the old name and why it was changed. No need for a dev_info > > > IMHO. > > > > > > It would be nice if you can check if the same mistake is made in other drivers, > > > and update those as well. It's easier if this is all done at the same time. > > > > > > > Then we should probably also apply the following patch I submitted : > > > > "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > > https://patchwork.kernel.org/patch/10553035/ > > > > and perhaps > > > > "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > > https://patchwork.kernel.org/patch/10553037/ > > The problem with this patch is that the existing naming scheme is very > similar while the new one offers no tangible benefits apart from being in > line with the rest of the kernel. That's still not a benefit for uAPI: > changing the name is certain to break user space applications. I agree with you on the patch for v4l2_i2c_subdev_init (I wrote 'perhaps'), but you don't say anything on the one about v4l2_spi_subdev_init :), which fixes an actual bug. I have 2 identical SPI-controlled sensors on the same board, and without my patch they get the same subdev name. Of course, I could fix that in the sensor driver itself, but that's not what we want, or do we ? Best Regards Philippe
On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote: > Hi Sakari, > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote: > > Hi Philippe, > > > > On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: > > > Hi Hans, Sakari and Luca > > > > > > On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote: > > > > On 26/08/18 22:41, Luca Ceresoli wrote: > > > > > Hi Sakari, > > > > > > > > > > On 25/08/2018 16:49, Sakari Ailus wrote: > > > > >> Hi Luca, > > > > >> > > > > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote: > > > > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes > > > > >>> it non-unique and less informative. > > > > >>> > > > > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a". > > > > >>> > > > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > > > > >>> --- > > > > >>> drivers/media/i2c/imx274.c | 1 - > > > > >>> 1 file changed, 1 deletion(-) > > > > >>> > > > > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > > > > >>> index 9b524de08470..570706695ca7 100644 > > > > >>> --- a/drivers/media/i2c/imx274.c > > > > >>> +++ b/drivers/media/i2c/imx274.c > > > > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, > > > > >>> imx274->client = client; > > > > >>> sd = &imx274->sd; > > > > >>> v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); > > > > >>> - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); > > > > >>> sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > > > >>> > > > > >>> /* initialize subdev media pad */ > > > > >> > > > > >> This ends up changing the entity as well as the sub-device name which may > > > > >> well break applications. > > > > > > > > > > Right, unfortunately. > > > > > > > > > >> On the other hand, you currently can't have more > > > > >> than one of these devices on a media device complex due to the name being > > > > >> specific to a driver, not the device. > > > > >> > > > > >> An option avoiding that would be to let the user choose by e.g. through a > > > > >> Kconfig option would avoid having to address that, but I really hate adding > > > > >> such options. > > > > > > > > > > I agree adding a Kconfig option just for this would be very annoying. > > > > > However I think the issue affects a few other drivers (sr030pc30.c and > > > > > s5c73m3-core.c apparently), thus maybe one option could serve them all. > > > > > > > > > >> I wonder what others think. If anyone ever needs to add another on a board > > > > >> so that it ends up being the part of the same media device complex > > > > >> (likely), then changing the name now rather than later would be the least > > > > >> pain. In this case I'd be leaning (slightly) towards accepting the patch > > > > >> and hoping there wouldn't be any fallout... I don't see any board (DT) > > > > >> containing imx274, at least not in the upstream kernel. > > > > > > > > > > I'll be OK with either decision. Should we keep it as is, then I think a > > > > > comment before that line would be appropriate to clarify it's not > > > > > correct but it is kept for backward userspace compatibility. This would > > > > > help avoid new driver writers doing the same mistake, and prevent other > > > > > people to send another patch like mine. > > > > > > > > In this end, this is a driver bug. I would just fix this, but add a comment > > > > that states the old name and why it was changed. No need for a dev_info > > > > IMHO. > > > > > > > > It would be nice if you can check if the same mistake is made in other drivers, > > > > and update those as well. It's easier if this is all done at the same time. > > > > > > > > > > Then we should probably also apply the following patch I submitted : > > > > > > "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > > > https://patchwork.kernel.org/patch/10553035/ > > > > > > and perhaps > > > > > > "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > > > https://patchwork.kernel.org/patch/10553037/ > > > > The problem with this patch is that the existing naming scheme is very > > similar while the new one offers no tangible benefits apart from being in > > line with the rest of the kernel. That's still not a benefit for uAPI: > > changing the name is certain to break user space applications. > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote 'perhaps'), > but you don't say anything on the one about v4l2_spi_subdev_init :), which > fixes an actual bug. I have 2 identical SPI-controlled sensors on the > same board, and without my patch they get the same subdev name. Of course, > I could fix that in the sensor driver itself, but that's not what we want, > or do we ? Good point. I missed the naming of the SPI devices ignored any bus information there. I'm rather inclined towards taking the SPI patch. Hans, Mauro, Laurent; any opinion on that?
Hello, On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote: > On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote: > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote: > >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: [snip] > >>> Then we should probably also apply the following patch I submitted : > >>> > >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > >>> > >>> https://patchwork.kernel.org/patch/10553035/ > >>> > >>> and perhaps > >>> > >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > >>> > >>> https://patchwork.kernel.org/patch/10553037/ > >> > >> The problem with this patch is that the existing naming scheme is very > >> similar while the new one offers no tangible benefits apart from being > >> in line with the rest of the kernel. That's still not a benefit for uAPI: > >> changing the name is certain to break user space applications. > > > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote > > 'perhaps'), but you don't say anything on the one about > > v4l2_spi_subdev_init :), which fixes an actual bug. I have 2 identical > > SPI-controlled sensors on the same board, and without my patch they get > > the same subdev name. Of course, I could fix that in the sensor driver > > itself, but that's not what we want, or do we ? > > Good point. I missed the naming of the SPI devices ignored any bus > information there. I'm rather inclined towards taking the SPI patch. Hans, > Mauro, Laurent; any opinion on that? I agree that the SPI patch makes sense, I think we should take it.
Hi. On Thu, Aug 30, 2018 at 12:23:23AM +0300, Laurent Pinchart wrote: > Hello, > > On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote: > > On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote: > > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote: > > >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: > > [snip] > > > >>> Then we should probably also apply the following patch I submitted : > > >>> > > >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > > >>> > > >>> https://patchwork.kernel.org/patch/10553035/ > > >>> > > >>> and perhaps > > >>> > > >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > > >>> > > >>> https://patchwork.kernel.org/patch/10553037/ > > >> > > >> The problem with this patch is that the existing naming scheme is very > > >> similar while the new one offers no tangible benefits apart from being > > >> in line with the rest of the kernel. That's still not a benefit for uAPI: > > >> changing the name is certain to break user space applications. > > > > > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote > > > 'perhaps'), but you don't say anything on the one about > > > v4l2_spi_subdev_init :), which fixes an actual bug. I have 2 identical > > > SPI-controlled sensors on the same board, and without my patch they get > > > the same subdev name. Of course, I could fix that in the sensor driver > > > itself, but that's not what we want, or do we ? > > > > Good point. I missed the naming of the SPI devices ignored any bus > > information there. I'm rather inclined towards taking the SPI patch. Hans, > > Mauro, Laurent; any opinion on that? > > I agree that the SPI patch makes sense, I think we should take it. Do I need to resend https://patchwork.kernel.org/patch/10553035/ "media: v4l2-common: v4l2_spi_subdev_init : generate unique name", adding Sakari's and Laurent's Acked-by ? or will that patch be taken automagically ? Philippe
On Thu, Aug 30, 2018 at 08:58:13AM +0200, Philippe De Muyter wrote: > Hi. > > On Thu, Aug 30, 2018 at 12:23:23AM +0300, Laurent Pinchart wrote: > > Hello, > > > > On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote: > > > On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote: > > > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote: > > > >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote: > > > > [snip] > > > > > >>> Then we should probably also apply the following patch I submitted : > > > >>> > > > >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name" > > > >>> > > > >>> https://patchwork.kernel.org/patch/10553035/ > > > >>> > > > >>> and perhaps > > > >>> > > > >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation" > > > >>> > > > >>> https://patchwork.kernel.org/patch/10553037/ > > > >> > > > >> The problem with this patch is that the existing naming scheme is very > > > >> similar while the new one offers no tangible benefits apart from being > > > >> in line with the rest of the kernel. That's still not a benefit for uAPI: > > > >> changing the name is certain to break user space applications. > > > > > > > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote > > > > 'perhaps'), but you don't say anything on the one about > > > > v4l2_spi_subdev_init :), which fixes an actual bug. I have 2 identical > > > > SPI-controlled sensors on the same board, and without my patch they get > > > > the same subdev name. Of course, I could fix that in the sensor driver > > > > itself, but that's not what we want, or do we ? > > > > > > Good point. I missed the naming of the SPI devices ignored any bus > > > information there. I'm rather inclined towards taking the SPI patch. Hans, > > > Mauro, Laurent; any opinion on that? > > > > I agree that the SPI patch makes sense, I think we should take it. > > Do I need to resend https://patchwork.kernel.org/patch/10553035/ "media: > v4l2-common: v4l2_spi_subdev_init : generate unique name", adding Sakari's > and Laurent's Acked-by ? or will that patch be taken automagically ? No need to.
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c index 9b524de08470..570706695ca7 100644 --- a/drivers/media/i2c/imx274.c +++ b/drivers/media/i2c/imx274.c @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client, imx274->client = client; sd = &imx274->sd; v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops); - strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name)); sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; /* initialize subdev media pad */
Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes it non-unique and less informative. Let the driver use the default name from i2c, e.g. "IMX274 2-001a". Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> --- drivers/media/i2c/imx274.c | 1 - 1 file changed, 1 deletion(-)