* [PATCH] media: i2c: imx258: Parse and register properties @ 2022-12-25 15:42 Robert Mader 2023-01-02 14:06 ` Jacopo Mondi 0 siblings, 1 reply; 6+ messages in thread From: Robert Mader @ 2022-12-25 15:42 UTC (permalink / raw) To: linux-kernel Cc: nicholas, javierm, jacopo, Robert Mader, Sakari Ailus, Mauro Carvalho Chehab, linux-media Analogous to e.g. the imx219. This enables propagating V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera can detect the correct rotation from the device tree and propagate it further to e.g. Pipewire. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- drivers/media/i2c/imx258.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c index eab5fc1ee2f7..85819043d1e3 100644 --- a/drivers/media/i2c/imx258.c +++ b/drivers/media/i2c/imx258.c @@ -9,6 +9,7 @@ #include <linux/pm_runtime.h> #include <media/v4l2-ctrls.h> #include <media/v4l2-device.h> +#include <media/v4l2-fwnode.h> #include <asm/unaligned.h> #define IMX258_REG_VALUE_08BIT 1 @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) { struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); struct v4l2_ctrl_handler *ctrl_hdlr; + struct v4l2_fwnode_device_properties props; s64 vblank_def; s64 vblank_min; s64 pixel_rate_min; @@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258) int ret; ctrl_hdlr = &imx258->ctrl_handler; - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); if (ret) return ret; @@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258) goto error; } + ret = v4l2_fwnode_device_parse(&client->dev, &props); + if (ret) + goto error; + + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops, + &props); + if (ret) + goto error; + imx258->sd.ctrl_handler = ctrl_hdlr; return 0; -- 2.39.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx258: Parse and register properties 2022-12-25 15:42 [PATCH] media: i2c: imx258: Parse and register properties Robert Mader @ 2023-01-02 14:06 ` Jacopo Mondi 2023-01-03 14:11 ` Robert Mader 0 siblings, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2023-01-02 14:06 UTC (permalink / raw) To: Robert Mader Cc: linux-kernel, nicholas, javierm, Sakari Ailus, Mauro Carvalho Chehab, linux-media Hi Robert On Sun, Dec 25, 2022 at 04:42:34PM +0100, Robert Mader wrote: > Analogous to e.g. the imx219. This enables propagating > V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera > can detect the correct rotation from the device tree > and propagate it further to e.g. Pipewire. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > drivers/media/i2c/imx258.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > index eab5fc1ee2f7..85819043d1e3 100644 > --- a/drivers/media/i2c/imx258.c > +++ b/drivers/media/i2c/imx258.c > @@ -9,6 +9,7 @@ > #include <linux/pm_runtime.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > +#include <media/v4l2-fwnode.h> > #include <asm/unaligned.h> > > #define IMX258_REG_VALUE_08BIT 1 > @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) > { > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > struct v4l2_ctrl_handler *ctrl_hdlr; > + struct v4l2_fwnode_device_properties props; Might be nicer to move this one line up > s64 vblank_def; > s64 vblank_min; > s64 pixel_rate_min; > @@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258) > int ret; > > ctrl_hdlr = &imx258->ctrl_handler; > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); I count 9 controls being registered before this patch, not 8. Do I count them right ? If that's case, as v4l2_ctrl_new_fwnode_properties() can register up to two controls (V4L2_CID_ROTATION and V4L2_CID_ORIENTATION) I would pre-reserve 11 controls not 10 to avoid relocations. > if (ret) > return ret; > > @@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258) > goto error; > } > > + ret = v4l2_fwnode_device_parse(&client->dev, &props); > + if (ret) > + goto error; > + > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops, > + &props); > + if (ret) > + goto error; > + The rest looks good to me! Thanks j > imx258->sd.ctrl_handler = ctrl_hdlr; > > return 0; > -- > 2.39.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx258: Parse and register properties 2023-01-02 14:06 ` Jacopo Mondi @ 2023-01-03 14:11 ` Robert Mader 2023-01-03 17:16 ` Jacopo Mondi 0 siblings, 1 reply; 6+ messages in thread From: Robert Mader @ 2023-01-03 14:11 UTC (permalink / raw) To: Jacopo Mondi Cc: linux-kernel, nicholas, javierm, Sakari Ailus, Mauro Carvalho Chehab, linux-media On 02.01.23 15:06, Jacopo Mondi wrote: > Hi Robert > > On Sun, Dec 25, 2022 at 04:42:34PM +0100, Robert Mader wrote: >> Analogous to e.g. the imx219. This enables propagating >> V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera >> can detect the correct rotation from the device tree >> and propagate it further to e.g. Pipewire. >> >> Signed-off-by: Robert Mader <robert.mader@collabora.com> >> --- >> drivers/media/i2c/imx258.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >> index eab5fc1ee2f7..85819043d1e3 100644 >> --- a/drivers/media/i2c/imx258.c >> +++ b/drivers/media/i2c/imx258.c >> @@ -9,6 +9,7 @@ >> #include <linux/pm_runtime.h> >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-device.h> >> +#include <media/v4l2-fwnode.h> >> #include <asm/unaligned.h> >> >> #define IMX258_REG_VALUE_08BIT 1 >> @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) >> { >> struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); >> struct v4l2_ctrl_handler *ctrl_hdlr; >> + struct v4l2_fwnode_device_properties props; > Might be nicer to move this one line up Can you say what's your reasoning? I personally slightly prefer alphabetical order, but no strong opinion :) >> s64 vblank_def; >> s64 vblank_min; >> s64 pixel_rate_min; >> @@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258) >> int ret; >> >> ctrl_hdlr = &imx258->ctrl_handler; >> - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); >> + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); > I count 9 controls being registered before this patch, not 8. Do I > count them right ? > > If that's case, as v4l2_ctrl_new_fwnode_properties() > can register up to two controls (V4L2_CID_ROTATION and > V4L2_CID_ORIENTATION) I would pre-reserve 11 controls not 10 to avoid > relocations. Indeed, looks like bumping this was forgotten in c6f9d67e2ac625e331f6a7f5715d2f809ff0a922 >> if (ret) >> return ret; >> >> @@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258) >> goto error; >> } >> >> + ret = v4l2_fwnode_device_parse(&client->dev, &props); >> + if (ret) >> + goto error; >> + >> + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops, >> + &props); >> + if (ret) >> + goto error; >> + > The rest looks good to me! > > Thanks > j Thanks! -- Robert Mader Consultant Software Developer Collabora Ltd. Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK Registered in England & Wales, no. 5513718 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx258: Parse and register properties 2023-01-03 14:11 ` Robert Mader @ 2023-01-03 17:16 ` Jacopo Mondi 2023-01-04 7:21 ` Sakari Ailus 0 siblings, 1 reply; 6+ messages in thread From: Jacopo Mondi @ 2023-01-03 17:16 UTC (permalink / raw) To: Robert Mader Cc: linux-kernel, nicholas, javierm, Sakari Ailus, Mauro Carvalho Chehab, linux-media Hi Robert On Tue, Jan 03, 2023 at 03:11:44PM +0100, Robert Mader wrote: > On 02.01.23 15:06, Jacopo Mondi wrote: > > Hi Robert > > > > On Sun, Dec 25, 2022 at 04:42:34PM +0100, Robert Mader wrote: > > > Analogous to e.g. the imx219. This enables propagating > > > V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera > > > can detect the correct rotation from the device tree > > > and propagate it further to e.g. Pipewire. > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > --- > > > drivers/media/i2c/imx258.c | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > > index eab5fc1ee2f7..85819043d1e3 100644 > > > --- a/drivers/media/i2c/imx258.c > > > +++ b/drivers/media/i2c/imx258.c > > > @@ -9,6 +9,7 @@ > > > #include <linux/pm_runtime.h> > > > #include <media/v4l2-ctrls.h> > > > #include <media/v4l2-device.h> > > > +#include <media/v4l2-fwnode.h> > > > #include <asm/unaligned.h> > > > > > > #define IMX258_REG_VALUE_08BIT 1 > > > @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) > > > { > > > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > > > struct v4l2_ctrl_handler *ctrl_hdlr; > > > + struct v4l2_fwnode_device_properties props; > > Might be nicer to move this one line up > > Can you say what's your reasoning? I personally slightly prefer > alphabetical order, but no strong opinion :) > I've often been instructed to try to respect the inverse-xmas-tree order when declaring variables, if possible. I now realize it's a sort of cargo cult, as the rule is not written anywhere, so I can't ask you to comply with what seems to be a personal preference :) > > > s64 vblank_def; > > > s64 vblank_min; > > > s64 pixel_rate_min; And anyway this breaks the rule already, so up to you, really > > > @@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258) > > > int ret; > > > > > > ctrl_hdlr = &imx258->ctrl_handler; > > > - ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8); > > > + ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10); > > I count 9 controls being registered before this patch, not 8. Do I > > count them right ? > > > > If that's case, as v4l2_ctrl_new_fwnode_properties() > > can register up to two controls (V4L2_CID_ROTATION and > > V4L2_CID_ORIENTATION) I would pre-reserve 11 controls not 10 to avoid > > relocations. > > Indeed, looks like bumping this was forgotten in > c6f9d67e2ac625e331f6a7f5715d2f809ff0a922 > Sorry, not your fault but since you're here... You can either make a patch that fixes the existing number of controls first then apply this one on top, or address the issue here, maybe with a small mention in the commit message along the lines of: While at it, reserve space for 3 additional controls even if v4l2_ctrl_new_fwnode_properties() can only register 2 of them, to fix the existing implementation which reserve space for 8 controls but actually registers 9. Thanks j > > > if (ret) > > > return ret; > > > > > > @@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258) > > > goto error; > > > } > > > > > > + ret = v4l2_fwnode_device_parse(&client->dev, &props); > > > + if (ret) > > > + goto error; > > > + > > > + ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops, > > > + &props); > > > + if (ret) > > > + goto error; > > > + > > The rest looks good to me! > > > > Thanks > > j > Thanks! > > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx258: Parse and register properties 2023-01-03 17:16 ` Jacopo Mondi @ 2023-01-04 7:21 ` Sakari Ailus 2023-01-04 12:47 ` Robert Mader 0 siblings, 1 reply; 6+ messages in thread From: Sakari Ailus @ 2023-01-04 7:21 UTC (permalink / raw) To: Jacopo Mondi Cc: Robert Mader, linux-kernel, nicholas, javierm, Mauro Carvalho Chehab, linux-media Hi Jacopo, Robert, On Tue, Jan 03, 2023 at 06:16:24PM +0100, Jacopo Mondi wrote: > Hi Robert > > On Tue, Jan 03, 2023 at 03:11:44PM +0100, Robert Mader wrote: > > On 02.01.23 15:06, Jacopo Mondi wrote: > > > Hi Robert > > > > > > On Sun, Dec 25, 2022 at 04:42:34PM +0100, Robert Mader wrote: > > > > Analogous to e.g. the imx219. This enables propagating > > > > V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera > > > > can detect the correct rotation from the device tree > > > > and propagate it further to e.g. Pipewire. > > > > > > > > Signed-off-by: Robert Mader <robert.mader@collabora.com> > > > > --- > > > > drivers/media/i2c/imx258.c | 13 ++++++++++++- > > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c > > > > index eab5fc1ee2f7..85819043d1e3 100644 > > > > --- a/drivers/media/i2c/imx258.c > > > > +++ b/drivers/media/i2c/imx258.c > > > > @@ -9,6 +9,7 @@ > > > > #include <linux/pm_runtime.h> > > > > #include <media/v4l2-ctrls.h> > > > > #include <media/v4l2-device.h> > > > > +#include <media/v4l2-fwnode.h> > > > > #include <asm/unaligned.h> > > > > > > > > #define IMX258_REG_VALUE_08BIT 1 > > > > @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) > > > > { > > > > struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); > > > > struct v4l2_ctrl_handler *ctrl_hdlr; > > > > + struct v4l2_fwnode_device_properties props; > > > Might be nicer to move this one line up > > > > Can you say what's your reasoning? I personally slightly prefer > > alphabetical order, but no strong opinion :) > > > > I've often been instructed to try to respect the inverse-xmas-tree I'd advise the same, unless there are other reasons to arrange the lines differently. -- Kind regards, Sakari Ailus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: i2c: imx258: Parse and register properties 2023-01-04 7:21 ` Sakari Ailus @ 2023-01-04 12:47 ` Robert Mader 0 siblings, 0 replies; 6+ messages in thread From: Robert Mader @ 2023-01-04 12:47 UTC (permalink / raw) To: Sakari Ailus, Jacopo Mondi Cc: linux-kernel, nicholas, javierm, Mauro Carvalho Chehab, linux-media Sure, makes sense! Send out v2 now. On 04.01.23 08:21, Sakari Ailus wrote: > Hi Jacopo, Robert, > > On Tue, Jan 03, 2023 at 06:16:24PM +0100, Jacopo Mondi wrote: >> Hi Robert >> >> On Tue, Jan 03, 2023 at 03:11:44PM +0100, Robert Mader wrote: >>> On 02.01.23 15:06, Jacopo Mondi wrote: >>>> Hi Robert >>>> >>>> On Sun, Dec 25, 2022 at 04:42:34PM +0100, Robert Mader wrote: >>>>> Analogous to e.g. the imx219. This enables propagating >>>>> V4L2_CID_CAMERA_SENSOR_ROTATION values so that libcamera >>>>> can detect the correct rotation from the device tree >>>>> and propagate it further to e.g. Pipewire. >>>>> >>>>> Signed-off-by: Robert Mader <robert.mader@collabora.com> >>>>> --- >>>>> drivers/media/i2c/imx258.c | 13 ++++++++++++- >>>>> 1 file changed, 12 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c >>>>> index eab5fc1ee2f7..85819043d1e3 100644 >>>>> --- a/drivers/media/i2c/imx258.c >>>>> +++ b/drivers/media/i2c/imx258.c >>>>> @@ -9,6 +9,7 @@ >>>>> #include <linux/pm_runtime.h> >>>>> #include <media/v4l2-ctrls.h> >>>>> #include <media/v4l2-device.h> >>>>> +#include <media/v4l2-fwnode.h> >>>>> #include <asm/unaligned.h> >>>>> >>>>> #define IMX258_REG_VALUE_08BIT 1 >>>>> @@ -1149,6 +1150,7 @@ static int imx258_init_controls(struct imx258 *imx258) >>>>> { >>>>> struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd); >>>>> struct v4l2_ctrl_handler *ctrl_hdlr; >>>>> + struct v4l2_fwnode_device_properties props; >>>> Might be nicer to move this one line up >>> Can you say what's your reasoning? I personally slightly prefer >>> alphabetical order, but no strong opinion :) >>> >> I've often been instructed to try to respect the inverse-xmas-tree > I'd advise the same, unless there are other reasons to arrange the lines > differently. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-01-04 12:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-25 15:42 [PATCH] media: i2c: imx258: Parse and register properties Robert Mader 2023-01-02 14:06 ` Jacopo Mondi 2023-01-03 14:11 ` Robert Mader 2023-01-03 17:16 ` Jacopo Mondi 2023-01-04 7:21 ` Sakari Ailus 2023-01-04 12:47 ` Robert Mader
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®