Re: [PATCH v4 08/17] media: i2c: rdacm21: Add dealy after OV490 reset
Hi Jacopo, There's still a s/dealy/delay/ in $SUBJECT On 12/04/2021 10:34, Jacopo Mondi wrote: > Add a delay after the OV490 chip is put in reset state. The reset > signal shall be held for at least 250 useconds. > > Signed-off-by: Jacopo Mondi I added this on v3... Reviewed-by: Kieran Bingham > --- > drivers/media/i2c/rdacm21.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 553e3f03752b..6be8ce130e78 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -469,7 +469,10 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > if (ret) > return ret; > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ > + /* > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration. > + * The reset signal has to be asserted for at least 250 useconds. Is it worth mentioning here that it is asserted to active low? Just to make it clear that holding it low for 250 uS is the desired effect? It might not be worth it - but perhaps that was the reason for some confusion here. Eitherway RB tag still stands I think. -- Kieran > + */ > ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > @@ -477,6 +480,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > + usleep_range(250, 500); > > ret = max9271_configure_gmsl_link(>serializer); > if (ret) >
Re: [PATCH 24/32] MAINTAINERS: update imi,rdacm2x-gmsl.yaml reference
Hi Mauro,
On 01/04/2021 13:17, Mauro Carvalho Chehab wrote:
> The file name: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> should be, instead:
> Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml.
>
> Update its cross-reference accordingly.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Fixes: e9f817689789 ("media: dt-bindings: media: i2c: Add bindings for IMI
> RDACM2x")
> Signed-off-by: Mauro Carvalho Chehab
Indeed, confirmed,
Reviewed-by: Kieran Bingham
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1644b6e9697c..b405ee71f730 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15258,7 +15258,7 @@ M:Laurent Pinchart
>
> M: Niklas Söderlund
> L: linux-me...@vger.kernel.org
> S: Maintained
> -F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F: Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml
> F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> F: drivers/media/i2c/rdacm21.c
>
Re: [PATCH v2] arm64: dts: renesas: falcon-cpu: Add GP LEDs
Hi Geert,
On 29/03/2021 09:30, Geert Uytterhoeven wrote:
> Hi Kieran,
>
> On Mon, Mar 22, 2021 at 6:20 PM Kieran Bingham
> wrote:
>> Three general purpose LEDs are provided on the Falcon CPU board.
>>
>> Connect GP_LED1, GP_LED2, and GP_LED3 to the gpio-leds frameworks as
>> indicator LEDs.
>>
>> These LEDs are arranged in a block of four LEDs on the board itself, but
>> the fourth LED is as yet unidentified.
>>
>> Signed-off-by: Kieran Bingham
>
> Thanks for your patch (which does not apply against renesas-devel)!
Oh, I'm sure I was based on renesas-devel/master when submitting that...
I guess I need to rebase my branch again.
>
>> --
>
> --- ;-)
Yup - sorry ;-)
>
>> v2:
>> - Move to r8a779a0-falcon-cpu.dtsi
>> - Define the colour, and function.
>
>> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
>> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
>> @@ -6,12 +6,37 @@
>> */
>>
>> #include
>> +#include
>> +
>> #include "r8a779a0.dtsi"
>>
>> / {
>> model = "Renesas Falcon CPU board";
>> compatible = "renesas,falcon-cpu", "renesas,r8a779a0";
>>
>> + leds {
>> + compatible = "gpio-leds";
>> +
>> + led1 {
>
> led-1?
>
> Documentation/devicetree/bindings/leds/leds-gpio.yaml says:
>
> # The first form is preferred, but fall back to just 'led' anywhere in the
> # node name to at least catch some child nodes.
> "(^led-[0-9a-f]$|led)":
Aha so picky ;-)
>> + gpios = < 18 GPIO_ACTIVE_HIGH>;
>> + color = ;
>> + function = LED_FUNCTION_INDICATOR;
>> + function-enumerator = <1>;
>> + };
>> + led2 {
>> + gpios = < 19 GPIO_ACTIVE_HIGH>;
>> + color = ;
>> + function = LED_FUNCTION_INDICATOR;
>> + function-enumerator = <2>;
>> + };
>> + led3 {
>> + gpios = < 20 GPIO_ACTIVE_HIGH>;
>> + color = ;
>> + function = LED_FUNCTION_INDICATOR;
>> + function-enumerator = <3>;
>> + };
>> + };
>> +
>> memory@4800 {
>> device_type = "memory";
>> /* first 128MB is reserved for secure area. */
>
> Reviewed-by: Geert Uytterhoeven
> i.e. will queue in renesas-devel for v5.13, with the above fixed.
Thanks.
For reference only, As previously discussed there is a fourth led in
this block of LEDs arranged on the board.
I can only confirm that it is not controllable by either GPIO4{17,21}.
But I have not been able to locate what might be controlling it.
So for now, it remains ignored. It might not be the safest to go
flipping all of the GPIOs randomly on this board ;-)
--
Kieran
>
> Gr{oetje,eeting}s,
>
> Geert
>
[PATCH v2] arm64: dts: renesas: falcon-cpu: Add GP LEDs
Three general purpose LEDs are provided on the Falcon CPU board.
Connect GP_LED1, GP_LED2, and GP_LED3 to the gpio-leds frameworks as
indicator LEDs.
These LEDs are arranged in a block of four LEDs on the board itself, but
the fourth LED is as yet unidentified.
Signed-off-by: Kieran Bingham
--
v2:
- Move to r8a779a0-falcon-cpu.dtsi
- Define the colour, and function.
---
.../boot/dts/renesas/r8a779a0-falcon-cpu.dtsi | 25 +++
1 file changed, 25 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
b/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
index 286a5d5b9b08..98aa918e716e 100644
--- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
@@ -6,12 +6,37 @@
*/
#include
+#include
+
#include "r8a779a0.dtsi"
/ {
model = "Renesas Falcon CPU board";
compatible = "renesas,falcon-cpu", "renesas,r8a779a0";
+ leds {
+ compatible = "gpio-leds";
+
+ led1 {
+ gpios = < 18 GPIO_ACTIVE_HIGH>;
+ color = ;
+ function = LED_FUNCTION_INDICATOR;
+ function-enumerator = <1>;
+ };
+ led2 {
+ gpios = < 19 GPIO_ACTIVE_HIGH>;
+ color = ;
+ function = LED_FUNCTION_INDICATOR;
+ function-enumerator = <2>;
+ };
+ led3 {
+ gpios = < 20 GPIO_ACTIVE_HIGH>;
+ color = ;
+ function = LED_FUNCTION_INDICATOR;
+ function-enumerator = <3>;
+ };
+ };
+
memory@4800 {
device_type = "memory";
/* first 128MB is reserved for secure area. */
--
2.25.1
Re: [PATCH v3 11/19] media: i2c: rdacm21: Add dealy after OV490 reset
Hi Jacopo, s/dealy/delay/ in $SUBJECT On 19/03/2021 16:41, Jacopo Mondi wrote: > Add a delay after the OV490 chip is put in reset state. The reset > signal shall be held for at least 250 useconds. > > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm21.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index babd14b21252..875bec9f7904 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -448,7 +448,10 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned > int val) > if (ret) > return ret; > > - /* Enable GPIO1 and hold OV490 in reset during max9271 configuration. */ > + /* > + * Enable GPIO1 and hold OV490 in reset during max9271 configuration. > + * The reset signal has to be asserted for at least 250 useconds. > + */ > ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > @@ -456,6 +459,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned > int val) > ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT); > if (ret) > return ret; > + usleep_range(250, 500); Aha, for a second there I thought the comment meant it had to be asserted for 250 uS before clearing it again. But it's 250 uS before using the OV490. Perhaps possible to update the comment a little, but nothing that matters. Reviewed-by: Kieran Bingham > ret = max9271_configure_gmsl_link(>serializer); > if (ret) >
Re: [PATCH v2 18/18] media: i2c: max9286: Rework comments in .bound()
On 15/03/2021 13:15, Jacopo Mondi wrote: > Re-phrase a comment in .bound() callback to make it clear we register > a subdev notifier and remove a redundant comment about disabling i2c > auto-ack. > > No functional changes intended. > > Signed-off-by: Jacopo Mondi Reviewed-by: Kieran Bingham > --- > drivers/media/i2c/max9286.c | 16 ++-- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index b6347639901e..16b2cb9b44a2 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -556,9 +556,9 @@ static int max9286_notify_bound(struct > v4l2_async_notifier *notifier, > subdev->name, src_pad, index); > > /* > - * We can only register v4l2_async_notifiers, which do not provide a > - * means to register a complete callback. bound_sources allows us to > - * identify when all remote serializers have completed their probe. > + * As we register a subdev notifiers we won't get a .complete() callback > + * here, so we have to use bound_sources to identify when all remote > + * serializers have probed. >*/ > if (priv->bound_sources != priv->source_mask) > return 0; > @@ -581,16 +581,12 @@ static int max9286_notify_bound(struct > v4l2_async_notifier *notifier, > /* >* All enabled sources have probed and enabled their reverse control >* channels: > + * - The reverse channel amplitude stays high >* - Verify all configuration links are properly detected > - * - Disable auto-ack as communication on the control channel are now > - * stable. > + * - Disable auto-ack as communications on the control channel are now > + * stable >*/ > max9286_check_config_link(priv, priv->source_mask); > - > - /* > - * Re-configure I2C with local acknowledge disabled after cameras have > - * probed. > - */ > max9286_configure_i2c(priv, false); > > return max9286_set_pixelrate(priv); >
Re: [PATCH v2 12/18] media: i2c: rdacm21: Give more time to OV490 to boot
On 15/03/2021 13:15, Jacopo Mondi wrote:
> It has been observed through repeated testing (250 boots) that in the
> 10% of the cases the RDACM21 initialization sequence errors out due a
> timeout waiting for the OV490 firmware to complete its boot phase.
>
> Albeit being the current timeout relatively large (300-600 milliseconds),
> doubling it reduces the sporadic error rate down to 1 over an 80 boot
> sequences test run.
>
> The firmware boot delay is unfortunately not characterized in the camera
> module manual.
>
I wonder if we could characterize this alone by pulling this down until
we see failures increase, with all the other fixes in place...
I don't think that's required, but it might be something to check later
if we don't get rid of that 1/80 failure.
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Jacopo Mondi
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/rdacm21.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 50a9b0d8255d..07cf077d8efd 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -53,7 +53,7 @@
> #define OV490_PID0x8080300a
> #define OV490_VER0x8080300b
> #define OV490_PID_TIMEOUT20
> -#define OV490_OUTPUT_EN_TIMEOUT 300
> +#define OV490_OUTPUT_EN_TIMEOUT 600
>
> #define OV490_GPIO0 BIT(0)
> #define OV490_SPWDN0 BIT(0)
>
Re: [PATCH v2 11/18] media: i2c: rdacm21: Fix OV10640 powerdown
Hi Jacopo,
On 15/03/2021 13:15, Jacopo Mondi wrote:
> The OV10640 image sensor powerdown signal is controlled by the first
> line of the OV490 GPIO pad #1, but the pad #0 identifier
> OV490_GPIO_OUTPUT_VALUE0 was erroneously used. As a result the image
> sensor powerdown signal was never asserted but was kept high by an
> internal pull-up resistor, causing sporadic failures during the
> image sensor startup phase.
>
> Fix this by using the correct GPIO pad identifier.
>
> While at it also fix the GPIO signal handling sequence, as the reset
> line was released before the powerdown one, and introduce the correct
> delays in between the two operations.
>
> Wait the mandatory 1 millisecond delay after the powerup lane is
Technically you wait 1.5 milliseconds below - but I think that's fine
here ;-)
> asserted. The reset delay is not characterized in the chip manual if
> not as "255 XVCLK + initialization". Wait for at least 3 milliseconds
> to guarantee the SCCB bus is available.
>
> This commit fixes a sporadic start-up error triggered by a failure to
> read the OV10640 chip ID:
> rdacm21 8-0054: OV10640 ID mismatch: (0x01)
>
> Fixes: a59f853b3b4b ("media: i2c: Add driver for RDACM21 camera module")
> Signed-off-by: Jacopo Mondi
Sometime it might be nice to see this look more like the GPIO
interfaces, but I think this is fine for now.
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/rdacm21.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 7bce55adfd7c..50a9b0d8255d 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device
> *dev)
> {
> u8 val;
>
> - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */
> ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> +
> + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE1, OV490_SPWDN0);
> + usleep_range(1500, 3000);
> ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> usleep_range(3000, 5000);
>
> /* Read OV10640 ID to test communications. */
>
Re: [PATCH v2 09/18] media: i2c: max9271: Introduce wake_up() function
On 15/03/2021 13:15, Jacopo Mondi wrote:
> The MAX9271 chip manual prescribes a delay of 5 milliseconds
> after the chip exists from low power state.
>
> Add a new function to the max9271 library driver that wakes up the chip
> with a dummy i2c transaction and implements the correct delay of 5
> milliseconds after the chip exits from low power state.
>
> Use the newly introduced function in the rdacm20 and rdacm21 camera
> drivers. The former was not respecting the required delay while the
> latter was waiting for a too-short timeout.
>
> Do not handle the initial i2c address configuration in the library
> driver function as the camera module drivers control address
> reprogramming.
I still think that the MAX9271_DEFAULT_ADDR belongs in the max9271
library, but this patch is functionally good to me, and correcting those
delays is certainly a good thing to fix here.
Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9271.c | 7 +++
> drivers/media/i2c/max9271.h | 9 +
> drivers/media/i2c/rdacm20.c | 2 +-
> drivers/media/i2c/rdacm21.c | 3 +--
> 4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 2c7dc7fb9846..f7bfe7266763 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -80,6 +80,13 @@ static int max9271_pclk_detect(struct max9271_device *dev)
> return -EIO;
> }
>
> +void max9271_wake_up(struct max9271_device *dev)
> +{
> + i2c_smbus_read_byte(dev->client);
> + usleep_range(5000, 8000);
> +}
> +EXPORT_SYMBOL_GPL(max9271_wake_up);
> +
> int max9271_set_serial_link(struct max9271_device *dev, bool enable)
> {
> int ret;
> diff --git a/drivers/media/i2c/max9271.h b/drivers/media/i2c/max9271.h
> index d78fb21441e9..dc5e4e70ba6f 100644
> --- a/drivers/media/i2c/max9271.h
> +++ b/drivers/media/i2c/max9271.h
> @@ -85,6 +85,15 @@ struct max9271_device {
> struct i2c_client *client;
> };
>
> +/**
> + * max9271_wake_up() - Wake up the serializer by issuing an i2c transaction
> + * @dev: The max9271 device
> + *
> + * This function shall be called before any other interaction with the
> + * serializer.
> + */
> +void max9271_wake_up(struct max9271_device *dev);
> +
> /**
> * max9271_set_serial_link() - Enable/disable serial link
> * @dev: The max9271 device
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index b9aaa0f7db42..2265ef7c65d4 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -459,7 +459,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer.client);
> + max9271_wake_up(>serializer);
>
> /* Serial link disabled during config as it needs a valid pixel clock.
> */
> ret = max9271_set_serial_link(>serializer, false);
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 179d107f494c..7bce55adfd7c 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -452,8 +452,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev)
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer.client);
> - usleep_range(3000, 5000);
> + max9271_wake_up(>serializer);
>
> /* Enable reverse channel and disable the serial link. */
> ret = max9271_set_serial_link(>serializer, false);
>
Re: [PATCH v2 06/18] media: i2c: max9271: Check max9271_write() return
Hi Jacopo,
On 15/03/2021 13:15, Jacopo Mondi wrote:
> Check the return value of the max9271_write() function in the
> max9271 library driver.
>
> While at it, modify an existing condition to be made identical
> to other checks.
Excellent, much better to catch write errors early.
Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9271.c | 30 +++---
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index c495582dcff6..2c7dc7fb9846 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -106,7 +106,10 @@ int max9271_set_serial_link(struct max9271_device *dev,
> bool enable)
>* Short delays here appear to show bit-errors in the writes following.
>* Therefore a conservative delay seems best here.
>*/
> - max9271_write(dev, 0x04, val);
> + ret = max9271_write(dev, 0x04, val);
> + if (ret < 0)
> + return ret;
> +
> usleep_range(5000, 8000);
>
> return 0;
> @@ -118,7 +121,7 @@ int max9271_configure_i2c(struct max9271_device *dev, u8
> i2c_config)
> int ret;
>
> ret = max9271_write(dev, 0x0d, i2c_config);
> - if (ret)
> + if (ret < 0)
> return ret;
>
> /* The delay required after an I2C bus configuration change is not
> @@ -143,7 +146,10 @@ int max9271_set_high_threshold(struct max9271_device
> *dev, bool enable)
>* Enable or disable reverse channel high threshold to increase
>* immunity to power supply noise.
>*/
> - max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
> + ret = max9271_write(dev, 0x08, enable ? ret | BIT(0) : ret & ~BIT(0));
> + if (ret < 0)
> + return ret;
> +
> usleep_range(2000, 2500);
>
> return 0;
> @@ -152,6 +158,8 @@ EXPORT_SYMBOL_GPL(max9271_set_high_threshold);
>
> int max9271_configure_gmsl_link(struct max9271_device *dev)
> {
> + int ret;
> +
> /*
>* Configure the GMSL link:
>*
> @@ -162,16 +170,24 @@ int max9271_configure_gmsl_link(struct max9271_device
> *dev)
>*
>* TODO: Make the GMSL link configuration parametric.
>*/
> - max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
> - MAX9271_EDC_1BIT_PARITY);
> + ret = max9271_write(dev, 0x07, MAX9271_DBL | MAX9271_HVEN |
> + MAX9271_EDC_1BIT_PARITY);
> + if (ret < 0)
> + return ret;
> +
> usleep_range(5000, 8000);
>
> /*
>* Adjust spread spectrum to +4% and auto-detect pixel clock
>* and serial link rate.
>*/
> - max9271_write(dev, 0x02, MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
> - MAX9271_PCLK_AUTODETECT | MAX9271_SERIAL_AUTODETECT);
> + ret = max9271_write(dev, 0x02,
> + MAX9271_SPREAD_SPECT_4 | MAX9271_R02_RES |
> + MAX9271_PCLK_AUTODETECT |
> + MAX9271_SERIAL_AUTODETECT);
> + if (ret < 0)
> + return ret;
> +
> usleep_range(5000, 8000);
>
> return 0;
>
Re: [PATCH v2 04/18] media: i2c: rdacm20: Replace goto with a loop
Hi Jacopo,
On 15/03/2021 13:14, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
>
> Replace it with a more compact for() loop.
>
> No functional changes intended.
>
> Reviewed-by: Kieran Bingham
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 29 +
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 4d9bac87cba8..e190fd2e611a 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
> */
> #define OV10635_PIXEL_RATE (4400)
>
> +#define OV10635_PID_TIMEOUT 3
> +
> static const struct ov10635_reg {
> u16 reg;
> u8 val;
> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
>
> static int rdacm20_initialize(struct rdacm20_device *dev)
> {
> - unsigned int retry = 3;
> + unsigned int i;
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> @@ -501,23 +503,18 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
> return ret;
> usleep_range(1, 15000);
>
> -again:
> - ret = ov10635_read16(dev, OV10635_PID);
> - if (ret < 0) {
> - if (retry--)
> - goto again;
> + for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> + ret = ov10635_read16(dev, OV10635_PID);
> + if (ret == OV10635_VERSION)
> + break;
> + else if (ret >= 0)
> + /* Sometimes we get a successful read but a wrong id. */
Trivial/nit, I'd have written "but an incorrect ID."
But that's not worthy of any respin.
> + dev_dbg(dev->dev, "OV10635 ID mismatch (%d)\n", ret);
Thanks, this alleviates my concerns from the previous version.
>
> - dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> - ret);
> - return -ENXIO;
> + usleep_range(1000, 2000);
> }
> -
> - if (ret != OV10635_VERSION) {
> - if (retry--)
> - goto again;
> -
> - dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> - ret);
> + if (i == OV10635_PID_TIMEOUT) {
> + dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> return -ENXIO;
> }
>
>
Re: [PATCH v2 03/18] media: i2c: rdacm20: Embedded 'serializer' field
Hi Jacopo,
in $SUBJECT s/Embedded/Embed/
But it's trivial so no worries unless there's another iteration.
On 15/03/2021 13:14, Jacopo Mondi wrote:
> There's no reason to allocate dynamically the 'serializer' field in
> the driver structure.
>
> Embed the field and adjust all its users in the driver.
>
> Reviewed-by: Kieran Bingham
Still holds ;-)
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 38 -
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index f7fd5ae955d0..4d9bac87cba8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -312,7 +312,7 @@ static const struct ov10635_reg {
>
> struct rdacm20_device {
> struct device *dev;
> - struct max9271_device *serializer;
> + struct max9271_device serializer;
> struct i2c_client *sensor;
> struct v4l2_subdev sd;
> struct media_padpad;
> @@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int
> enable)
> {
> struct rdacm20_device *dev = sd_to_rdacm20(sd);
>
> - return max9271_set_serial_link(dev->serializer, enable);
> + return max9271_set_serial_link(>serializer, enable);
> }
>
> static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -456,11 +456,11 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> - dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer->client);
> + dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> + i2c_smbus_read_byte(dev->serializer.client);
>
> /* Serial link disabled during config as it needs a valid pixel clock.
> */
> - ret = max9271_set_serial_link(dev->serializer, false);
> + ret = max9271_set_serial_link(>serializer, false);
> if (ret)
> return ret;
>
> @@ -468,35 +468,35 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
>* Ensure that we have a good link configuration before attempting to
>* identify the device.
>*/
> - max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -MAX9271_I2CSLVTO_1024US |
> -MAX9271_I2CMSTBT_105KBPS);
> + max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS |
> + MAX9271_I2CSLVTO_1024US |
> + MAX9271_I2CMSTBT_105KBPS);
>
> - max9271_configure_gmsl_link(dev->serializer);
> + max9271_configure_gmsl_link(>serializer);
>
> - ret = max9271_verify_id(dev->serializer);
> + ret = max9271_verify_id(>serializer);
> if (ret < 0)
> return ret;
>
> - ret = max9271_set_address(dev->serializer, dev->addrs[0]);
> + ret = max9271_set_address(>serializer, dev->addrs[0]);
> if (ret < 0)
> return ret;
> - dev->serializer->client->addr = dev->addrs[0];
> + dev->serializer.client->addr = dev->addrs[0];
>
> /*
>* Reset the sensor by cycling the OV10635 reset signal connected to the
>* MAX9271 GPIO1 and verify communication with the OV10635.
>*/
> - ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
>
> - ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> usleep_range(1, 15000);
>
> - ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> usleep_range(1, 15000);
> @@ -560,13 +560,7 @@ static int rdacm20_probe(struct i2c_client *client)
> if (!dev)
> return -ENOMEM;
> dev->dev = >dev;
> -
> - dev->serializer = devm_kzalloc(>dev, sizeof(*dev->serializer),
> -GFP_KERNEL);
> - if (!dev->serializer)
> - return -ENOMEM;
> -
> - dev->serializer->client = client;
> + dev->serializer.client = client;
>
> ret = of_property_read_u32_array(client->dev.of_node, "reg",
>dev->addrs, 2);
>
Re: [PATCH v2 01/18] media: i2c: rdamc21: Fix warning on u8 cast
Hi Jacopo, On 15/03/2021 13:14, Jacopo Mondi wrote: > Sparse reports a warning on a cast to u8 of a 16 bits constant. > > drivers/media/i2c/rdacm21.c:348:62: warning: cast truncates bits > from constant value (300a becomes a) > > Even if the behaviour is intended, silence the sparse warning replacing > the cast with a bitwise & operation. > > Reported-by: Hans Verkuil > Signed-off-by: Jacopo Mondi Sounds good to me. Reviewed-by: Kieran Bingham > --- > drivers/media/i2c/rdacm21.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index dcc21515e5a4..179d107f494c 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -345,7 +345,7 @@ static int ov10640_initialize(struct rdacm21_device *dev) > /* Read OV10640 ID to test communications. */ > ov490_write_reg(dev, OV490_SCCB_SLAVE0_DIR, OV490_SCCB_SLAVE_READ); > ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_HIGH, OV10640_CHIP_ID >> 8); > - ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, (u8)OV10640_CHIP_ID); > + ov490_write_reg(dev, OV490_SCCB_SLAVE0_ADDR_LOW, OV10640_CHIP_ID & > 0xff); > > /* Trigger SCCB slave transaction and give it some time to complete. */ > ov490_write_reg(dev, OV490_HOST_CMD, OV490_HOST_CMD_TRIGGER); >
Re: [PATCH] arm64: dts: renesas: falcon: Add GP LEDs
On 05/03/2021 14:10, Geert Uytterhoeven wrote:
> Hi Kieran,
>
> On Thu, Mar 4, 2021 at 5:53 PM Kieran Bingham
> wrote:
>> Three general purpose LEDs are provided on the Falcon CPU board.
>>
>> Connect GP_LED1, GP_LED2, and GP_LED3 to the gpio-leds frameworks.
>> These LEDs are arranged in a block of four LEDs on the board itself, but
>> the fourth LED is as yet unidentified.
>>
>> Signed-off-by: Kieran Bingham
>
> Thanks for your patch!
>
>> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
>
> I believe the LEDs are on the CPU board, so they belong in
> r8a779a0-falcon-cpu.dtsi instead?
Yes, I hadn't quite grasped the layouts of this new board yet.
Sorry - I'll move it.
>
>> @@ -20,6 +20,20 @@ aliases {
>> chosen {
>> stdout-path = "serial0:115200n8";
>> };
>> +
>> + leds {
>> + compatible = "gpio-leds";
>> +
>> + led1 {
>> + gpios = < 18 GPIO_ACTIVE_HIGH>;
>
> Any need for other properties from
> Documentation/devicetree/bindings/leds/common.yaml, like
> color = ?
Oh, I wasn't aware of those. But indeed the LED is green, so it seems
appropriate.
I'll test it out, and see if I can make it glow red (jokes).
>
>> + };
>> + led2 {
>> + gpios = < 19 GPIO_ACTIVE_HIGH>;
>> + };
>> + led3 {
>> + gpios = < 20 GPIO_ACTIVE_HIGH>;
>> + };
>> + };
>> };
>
> Gr{oetje,eeting}s,
>
> Geert
>
[PATCH] arm64: dts: renesas: falcon: Add GP LEDs
Three general purpose LEDs are provided on the Falcon CPU board.
Connect GP_LED1, GP_LED2, and GP_LED3 to the gpio-leds frameworks.
These LEDs are arranged in a block of four LEDs on the board itself, but
the fourth LED is as yet unidentified.
Signed-off-by: Kieran Bingham
---
arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts | 14 ++
1 file changed, 14 insertions(+)
diff --git a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
index 5617b81dd7dc..a18f84128fe1 100644
--- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
+++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon.dts
@@ -20,6 +20,20 @@ aliases {
chosen {
stdout-path = "serial0:115200n8";
};
+
+ leds {
+ compatible = "gpio-leds";
+
+ led1 {
+ gpios = < 18 GPIO_ACTIVE_HIGH>;
+ };
+ led2 {
+ gpios = < 19 GPIO_ACTIVE_HIGH>;
+ };
+ led3 {
+ gpios = < 20 GPIO_ACTIVE_HIGH>;
+ };
+ };
};
{
--
2.25.1
Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop
On 24/02/2021 20:27, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Feb 22, 2021 at 04:06:43PM +0100, Jacopo Mondi wrote:
>> On Mon, Feb 22, 2021 at 03:05:03AM +0200, Laurent Pinchart wrote:
>>> Hi Jacopo,
>>>
>>> Reviewed-by: Laurent Pinchart
>>>
>>> On Wed, Feb 17, 2021 at 01:01:26PM +, Kieran Bingham wrote:
>>>> On 16/02/2021 17:41, Jacopo Mondi wrote:
>>>>> During the camera module initialization the image sensor PID is read to
>>>>> verify it can correctly be identified. The current implementation is
>>>>> rather confused and uses a loop implemented with a label and a goto.
>>>>>
>>>>> Replace it with a more compact for() loop.
>>>>>
>>>>> No functional changes intended.
>>>>
>>>> I think there is a functional change in here, but I almost like it.
>>>>
>>>> Before, if the read was successful, it would check to see if the
>>>> OV10635_PID == OV10635_VERSION, and if not it would print that the read
>>>> was successful but a mismatch.
>>>>
>>>> Now - it will retry again instead, and if at the end of the retries it
>>>> still fails then it's a failure.
>>>>
>>>> This means we perhaps don't get told if the device id is not correct in
>>>> the same way, but it also means that if the VERSION was not correct
>>>> because of a read error (which I believe i've seen occur), it will retry.
So - to be clear here, I meant a 'read error', as in perhaps a
one-bit-flip or something else, not an error detected and propogated by
the I2C controllers.
I.e. ... something happening on the bus that gives a different result
but the 'read' was successful it's just that it returns a different
value than expected.
Given our noisy bus, not certain bus speeds, etc etc, I believe this can
happen.
>>>
>>> I was going to ask about that, whether we can have a successful I2C read
>>> operation that would return incorrect data. If we do, aren't we screwed
>>> ? If there's a non-negligible probability that reads will return
>>> incorrect data without any way to know about it (for other registers
>>> than the version register of course), then I would consider that writes
>>> could fail the same way, and that would mean an unusable device,
>>> wouldn't it ?
>>>
>>> If, on the other hand, read failures can always (or nearly always,
>>> ignoring space neutrinos and similar niceties) be detected, then I think
>>> we should avoid the functional change.
>>>
>>>> Because there is a functional change you might want to update the
>>>> commit, but I still think this is a good change overall.
>>
>> I'm not sure I got your concerns to be honest :/
>> yes before the code flow was like
>>
>> ret = ov10635_read();
>> if (ret < 0) {
>>
>> }
>>
>> if (ret != PID) {
>>
And so here you might have had a 'successful' read of the wrong value,
which means that ret > 0 but != PID.
>> }
>>
>> But the condition ret != PID implied ret < 0 so I don't really get
>> what changes, apart from the fact that in the previous version we
>> could have had two different error messages for the same issue, and yes,
>> I saw ID mistmatch happening but the value of knowing the i2c read
>> didn't fail but the read data was garbage (usually it's 0x01 when it
>> fails iirc) is, well, questionable.
>
> That's worrying :-S May we should add a warning message when the read
> succeeds but the ID doesn't match, to at least have a way to track the
> issue, and see if other changes get rid of this problem ?
>
Ok, now I'm confused, that's what I was talking about!
Before we did do this, and now we don't. Ergo - functional change.
>> I'm sorry I didn't fully get this comment.
>
> You're right, I had missed that the current code retried in case of a
> version number mismatch. There's no functional change.
I still think there's a functional change, but I'm not all too worried
about it.
As I said before, I think it's worth the retry in that event, which
didn't happen before, so my tag still holds.
> Reviewed-by: Laurent Pinchart
>
>>>> Reviewed-by: Kieran Bingham
>>>>
>>>>> Signed-off-by: Jacopo Mondi
>>>>> ---
>>>>> drivers/media/i2c/rdacm20.c | 27 ++-
>>>>> 1 file changed, 10 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/i2c/rdac
Re: [PATCH] scripts/gdb: document lx_current is only supported by x86
Hi Barry
On 21/02/2021 21:35, Barry Song wrote:
> lx_current depends on the per_cpu current_task which exists on x86 only:
>
> arch$ git grep current_task | grep -i per_cpu
> x86/include/asm/current.h:DECLARE_PER_CPU(struct task_struct *, current_task);
> x86/kernel/cpu/common.c:DEFINE_PER_CPU(struct task_struct *, current_task)
> cacheline_aligned =
> x86/kernel/cpu/common.c:EXPORT_PER_CPU_SYMBOL(current_task);
> x86/kernel/cpu/common.c:DEFINE_PER_CPU(struct task_struct *, current_task) =
> _task;
> x86/kernel/cpu/common.c:EXPORT_PER_CPU_SYMBOL(current_task);
> x86/kernel/smpboot.c: per_cpu(current_task, cpu) = idle;
>
> On other architectures, lx_current() will lead to a python exception:
> (gdb) p $lx_current().pid
> Python Exception No symbol "current_task" in current
> context.:
> Error occurred in Python: No symbol "current_task" in current context.
>
> To avoid more people struggling and wasting time in other architectures,
> document it.
>
> Cc: Jan Kiszka
> Signed-off-by: Barry Song
> ---
> Documentation/dev-tools/gdb-kernel-debugging.rst | 2 +-
> scripts/gdb/linux/cpus.py| 10 --
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/dev-tools/gdb-kernel-debugging.rst
> b/Documentation/dev-tools/gdb-kernel-debugging.rst
> index 4756f6b3a04e..1586901b683c 100644
> --- a/Documentation/dev-tools/gdb-kernel-debugging.rst
> +++ b/Documentation/dev-tools/gdb-kernel-debugging.rst
> @@ -114,7 +114,7 @@ Examples of using the Linux-provided gdb helpers
> [ 0.00] BIOS-e820: [mem 0x0009fc00-0x0009]
> reserved
>
>
> -- Examine fields of the current task struct::
> +- Examine fields of the current task struct(supported by x86 only)::
>
> (gdb) p $lx_current().pid
> $1 = 4998
> diff --git a/scripts/gdb/linux/cpus.py b/scripts/gdb/linux/cpus.py
> index 008e62f3190d..f382762509d3 100644
> --- a/scripts/gdb/linux/cpus.py
> +++ b/scripts/gdb/linux/cpus.py
> @@ -156,6 +156,13 @@ Note that VAR has to be quoted as string."""
>
> PerCpu()
>
> +def get_current_task(cpu):
> +if utils.is_target_arch("x86"):
> + var_ptr = gdb.parse_and_eval("_task")
> + return per_cpu(var_ptr, cpu).dereference()
> +else:
> +raise gdb.GdbError("Sorry, obtaining the current task is not yet "
> + "supported with this arch")
I've wondered in the past how we should handle the architecture specific
layers.
Perhaps we need to have an interface of functionality to implement on
each architecture so that we can create a per-arch set of helpers.
or break it up into arch specific subdirs at least...
> class LxCurrentFunc(gdb.Function):
> """Return current task.
> @@ -167,8 +174,7 @@ number. If CPU is omitted, the CPU of the current context
> is used."""
> super(LxCurrentFunc, self).__init__("lx_current")
>
> def invoke(self, cpu=-1):
> -var_ptr = gdb.parse_and_eval("_task")
> -return per_cpu(var_ptr, cpu).dereference()
> +return get_current_task(cpu)
>
And then perhaps we simply shouldn't even expose commands which can not
be supported on those architectures?
Is it easy to disable this command if it's not supportable on the
architecture?
Presumably you are working on non-x86, have you investigated adding this
support for your architecture (arm/arm64?)?
The fact you have run the command implies it would be useful for you ?
> LxCurrentFunc()
>
--
Regards
--
Kieran
Re: [PATCH 13/16] media: i2c: rdacm2x: Implement .init() subdev op
Hi Jacopo,
On 16/02/2021 17:41, Jacopo Mondi wrote:
> The current probe() procedure of the RDACM20 and RDACM20 performs
and RDACM21?
> initialization of the serializer image sensors and increases the noise
Of the serializer 'and' image sensors ?
or perhaps just
s/serializer/serializer,/ ?
> immunity threshold as last operation, which is then compensated by the
as a last operation
or
as a final operation
> remote deserializer by increasing the reverse channel signal amplitude
> once all remotes have bound.
>
> The probe routine is then run without noise immunity activated which
> in noisy environment conditions makes the probe sequence less reliable as
> the chips configuration requires a relevant amount of i2c transactions.
s/relevant/relatively high/ ?
>
> Break chip initialization in two:
> - At probe time only configure the serializer's reverse channel with
> noise immunity activated, to reduce the number of transactions
> performed without noise immunity protection
> - Move the chips initialization to the .init() core subdev operation to
> be invoked by the deserializer after the camera has probed and it has
> increased the reverse channel amplitude
Is this the op you said was deprecated?
Functionally in this code, it seems fine, but as mentioned on the next
patch, I suspect it might need squashing to make sure it stays functional...
I'm not fully sure of the implications of this patch, but your tests
have reportede that this series is helping a lot with reliability so I
don't want to block it.
The code changes themselves look ok, with the following thougts:
- If this op/methodology is deprecated it might be harder to get
acceptance?
- now we have _init and _initialise - should that be made more
distinct?
- Seeing the duplication of the MAX9271_DEFAULT_ADDR / ping again
really makes me want to see that wrapped in the max9271.c ;-)
- Likely needs squashed with relevant changes in max9286?
But even with those thoughts, I don't think this is necessarily wrong so:
Reviewed-by: Kieran Bingham
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 65 ++---
> drivers/media/i2c/rdacm21.c | 65 ++---
> 2 files changed, 78 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 39e4b4241870..0632ef98eea7 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -437,36 +437,12 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> - .s_stream = rdacm20_s_stream,
> -};
> -
> -static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> - .enum_mbus_code = rdacm20_enum_mbus_code,
> - .get_fmt= rdacm20_get_fmt,
> - .set_fmt= rdacm20_get_fmt,
> -};
> -
> -static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> - .video = _video_ops,
> - .pad= _subdev_pad_ops,
> -};
> -
> -static int rdacm20_initialize(struct rdacm20_device *dev)
> +static int rdacm20_init(struct v4l2_subdev *sd, unsigned int val)
> {
> + struct rdacm20_device *dev = sd_to_rdacm20(sd);
> unsigned int i;
> int ret;
>
> - /* Verify communication with the MAX9271: ping to wakeup. */
> - dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer.client);
> - usleep_range(5000, 8000);
> -
> - /* Serial link disabled during config as it needs a valid pixel clock.
> */
> - ret = max9271_set_serial_link(>serializer, false);
> - if (ret)
> - return ret;
> -
> /*
>* Ensure that we have a good link configuration before attempting to
>* identify the device.
> @@ -537,6 +513,43 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>
> dev_info(dev->dev, "Identified RDACM20 camera module\n");
>
> + return 0;
> +}
> +
> +static const struct v4l2_subdev_core_ops rdacm20_core_ops = {
> + .init = rdacm20_init,
> +};
> +
> +static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> + .s_stream = rdacm20_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops rdacm20_subdev_pad_ops = {
> + .enum_mbus_code = rdacm20_enum_mbus_code,
> + .get_fmt= rdacm20_get_fmt,
> + .set_fmt= rdacm20_get_fmt,
> +};
> +
> +static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> + .core = _core_ops,
> + .video = _video_ops,
> + .pad= _subdev_pad_ops,
> +}
Re: [PATCH 16/16] media: i2c: gmsl: Use 339Kbps I2C bit-rate
On 16/02/2021 17:41, Jacopo Mondi wrote:
> With the camera modules initialization routines now running with
> the noise immunity threshold enabled, it is possible to restore
> the bit rate of the I2C transactions transported on the GMSL control
> channel to 339 Kbps.
>
> The 339 Kbps bit rate represents the default setting for the serializer
> and the deserializer chips, and the setup/hold time and slave timeout
> time in use are calibrate to support that rate.
s/calibrate/calibrated/
Does that mean the setup/hold time and timeouts should be adjusted based
on the i2c speed? (which we have not been doing?)
With all of your other reliability improvements, does *this* change
alone have any difference or impact on reliability?
I.e. if we go slower (stay at current speed) - would we be more reliable?
Is there a reliability improvement by making this speed faster?
I presume we don't have a way to convey the i2c bus speed between the
max9286 and the max9271 currently? Seems a bit like a bus parameter
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 2 +-
> drivers/media/i2c/rdacm20.c | 2 +-
> drivers/media/i2c/rdacm21.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index aa01d5bb79ef..0b620f2f8c41 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -330,7 +330,7 @@ static int max9286_i2c_mux_init(struct max9286_priv *priv)
> static void max9286_configure_i2c(struct max9286_priv *priv, bool localack)
> {
> u8 config = MAX9286_I2CSLVSH_469NS_234NS | MAX9286_I2CSLVTO_1024US |
> - MAX9286_I2CMSTBT_105KBPS;
> + MAX9286_I2CMSTBT_339KBPS;
>
> if (localack)
> config |= MAX9286_I2CLOCACK;
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 0632ef98eea7..d45e8b0e52a0 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -450,7 +450,7 @@ static int rdacm20_init(struct v4l2_subdev *sd, unsigned
> int val)
> ret = max9271_configure_i2c(>serializer,
> MAX9271_I2CSLVSH_469NS_234NS |
> MAX9271_I2CSLVTO_1024US |
> - MAX9271_I2CMSTBT_105KBPS);
> + MAX9271_I2CMSTBT_339KBPS);
> if (ret)
> return ret;
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index 80b6f16f87a8..552985026458 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -442,7 +442,7 @@ static int rdacm21_init(struct v4l2_subdev *sd, unsigned
> int val)
> ret = max9271_configure_i2c(>serializer,
> MAX9271_I2CSLVSH_469NS_234NS |
> MAX9271_I2CSLVTO_1024US |
> - MAX9271_I2CMSTBT_105KBPS);
> + MAX9271_I2CMSTBT_339KBPS);
> if (ret)
> return ret;
>
>
Re: [PATCH 14/16] media: i2c: max9286: Initialize remotes when bound
On 16/02/2021 17:41, Jacopo Mondi wrote:
> With the introduction of the .init() core subdev operation in the
> max9271 GMSL serializer, the max9286 deserializer needs to explicitly
> initialize the remote devices by calling the .init() subdev operation on
> each probed camera.
>
> Call the .init() subdev operation at remote bound time and toggle
> the reverse channel amplitude to compensate for the remote ends
> noise immunity threshold.
>
Should this patch be merged with the previous one to keep the RDACM2x
and max9286 usage aligned?
I expect it won't compile fail, but it would fail a test (if bisecting
was testing the capture).
Seems to look ok, given the previous patch as a dependency:
Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 24 +++-
> 1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7913b5f2249e..c41284de89b6 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -554,25 +554,39 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
> dev_dbg(>client->dev, "Bound %s pad: %u on index %u\n",
> subdev->name, src_pad, index);
>
> + /*
> + * Initialize the remote camera. Increase the channel amplitude
> + * to compensate for the remote noise immunity threshold.
> + */
> + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> + ret = v4l2_subdev_call(subdev, core, init, 0);
> + if (ret) {
> + dev_err(>client->dev,
> + "Failed to initialize camera device %u\n", index);
> + return ret;
> + }
> +
> /*
>* We can only register v4l2_async_notifiers, which do not provide a
>* means to register a complete callback. bound_sources allows us to
>* identify when all remote serializers have completed their probe.
>*/
> - if (priv->bound_sources != priv->source_mask)
> + if (priv->bound_sources != priv->source_mask) {
> + /*
> + * If not all remotes have probed yet, restore the initial
> + * reverse channel amplitude to allow the next camera to probe.
> + */
> + max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
Oh - wow, now I see why we definitely need to store both the initial and
the current value.
> return 0;
> + }
>
> /*
>* All enabled sources have probed and enabled their reverse control
>* channels:
> - *
> - * - Increase the reverse channel amplitude to compensate for the
> - * remote ends high threshold
>* - Verify all configuration links are properly detected
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> - max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
>
Re: [PATCH 15/16] media: i2c: max9286: Rework comments in .bound()
On 16/02/2021 17:41, Jacopo Mondi wrote:
> Re-phrase a comment in .bound() callback to make it clear we register
> a subdev notifier and remove a redundant comment about disabling i2c
> auto-ack.
>
> No functional changes intended.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 16 ++--
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index c41284de89b6..aa01d5bb79ef 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -567,9 +567,9 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
> }
>
> /*
> - * We can only register v4l2_async_notifiers, which do not provide a
> - * means to register a complete callback. bound_sources allows us to
> - * identify when all remote serializers have completed their probe.
> + * As we register a subdev notifiers we won't get a .complete() callback
> + * here, so we have to use bound_sources to identify when all remote
> + * serializers have probed.
s/subdev notifiers/subdev notifier/ ?
... I'm not sure that helps me ? :S
>*/
> if (priv->bound_sources != priv->source_mask) {
> /*
> @@ -583,16 +583,12 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
> /*
>* All enabled sources have probed and enabled their reverse control
>* channels:
> + * - The reverse channel amplitude stays high
Isn't this part of the previous patch?
>* - Verify all configuration links are properly detected
> - * - Disable auto-ack as communication on the control channel are now
> - * stable.
> + * - Disable auto-ack as communications on the control channel are now
> + * stable
>*/
> max9286_check_config_link(priv, priv->source_mask);
> -
> - /*
> - * Re-configure I2C with local acknowledge disabled after cameras have
> - * probed.
> - */
> max9286_configure_i2c(priv, false);
>
> return max9286_set_pixelrate(priv);
>
Re: [PATCH 12/16] media: i2c: max9286: Define high channel amplitude
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > Provide a macro to define the reverse channel amplitude to > be used to compensate the remote serializer noise immunity. > > While at it, update a comment. Reviewed-by: Kieran Bingham > > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/max9286.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 4afb5ca06448..7913b5f2249e 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -113,6 +113,7 @@ > #define MAX9286_REV_TRF(n) ((n) << 4) > #define MAX9286_REV_AMP(n) n) - 30) / 10) << 1) /* in mV */ > #define MAX9286_REV_AMP_XBIT(0) > +#define MAX9286_REV_AMP_HIGH 170 > /* Register 0x3f */ > #define MAX9286_EN_REV_CFG BIT(6) > #define MAX9286_REV_FLEN(n) ((n) - 20) > @@ -566,12 +567,12 @@ static int max9286_notify_bound(struct > v4l2_async_notifier *notifier, >* channels: >* >* - Increase the reverse channel amplitude to compensate for the > - * remote ends high threshold, if not done already > + * remote ends high threshold >* - Verify all configuration links are properly detected >* - Disable auto-ack as communication on the control channel are now >* stable. >*/ > - max9286_reverse_channel_setup(priv, 170); > + max9286_reverse_channel_setup(priv, MAX9286_REV_AMP_HIGH); > max9286_check_config_link(priv, priv->source_mask); > > /* >
Re: [PATCH 11/16] media: i2c: max9286: Cache channel amplitude
On 16/02/2021 17:41, Jacopo Mondi wrote:
> Cache the current channel amplitude in a driver variable
> to skip updating it if the new requested value is the same
> as the currently configured one.
>
> Signed-off-by: Jacopo Mondi
I like this model better than deciding outside of the call :-)
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/max9286.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1f14cd817fbf..4afb5ca06448 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -164,6 +164,7 @@ struct max9286_priv {
> bool mux_open;
>
> u32 init_rev_chan_mv;
> + u32 rev_chan_mv;
>
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
> @@ -340,8 +341,15 @@ static void max9286_configure_i2c(struct max9286_priv
> *priv, bool localack)
> static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> unsigned int chan_amplitude)
> {
> + u8 chan_config;
> +
> + if (priv->rev_chan_mv == chan_amplitude)
> + return;
> +
> + priv->rev_chan_mv = chan_amplitude;
> +
> /* Reverse channel transmission time: default to 1. */
> - u8 chan_config = MAX9286_REV_TRF(1);
> + chan_config = MAX9286_REV_TRF(1);
>
> /*
>* Reverse channel setup.
> @@ -563,8 +571,7 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> - if (priv->init_rev_chan_mv < 170)
> - max9286_reverse_channel_setup(priv, 170);
> + max9286_reverse_channel_setup(priv, 170);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
>
Re: [PATCH 10/16] media: i2c: max9286: Rename reverse_channel_mv
Hi Jacopo,
On 16/02/2021 17:41, Jacopo Mondi wrote:
> Rename the reverse_channel_mv variable to init_rev_chan_mv as
> the next patches will cache the reverse channel amplitude in
> a new driver variable.
>
I've been trying to figure out if we really do need two variables to
store this now, but I can't see an easy way to factor out the
initialisation value, and I like the idea of caching the current stored
value.
So
Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 1d9951215868..1f14cd817fbf 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,7 +163,7 @@ struct max9286_priv {
> unsigned int mux_channel;
> bool mux_open;
>
> - u32 reverse_channel_mv;
> + u32 init_rev_chan_mv;
>
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
> @@ -563,7 +563,7 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> - if (priv->reverse_channel_mv < 170)
> + if (priv->init_rev_chan_mv < 170)
> max9286_reverse_channel_setup(priv, 170);
> max9286_check_config_link(priv, priv->source_mask);
>
> @@ -971,7 +971,7 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> - max9286_reverse_channel_setup(priv, priv->reverse_channel_mv);
> + max9286_reverse_channel_setup(priv, priv->init_rev_chan_mv);
>
> /*
>* Enable GMSL links, mask unused ones and autodetect link
> @@ -1236,9 +1236,9 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> if (of_property_read_u32(dev->of_node,
>"maxim,reverse-channel-microvolt",
>_channel_microvolt))
> - priv->reverse_channel_mv = 170;
> + priv->init_rev_chan_mv = 170;
> else
> - priv->reverse_channel_mv = reverse_channel_microvolt / 1000U;
> + priv->init_rev_chan_mv = reverse_channel_microvolt / 1000U;
>
> priv->route_mask = priv->source_mask;
>
>
Re: [PATCH 09/16] media: i2c: rdacm21: Re-work OV10640 initialization
Hi Jacopo,
On 16/02/2021 17:41, Jacopo Mondi wrote:
> The OV10640 image sensor reset and powerdown on signals are controlled
> by the embedded OV490 ISP. The current reset procedure does not respect
> the 1 millisecond power-up delay and releases the reset signal before
> the powerdown one.
>
> Fix the OV10640 power up sequence by releasing the powerdown signal,
> waiting the mandatory 1 millisecond power up delay and then releasing
> the reset signal. The reset delay is not characterized in the chip
> manual if not as "255 XVCLK + initialization". Wait for at least 3
> milliseconds to guarantee the SCCB bus is available.
>
> This commit fixes a sporadic start-up error triggered by a failure to
> read the OV10640 chip ID:
> rdacm21 8-0054: OV10640 ID mismatch: (0x01)
Have you done a similar initialisation rework for the RDACM21 as you do
in [03/16] of this series for the RDACM20 (or was it already better
perhaps?)
Only interested because of noting that I think the 'mismatch' check is
now gone from the RDAMC20.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm21.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> index b22a2ca5340b..c420a6b96879 100644
> --- a/drivers/media/i2c/rdacm21.c
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -333,13 +333,15 @@ static int ov10640_initialize(struct rdacm21_device
> *dev)
> {
> u8 val;
>
> - /* Power-up OV10640 by setting RESETB and PWDNB pins high. */
> + /* Power-up OV10640 by setting PWDNB and RESETB pins high. */
> ov490_write_reg(dev, OV490_GPIO_SEL0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_SEL1, OV490_SPWDN0);
> ov490_write_reg(dev, OV490_GPIO_DIRECTION0, OV490_GPIO0);
> ov490_write_reg(dev, OV490_GPIO_DIRECTION1, OV490_SPWDN0);
> - ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> +
> ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_SPWDN0);
> + usleep_range(1500, 3000);
> + ov490_write_reg(dev, OV490_GPIO_OUTPUT_VALUE0, OV490_GPIO0);
> usleep_range(3000, 5000);
>
> /* Read OV10640 ID to test communications. */
>
Re: [PATCH 08/16] media: i2c: max9286: Adjust parameters indent
On 16/02/2021 17:41, Jacopo Mondi wrote: > The parameters to max9286_i2c_mux_configure() fits on the previous > line. Adjust it. > > Cosmetic change only. Cosmetic tag ;-) Reviewed-by: Kieran Bingham > > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/max9286.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 6fd4d59fcc72..1d9951215868 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -287,9 +287,8 @@ static int max9286_i2c_mux_select(struct i2c_mux_core > *muxc, u32 chan) > > priv->mux_channel = chan; > > - max9286_i2c_mux_configure(priv, > - MAX9286_FWDCCEN(chan) | > - MAX9286_REVCCEN(chan)); > + max9286_i2c_mux_configure(priv, MAX9286_FWDCCEN(chan) | > + MAX9286_REVCCEN(chan)); > > return 0; > } >
Re: [PATCH 07/16] media: i2c: rdacm2x: Fix wake up delay
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > The MAX9271 chip manual prescribes a delay of 5 milliseconds > after the chip exists from low power state. > > Adjust the required delay in the rdacm21 camera module and add it > to the rdacm20 that currently doesn't implement one. > This sounds to me like it should be a common function in the max9271 module: > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > usleep_range(5000, 8000); Especially as that MAX9271_DEFAULT_ADDR should probably be handled directly in the max9271.c file too, and the RDACM's shouldn't care about it. If we end up moving the max9271 'library' into more of a module/device then this would have to be done in it's 'probe' anyway, so it's likely better handled down there...? But ... it's not essential at this point in the series, so if you want to keep this patch as is, Reviewed-by: Kieran Bingham > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm20.c | 1 + > drivers/media/i2c/rdacm21.c | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index ea30cc936531..39e4b4241870 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -460,6 +460,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > + usleep_range(5000, 8000); > > /* Serial link disabled during config as it needs a valid pixel clock. > */ > ret = max9271_set_serial_link(>serializer, false); > diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c > index 179d107f494c..b22a2ca5340b 100644 > --- a/drivers/media/i2c/rdacm21.c > +++ b/drivers/media/i2c/rdacm21.c > @@ -453,7 +453,7 @@ static int rdacm21_initialize(struct rdacm21_device *dev) > /* Verify communication with the MAX9271: ping to wakeup. */ > dev->serializer.client->addr = MAX9271_DEFAULT_ADDR; > i2c_smbus_read_byte(dev->serializer.client); > - usleep_range(3000, 5000); > + usleep_range(5000, 8000); > > /* Enable reverse channel and disable the serial link. */ > ret = max9271_set_serial_link(>serializer, false); >
Re: [PATCH 06/16] media: i2c: rdacm20: Re-work ov10635 reset
On 16/02/2021 17:41, Jacopo Mondi wrote:
> The OV10635 image sensor embedded in the camera module is currently
> reset after the MAX9271 initialization with two long delays that were
> most probably not correctly characterized.
>
> Re-work the image sensor reset procedure by holding the chip in reset
> during the MAX9271 configuration, removing the long sleep delays and
> only wait after the chip exits from reset for 350-500 microseconds
> interval, which is larger than the minimum (2048 * (1 / XVCLK)) timeout
> characterized in the chip manual.
Holding the OV10635 in reset earlier sounds good to me, but I don't know
beyond that what implications there would be. If it's working better
that's good though.
Reviewed-by: Kieran Bingham
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 25 +++--
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index e982373908f2..ea30cc936531 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -477,6 +477,15 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
> if (ret)
> return ret;
>
> + /* Hold OV10635 in reset during max9271 configuration. */
> + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> +
> + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> +
> ret = max9271_configure_gmsl_link(>serializer);
> if (ret)
> return ret;
> @@ -490,23 +499,11 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
> return ret;
> dev->serializer.client->addr = dev->addrs[0];
>
> - /*
> - * Reset the sensor by cycling the OV10635 reset signal connected to the
> - * MAX9271 GPIO1 and verify communication with the OV10635.
> - */
> - ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> -
> - ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> - if (ret)
> - return ret;
> - usleep_range(1, 15000);
> -
> + /* Release ov10635 from reset and initialize it. */
> ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> - usleep_range(1, 15000);
> + usleep_range(350, 500);
>
> for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> ret = ov10635_read16(dev, OV10635_PID);
>
Re: [PATCH 05/16] media: i2c: rdacm20: Check return values
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > The camera module initialization routine does not check the return > value of a few functions. Fix that. > Sounds quite valid to me. Reviewed-by: Kieran Bingham > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm20.c | 13 + > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index 56406d82b5ac..e982373908f2 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -470,11 +470,16 @@ static int rdacm20_initialize(struct rdacm20_device > *dev) >* Ensure that we have a good link configuration before attempting to >* identify the device. >*/ > - max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS | > - MAX9271_I2CSLVTO_1024US | > - MAX9271_I2CMSTBT_105KBPS); > + ret = max9271_configure_i2c(>serializer, > + MAX9271_I2CSLVSH_469NS_234NS | > + MAX9271_I2CSLVTO_1024US | > + MAX9271_I2CMSTBT_105KBPS); > + if (ret) > + return ret; > > - max9271_configure_gmsl_link(>serializer); > + ret = max9271_configure_gmsl_link(>serializer); > + if (ret) > + return ret; > > ret = max9271_verify_id(>serializer); > if (ret < 0) >
Re: [PATCH 03/16] media: i2c: rdacm20: Replace goto with a loop
Hi Jacopo,
On 16/02/2021 17:41, Jacopo Mondi wrote:
> During the camera module initialization the image sensor PID is read to
> verify it can correctly be identified. The current implementation is
> rather confused and uses a loop implemented with a label and a goto.
>
> Replace it with a more compact for() loop.
>
> No functional changes intended.
I think there is a functional change in here, but I almost like it.
Before, if the read was successful, it would check to see if the
OV10635_PID == OV10635_VERSION, and if not it would print that the read
was successful but a mismatch.
Now - it will retry again instead, and if at the end of the retries it
still fails then it's a failure.
This means we perhaps don't get told if the device id is not correct in
the same way, but it also means that if the VERSION was not correct
because of a read error (which I believe i've seen occur), it will retry.
Because there is a functional change you might want to update the
commit, but I still think this is a good change overall.
Reviewed-by: Kieran Bingham
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 27 ++-
> 1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 4d9bac87cba8..6504ed0bd3bc 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -59,6 +59,8 @@
> */
> #define OV10635_PIXEL_RATE (4400)
>
> +#define OV10635_PID_TIMEOUT 3
> +
> static const struct ov10635_reg {
> u16 reg;
> u8 val;
> @@ -452,7 +454,7 @@ static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
>
> static int rdacm20_initialize(struct rdacm20_device *dev)
> {
> - unsigned int retry = 3;
> + unsigned int i;
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> @@ -501,23 +503,14 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
> return ret;
> usleep_range(1, 15000);
>
> -again:
> - ret = ov10635_read16(dev, OV10635_PID);
> - if (ret < 0) {
> - if (retry--)
> - goto again;
> -
> - dev_err(dev->dev, "OV10635 ID read failed (%d)\n",
> - ret);
> - return -ENXIO;
> + for (i = 0; i < OV10635_PID_TIMEOUT; ++i) {
> + ret = ov10635_read16(dev, OV10635_PID);
> + if (ret == OV10635_VERSION)
> + break;
> + usleep_range(1000, 2000);
> }
> -
> - if (ret != OV10635_VERSION) {
> - if (retry--)
> - goto again;
> -
> - dev_err(dev->dev, "OV10635 ID mismatch (0x%04x)\n",
> - ret);
> + if (i == OV10635_PID_TIMEOUT) {
> + dev_err(dev->dev, "OV10635 ID read failed (%d)\n", ret);
> return -ENXIO;
> }
>
>
Re: [PATCH 04/16] media: i2c: rdacm20: Report camera module name
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > When the device is identified the driver currently reports the > names of the chips embedded in the camera module. > > Report the name of the camera module itself instead. > Cosmetic change only. Aha, there go all my scripts that rely on this string to know if the camera was found ... but I can fix that ;-) Reviewed-by: Kieran Bingham > > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm20.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index 6504ed0bd3bc..56406d82b5ac 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -532,7 +532,7 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > if (ret) > return ret; > > - dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); > + dev_info(dev->dev, "Identified RDACM20 camera module\n"); > > /* >* Set reverse channel high threshold to increase noise immunity. >
Re: [PATCH 02/16] media: i2c: rdacm20: Embedded 'serializer' field
Hi Jacopo,
On 16/02/2021 17:41, Jacopo Mondi wrote:
> There's no reason to allocate dynamically the 'serializer' field in
> the driver structure.
>
> Embed the field and adjust all its users in the driver.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/rdacm20.c | 38 -
> 1 file changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index f7fd5ae955d0..4d9bac87cba8 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -312,7 +312,7 @@ static const struct ov10635_reg {
>
> struct rdacm20_device {
> struct device *dev;
> - struct max9271_device *serializer;
> + struct max9271_device serializer;
> struct i2c_client *sensor;
> struct v4l2_subdev sd;
> struct media_padpad;
> @@ -399,7 +399,7 @@ static int rdacm20_s_stream(struct v4l2_subdev *sd, int
> enable)
> {
> struct rdacm20_device *dev = sd_to_rdacm20(sd);
>
> - return max9271_set_serial_link(dev->serializer, enable);
> + return max9271_set_serial_link(>serializer, enable);
> }
>
> static int rdacm20_enum_mbus_code(struct v4l2_subdev *sd,
> @@ -456,11 +456,11 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
> int ret;
>
> /* Verify communication with the MAX9271: ping to wakeup. */
> - dev->serializer->client->addr = MAX9271_DEFAULT_ADDR;
> - i2c_smbus_read_byte(dev->serializer->client);
> + dev->serializer.client->addr = MAX9271_DEFAULT_ADDR;
> + i2c_smbus_read_byte(dev->serializer.client);
>
> /* Serial link disabled during config as it needs a valid pixel clock.
> */
> - ret = max9271_set_serial_link(dev->serializer, false);
> + ret = max9271_set_serial_link(>serializer, false);
> if (ret)
> return ret;
>
> @@ -468,35 +468,35 @@ static int rdacm20_initialize(struct rdacm20_device
> *dev)
>* Ensure that we have a good link configuration before attempting to
>* identify the device.
>*/
> - max9271_configure_i2c(dev->serializer, MAX9271_I2CSLVSH_469NS_234NS |
> -MAX9271_I2CSLVTO_1024US |
> -MAX9271_I2CMSTBT_105KBPS);
> + max9271_configure_i2c(>serializer, MAX9271_I2CSLVSH_469NS_234NS |
> + MAX9271_I2CSLVTO_1024US |
> + MAX9271_I2CMSTBT_105KBPS);
>
> - max9271_configure_gmsl_link(dev->serializer);
> + max9271_configure_gmsl_link(>serializer);
>
> - ret = max9271_verify_id(dev->serializer);
> + ret = max9271_verify_id(>serializer);
> if (ret < 0)
> return ret;
>
> - ret = max9271_set_address(dev->serializer, dev->addrs[0]);
> + ret = max9271_set_address(>serializer, dev->addrs[0]);
> if (ret < 0)
> return ret;
> - dev->serializer->client->addr = dev->addrs[0];
> + dev->serializer.client->addr = dev->addrs[0];
>
> /*
>* Reset the sensor by cycling the OV10635 reset signal connected to the
>* MAX9271 GPIO1 and verify communication with the OV10635.
>*/
> - ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_enable_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
>
> - ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_clear_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> usleep_range(1, 15000);
>
> - ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_set_gpios(>serializer, MAX9271_GPIO1OUT);
> if (ret)
> return ret;
> usleep_range(10000, 15000);
> @@ -560,13 +560,7 @@ static int rdacm20_probe(struct i2c_client *client)
> if (!dev)
> return -ENOMEM;
> dev->dev = >dev;
> -
> - dev->serializer = devm_kzalloc(>dev, sizeof(*dev->serializer),
> -GFP_KERNEL);
> - if (!dev->serializer)
> - return -ENOMEM;
Every allocation removed is a win.
Reviewed-by: Kieran Bingham
> -
> - dev->serializer->client = client;
> + dev->serializer.client = client;
>
> ret = of_property_read_u32_array(client->dev.of_node, "reg",
>dev->addrs, 2);
>
Re: [PATCH 01/16] media: i2c: rdacm20: Enable noise immunity
Hi Jacopo, On 16/02/2021 17:41, Jacopo Mondi wrote: > Enable the noise immunity threshold at the end of the rdacm20 > initialization routine. > > The rdcam20 camera module has been so far tested with a startup > delay that allowed the embedded MCU to program the serializer. If > the initialization routine is run before the MCU programs the > serializer and the image sensor and their addresses gets changed > by the rdacm20 driver it is required to manually enable the noise > immunity threshold to make the communication on the control channel > more reliable. > Oh, this is interesting, ... booting up without the delays would be ... much nicer. > Signed-off-by: Jacopo Mondi > --- > drivers/media/i2c/rdacm20.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c > index 90eb73f0e6e9..f7fd5ae955d0 100644 > --- a/drivers/media/i2c/rdacm20.c > +++ b/drivers/media/i2c/rdacm20.c > @@ -541,7 +541,13 @@ static int rdacm20_initialize(struct rdacm20_device *dev) > > dev_info(dev->dev, "Identified MAX9271 + OV10635 device\n"); > > - return 0; > + /* > + * Set reverse channel high threshold to increase noise immunity. > + * > + * This should be compensated by increasing the reverse channel > + * amplitude on the remote deserializer side. > + */ > + return max9271_set_high_threshold(>serializer, true); Does this work 'out of the box' ? I.e. if this patch is applied, I assume it is required to remove the regulator delays that I/we have in DT? Likewise, does that note mean this patch must also be accompanied by the update in max9286 somehow? I guess we can't keep 'test bisectability' with this very easily so it probably doesn't matter too much, the end result will be the interesting part. Reviewed-by: Kieran Bingham > } > > static int rdacm20_probe(struct i2c_client *client) >
Re: [PATCH] media: i2c/Kconfig: Select FWNODE for OV772x sensor
On 20/01/2021 10:36, Sakari Ailus wrote:
> On Wed, Jan 20, 2021 at 10:17:14AM +0000, Kieran Bingham wrote:
>> Hi Lad,
>>
>> On 20/01/2021 09:01, Lad Prabhakar wrote:
>>> Fix OV772x build breakage by selecting V4L2_FWNODE config:
>>>
>>> ia64-linux-ld: drivers/media/i2c/ov772x.o: in function `ov772x_probe':
>>> ov772x.c:(.text+0x1ee2): undefined reference to
>>> `v4l2_fwnode_endpoint_alloc_parse'
>>> ia64-linux-ld: ov772x.c:(.text+0x1f12): undefined reference to
>>> `v4l2_fwnode_endpoint_free'
>>> ia64-linux-ld: ov772x.c:(.text+0x2212): undefined reference to
>>> `v4l2_fwnode_endpoint_alloc_parse'
>>>
>>> Fixes: 8a10b4e3601e ("media: i2c: ov772x: Parse endpoint properties")
>>> Reported-by: kernel test robot
>>> Signed-off-by: Lad Prabhakar
>>
>> I see this driver uses subdev API too.
>>
>> Should the driver also select VIDEO_V4L2_SUBDEV_API?
>
> Yes, it should. Another patch? This one fixes a compilation problem.
Yes, it's probably another patch, because indeed this is a specific fix.
I wonder if that means the builders haven't been able to construct a
combination without VIDEO_V4L2_SUBDEV_API...
--
Kieran
>> Or is that covered sufficiently already on any platforms that would use
>> the driver?
>>
>> Reviewed-by: Kieran Bingham
>>
>>> ---
>>> drivers/media/i2c/Kconfig | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>> index eddb10220953..bb1b5a340431 100644
>>> --- a/drivers/media/i2c/Kconfig
>>> +++ b/drivers/media/i2c/Kconfig
>>> @@ -1013,6 +1013,7 @@ config VIDEO_OV772X
>>> tristate "OmniVision OV772x sensor support"
>>> depends on I2C && VIDEO_V4L2
>>> select REGMAP_SCCB
>>> + select V4L2_FWNODE
>>> help
>>> This is a Video4Linux2 sensor driver for the OmniVision
>>> OV772x camera.
>>>
>>
>
Re: [PATCH] media: i2c/Kconfig: Select FWNODE for OV772x sensor
Hi Lad,
On 20/01/2021 09:01, Lad Prabhakar wrote:
> Fix OV772x build breakage by selecting V4L2_FWNODE config:
>
> ia64-linux-ld: drivers/media/i2c/ov772x.o: in function `ov772x_probe':
> ov772x.c:(.text+0x1ee2): undefined reference to
> `v4l2_fwnode_endpoint_alloc_parse'
> ia64-linux-ld: ov772x.c:(.text+0x1f12): undefined reference to
> `v4l2_fwnode_endpoint_free'
> ia64-linux-ld: ov772x.c:(.text+0x2212): undefined reference to
> `v4l2_fwnode_endpoint_alloc_parse'
>
> Fixes: 8a10b4e3601e ("media: i2c: ov772x: Parse endpoint properties")
> Reported-by: kernel test robot
> Signed-off-by: Lad Prabhakar
I see this driver uses subdev API too.
Should the driver also select VIDEO_V4L2_SUBDEV_API?
Or is that covered sufficiently already on any platforms that would use
the driver?
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index eddb10220953..bb1b5a340431 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1013,6 +1013,7 @@ config VIDEO_OV772X
> tristate "OmniVision OV772x sensor support"
> depends on I2C && VIDEO_V4L2
> select REGMAP_SCCB
> + select V4L2_FWNODE
> help
> This is a Video4Linux2 sensor driver for the OmniVision
> OV772x camera.
>
Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
Hi Daniel, On 18/01/2021 00:34, Daniel Scally wrote: > ACPI devices with _HID INT3472 are currently matched to the tps68470 > driver, however this does not cover all situations in which that _HID > occurs. We've encountered three possibilities: > > 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing > a physical tps68470 device) that requires a GPIO and OpRegion driver > 2. On devices designed for Windows, an ACPI device with _HID INT3472 > (again representing a physical tps68470 device) which requires GPIO, > Clock and Regulator drivers. > 3. On other devices designed for Windows, an ACPI device with _HID > INT3472 which does NOT represent a physical tps68470, and is instead > used as a dummy device to group some system GPIO lines which are meant > to be consumed by the sensor that is dependent on this entry. > > This commit adds a new module, registering a platform driver to deal > with the 3rd scenario plus an i2c-driver to deal with #1 and #2, by > querying the CLDB buffer found against INT3472 entries to determine > which is most appropriate. > > Suggested-by: Laurent Pinchart > Signed-off-by: Daniel Scally > --- > Changes in v2: > > - Switched to a module registering a platform driver to run > the dummy ACPI devices, plus an i2c driver to replace and extend > the existing tps68470 driver > - Added clock handling functions to the int3472-discrete driver > - A whole bunch of other changes too numerous to enumerate > MAINTAINERS | 5 + > drivers/platform/x86/Kconfig | 25 + > drivers/platform/x86/Makefile | 4 + > .../platform/x86/intel_skl_int3472_common.c | 100 > .../platform/x86/intel_skl_int3472_common.h | 100 > .../platform/x86/intel_skl_int3472_discrete.c | 496 ++ > .../platform/x86/intel_skl_int3472_tps68470.c | 145 + > 7 files changed, 875 insertions(+) > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h > create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a091b496fdd8..c4ed8c3bc58e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9147,6 +9147,11 @@ S: Maintained > F: arch/x86/include/asm/intel_scu_ipc.h > F: drivers/platform/x86/intel_scu_* > > +INTEL SKL INT3472 ACPI DEVICE DRIVER > +M: Daniel Scally > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada > L: platform-driver-...@vger.kernel.org > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 91e6176cdfbd..916b077df2d5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE > device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m > for Type-C device. > > +config INTEL_SKL_INT3472 > + tristate "Intel SkyLake ACPI INT3472 Driver" > + depends on X86 && ACPI > + select REGMAP_I2C I've tried compiling this as a built in and a module and on my minimal config I had failures on both for regulator_register and regulator_unregister. I suspect this needs to have either a selects or a depends upon CONFIG_REGULATOR -- Regards Kieran > + help > + This driver adds support for the INT3472 ACPI devices found on some > + Intel SkyLake devices. > + > + There are 3 kinds of INT3472 ACPI device possible; two for devices > + designed for Windows (either with or without a physical tps68470 > + PMIC) and one designed for Chrome OS. This driver handles all three > + situations by discovering information it needs to discern them at > + runtime. > + > + If your device was designed for Chrome OS, this driver will provide > + an ACPI operation region, which must be available before any of the > + devices using this are probed. For this reason, you should select Y > + if your device was designed for ChromeOS. This option also configures > + the designware-i2c driver to be built-in, for the same reason. > + > + Say Y or M here if you have a SkyLake device designed for use > + with Windows or ChromeOS. Say N here if you are not sure. > + > + The module will be named "intel-skl-int3472" > + > config INTEL_HID_EVENT > tristate "INTEL HID Event" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 581475f59819..ae29c66842ca 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o > obj-$(CONFIG_INTEL_INT0002_VGPIO)+= intel_int0002_vgpio.o > obj-$(CONFIG_INTEL_MENLOW)
Re: [PATCH 3/9] media: mtk-vcodec: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Matthias Brugger
> Signed-off-by: Ricardo Ribalda
There's also another memset.*reserved occurring in
platform/mtk-vcodec/mtk_vcodec_enc.c: vidioc_enum_fmt()
While v4l2-core/v4l2-ioctl.c has:
> static int v4l_enum_fmt(const struct v4l2_ioctl_ops *ops,
> struct file *file, void *fh, void *arg)
> {
> struct video_device *vdev = video_devdata(file);
> struct v4l2_fmtdesc *p = arg;
> int ret = check_fmt(file, p->type);
> u32 mbus_code;
> u32 cap_mask;
>
> if (ret)
> return ret;
> ret = -EINVAL;
>
> if (!(vdev->device_caps & V4L2_CAP_IO_MC))
> p->mbus_code = 0;
>
> mbus_code = p->mbus_code;
> CLEAR_AFTER_FIELD(p, type);
So would that mean ^^^ should also be sufficient to remove the need for
that memset?
With that fixed or resolved:
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 5 -
> drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 8
> 2 files changed, 13 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> index c768a587a944..d746c41ea805 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> @@ -715,12 +715,7 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> }
> }
>
> - for (i = 0; i < pix_fmt_mp->num_planes; i++)
> - memset(&(pix_fmt_mp->plane_fmt[i].reserved[0]), 0x0,
> -sizeof(pix_fmt_mp->plane_fmt[0].reserved));
> -
> pix_fmt_mp->flags = 0;
> - memset(_fmt_mp->reserved, 0x0, sizeof(pix_fmt_mp->reserved));
> return 0;
> }
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> index 21de1431cfcb..db1f62cc38b3 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
> @@ -320,13 +320,7 @@ static int vidioc_try_fmt(struct v4l2_format *f,
> }
> }
>
> - for (i = 0; i < pix_fmt_mp->num_planes; i++)
> - memset(&(pix_fmt_mp->plane_fmt[i].reserved[0]), 0x0,
> -sizeof(pix_fmt_mp->plane_fmt[0].reserved));
> -
> pix_fmt_mp->flags = 0;
> - memset(_fmt_mp->reserved, 0x0,
> - sizeof(pix_fmt_mp->reserved));
>
> return 0;
> }
> @@ -532,8 +526,6 @@ static int vidioc_venc_g_fmt(struct file *file, void
> *priv,
> for (i = 0; i < pix->num_planes; i++) {
> pix->plane_fmt[i].bytesperline = q_data->bytesperline[i];
> pix->plane_fmt[i].sizeimage = q_data->sizeimage[i];
> - memset(&(pix->plane_fmt[i].reserved[0]), 0x0,
> -sizeof(pix->plane_fmt[i].reserved));
> }
>
> pix->flags = 0;
>
Re: [PATCH 2/9] media: mtk-mdp: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Matthias Brugger
> Signed-off-by: Ricardo Ribalda
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 724c7333b6e5..ace4528cdc5e 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -199,7 +199,6 @@ static const struct mtk_mdp_fmt
> *mtk_mdp_try_fmt_mplane(struct mtk_mdp_ctx *ctx,
> pix_mp->ycbcr_enc = ctx->ycbcr_enc;
> pix_mp->quantization = ctx->quant;
> }
> - memset(pix_mp->reserved, 0, sizeof(pix_mp->reserved));
>
> max_w = variant->pix_max->target_rot_dis_w;
> max_h = variant->pix_max->target_rot_dis_h;
> @@ -247,8 +246,6 @@ static const struct mtk_mdp_fmt
> *mtk_mdp_try_fmt_mplane(struct mtk_mdp_ctx *ctx,
> pix_mp->plane_fmt[i].bytesperline = bpl;
> if (pix_mp->plane_fmt[i].sizeimage < sizeimage)
> pix_mp->plane_fmt[i].sizeimage = sizeimage;
> - memset(pix_mp->plane_fmt[i].reserved, 0,
> -sizeof(pix_mp->plane_fmt[i].reserved));
> mtk_mdp_dbg(2, "[%d] p%d, bpl:%d, sizeimage:%u (%u)", ctx->id,
> i, bpl, pix_mp->plane_fmt[i].sizeimage, sizeimage);
> }
>
Re: [PATCH] media: staging/intel-ipu3 : Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Sakari Ailus
> Signed-off-by: Ricardo Ribalda
This is just 9/9 of the series right ? ;-)
--
Kieran
> ---
> drivers/staging/media/ipu3/ipu3-v4l2.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c
> b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 4dc8d9165f63..60aa02eb7d2a 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> @@ -773,9 +773,6 @@ static int imgu_try_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
>
> pixm->pixelformat = fmt->fourcc;
>
> - memset(pixm->plane_fmt[0].reserved, 0,
> -sizeof(pixm->plane_fmt[0].reserved));
> -
> return 0;
> }
>
>
Re: [PATCH 1/9] media: ipu3-cio2: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Sakari Ailus
> Signed-off-by: Ricardo Ribalda
Reviewed-by: Kieran Bingham
> ---
> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 36e354ecf71e..c5376de8cb8a 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1094,12 +1094,9 @@ static int cio2_v4l2_try_fmt(struct file *file, void
> *fh, struct v4l2_format *f)
> mpix->pixelformat = fmt->fourcc;
> mpix->colorspace = V4L2_COLORSPACE_RAW;
> mpix->field = V4L2_FIELD_NONE;
> - memset(mpix->reserved, 0, sizeof(mpix->reserved));
> mpix->plane_fmt[0].bytesperline = cio2_bytesperline(mpix->width);
> mpix->plane_fmt[0].sizeimage = mpix->plane_fmt[0].bytesperline *
> mpix->height;
> - memset(mpix->plane_fmt[0].reserved, 0,
> -sizeof(mpix->plane_fmt[0].reserved));
>
> /* use default */
> mpix->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>
Re: [PATCH 9/9] media: staging/intel-ipu3 : Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Sakari Ailus
> Signed-off-by: Ricardo Ribalda
> ---
> drivers/staging/media/ipu3/ipu3-v4l2.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c
> b/drivers/staging/media/ipu3/ipu3-v4l2.c
> index 4dc8d9165f63..60aa02eb7d2a 100644
> --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
The ipu3-css.c also has a memset on the plane formats in the
migu_css_queue_init(), but that's possibly still out of scope for this
patch ('just')
struct v4l2_pix_format_mplane *const f = >fmt.mpix;
memset(f->reserved, 0, sizeof(f->reserved));
I can't yet see anything that clears the reserved formats on queue
initialisations, so I don't think we can remove that yet unless I've
missed something anyway.
Seems like there is a lot more that could be cleared in core ...
But - as I said, I think that's out of scope here so
Reviewed-by: Kieran Bingham
> @@ -773,9 +773,6 @@ static int imgu_try_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
>
> pixm->pixelformat = fmt->fourcc;
>
> - memset(pixm->plane_fmt[0].reserved, 0,
> -sizeof(pixm->plane_fmt[0].reserved));
> -
> return 0;
> }
>
>
Re: [PATCH 8/9] media: vicodec: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Hans Verkuil
> Signed-off-by: Ricardo Ribalda
Reviewed-by: Kieran Bingham
> ---
> drivers/media/test-drivers/vicodec/vicodec-core.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c
> b/drivers/media/test-drivers/vicodec/vicodec-core.c
> index 025f3ff77302..33f1c893c1b6 100644
> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> @@ -811,9 +811,6 @@ static int vidioc_g_fmt(struct vicodec_ctx *ctx, struct
> v4l2_format *f)
> pix_mp->xfer_func = ctx->state.xfer_func;
> pix_mp->ycbcr_enc = ctx->state.ycbcr_enc;
> pix_mp->quantization = ctx->state.quantization;
> - memset(pix_mp->reserved, 0, sizeof(pix_mp->reserved));
> - memset(pix_mp->plane_fmt[0].reserved, 0,
> -sizeof(pix_mp->plane_fmt[0].reserved));
> break;
> default:
> return -EINVAL;
> @@ -886,8 +883,6 @@ static int vidioc_try_fmt(struct vicodec_ctx *ctx, struct
> v4l2_format *f)
> info->sizeimage_mult / info->sizeimage_div;
> if (pix_mp->pixelformat == V4L2_PIX_FMT_FWHT)
> plane->sizeimage += sizeof(struct fwht_cframe_hdr);
> - memset(pix_mp->reserved, 0, sizeof(pix_mp->reserved));
> - memset(plane->reserved, 0, sizeof(plane->reserved));
> break;
> default:
> return -EINVAL;
>
Re: [PATCH 7/9] media: ti-vpe: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
Reviewed-by: Kieran Bingham
> Cc: Benoit Parrot
> Signed-off-by: Ricardo Ribalda
> ---
> drivers/media/platform/ti-vpe/vpe.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/media/platform/ti-vpe/vpe.c
> b/drivers/media/platform/ti-vpe/vpe.c
> index 779dd74b82d0..10251b787674 100644
> --- a/drivers/media/platform/ti-vpe/vpe.c
> +++ b/drivers/media/platform/ti-vpe/vpe.c
> @@ -1683,7 +1683,6 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct
> v4l2_format *f,
> }
> }
>
> - memset(pix->reserved, 0, sizeof(pix->reserved));
> for (i = 0; i < pix->num_planes; i++) {
> plane_fmt = >plane_fmt[i];
> depth = fmt->vpdma_fmt[i]->depth;
> @@ -1713,7 +1712,6 @@ static int __vpe_try_fmt(struct vpe_ctx *ctx, struct
> v4l2_format *f,
> plane_fmt->bytesperline *
> depth) >> 3;
> }
> - memset(plane_fmt->reserved, 0, sizeof(plane_fmt->reserved));
> }
>
> return 0;
>
Re: [PATCH 6/9] media: sum4i-csi: Do not zero reserved fields
Hi Ricardo,
Well I've started, so I may as well finish and do the rest too.
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
Indeed, these are the only memsets here ...
With the $TITLE fixed as spotted by Ezequiel,
Reviewed-by: Kieran Bingham
>
> Cc: Maxime Ripard
> Cc: Chen-Yu Tsai
> Signed-off-by: Ricardo Ribalda
> ---
> drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> index 1a2f65d83a6c..4785faddf630 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_v4l2.c
> @@ -113,8 +113,6 @@ static void _sun4i_csi_try_fmt(struct sun4i_csi *csi,
> pix->num_planes = _fmt->num_planes;
> pix->pixelformat = _fmt->fourcc;
>
> - memset(pix->reserved, 0, sizeof(pix->reserved));
> -
> /* Align the width and height on the subsampling */
> width = ALIGN(pix->width, _fmt->hsub);
> height = ALIGN(pix->height, _fmt->vsub);
> @@ -131,8 +129,6 @@ static void _sun4i_csi_try_fmt(struct sun4i_csi *csi,
> bpl = pix->width / hsub * _fmt->bpp[i] / 8;
> pix->plane_fmt[i].bytesperline = bpl;
> pix->plane_fmt[i].sizeimage = bpl * pix->height / vsub;
> - memset(pix->plane_fmt[i].reserved, 0,
> -sizeof(pix->plane_fmt[i].reserved));
> }
> }
>
>
Re: [PATCH 5/9] media: jpu: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: Mikhail Ulyanov
> Signed-off-by: Ricardo Ribalda
> ---
> drivers/media/platform/rcar_jpu.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/media/platform/rcar_jpu.c
> b/drivers/media/platform/rcar_jpu.c
> index 9b99ff368698..2bddc957cb87 100644
> --- a/drivers/media/platform/rcar_jpu.c
> +++ b/drivers/media/platform/rcar_jpu.c
There's a memset(cap->reserved...) in jpu_querycap()
Is that also applicable and covered by the core?
Looking at v4l_querycap() it doesn't seem to be so:
Reviewed-by: Kieran Bingham
> @@ -793,7 +793,6 @@ static int __jpu_try_fmt(struct jpu_ctx *ctx, struct
> jpu_fmt **fmtinfo,
> pix->colorspace = fmt->colorspace;
> pix->field = V4L2_FIELD_NONE;
> pix->num_planes = fmt->num_planes;
> - memset(pix->reserved, 0, sizeof(pix->reserved));
>
> jpu_bound_align_image(>width, JPU_WIDTH_MIN, JPU_WIDTH_MAX,
> fmt->h_align, >height, JPU_HEIGHT_MIN,
> @@ -808,8 +807,6 @@ static int __jpu_try_fmt(struct jpu_ctx *ctx, struct
> jpu_fmt **fmtinfo,
> pix->plane_fmt[0].sizeimage = JPU_JPEG_HDR_SIZE +
> (JPU_JPEG_MAX_BYTES_PER_PIXEL * w * h);
> pix->plane_fmt[0].bytesperline = 0;
> - memset(pix->plane_fmt[0].reserved, 0,
> -sizeof(pix->plane_fmt[0].reserved));
> } else {
> unsigned int i, bpl = 0;
>
> @@ -822,8 +819,6 @@ static int __jpu_try_fmt(struct jpu_ctx *ctx, struct
> jpu_fmt **fmtinfo,
> for (i = 0; i < pix->num_planes; ++i) {
> pix->plane_fmt[i].bytesperline = bpl;
> pix->plane_fmt[i].sizeimage = bpl * h * fmt->bpp[i] / 8;
> - memset(pix->plane_fmt[i].reserved, 0,
> -sizeof(pix->plane_fmt[i].reserved));
> }
> }
>
>
Re: [PATCH 4/9] media: fdp1: Do not zero reserved fields
Hi Ricardo,
On 11/01/2021 14:54, Ricardo Ribalda wrote:
> Core code already clears reserved fields of struct
> v4l2_pix_format_mplane, check: 4e1e0eb0e074 ("media: v4l2-ioctl: Zero
> v4l2_plane_pix_format reserved fields").
>
> Cc: linux-renesas-...@vger.kernel.org
> Signed-off-by: Ricardo Ribalda
I love a good cleanup series.
Reviewed-by: Kieran Bingham
> ---
> drivers/media/platform/rcar_fdp1.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar_fdp1.c
> b/drivers/media/platform/rcar_fdp1.c
> index c9448de885b6..01c1fbb97bf6 100644
> --- a/drivers/media/platform/rcar_fdp1.c
> +++ b/drivers/media/platform/rcar_fdp1.c
> @@ -1439,8 +1439,6 @@ static void fdp1_compute_stride(struct
> v4l2_pix_format_mplane *pix,
> pix->plane_fmt[i].sizeimage = pix->plane_fmt[i].bytesperline
> * pix->height / vsub;
>
> - memset(pix->plane_fmt[i].reserved, 0,
> -sizeof(pix->plane_fmt[i].reserved));
> }
>
> if (fmt->num_planes == 3) {
> @@ -1448,8 +1446,6 @@ static void fdp1_compute_stride(struct
> v4l2_pix_format_mplane *pix,
> pix->plane_fmt[2].bytesperline = pix->plane_fmt[1].bytesperline;
> pix->plane_fmt[2].sizeimage = pix->plane_fmt[1].sizeimage;
>
> - memset(pix->plane_fmt[2].reserved, 0,
> -sizeof(pix->plane_fmt[2].reserved));
> }
> }
>
>
Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Hi Dan,
On 04/01/2021 22:02, Daniel Scally wrote:
>>>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>>>> +/*
>>>>>>> + * Extend this array with ACPI Hardware IDs of devices known to be
>>>>>>> working
>>>>>>> + * plus the number of link-frequencies expected by their drivers,
>>>>>>> along with
>>>>>>> + * the frequency values in hertz. This is somewhat opportunistic way
>>>>>>> of adding
>>>>>>> + * support for this for now in the hopes of a better source for the
>>>>>>> information
>>>>>>> + * (possibly some encoded value in the SSDB buffer that we're unaware
>>>>>>> of)
>>>>>>> + * becoming apparent in the future.
>>>>>>> + *
>>>>>>> + * Do not add an entry for a sensor that is not actually supported.
>>>>>>> + */
>>>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>> I don't know if these are expressed anywhere else but would it be
>>>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>>>> that is represented by this HID?
>>>>>>
>>>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>>>
>>>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>>>> anyone looking here to add a new sensor might want to investigate what
>>>>>> was already in the table?
>>>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>>>
>>>>>
>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>> + /* Sensor OVTI5693 */
>>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>> + /* Sensor OVTI2680 */
>>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>>
>>>>> As an inline comment won't fit for the sensors that we know
>>>>> link-frequencies for. That sound ok?
>>>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>>>
>>>> + /* Omnivision OV5693 */
>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>> + /* Omnivision OV2680 */
>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>>
>>>> but otherwise, yes a comment the line before works for me, as you are
>>>> right - at the end would not be practical.
>>> Works for me
>>>>>>> +static void cio2_bridge_create_fwnode_properties(
>>>>>>> + struct cio2_sensor *sensor,
>>>>>>> + const struct cio2_sensor_config *cfg)
>>>>>>> +{
>>>>>>> + unsigned int i;
>>>>>>> +
>>>>>>> + sensor->prop_names = prop_names;
>>>>>>> +
>>>>>>> + for (i = 0; i < CIO2_MAX_LANES; i++)
>>>>>>> + sensor->data_lanes[i] = i + 1;
>>>>>> Does something support lane swapping somewhere?
>>>>>> I assume this is just mapping each lane directly through.
>>>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>>>> is just mapping them directly
>>>> So is this needed? Or is it some future compatibility thing?
>>>>
>>>> I haven't seen where it's used yet, but I'm not too worried about it
>>>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>>>> constant...
>>>
>>> The endpoints need to have the data-lanes property which passes an array
>>> of data lanes, but there may well be a better way of doing this. I'm
>>> just using the lanes member of the ssdb data structure to tell the
>>> property how many members of the array to look at:
>>>
>>>
>>> + sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>>> + sensor->prop_names.data_lanes,
>>> + sensor->data_lanes,
>>> + sensor->ssdb.lanes);
Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Hi Dan,
On 04/01/2021 15:31, Daniel Scally wrote:
> Hi Kieran
>
> On 04/01/2021 15:13, Kieran Bingham wrote:
>> Hi Dan,
>>
>> On 04/01/2021 13:55, Daniel Scally wrote:
>>> Hi Kieran
>>>
>>> On 04/01/2021 13:35, Kieran Bingham wrote:
>>>>> +/*
>>>>> + * Extend this array with ACPI Hardware IDs of devices known to be
>>>>> working
>>>>> + * plus the number of link-frequencies expected by their drivers, along
>>>>> with
>>>>> + * the frequency values in hertz. This is somewhat opportunistic way of
>>>>> adding
>>>>> + * support for this for now in the hopes of a better source for the
>>>>> information
>>>>> + * (possibly some encoded value in the SSDB buffer that we're unaware of)
>>>>> + * becoming apparent in the future.
>>>>> + *
>>>>> + * Do not add an entry for a sensor that is not actually supported.
>>>>> + */
>>>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>> I don't know if these are expressed anywhere else but would it be
>>>> helpful to add a comment, or indicator as to what the actual sensor is
>>>> that is represented by this HID?
>>>>
>>>> I can make an assumption about what an OVTI2680 might be, but the
>>>> INT33BE is quite opaque. It's not clear what support that adds.
>>>>
>>>> Unless no one cares what the sensor is that is, but I would anticipate
>>>> anyone looking here to add a new sensor might want to investigate what
>>>> was already in the table?
>>> Yeah good point. I'll probably alternate comment and entry then, like:
>>>
>>>
>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>> + /* Sensor OVTI5693 */
>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>> + /* Sensor OVTI2680 */
>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>>
>>> As an inline comment won't fit for the sensors that we know
>>> link-frequencies for. That sound ok?
>> I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
>>
>> +/* Omnivision OV5693 */
>> +CIO2_SENSOR_CONFIG("INT33BE", 0),
>> +/* Omnivision OV2680 */
>> +CIO2_SENSOR_CONFIG("OVTI2680", 0),
>>
>> but otherwise, yes a comment the line before works for me, as you are
>> right - at the end would not be practical.
> Works for me
>>>>> +static void cio2_bridge_create_fwnode_properties(
>>>>> + struct cio2_sensor *sensor,
>>>>> + const struct cio2_sensor_config *cfg)
>>>>> +{
>>>>> + unsigned int i;
>>>>> +
>>>>> + sensor->prop_names = prop_names;
>>>>> +
>>>>> + for (i = 0; i < CIO2_MAX_LANES; i++)
>>>>> + sensor->data_lanes[i] = i + 1;
>>>> Does something support lane swapping somewhere?
>>>> I assume this is just mapping each lane directly through.
>>> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
>>> is just mapping them directly
>> So is this needed? Or is it some future compatibility thing?
>>
>> I haven't seen where it's used yet, but I'm not too worried about it
>> though, just not sure what value an array of [1, 2, 3, 4] gives if it's
>> constant...
>
> The endpoints need to have the data-lanes property which passes an array
> of data lanes, but there may well be a better way of doing this. I'm
> just using the lanes member of the ssdb data structure to tell the
> property how many members of the array to look at:
>
>
> + sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> + sensor->prop_names.data_lanes,
> + sensor->data_lanes,
> + sensor->ssdb.lanes);
>
>
> So if sensor->ssdb.lanes is 2, even though it's passed a pointer to the
> first member of an array of 4 members, the size calculation of that
> macro limits it to just those in use. I.E. if sensor->ssdb.lanes is 2
> then the property will be given the size 2 * sizeof(u32), and so when
> its parsed only [1, 2] will be read.
Aha, I see, ok - so we are populating an array of [1, 2, 3, 4] for each
sensor that we add.
What about creating the data_lanes once as a const static array and
mapping to that?
/*
* Map the lane arrangement, which is fixed for the IPU3.
*/
static const int data_lanes[CIO2_MAX_LANES] = { 1, 2, 3, 4 };
...
sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
sensor->prop_names.data_lanes,
data_lanes,
sensor->ssdb.lanes);
...
Then we don't need the loop to populate the array for each sensor
anymore, or the data_lanes in the sensor struct?
--
Regards
--
Kieran
Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
Hi Dan,
On 04/01/2021 13:55, Daniel Scally wrote:
> Hi Kieran
>
> On 04/01/2021 13:35, Kieran Bingham wrote:
>>> +/*
>>> + * Extend this array with ACPI Hardware IDs of devices known to be working
>>> + * plus the number of link-frequencies expected by their drivers, along
>>> with
>>> + * the frequency values in hertz. This is somewhat opportunistic way of
>>> adding
>>> + * support for this for now in the hopes of a better source for the
>>> information
>>> + * (possibly some encoded value in the SSDB buffer that we're unaware of)
>>> + * becoming apparent in the future.
>>> + *
>>> + * Do not add an entry for a sensor that is not actually supported.
>>> + */
>>> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
>>> + CIO2_SENSOR_CONFIG("INT33BE", 0),
>>> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>> I don't know if these are expressed anywhere else but would it be
>> helpful to add a comment, or indicator as to what the actual sensor is
>> that is represented by this HID?
>>
>> I can make an assumption about what an OVTI2680 might be, but the
>> INT33BE is quite opaque. It's not clear what support that adds.
>>
>> Unless no one cares what the sensor is that is, but I would anticipate
>> anyone looking here to add a new sensor might want to investigate what
>> was already in the table?
>
> Yeah good point. I'll probably alternate comment and entry then, like:
>
>
> +static const struct cio2_sensor_config cio2_supported_sensors[] = {
> + /* Sensor OVTI5693 */
> + CIO2_SENSOR_CONFIG("INT33BE", 0),
> + /* Sensor OVTI2680 */
> + CIO2_SENSOR_CONFIG("OVTI2680", 0),
>
> As an inline comment won't fit for the sensors that we know link-frequencies
> for. That sound ok?
I might put the whole vendor name in, and no need to prefix 'Sensor' IMO.
+ /* Omnivision OV5693 */
+ CIO2_SENSOR_CONFIG("INT33BE", 0),
+ /* Omnivision OV2680 */
+ CIO2_SENSOR_CONFIG("OVTI2680", 0),
but otherwise, yes a comment the line before works for me, as you are
right - at the end would not be practical.
>>> +static void cio2_bridge_create_fwnode_properties(
>>> + struct cio2_sensor *sensor,
>>> + const struct cio2_sensor_config *cfg)
>>> +{
>>> + unsigned int i;
>>> +
>>> + sensor->prop_names = prop_names;
>>> +
>>> + for (i = 0; i < CIO2_MAX_LANES; i++)
>>> + sensor->data_lanes[i] = i + 1;
>> Does something support lane swapping somewhere?
>> I assume this is just mapping each lane directly through.
>
> I think Sakari said remapping isn't supported in the CIO2 - so yeah this
> is just mapping them directly
So is this needed? Or is it some future compatibility thing?
I haven't seen where it's used yet, but I'm not too worried about it
though, just not sure what value an array of [1, 2, 3, 4] gives if it's
constant...
>> Otherwise, I'm quite looking forwards to all of this ;-)
>>
>> Reviewed-by: Kieran Bingham
> Thanks very much!
--
Kieran
--
Regards
--
Kieran
Re: [PATCH v4 15/15] ipu3-cio2: Add cio2-bridge to ipu3-cio2 driver
by this HID?
I can make an assumption about what an OVTI2680 might be, but the
INT33BE is quite opaque. It's not clear what support that adds.
Unless no one cares what the sensor is that is, but I would anticipate
anyone looking here to add a new sensor might want to investigate what
was already in the table?
> +};
> +
> +static const struct cio2_property_names prop_names = {
> + .clock_frequency = "clock-frequency",
> + .rotation = "rotation",
> + .bus_type = "bus-type",
> + .data_lanes = "data-lanes",
> + .remote_endpoint = "remote-endpoint",
> + .link_frequencies = "link-frequencies",
> +};
> +
> +static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
> + void *data, u32 size)
> +{
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + union acpi_object *obj;
> + acpi_status status;
> + int ret = 0;
> +
> + status = acpi_evaluate_object(adev->handle, id, NULL, );
> + if (ACPI_FAILURE(status))
> + return -ENODEV;
> +
> + obj = buffer.pointer;
> + if (!obj) {
> + dev_err(>dev, "Couldn't locate ACPI buffer\n");
> + return -ENODEV;
> + }
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + dev_err(>dev, "Not an ACPI buffer\n");
> + ret = -ENODEV;
> + goto out_free_buff;
> + }
> +
> + if (obj->buffer.length > size) {
> + dev_err(>dev, "Given buffer is too small\n");
> + ret = -EINVAL;
> + goto out_free_buff;
> + }
> +
> + memcpy(data, obj->buffer.pointer, obj->buffer.length);
> +
> +out_free_buff:
> + kfree(buffer.pointer);
> + return ret;
> +}
> +
> +static void cio2_bridge_create_fwnode_properties(
> + struct cio2_sensor *sensor,
> + const struct cio2_sensor_config *cfg)
> +{
> + unsigned int i;
> +
> + sensor->prop_names = prop_names;
> +
> + for (i = 0; i < CIO2_MAX_LANES; i++)
> + sensor->data_lanes[i] = i + 1;
Does something support lane swapping somewhere?
I assume this is just mapping each lane directly through.
Otherwise, I'm quite looking forwards to all of this ;-)
Reviewed-by: Kieran Bingham
> +
> + sensor->local_ref[0].node = >swnodes[SWNODE_CIO2_ENDPOINT];
> + sensor->remote_ref[0].node = >swnodes[SWNODE_SENSOR_ENDPOINT];
> +
> + sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
> + sensor->prop_names.clock_frequency,
> + sensor->ssdb.mclkspeed);
> + sensor->dev_properties[1] = PROPERTY_ENTRY_U8(
> + sensor->prop_names.rotation,
> + sensor->ssdb.degree);
> +
> + 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,
> + sensor->data_lanes,
> + sensor->ssdb.lanes);
> + sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
> + sensor->prop_names.remote_endpoint,
> + sensor->local_ref);
> +
> + if (cfg->nr_link_freqs > 0)
> + sensor->ep_properties[3] = PROPERTY_ENTRY_U64_ARRAY_LEN(
> +
> sensor->prop_names.link_frequencies,
> + cfg->link_freqs,
> + cfg->nr_link_freqs);
> +
> + sensor->cio2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
> + sensor->prop_names.data_lanes,
> + sensor->data_lanes,
> + sensor->ssdb.lanes);
> + sensor->cio2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(
> + sensor->prop_names.remote_endpoint,
> + sensor->remote_ref);
> +}
> +
> +static void cio2_bridge_init_swnode_names(struct cio2_sensor *sensor)
> +{
> + snprintf(sensor->node_names.remote_port,
> + sizeof(sensor->node_names.remote_port),
> + SWNODE_GRAPH_PORT_NAME_FMT, sensor->ss
Re: [PATCH] media: vsp1: Fix an error handling path in the probe function
Hi Christophe,
On 12/12/2020 17:41, Christophe JAILLET wrote:
> A previous 'rcar_fcp_get()' call must be undone in the error handling path,
> as already done in the remove function.
Reviewed-by: Kieran Bingham
> Fixes: 94fcdf829793 ("[media] v4l: vsp1: Add FCP support")
> Signed-off-by: Christophe JAILLET
> ---
> drivers/media/platform/vsp1/vsp1_drv.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c
> index dc62533cf32c..aa66e4f5f3f3 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -882,8 +882,10 @@ static int vsp1_probe(struct platform_device *pdev)
> }
>
> done:
> - if (ret)
> + if (ret) {
> pm_runtime_disable(>dev);
> + rcar_fcp_put(vsp1->fcp);
> + }
>
> return ret;
> }
>
Re: [PATCH 18/18] ipu3: Add driver for dummy INT3472 ACPI device
Hi Daniel,
On 30/11/2020 13:31, Daniel Scally wrote:
> On platforms where ACPI is designed for use with Windows, resources
> that are intended to be consumed by sensor devices are sometimes in
> the _CRS of a dummy INT3472 device upon which the sensor depends. This
> driver binds to the dummy acpi device (which does not represent a
> physical PMIC) and maps them into GPIO lines and regulators for use by
> the sensor device instead.
>
> Suggested-by: Laurent Pinchart
> Signed-off-by: Daniel Scally
> ---
> Changes since RFC v3:
>
> - Patch introduced
>
> This patch contains the bits of this process that we're least sure about.
> The sensors in scope for this work are called out as dependent (in their
> DSDT entry's _DEP) on a device with _HID INT3472. These come in at least
> 2 kinds; those with an I2cSerialBusV2 entry (which we presume therefore
> are legitimate tps68470 PMICs that need handling by those drivers - work
> on that in the future). And those without an I2C device. For those without
> an I2C device they instead have an array of GPIO pins defined in _CRS. So
> for example, my Lenovo Miix 510's OVTI2680 sensor is dependent on one of
> the _latter_ kind of INT3472 devices, with this _CRS:
>
> Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
> {
> Name (SBUF, ResourceTemplate ()
> {
> GpioIo (Exclusive, PullDefault, 0x, 0x,
> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
> 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0079
> }
> GpioIo (Exclusive, PullDefault, 0x, 0x,
> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
> 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x007A
> }
> GpioIo (Exclusive, PullDefault, 0x, 0x,
> IoRestrictionOutputOnly, "\\_SB.PCI0.GPI0",
> 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x008F
> }
> })
> Return (SBUF) /* \_SB_.PCI0.PMI1._CRS.SBUF */
> }
>
> and the same device has a _DSM Method, which returns 32-bit ints where
> the second lowest byte we noticed to match the pin numbers of the GPIO
> lines:
>
> Method (_DSM, 4, NotSerialized) // _DSM: Device-Specific Method
> {
> If ((Arg0 == ToUUID ("79234640-9e10-4fea-a5c1-b5aa8b19756f")))
> {
> If ((Arg2 == One))
> {
> Return (0x03)
> }
>
> If ((Arg2 == 0x02))
> {
> Return (0x01007900)
> }
>
> If ((Arg2 == 0x03))
> {
> Return (0x01007A0C)
> }
>
> If ((Arg2 == 0x04))
> {
> Return (0x01008F01)
> }
> }
>
> Return (Zero)
> }
>
> We know that at least some of those pins have to be toggled active for the
> sensor devices to be available in i2c, so the conclusion we came to was
> that those GPIO entries assigned to the INT3472 device actually represent
> GPIOs and regulators to be consumed by the sensors themselves. Tsuchiya
> noticed that the lowest byte in the return values of the _DSM method
> seemed to represent the type or function of the GPIO line, and we
> confirmed that by testing on each surface device that GPIO lines where the
> low byte in the _DSM entry for that pin was 0x0d controlled the privacy
> LED of the cameras.
>
> We're guessing as to the exact meaning of the function byte, but I
> conclude they're something like this:
>
> 0x00 - probably a reset GPIO
> 0x01 - regulator for the sensor
> 0x0c - regulator for the sensor
> 0x0b - regulator again, but for a VCM or EEPROM
> 0x0d - privacy led (only one we're totally confident of since we can see
>it happen!)
>
> After much internal debate I decided to write this as a standalone
> acpi_driver. Alternative options we considered:
>
> 1. Squash all this into the cio2-bridge code, which I did originally write
> but decided I didn't like.
> 2. Extend the existing tps68470 mfd driver...they share an ACPI ID so this
> kinda makes sense, but ultimately given there is no actual physical
> tps68470 in the scenario this patch handles I decided I didn't like this
> either.
>
I would agree, keeping this in a unit file on it's own makes sense to me.
I'm a bit worried about what happens if the tps68470 is also compiled
in... Does the right device get mapped in the event that there are also
actual devices already supported by the tps68470 mfd driver on the
device as well?
> MAINTAINERS| 7 +
> drivers/media/pci/intel/ipu3/Kconfig | 14 +
> drivers/media/pci/intel/ipu3/Makefile | 1 +
> drivers/media/pci/intel/ipu3/int3472.c | 381 +
> drivers/media/pci/intel/ipu3/int3472.h | 96 +++
> 5 files changed, 499 insertions(+)
> create mode 100644 drivers/media/pci/intel/ipu3/int3472.c
> create mode 100644
Re: [PATCH] net: cisco: remove trailing semicolon in macro definition
Hi Tom, On 27/11/2020 17:58, t...@redhat.com wrote: > From: Tom Rix > > The macro use will already have a semicolon. > > Signed-off-by: Tom Rix Seems to be the only occurrence in this file. Reviewed-by: Kieran Bingham > --- > drivers/net/wireless/cisco/airo.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/cisco/airo.c > b/drivers/net/wireless/cisco/airo.c > index 74acf9af2adb..ba62bb2995d9 100644 > --- a/drivers/net/wireless/cisco/airo.c > +++ b/drivers/net/wireless/cisco/airo.c > @@ -5785,7 +5785,7 @@ static int airo_get_quality (StatusRid *status_rid, > CapabilityRid *cap_rid) > } > > #define airo_get_max_quality(cap_rid) (memcmp((cap_rid)->prodName, "350", 3) > ? 0x20 : 0xa0) > -#define airo_get_avg_quality(cap_rid) (memcmp((cap_rid)->prodName, "350", 3) > ? 0x10 : 0x50); > +#define airo_get_avg_quality(cap_rid) (memcmp((cap_rid)->prodName, "350", 3) > ? 0x10 : 0x50) > > /*--*/ > /* >
Re: [PATCH 2/2] media: rdacm20: Enable GPIO1 explicitly
Hi Jacopo,
On 20/11/2020 16:15, Jacopo Mondi wrote:
> The MAX9271 GPIO1 line that controls the sensor reset is by default
> enabled after a serializer chip reset.
>
> As rdacm20 does not go through an explicit serializer reset, make sure
> GPIO1 is enabled to make the camera module driver more robust.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi
Looks helpful to ensure a GPIO is enabled before toggling ;-)
Reading the datasheet, GPIO1EN defaults to enabled, so I assume this was
already working - however I think being explicit is a good thing anyway.
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/rdacm20.c | 13 +++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 1ed928c4ca70..16bcb764b0e0 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -487,9 +487,18 @@ static int rdacm20_initialize(struct rdacm20_device *dev)
>* Reset the sensor by cycling the OV10635 reset signal connected to the
>* MAX9271 GPIO1 and verify communication with the OV10635.
>*/
> - max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + ret = max9271_enable_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> +
> + ret = max9271_clear_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> usleep_range(1, 15000);
> - max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> +
> + ret = max9271_set_gpios(dev->serializer, MAX9271_GPIO1OUT);
> + if (ret)
> + return ret;
> usleep_range(1, 15000);
>
> again:
> --
> 2.29.1
>
Re: [PATCH 1/2] media: max9271: Fix GPIO enable/disable
Hi Jacopo,
On 20/11/2020 16:15, Jacopo Mondi wrote:
> Fix GPIO enable/disable operations which wrongly read the 0x0f register
> to obtain the current mask of the enabled lines instead of using
> the correct 0x0e register.
>
> Also fix access to bit 0 of the register which is marked as reserved.
>
> Fixes: 34009bffc1c6 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Jacopo Mondi
Took me a few reads of this and the datasheet to be sure :S
But now I am :-D
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/max9271.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/max9271.c b/drivers/media/i2c/max9271.c
> index 0f6f7a092a46..c247db569bab 100644
> --- a/drivers/media/i2c/max9271.c
> +++ b/drivers/media/i2c/max9271.c
> @@ -223,12 +223,12 @@ int max9271_enable_gpios(struct max9271_device *dev, u8
> gpio_mask)
> {
> int ret;
>
> - ret = max9271_read(dev, 0x0f);
> + ret = max9271_read(dev, 0x0e);
> if (ret < 0)
> return 0;
>
> /* BIT(0) reserved: GPO is always enabled. */
> - ret |= gpio_mask | BIT(0);
> + ret |= (gpio_mask & ~BIT(0));
> ret = max9271_write(dev, 0x0e, ret);
> if (ret < 0) {
> dev_err(>client->dev, "Failed to enable gpio (%d)\n", ret);
> @@ -245,12 +245,12 @@ int max9271_disable_gpios(struct max9271_device *dev,
> u8 gpio_mask)
> {
> int ret;
>
> - ret = max9271_read(dev, 0x0f);
> + ret = max9271_read(dev, 0x0e);
> if (ret < 0)
> return 0;
>
> /* BIT(0) reserved: GPO cannot be disabled */
> - ret &= (~gpio_mask | BIT(0));
> + ret &= ~(gpio_mask | BIT(0));
> ret = max9271_write(dev, 0x0e, ret);
> if (ret < 0) {
> dev_err(>client->dev, "Failed to disable gpio (%d)\n",
> ret);
>
Re: [PATCH] media: i2c: rdacm20: Constify static structs
Hi Rikard,
Thank you for the fix.
On 19/11/2020 22:22, Rikard Falkeborn wrote:
> The only usage of rdacm20_video_ops is to assign it to the video field
> in the v4l2_subdev_ops struct which is a pointer to const, and the only
> usage of rdacm20_subdev_ops is to pass its address to
> v4l2_i2c_subdev_init() which accepts a pointer to const. Make them const
> to allow the compiler to put them in read-only memory.
>
> Signed-off-by: Rikard Falkeborn
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/rdacm20.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/rdacm20.c b/drivers/media/i2c/rdacm20.c
> index 1ed928c4ca70..ac00a6743623 100644
> --- a/drivers/media/i2c/rdacm20.c
> +++ b/drivers/media/i2c/rdacm20.c
> @@ -435,7 +435,7 @@ static int rdacm20_get_fmt(struct v4l2_subdev *sd,
> return 0;
> }
>
> -static struct v4l2_subdev_video_ops rdacm20_video_ops = {
> +static const struct v4l2_subdev_video_ops rdacm20_video_ops = {
> .s_stream = rdacm20_s_stream,
> };
>
> @@ -445,7 +445,7 @@ static const struct v4l2_subdev_pad_ops
> rdacm20_subdev_pad_ops = {
> .set_fmt= rdacm20_get_fmt,
> };
>
> -static struct v4l2_subdev_ops rdacm20_subdev_ops = {
> +static const struct v4l2_subdev_ops rdacm20_subdev_ops = {
> .video = _video_ops,
> .pad= _subdev_pad_ops,
> };
>
Re: [PATCH v5 7/8] [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel amplitude
Hi Jacopo,
On 16/11/2020 13:53, Jacopo Mondi wrote:
> Use the newly introduced 'maxim,initial-reverse-channel-mV'
> property to specify the initial reverse channel amplitude when
> the remote serializers are pre-programmed with noise immunity threshold
> enabled.
>
> Signed-off-by: Jacopo Mondi
Tested-by: Kieran Bingham
Reviewed-by: Kieran Bingham
> ---
> arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
> b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
> index 6f4798859542..f14a133b7302 100644
> --- a/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
> +++ b/arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi
> @@ -128,6 +128,7 @@ gmsl-deserializer@4c {
> compatible = "maxim,max9286";
> reg = <0x4c>;
> poc-supply = <_12v>;
> + maxim,initial-reverse-channel-mV = <170>;
>
> ports {
> #address-cells = <1>;
> @@ -263,6 +264,7 @@ gmsl-deserializer@6c {
> compatible = "maxim,max9286";
> reg = <0x6c>;
> poc-supply = <_12v>;
> + maxim,initial-reverse-channel-mV = <170>;
>
> ports {
> #address-cells = <1>;
>
Re: [PATCH v5 0/8] media: i2c: Add RDACM21 camera module
Hi Jacopo, On 16/11/2020 13:52, Jacopo Mondi wrote: > Hello, >this v5 mostly changes how the rdacm21 driver handles writes to the > OV490 chip by handling the 'high' part of the sensor register through an > helper function as suggested by Kieran and Laurent. > > The diff is available at: > https://paste.debian.net/1172700/ > > Minor fixes in patches subjects and in bindings as reported by Sergei and > Kieran. > > Bindings have now been sent to DT people now that they've been reviewed > by Kieran. > > v4: https://patchwork.linuxtv.org/project/linux-media/list/?series=3847 > v3: https://patchwork.linuxtv.org/project/linux-media/list/?series=3657 > > Background in v1 cover letter: > https://www.spinics.net/lists/linux-renesas-soc/msg52886.html > > I have tested on Eagle V3M with 4 RDACM21, but the whole point of > this series is to retain compatibility with RDACM20. > > For this reason I have included 2 patches on top, not intended for merge > that re-propose DTS support for the MAXIM max9286 expansion board connected > to Salvator-X and add the newly introduced property to the DTS file. > > Kieran, I know you have a working setup with RDACM20, the final patches are > meant for ease your testing. Can you give this series a spin ? After some rabbit-holes :-D: A 5-camera capture on Salvator-X - https://pasteboard.co/JAW0PSr.png For this series, on both Eagle-V3M and Salvator-X Tested-by: Kieran Bingham > Series based on latest renesas-drivers tag: > renesas-drivers-2020-11-10-v5.10-rc3 > > Thanks > j > > Jacopo Mondi (7): > media: i2c: Add driver for RDACM21 camera module > dt-bindings: media: max9286: Document > 'maxim,initial-reverse-channel-mV' > media: i2c: max9286: Break-out reverse channel setup > media: i2c: max9286: Make channel amplitude programmable > media: i2c: max9286: Configure reverse channel amplitude > [DNI] arm64: dts: renesas: salvator-x-max9286: Specify channel > amplitude > [DNI] arm64: dts: renesas: eagle: Specify channel amplitude > > Laurent Pinchart (1): > arm64: dts: renesas: salvator-x: Add MAX9286 expansion board > > .../bindings/media/i2c/maxim,max9286.yaml | 23 + > MAINTAINERS | 12 + > .../arm64/boot/dts/renesas/r8a77970-eagle.dts | 1 + > .../boot/dts/renesas/salvator-x-max9286.dtsi | 396 > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile| 2 + > drivers/media/i2c/max9286.c | 58 +- > drivers/media/i2c/rdacm21.c | 595 ++ > 8 files changed, 1087 insertions(+), 13 deletions(-) > create mode 100644 arch/arm64/boot/dts/renesas/salvator-x-max9286.dtsi > create mode 100644 drivers/media/i2c/rdacm21.c > > -- > 2.29.1 >
Re: [PATCH v5 1/8] media: i2c: Add driver for RDACM21 camera module
On 17/11/2020 13:36, Jacopo Mondi wrote: > Hi Kieran, > > On Mon, Nov 16, 2020 at 02:47:49PM +0000, Kieran Bingham wrote: >> On 16/11/2020 13:52, Jacopo Mondi wrote: >>> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images >>> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision >>> OV490 ISP and a Maxim MAX9271 GMSL serializer. >>> >>> The driver uses the max9271 library module, to maximize code reuse with >>> other camera module drivers using the same serializer, such as rdacm20. >>> >>> Signed-off-by: Jacopo Mondi >>> --- >>> MAINTAINERS | 12 + >>> drivers/media/i2c/Kconfig | 13 + >>> drivers/media/i2c/Makefile | 2 + >>> drivers/media/i2c/rdacm21.c | 595 >>> 4 files changed, 622 insertions(+) >>> create mode 100644 drivers/media/i2c/rdacm21.c >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index cef6b6090d76..9a5026fd6788 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -14750,6 +14750,18 @@ F: drivers/media/i2c/max9271.c >>> F: drivers/media/i2c/max9271.h >>> F: drivers/media/i2c/rdacm20.c >>> >>> +RDACM21 Camera Sensor >>> +M: Jacopo Mondi >>> +M: Kieran Bingham >>> +M: Laurent Pinchart >>> +M: Niklas Söderlund >>> +L: linux-me...@vger.kernel.org >>> +S: Maintained >>> +F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml >>> +F: drivers/media/i2c/max9271.c >>> +F: drivers/media/i2c/max9271.h >>> +F: drivers/media/i2c/rdacm21.c >>> + >>> RDC R-321X SoC >>> M: Florian Fainelli >>> S: Maintained >>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >>> index c64326ca331c..64f4316d11ad 100644 >>> --- a/drivers/media/i2c/Kconfig >>> +++ b/drivers/media/i2c/Kconfig >>> @@ -1185,6 +1185,19 @@ config VIDEO_RDACM20 >>> This camera should be used in conjunction with a GMSL >>> deserialiser such as the MAX9286. >>> >>> +config VIDEO_RDACM21 >>> + tristate "IMI RDACM21 camera support" >>> + depends on I2C >>> + select V4L2_FWNODE >>> + select VIDEO_V4L2_SUBDEV_API >>> + select MEDIA_CONTROLLER >>> + help >>> + This driver supports the IMI RDACM21 GMSL camera, used in >>> + ADAS systems. >>> + >>> + This camera should be used in conjunction with a GMSL >>> + deserialiser such as the MAX9286. >>> + >>> config VIDEO_RJ54N1 >>> tristate "Sharp RJ54N1CB0C sensor support" >>> depends on I2C && VIDEO_V4L2 >>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >>> index f0a77473979d..f3641b58929d 100644 >>> --- a/drivers/media/i2c/Makefile >>> +++ b/drivers/media/i2c/Makefile >>> @@ -122,6 +122,8 @@ obj-$(CONFIG_VIDEO_IMX355) += imx355.o >>> obj-$(CONFIG_VIDEO_MAX9286)+= max9286.o >>> rdacm20-camera_module-objs := rdacm20.o max9271.o >>> obj-$(CONFIG_VIDEO_RDACM20)+= rdacm20-camera_module.o >>> +rdacm21-camera_module-objs := rdacm21.o max9271.o >>> +obj-$(CONFIG_VIDEO_RDACM21)+= rdacm21-camera_module.o >>> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o >>> >>> obj-$(CONFIG_SDR_MAX2175) += max2175.o >>> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c >>> new file mode 100644 >>> index ..5f9267e26258 >>> --- /dev/null >>> +++ b/drivers/media/i2c/rdacm21.c >>> @@ -0,0 +1,595 @@ >>> +// SPDX-License-Identifier: GPL-2.0+ >>> +/* >>> + * IMI RDACM21 GMSL Camera Driver >>> + * >>> + * Copyright (C) 2017-2020 Jacopo Mondi >>> + * Copyright (C) 2017-2019 Kieran Bingham >>> + * Copyright (C) 2017-2019 Laurent Pinchart >>> + * Copyright (C) 2017-2019 Niklas Söderlund >>> + * Copyright (C) 2016 Renesas Electronics Corporation >>> + * Copyright (C) 2015 Cogent Embedded, Inc. >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> +#include "max9271.h" >>> + >>> +#define OV10640_ID_LOW 0xa6 Should we keep the
Re: [PATCH v5 1/8] media: i2c: Add driver for RDACM21 camera module
On 16/11/2020 13:52, Jacopo Mondi wrote:
> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images
> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision
> OV490 ISP and a Maxim MAX9271 GMSL serializer.
>
> The driver uses the max9271 library module, to maximize code reuse with
> other camera module drivers using the same serializer, such as rdacm20.
>
> Signed-off-by: Jacopo Mondi
> ---
> MAINTAINERS | 12 +
> drivers/media/i2c/Kconfig | 13 +
> drivers/media/i2c/Makefile | 2 +
> drivers/media/i2c/rdacm21.c | 595
> 4 files changed, 622 insertions(+)
> create mode 100644 drivers/media/i2c/rdacm21.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cef6b6090d76..9a5026fd6788 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14750,6 +14750,18 @@ F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> F: drivers/media/i2c/rdacm20.c
>
> +RDACM21 Camera Sensor
> +M: Jacopo Mondi
> +M: Kieran Bingham
> +M: Laurent Pinchart
> +M: Niklas Söderlund
> +L: linux-me...@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F: drivers/media/i2c/max9271.c
> +F: drivers/media/i2c/max9271.h
> +F: drivers/media/i2c/rdacm21.c
> +
> RDC R-321X SoC
> M: Florian Fainelli
> S: Maintained
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c64326ca331c..64f4316d11ad 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1185,6 +1185,19 @@ config VIDEO_RDACM20
> This camera should be used in conjunction with a GMSL
> deserialiser such as the MAX9286.
>
> +config VIDEO_RDACM21
> + tristate "IMI RDACM21 camera support"
> + depends on I2C
> + select V4L2_FWNODE
> + select VIDEO_V4L2_SUBDEV_API
> + select MEDIA_CONTROLLER
> + help
> + This driver supports the IMI RDACM21 GMSL camera, used in
> + ADAS systems.
> +
> + This camera should be used in conjunction with a GMSL
> + deserialiser such as the MAX9286.
> +
> config VIDEO_RJ54N1
> tristate "Sharp RJ54N1CB0C sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index f0a77473979d..f3641b58929d 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -122,6 +122,8 @@ obj-$(CONFIG_VIDEO_IMX355)+= imx355.o
> obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> rdacm20-camera_module-objs := rdacm20.o max9271.o
> obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> +rdacm21-camera_module-objs := rdacm21.o max9271.o
> +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>
> obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> new file mode 100644
> index 00000000..5f9267e26258
> --- /dev/null
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -0,0 +1,595 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IMI RDACM21 GMSL Camera Driver
> + *
> + * Copyright (C) 2017-2020 Jacopo Mondi
> + * Copyright (C) 2017-2019 Kieran Bingham
> + * Copyright (C) 2017-2019 Laurent Pinchart
> + * Copyright (C) 2017-2019 Niklas Söderlund
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2015 Cogent Embedded, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include "max9271.h"
> +
> +#define OV10640_ID_LOW 0xa6
> +
> +#define OV490_I2C_ADDRESS0x24
> +
> +#define OV490_PAGE_HIGH_REG 0xfffd
> +#define OV490_PAGE_LOW_REG 0xfffe
> +
> +#define OV490_DVP_CTRL3 0x80286009
> +
> +#define OV490_ODS_CTRL_FRAME_OUTPUT_EN 0x0c
> +#define OV490_ODS_CTRL 0x8029d000
> +
> +#define OV490_ID_VAL 0x0490
> +#define OV490_ID(_p, _v) _p) & 0xff) << 8) | ((_v) & 0xff))
> +#define OV490_PID0x8080300a
> +#define OV490_VER0x8080300b
> +
> +#define OV490_ISP_HSIZE_LOW 0x80820060
> +#define OV490_ISP_HSIZE_HIGH 0x80820061
> +#define OV490_ISP_VSIZE_LOW 0x80820062
> +#define OV490_ISP_VSIZE_HIGH 0x80820063
> +
> +#define OV10640_PIXEL_RATE (5500)
> +
> +struct rdacm21_device {
> + struct device
Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
On 16/11/2020 10:03, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Mon, Nov 16, 2020 at 11:08:33AM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> On Sat, Nov 14, 2020 at 03:04:57PM +0100, Jacopo Mondi wrote:
>>> On Thu, Nov 12, 2020 at 10:31:05PM +, Kieran Bingham wrote:
>>>> Hi Jacopo,
>>>
>>> [snip]
>>>
>>>>> + /* Wait for firmware boot by reading streamon status. */
>>>>> + ov490_write(dev, 0xfffd, 0x80);
>>>>> + ov490_write(dev, 0xfffe, 0x29);
>>>>> + usleep_range(100, 150);
>>>>> + for (timeout = 300; timeout > 0; timeout--) {
>>>>> + ov490_read(dev, 0xd000, );
>>>>> + if (val == 0x0c)
>>>>
>>>> What is 0x0c here? Is it something we can better describe in a #define?
>>>>
>>>
>>> The 0x0c value itself means "frame output enable" + "whole frame
>>> output enable". I don't think it has much value to define it,
>>> otherwise we would need to define also the register 8029d000
>>
>> Shouldn't we have macros for *all* register addresses and fields ?
>>
>
> I'm not sure it's worth it, we have a single register-value table, and
> the way ov490 is programmed, as you can see is to specify the high
> bytes of the 32-bits register to write in the special 'page' registers
> 0xfffd, 0xfffe (which I've not found documented)
>
> ov490_write(dev, 0xfffd, 0x80);
> ov490_write(dev, 0xfffe, 0x29);
> ov490_read(dev, 0xd000, );
>
> This, to my understanding reads register 0x8029d000
>
> We would need three macros, maybe a
> PAGE_HIGH(reg) (u8)(reg >> 24)
> PAGE_LOW(reg) (u8)(reg >> 16)
> REG_LOW(reg)(u16)(reg)
>
> To that's a lot of churn for no gain imho, the code isn't much more
> clear
To me it means we need a couple of helper functions.
ov490_write_long()
ov490_read_long().
(Or other names if appropriate)
Wouldn't that make the code clearer, and more maintainable? And then
allow more correct register definitions to be created?
>>> Also, the ov490 is programmed loading the content of a SPI Flash chip,
>>> I guess it's just known that "output enabled" is required to have
>>> stream operations properly working.
>>>
>>>>> + break;
>>>>> + mdelay(1);
>>>>> + }
>>>>> + if (!timeout) {
>>>>> + dev_err(dev->dev, "Timeout firmware boot wait\n");
>>>>> + return -ENODEV;
>>>>> + }
>>>>> + dev_dbg(dev->dev, "Firmware booted in %u msec\n", 300 - timeout);
>>>>> +
>>>>> + /* Read OV10640 Id to test communications. */
>>>>> + ov490_write(dev, 0xfffd, 0x80);
>>>>> + ov490_write(dev, 0xfffe, 0x19);
The other one was a write to 0x80, 0x29, why is this now to 0x19?
>>>>> + usleep_range(100, 150);
>
> Not to add that I don't have register 0x80195000 in the documentation I've
Aha, so writing to 0xfffd, 0xfffe, 'sets' the top page. I see.
And I presume there must always be a usleep_range(100, 150) after any
page change?
Really sounds like a helper function to me, which keeps track of the
current page, and updates the page when needed.
> access to (the master SCCB control page is at address 0x8090
>
>>>>> +
>>>>> + ov490_write(dev, 0x5000, 0x01);
>>>>> + ov490_write(dev, 0x5001, 0x30);
>>>>> + ov490_write(dev, 0x5002, 0x0a)
>>>>> + ov490_write(dev, 0xfffe, 0x80);
>
> This sequence in example, reads the 0x300a register of the slave
> (ov10640) by programming registers
> 0x80195000 0x1
> 0x80195001 0x30
> 0x80195002 0x0a
>
>>>>> + usleep_range(100, 150);
>>>>> + ov490_write(dev, 0xc0, 0xc1);
>
> Triggering a transaction writing 0xc1 to 0x808000c0 (0xc1
> undocumented)
Do we do many reads of the connected sensor? or is this the only one.
If you know that's what's happening, I'd be tempted to say we should
wrap that up in a function to make it clearer too.
>>>>> + ov490_write(dev, 0xfffe, 0x19);
>>>>> + usleep_range(1000, 1500);
>>>>> + ov490_read(dev, 0x5000, );
>
> and reading back the transaction result at address 0x80195000
>
> I got these parts from
> https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0040-H3-MAX9286-TI964-s
Re: [PATCH v4 5/8] media: i2c: max9286: Configure reverse channel amplitude
Hi Jacopo,
On 12/11/2020 16:27, Jacopo Mondi wrote:
> Adjust the initial reverse channel amplitude parsing from
> firmware interface the 'maxim,initial-reverse-channel-mV'
> property.
>
> This change is required for both rdacm20 and rdacm21 camera
> modules to be correctly probed when used in combination with
> the max9286 deserializer.
>
> Signed-off-by: Jacopo Mondi
Reviewed-by: Kieran Bingham
> ---
> drivers/media/i2c/max9286.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 31e27d0f34f1..11ba047f3793 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,6 +163,8 @@ struct max9286_priv {
> unsigned int mux_channel;
> bool mux_open;
>
> + u32 reverse_channel_mV;
> +
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
>
> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
>* All enabled sources have probed and enabled their reverse control
>* channels:
>*
> + * - Increase the reverse channel amplitude to compensate for the
> + * remote ends high threshold, if not done already
>* - Verify all configuration links are properly detected
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> + if (priv->reverse_channel_mV < 170)
> + max9286_reverse_channel_setup(priv, 170);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
> @@ -967,7 +973,7 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> - max9286_reverse_channel_setup(priv, 170);
> + max9286_reverse_channel_setup(priv, priv->reverse_channel_mV);
>
> /*
>* Enable GMSL links, mask unused ones and autodetect link
> @@ -1235,6 +1241,18 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> }
> of_node_put(node);
>
> + /*
> + * Parse the initial value of the reverse channel amplitude from
> + * the firmware interface.
> + *
> + * Default it to 170mV for backward compatibility with DTB that do not
> + * provide the property.
> + */
> + if (of_property_read_u32(dev->of_node,
> + "maxim,initial-reverse-channel-mV",
> + >reverse_channel_mV))
> + priv->reverse_channel_mV = 170;
> +
> priv->route_mask = priv->source_mask;
>
> return 0;
>
Re: [PATCH v4 2/8] dt-bindings: media: max9286: Document 'maxim,,initial-reverse-channel-mV"
Hi Jacopo,
in $SUBJECT, there's a double ',' between maxim,,initial and it swaps
from a single quote to a double quote which you might want to fix too.
On 12/11/2020 16:27, Jacopo Mondi wrote:
> Document the 'initial-reverse-channel-mV' vendor property in the
> bindings document of the max9286 driver.
>
> The newly introduced property allows to specify the initial
s/to specify/specifying/
> configuration of the GMSL reverse control channel to accommodate
> remote serializers pre-programmed with the high threshold power
> supply noise immunity enabled.
>
> Signed-off-by: Jacopo Mondi
> ---
> .../bindings/media/i2c/maxim,max9286.yaml | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index 9ea827092fdd..c506a0261325 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -51,6 +51,26 @@ properties:
>'#gpio-cells':
> const: 2
>
> + maxim,initial-reverse-channel-mV:
> +$ref: '/schemas/types.yaml#/definitions/uint32'
> +minimum: 30
> +maximum: 200
> +default: 170
> +description: |
> + Initial amplitude of the reverse control channel, in millivolts.
> +
> + The initial amplitude shall be adjusted to a value compatible with the
> + configuration of the connected remote serializer.
> +
> + Some camera modules (in example RDACM20) include an on-board MCU that
s/in example/for example/
> + pre-programs the embedded serializer with power supply noise immunity
> + (high-threshold) enabled. A typical value of the deserializer's reverse
> + channel amplitude to communicate with pre-programmed serializers is
> 170mV.
> +
> + A typical value for the reverse channel amplitude to communicate with
> + a remote serializer whose high-threshold noise immunity is not enabled
> + is 100mV.
> +
>ports:
> type: object
> description: |
> @@ -221,6 +241,7 @@ required:
>- ports
>- i2c-mux
>- gpio-controller
> + - maxim,initial-reverse-channel-mV
>
> additionalProperties: false
>
> @@ -243,6 +264,8 @@ examples:
> gpio-controller;
> #gpio-cells = <2>;
>
> +maxim,initial-reverse-channel-mV = <170>;
> +
Sounds good to me.
Reviewed-by: Kieran Bingham
> ports {
>#address-cells = <1>;
>#size-cells = <0>;
>
Re: [PATCH v4 1/8] media: i2c: Add driver for RDACM21 camera module
Hi Jacopo,
On 12/11/2020 16:27, Jacopo Mondi wrote:
> The RDACM21 is a GMSL camera supporting 1280x1080 resolution images
> developed by IMI based on an Omnivision OV10640 sensor, an Omnivision
> OV490 ISP and a Maxim MAX9271 GMSL serializer.
>
> The driver uses the max9271 library module, to maximize code reuse with
> other camera module drivers using the same serializer, such as rdacm20.
>
> Signed-off-by: Jacopo Mondi
> ---
> MAINTAINERS | 12 +
> drivers/media/i2c/Kconfig | 13 +
> drivers/media/i2c/Makefile | 2 +
> drivers/media/i2c/rdacm21.c | 539
> 4 files changed, 566 insertions(+)
> create mode 100644 drivers/media/i2c/rdacm21.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cef6b6090d76..9a5026fd6788 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14750,6 +14750,18 @@ F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> F: drivers/media/i2c/rdacm20.c
>
> +RDACM21 Camera Sensor
> +M: Jacopo Mondi
> +M: Kieran Bingham
> +M: Laurent Pinchart
> +M: Niklas Söderlund
> +L: linux-me...@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/media/i2c/rdacm2x-gmsl.yaml
> +F: drivers/media/i2c/max9271.c
> +F: drivers/media/i2c/max9271.h
> +F: drivers/media/i2c/rdacm21.c
> +
> RDC R-321X SoC
> M: Florian Fainelli
> S: Maintained
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index c64326ca331c..64f4316d11ad 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1185,6 +1185,19 @@ config VIDEO_RDACM20
> This camera should be used in conjunction with a GMSL
> deserialiser such as the MAX9286.
>
> +config VIDEO_RDACM21
> + tristate "IMI RDACM21 camera support"
> + depends on I2C
> + select V4L2_FWNODE
> + select VIDEO_V4L2_SUBDEV_API
> + select MEDIA_CONTROLLER
> + help
> + This driver supports the IMI RDACM21 GMSL camera, used in
> + ADAS systems.
> +
> + This camera should be used in conjunction with a GMSL
> + deserialiser such as the MAX9286.
> +
> config VIDEO_RJ54N1
> tristate "Sharp RJ54N1CB0C sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index f0a77473979d..f3641b58929d 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -122,6 +122,8 @@ obj-$(CONFIG_VIDEO_IMX355)+= imx355.o
> obj-$(CONFIG_VIDEO_MAX9286) += max9286.o
> rdacm20-camera_module-objs := rdacm20.o max9271.o
> obj-$(CONFIG_VIDEO_RDACM20) += rdacm20-camera_module.o
> +rdacm21-camera_module-objs := rdacm21.o max9271.o
> +obj-$(CONFIG_VIDEO_RDACM21) += rdacm21-camera_module.o
> obj-$(CONFIG_VIDEO_ST_MIPID02) += st-mipid02.o
>
> obj-$(CONFIG_SDR_MAX2175) += max2175.o
> diff --git a/drivers/media/i2c/rdacm21.c b/drivers/media/i2c/rdacm21.c
> new file mode 100644
> index 00000000..81529f17b562
> --- /dev/null
> +++ b/drivers/media/i2c/rdacm21.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IMI RDACM21 GMSL Camera Driver
> + *
> + * Copyright (C) 2017-2020 Jacopo Mondi
> + * Copyright (C) 2017-2019 Kieran Bingham
> + * Copyright (C) 2017-2019 Laurent Pinchart
> + * Copyright (C) 2017-2019 Niklas Söderlund
> + * Copyright (C) 2016 Renesas Electronics Corporation
> + * Copyright (C) 2015 Cogent Embedded, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include "max9271.h"
> +
> +#define OV10640_I2C_ADDRESS 0x30
> +#define OV10640_ID_LOW 0xa6
> +
> +#define OV490_I2C_ADDRESS0x24
> +
> +#define OV490_ISP_HSIZE_LOW 0x60
> +#define OV490_ISP_HSIZE_HIGH 0x61
> +#define OV490_ISP_VSIZE_LOW 0x62
> +#define OV490_ISP_VSIZE_HIGH 0x63
> +
> +#define OV490_PID0x300a
> +#define OV490_VER0x300b
> +#define OV490_ID_VAL 0x0490
> +#define OV490_ID(_p, _v) _p) & 0xff) << 8) | ((_v) & 0xff))
> +
> +#define OV10640_PIXEL_RATE (5500)
> +
> +struct rdacm21_device {
> + struct device *dev;
> + struct max9271_device *serializer;
> + struct i2c_client *isp;
> + struct v4l2_subdev sd;
> + struct media_padpad;
> + struct v4l2_mbus_framefmt fmt;
>
Re: [PATCH v3 2/7] dt-bindings: media: max9286: Document 'maxim,high-threshold'
Hi Jacopo, On 16/10/2020 14:04, Geert Uytterhoeven wrote: > Hi Jacopo, > > On Fri, Oct 16, 2020 at 2:56 PM Jacopo Mondi wrote: >> On Fri, Oct 16, 2020 at 01:50:34PM +0200, Geert Uytterhoeven wrote: >>> On Fri, Oct 16, 2020 at 12:09 PM Jacopo Mondi >>> wrote: Document the 'maxim,high-threshold' vendor property in the bindings document of the max9286 driver. The newly introduced boolean property allows controlling the initial configuration of the GMSL reverse control channel to accommodate remote serializers pre-programmed with the high threshold power supply noise immunity enabled. Signed-off-by: Jacopo Mondi >>> >>> Thanks for your patch! >>> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml @@ -51,6 +51,19 @@ properties: '#gpio-cells': const: 2 + maxim,high-threshold: +description: | + A boolean property to increase the initial amplitude of the reverse + control channel to compensate for remote serializers pre-programmed with + high threshold noise-immunity. + + Some camera modules (in example the RDACM20 one) include an on-board MCU + that pre-programs the embedded serializer with reverse channel power + supply noise immunity enabled. The deserializer shall increase its + reverse channel amplitude to compensate that and be able to communicate + with the remote end. +type: boolean >>> >>> Does this "high" threshold correspond to some numerical value? >>> I.e. could we run into a future need to support more values than just >>> true/false? >>> If yes, we may want to use a numerical value from the start. >> >> So, this boolean property controls the initial setting of the reverse >> channel amplitude, which has to be opportunely adjusted to be able to >> probe the remote devices as i2c messages are bridged on the reverse >> channel from the deserializer to the remote serializers. >> >> It also implies that if the initial setting is not "high" it has to be >> increased after the remotes have probed, as it is assumed at the end >> the remotes' probe() routine they have enabled their high threshold >> noise immunity feature. >> >> The register that controls the de-serializer reverse channel amplitude >> is not fully documented in the chip manual version I have, but the >> application developer guide shows that it can range from 30mV to >> 200mV. The same developer guide shows 100mV and 170mV as "initial" and >> "high threshold" values to be used. >> >> On the serializer's side (MAX9271 at least) high threshold is a >> boolean control, it is either enabled or disabled. >> >> Hence I suspect that for the max9286-max9271 combo, using fixed 100mV >> and 170mV values is enough. Other serializers might allow a more fine >> grained control of the noise cancelling threshold, and might require a >> more precise adjustment of the deserializer side. I'm honestly not >> sure at this point not having seen any other one. >> >> So yes, this is a deserializer setting that depends on the serializer >> in use, and I'm not 100% comfortable expressing it as a deserializer >> property. That said, having an initial numerical value might serve the >> same purpose allowing more flexibility, if other serializers require a >> more precise control of the initial amplitude. And one of the benefits of your series, is that you've codified the conversion from the input value to the register value, which is really helpful. I think the fact that you've implemented the required feature, means exposing the value here makes more sense. However, once exposed outside of the driver, I'd add some clamping on the allowed min/max values in the max9286_reverse_channel_setup() function. >> >> After the remotes have probed, we could also re-explore the >> possibility of getting the bus configuration using get_mbus_config, as >> detailed in the v1 cover letter [1] as at that time their sub-devices >> have been registered and we can call operations on them. But again, >> this might be not required at all. > > Sounds like "maxim,-threshold-mV" is the way to go? > If you don't know the exact register configuration, and need to > convert to a boolean in the driver, you can still use something like > "threshold < 150 ? ... : ...". This sounds like a reasonable path to me - I'm still really weary that it's defining it on the deser, rather than the serialiser - but perhaps there's no current option to do that. I presume we can't wait until we can obtain a value through get_mbus_config() ? That's going to be chicken and egg, as the camera can't probe until we have the i2c channels set up ... yet the information we want, doesn't need the bus to be configured... so there's indeed some missing pieces to the puzzle for that to happen ;-( also - I guess the
Re: [PATCH v3 5/7] media: i2c: max9286: Configure reverse channel amplitude
Hi Jacopo,
On 16/10/2020 13:06, Jacopo Mondi wrote:
> Adjust reverse channel amplitude according to the presence of
> the 'high-threshold" DTS property.
>
> If no high threshold compensation is required, start with a low
> amplitude (100mV) and increase it after the remote serializers
> have probed and have enabled noise immunity on their reverse
> channels.
>
> If high threshold compensation is required, configure the reverse
> channel with a 170mV amplitude before the remote serializers have
> probed.
>
> This change is required for both rdacm20 and rdacm21 camera modules
> to be correctly probed when used in combination with the max9286
> deserializer.
My only fear here would be that perhaps on other cameras we need a more
fine-grained control of the amplitudes?
But I'll leave that discussion to the binding itself,
For this patch ...
Reviewed-by: Kieran Bingham
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 31e27d0f34f1..4c72e1e6b27b 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,6 +163,8 @@ struct max9286_priv {
> unsigned int mux_channel;
> bool mux_open;
>
> + bool high_threshold;
> +
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
>
> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
>* All enabled sources have probed and enabled their reverse control
>* channels:
>*
> + * - Increase the reverse channel amplitude to compensate for the
> + * remote ends high threshold, if not done already
>* - Verify all configuration links are properly detected
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> + if (!priv->high_threshold)
> + max9286_reverse_channel_setup(priv, 170);
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
> @@ -967,7 +973,12 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> - max9286_reverse_channel_setup(priv, 170);
> +
> + /*
> + * Compensate the remote end high threshold with a larger channel
> + * amplitude if necessary.
> + */
> + max9286_reverse_channel_setup(priv, priv->high_threshold ? 170 : 100);
>
> /*
>* Enable GMSL links, mask unused ones and autodetect link
> @@ -1235,6 +1246,12 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> }
> of_node_put(node);
>
> + /*
> + * Parse 'high_threshold' property to configure the reverse channel
> + * amplitude.
> + */
> + priv->high_threshold = device_property_present(dev, "high_threshold");
> +
> priv->route_mask = priv->source_mask;
>
> return 0;
>
Re: [PATCH v2 4/7] media: i2c: max9286: Make channel amplitude programmable
Hi Jacopo,
On 15/10/2020 19:27, Jacopo Mondi wrote:
> Instrument the function that configures the reverse channel with a
> programmable amplitude value.
>
> This change serves to prepare to adjust the reverse channel amplitude
> depending on the remote end high-threshold configuration.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 89a7248f5c25..163e102199e3 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -906,19 +906,29 @@ static void max9286_v4l2_unregister(struct max9286_priv
> *priv)
> * Probe/Remove
> */
>
> -static void max9286_reverse_channel_setup(struct max9286_priv *priv)
> +static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> + unsigned int chan_amplitude)
> {
> + /* Reverse channel transmission time: default to 1. */
> + u8 chan_config = MAX9286_REV_TRF(1);
> +
> /*
>* Reverse channel setup.
>*
>* - Enable custom reverse channel configuration (through register 0x3f)
>* and set the first pulse length to 35 clock cycles.
> - * - Increase the reverse channel amplitude to 170mV to accommodate the
> - * high threshold enabled by the serializer driver.
> + * - Adjust reverse channel amplitude: values > 130 are programmed
> + * using the additional +100mV REV_AMP_X boost flag
>*/
> max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> - max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> - MAX9286_REV_AMP_X);
> +
Should we also clamp to min/max values at all?
Probably not needed, as it's only an internal helper.
> + if (chan_amplitude > 100) {
> + /* It is not possible express values (100 < x < 130) */
'possible to express'
> + chan_amplitude = chan_amplitude < 130
> +? 30 : chan_amplitude - 100;
We could round < 115 to 100, and >= 115 to 130, but that's probably more
effort that it's worth, so I think this is fine.
I think it's really helpful to codify this parameter though:
Reviewed-by: Kieran Bingham
> + chan_config |= MAX9286_REV_AMP_X;
> + }
> + max9286_write(priv, 0x3b, chan_config |
> MAX9286_REV_AMP(chan_amplitude));
> usleep_range(2000, 2500);
> }
>
> @@ -957,7 +967,7 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> - max9286_reverse_channel_setup(priv);
> + max9286_reverse_channel_setup(priv, 170);
>
> /*
>* Enable GMSL links, mask unused ones and autodetect link
>
Re: [PATCH v2 3/7] media: i2c: max9286: Break-out reverse channel setup
Hi Jacopo,
On 15/10/2020 19:27, Jacopo Mondi wrote:
> Break out the reverse channel setup configuration procedure to its own
> function.
>
> This change prepares for configuring the reverse channel conditionally
> to the remote side high threshold configuration.
>
> No functional changes intended.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 30 +-
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index d969ac21a058..89a7248f5c25 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -906,6 +906,22 @@ static void max9286_v4l2_unregister(struct max9286_priv
> *priv)
> * Probe/Remove
> */
>
> +static void max9286_reverse_channel_setup(struct max9286_priv *priv)
> +{
> + /*
> + * Reverse channel setup.
> + *
> + * - Enable custom reverse channel configuration (through register 0x3f)
> + * and set the first pulse length to 35 clock cycles.
> + * - Increase the reverse channel amplitude to 170mV to accommodate the
> + * high threshold enabled by the serializer driver.
> + */
> + max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> + max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> + MAX9286_REV_AMP_X);
> + usleep_range(2000, 2500);
> +}
This gets moved later on in the same series. It's probably better to put
it in the right place now.
With that,
Reviewed-by: Kieran Bingham
> +
> static int max9286_setup(struct max9286_priv *priv)
> {
> /*
> @@ -941,19 +957,7 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> -
> - /*
> - * Reverse channel setup.
> - *
> - * - Enable custom reverse channel configuration (through register 0x3f)
> - * and set the first pulse length to 35 clock cycles.
> - * - Increase the reverse channel amplitude to 170mV to accommodate the
> - * high threshold enabled by the serializer driver.
> - */
> - max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> - max9286_write(priv, 0x3b, MAX9286_REV_TRF(1) | MAX9286_REV_AMP(70) |
> - MAX9286_REV_AMP_X);
> - usleep_range(2000, 2500);
> + max9286_reverse_channel_setup(priv);
>
> /*
>* Enable GMSL links, mask unused ones and autodetect link
>
Re: [PATCH v2 5/7] media: i2c: max9286: Configure reverse channel amplitude
Hi Jacopo,
On 15/10/2020 19:27, Jacopo Mondi wrote:
> Adjust reverse channel amplitude according to the presence of
> the 'high-threshold" DTS property.
>
> If no high threshold compensation is required, start with a low
> amplitude (100mV) and increase it after the remote serializers
> have probed and have enabled noise immunity on their reverse
> channels.
>
> If high threshold compensation is required, configure the reverse
> channel with a 170mV amplitude before the remote serializers have
> probed.
>
> This change is required for both rdacm20 and rdacm21 camera modules
> to be correctly probed when used in combination with the max9286
> deserializer.
>
> Signed-off-by: Jacopo Mondi
> ---
> drivers/media/i2c/max9286.c | 74 +++--
> 1 file changed, 47 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 163e102199e3..4b59a9e0228a 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -163,6 +163,8 @@ struct max9286_priv {
> unsigned int mux_channel;
> bool mux_open;
>
> + bool high_threshold;
> +
> struct v4l2_ctrl_handler ctrls;
> struct v4l2_ctrl *pixelrate;
>
> @@ -436,6 +438,32 @@ static int max9286_check_config_link(struct max9286_priv
> *priv,
> return 0;
> }
>
> +static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> + unsigned int chan_amplitude)
This looks like you're adding a new function - how about we add this
function here, in the first place when you add it in 3/7
> +{
> + /* Reverse channel transmission time: default to 1. */
> + u8 chan_config = MAX9286_REV_TRF(1);
> +
> + /*
> + * Reverse channel setup.
> + *
> + * - Enable custom reverse channel configuration (through register 0x3f)
> + * and set the first pulse length to 35 clock cycles.
> + * - Adjust reverse channel amplitude: values > 130 are programmed
> + * using the additional +100mV REV_AMP_X boost flag
> + */
> + max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> +
> + if (chan_amplitude > 100) {
> + /* It is not possible express values (100 < x < 130) */
> + chan_amplitude = chan_amplitude < 130
> +? 30 : chan_amplitude - 100;
> + chan_config |= MAX9286_REV_AMP_X;
> + }
> + max9286_write(priv, 0x3b, chan_config |
> MAX9286_REV_AMP(chan_amplitude));
> + usleep_range(2000, 2500);
> +}
> +
> /*
> -
> * V4L2 Subdev
> */
> @@ -531,10 +559,14 @@ static int max9286_notify_bound(struct
> v4l2_async_notifier *notifier,
>* All enabled sources have probed and enabled their reverse control
>* channels:
>*
> + * - Increase the reverse channel amplitude to compensate for the
> + * remote ends high threshold, if not done already
>* - Verify all configuration links are properly detected
>* - Disable auto-ack as communication on the control channel are now
>* stable.
>*/
> + if (!priv->high_threshold)
> + max9286_reverse_channel_setup(priv, 170);
is it troublesome to re-set it if it's already set? I guess it's just
unnecessary. so that's fine.
> max9286_check_config_link(priv, priv->source_mask);
>
> /*
> @@ -906,32 +938,6 @@ static void max9286_v4l2_unregister(struct max9286_priv
> *priv)
> * Probe/Remove
> */
>
> -static void max9286_reverse_channel_setup(struct max9286_priv *priv,
> - unsigned int chan_amplitude)
> -{
> - /* Reverse channel transmission time: default to 1. */
> - u8 chan_config = MAX9286_REV_TRF(1);
> -
> - /*
> - * Reverse channel setup.
> - *
> - * - Enable custom reverse channel configuration (through register 0x3f)
> - * and set the first pulse length to 35 clock cycles.
> - * - Adjust reverse channel amplitude: values > 130 are programmed
> - * using the additional +100mV REV_AMP_X boost flag
> - */
> - max9286_write(priv, 0x3f, MAX9286_EN_REV_CFG | MAX9286_REV_FLEN(35));
> -
> - if (chan_amplitude > 100) {
> - /* It is not possible express values (100 < x < 130) */
> - chan_amplitude = chan_amplitude < 130
> -? 30 : chan_amplitude - 100;
> - chan_config |= MAX9286_REV_AMP_X;
> - }
> - max9286_write(priv, 0x3b, chan_config |
> MAX9286_REV_AMP(chan_amplitude));
> - usleep_range(2000, 2500);
> -}
> -
> static int max9286_setup(struct max9286_priv *priv)
> {
> /*
> @@ -967,7 +973,15 @@ static int max9286_setup(struct max9286_priv *priv)
>* only. This should be disabled after the mux is initialised.
>*/
> max9286_configure_i2c(priv, true);
> -
Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
Hi Hans, On 14/10/2020 12:23, Hans de Goede wrote: > Hi, > > On 10/14/20 1:09 PM, Kieran Bingham wrote: >> Hi Hans, Sasha, >> >> As mentioned on https://github.com/linux-surface/kernel/issues/63, I'm >> afraid I've bisected a boot time issue on the Microsoft Surface Go 2 to >> this commit on the stable 5.8 tree. >> >> The effect as reported there is that the boot process stalls just after >> loading the usbhid module. >> >> Typing, or interacting with the Keyboard (Type Cover) at that point >> appears to cause usb bus resets, but I don't know if that's a related >> symptom or just an effect of some underlying root cause. >> >> I have been running a linux-media kernel on this device without issue. >> >> Is this commit in 5.9? I'll build a vanilla v5.9 kernel and see if it >> occurs there too. > > Yes the commit is in 5.9 too. Still would be interesting to see if 5.9 hits > this issue too. I guess it will, but as I mentioned in: Indeed, I've just tested vanilla 5.9 on a Surface Go 2 and that's broken I'm afraid. Back traces on 5.9 [0] show hung tasks on both the i915_driver_probe, and the tps68470_gpio_probe. I suspect the i915 failure of course is the reason a desktop environment can't be reached, however I was able to ssh in at least ;-) [0] https://paste.debian.net/1167081/ Given that this is now a mainline bug, I'll reply to the mainline patch and suggest a revert there, rather than here on the stable patches. As for fixing it ... I don't have enough visibility of the problem for both this, and the issues that were fixed by this patch. Let me know if there's anything you'd like me to investigate further, or dig into specifically. -- Kieran > https://github.com/linux-surface/kernel/issues/63 > > I do not understand why this commit is causing this issue. > > So I just checked and the whole acpidump is not using I2C > opregion stuff at all: > > [hans@x1 microsoft-surface-go2]$ ack GenericSerialBus *.dsl > [hans@x1 microsoft-surface-go2]$ > > And there is only 1 _REG handler which is for the > embedded-controller. > > So this patch should not make a difference at all on the GO2, > other then maybe a subtle timing difference somewhere ... ? > > Regards, > > Hans > > >> On 21/09/2020 15:40, Sasha Levin wrote: >>> From: Hans de Goede >>> >>> [ Upstream commit 21653a4181ff292480599dad996a2b759ccf050f ] >>> >>> Some ACPI i2c-devices _STA method (which is used to detect if the device >>> is present) use autodetection code which probes which device is present >>> over i2c. This requires the I2C ACPI OpRegion handler to be registered >>> before we enumerate i2c-clients under the i2c-adapter. >>> >>> This fixes the i2c touchpad on the Lenovo ThinkBook 14-IIL and >>> ThinkBook 15 IIL not getting an i2c-client instantiated and thus not >>> working. >>> >>> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1842039 >>> Signed-off-by: Hans de Goede >>> Reviewed-by: Mika Westerberg >>> Signed-off-by: Wolfram Sang >>> Signed-off-by: Sasha Levin >>> --- >>> drivers/i2c/i2c-core-base.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >>> index 4f09d4c318287..7031393c74806 100644 >>> --- a/drivers/i2c/i2c-core-base.c >>> +++ b/drivers/i2c/i2c-core-base.c >>> @@ -1336,8 +1336,8 @@ static int i2c_register_adapter(struct >>> i2c_adapter *adap) >>> /* create pre-declared device nodes */ >>> of_i2c_register_devices(adap); >>> - i2c_acpi_register_devices(adap); >>> i2c_acpi_install_space_handler(adap); >>> + i2c_acpi_register_devices(adap); >>> if (adap->nr < __i2c_first_dynamic_bus_num) >>> i2c_scan_static_board_info(adap); >>> >> >> > -- Regards -- Kieran
Re: [PATCH AUTOSEL 5.8 17/20] i2c: core: Call i2c_acpi_install_space_handler() before i2c_acpi_register_devices()
Hi Hans, Sasha, As mentioned on https://github.com/linux-surface/kernel/issues/63, I'm afraid I've bisected a boot time issue on the Microsoft Surface Go 2 to this commit on the stable 5.8 tree. The effect as reported there is that the boot process stalls just after loading the usbhid module. Typing, or interacting with the Keyboard (Type Cover) at that point appears to cause usb bus resets, but I don't know if that's a related symptom or just an effect of some underlying root cause. I have been running a linux-media kernel on this device without issue. Is this commit in 5.9? I'll build a vanilla v5.9 kernel and see if it occurs there too. -- Regards Kieran On 21/09/2020 15:40, Sasha Levin wrote: > From: Hans de Goede > > [ Upstream commit 21653a4181ff292480599dad996a2b759ccf050f ] > > Some ACPI i2c-devices _STA method (which is used to detect if the device > is present) use autodetection code which probes which device is present > over i2c. This requires the I2C ACPI OpRegion handler to be registered > before we enumerate i2c-clients under the i2c-adapter. > > This fixes the i2c touchpad on the Lenovo ThinkBook 14-IIL and > ThinkBook 15 IIL not getting an i2c-client instantiated and thus not > working. > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1842039 > Signed-off-by: Hans de Goede > Reviewed-by: Mika Westerberg > Signed-off-by: Wolfram Sang > Signed-off-by: Sasha Levin > --- > drivers/i2c/i2c-core-base.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 4f09d4c318287..7031393c74806 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -1336,8 +1336,8 @@ static int i2c_register_adapter(struct i2c_adapter > *adap) > > /* create pre-declared device nodes */ > of_i2c_register_devices(adap); > - i2c_acpi_register_devices(adap); > i2c_acpi_install_space_handler(adap); > + i2c_acpi_register_devices(adap); > > if (adap->nr < __i2c_first_dynamic_bus_num) > i2c_scan_static_board_info(adap); > -- Regards -- Kieran
Re: [PATCH] scripts/gdb: fix list_for_each
On 23/09/2020 14:13, George Prekas wrote:
> Hi Kieran,
>
> On 9/22/2020 2:11 PM, Kieran Bingham wrote:
>> Hi George,
>>
>> On 22/09/2020 18:17, Prekas, George wrote:
>>>
>>> On 9/22/2020 9:32 AM, Jan Kiszka wrote:
>>>>
>>>> On 22.09.20 16:28, George Prekas wrote:
>>>>> If the next pointer is NULL, list_for_each gets stuck in an infinite
>>>>> loop.
>>>>>
>>>>> Signed-off-by: George Prekas
>>>>> ---
>>>>> scripts/gdb/linux/lists.py | 2 ++
>>>>> 1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py
>>>>> index c487ddf09d38..424a91c1aa8b 100644
>>>>> --- a/scripts/gdb/linux/lists.py
>>>>> +++ b/scripts/gdb/linux/lists.py
>>>>> @@ -27,6 +27,8 @@ def list_for_each(head):
>>>>> raise TypeError("Must be struct list_head not {}"
>>>>> .format(head.type))
>>>>>
>>>>> + if head['next'] == 0:
>>>>> + return
>>>>> node = head['next'].dereference()
>>>>> while node.address != head.address:
>>>>> yield node.address
>>>>
>>>> Obviously, infinite loops are bad and should be avoided. But NULL is
>>>> bug, isn't it? Shouldn't we report such a corruption?
>>>>
>>>
>>> Hi Jan,
>>>
>>> Is it a bug? Or does it mean that the list is empty?
>>
>> A correctly initialised (empty) list_head has the next, and prev
>> pointers pointing to itself
>>
>
> You are right, actually.
>
>>
>>> Let me give some background. If you do the following:
>>>
>>> $ qemu-system-x86_64 -nographic -m 1024 -kernel
>>> build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null &
>>> $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set
>>> auto-load safe-path /' -ex 'lx-symbols'
>>
>> I suspect this is trying to load modules before the kernel is actually
>> fully loaded and running, so nothing is yet initialised.
>>
>>
>>> You will see:
>>>
>>> loading vmlinux
>>> scanning for modules in /home/ubuntu/linux-5.8.10
>>> no module object found for ''
>>>
>>> And the last line repeats forever. This happens because modules.next ==
>>> NULL. This is the Python stack trace:
>>>
>>>[...]
>>>
>>> This patch tries to fix the above problem.
>>
>> Does it fix it for you ?
>>
>> I expect it allows the boot process to continue, but the lx-symbols
>> command will not have completed successfully (or rather I expect it will
>> not have found anything to load).
>>
>> I suspect adding defensive checks in here might be helpful but I think
>> the reality is the code is being called at the wrong time.
>>
>> The fact that it 'can' be called at the wrong time is where we might
>> need to be more defensive.
>>
>
> At that point in time, the kernel has not even started so it does not
> have any loaded modules. In fact, as you said, the modules linked list
> is uninitialized. So with this patch, lx-symbols does not get stuck in
> an infinite loop and loads only the vmlinux symbols.
>
> Maybe, I should rephrase the commit message to say that list_for_each
> gets stuck in an infinite loop on uninitialized linked lists.
>
> Do you think that list_for_each should handle uninitialized lists? If
> yes, how do you propose to handle them?
>
> 1. Treat them as empty lists (this patch).
> 2. Print a warning and treat them as empty lists.
> 3. Raise exception and treat them as empty lists.
>
> I would go with option 1. For traversal purposes an uninitialized list
> is the same as an empty list; it has no elements. I am happy, though, to
> change the patch to another option if you believe it would be better.
I would choose 2 personally.
While debugging, if anyone hits an uninitialised linked-list - that's a
problem they want to know about, not ignore.
--
Kieran
> --
> George
>
Re: [PATCH] scripts/gdb: fix list_for_each
Hi George,
On 22/09/2020 18:17, Prekas, George wrote:
>
> On 9/22/2020 9:32 AM, Jan Kiszka wrote:
>>
>> On 22.09.20 16:28, George Prekas wrote:
>>> If the next pointer is NULL, list_for_each gets stuck in an infinite
>>> loop.
>>>
>>> Signed-off-by: George Prekas
>>> ---
>>> scripts/gdb/linux/lists.py | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/scripts/gdb/linux/lists.py b/scripts/gdb/linux/lists.py
>>> index c487ddf09d38..424a91c1aa8b 100644
>>> --- a/scripts/gdb/linux/lists.py
>>> +++ b/scripts/gdb/linux/lists.py
>>> @@ -27,6 +27,8 @@ def list_for_each(head):
>>> raise TypeError("Must be struct list_head not {}"
>>> .format(head.type))
>>>
>>> + if head['next'] == 0:
>>> + return
>>> node = head['next'].dereference()
>>> while node.address != head.address:
>>> yield node.address
>>
>> Obviously, infinite loops are bad and should be avoided. But NULL is
>> bug, isn't it? Shouldn't we report such a corruption?
>>
>
> Hi Jan,
>
> Is it a bug? Or does it mean that the list is empty?
A correctly initialised (empty) list_head has the next, and prev
pointers pointing to itself
See:
https://elixir.bootlin.com/linux/latest/source/include/linux/list.h#L33
You can see that the implementation of list_empty() checks for this at:
https://elixir.bootlin.com/linux/latest/source/include/linux/list.h#L280
/**
* list_empty - tests whether a list is empty
* @head: the list to test.
*/
static inline int list_empty(const struct list_head *head)
{
return READ_ONCE(head->next) == head;
}
> Let me give some background. If you do the following:
>
> $ qemu-system-x86_64 -nographic -m 1024 -kernel
> build/arch/x86/boot/bzImage -s -S < /dev/null > /dev/null &
> $ gdb -q build/vmlinux -ex 'target remote localhost:1234' -iex 'set
> auto-load safe-path /' -ex 'lx-symbols'
I suspect this is trying to load modules before the kernel is actually
fully loaded and running, so nothing is yet initialised.
> You will see:
>
> loading vmlinux
> scanning for modules in /home/ubuntu/linux-5.8.10
> no module object found for ''
>
> And the last line repeats forever. This happens because modules.next ==
> NULL. This is the Python stack trace:
>
> File ".../symbols.py", line 174, in invoke
> self.load_all_symbols()
> File ".../symbols.py", line 161, in load_all_symbols
> [self.load_module_symbols(module) for module in module_list]
> File ".../symbols.py", line 161, in
> [self.load_module_symbols(module) for module in module_list]
> File ".../modules.py", line 30, in module_list
> for module in lists.list_for_each_entry(modules, module_ptr_type,
> "list"):
> File ".../lists.py", line 41, in list_for_each_entry
> for node in list_for_each(head):
> File ".../lists.py", line 31, in list_for_each
> traceback.print_stack()
>
> This patch tries to fix the above problem.
Does it fix it for you ?
I expect it allows the boot process to continue, but the lx-symbols
command will not have completed successfully (or rather I expect it will
not have found anything to load).
I suspect adding defensive checks in here might be helpful but I think
the reality is the code is being called at the wrong time.
The fact that it 'can' be called at the wrong time is where we might
need to be more defensive.
> George
--
Kieran
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Hi Andy, On 17/09/2020 15:08, Andy Shevchenko wrote: > On Thu, Sep 17, 2020 at 4:31 PM Kieran Bingham > wrote: >> On 17/09/2020 10:47, Dan Scally wrote: >>> On 17/09/2020 08:53, Greg KH wrote: >>>> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote: > >>>>> drivers/staging/media/ipu3/Kconfig | 15 + >>>>> drivers/staging/media/ipu3/Makefile | 1 + >>>>> drivers/staging/media/ipu3/cio2-bridge.c | 448 +++ >>>> Why does this have to be in drivers/staging/ at all? Why not spend the >>>> time to fix it up properly and get it merged correctly? It's a very >>>> small driver, and should be smaller, so it should not be a lot of work >>>> to do. And it would be faster to do that than to take it through >>>> staging... >>> I was just under the impression that that was the process to be honest, >>> if that's not right I'll just move it directly to drivers/media/ipu3 >> >> The IPU3 driver is still in staging (unless I've missed something), so I >> think this cio2-bridge should stay with it. > > You missed something. > https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/pci/intel > > IPU3 from Freescale (IIRC) is a different story. Ayee, ok so we have 'half' the driver for IPU3 out of staging. >From my understanding, the IPU3 consists of two components, the CIO2 (CSI2 capture), and the IMGU (the ISP). - drivers/media/pci/intel/ipu3 This is indeed the CIO2 component (config VIDEO_IPU3_CIO2), and that is the part that this bridge relates to, so in fact this cio2-bridge should probably go there indeed. No need to go through staging. The files remaining at: - drivers/staging/media/ipu3 are in fact also for the IPU3 but the ISP component (VIDEO_IPU3_IMGU). I'm sorry for the confusion, I knew that the ISP was still in staging, I hadn't realised the CSI2 receiver (CIO2) was not. >> Hopefully with more users of the IPU3 brought in by this cio2-bridge, >> that will help gather momentum to get the IPU3 developments required >> completed and moved into drivers/media. -- Regards -- Kieran
Re: [RFC PATCH] Add bridge driver to connect sensors to CIO2 device via software nodes on ACPI platforms
Hi Dan, Greg, On 17/09/2020 10:47, Dan Scally wrote: > Hi Greg - thanks for the comments, appreciate it (sorry there's so many, > I'm new to both C and kernel work) > > On 17/09/2020 08:53, Greg KH wrote: >> On Wed, Sep 16, 2020 at 10:36:18PM +0100, Daniel Scally wrote: >>> MAINTAINERS | 6 + >>> drivers/media/pci/intel/ipu3/ipu3-cio2.c | 67 +++- >> staging drivers should be self-contained, and not modify stuff outside >> of drivers/staging/ >> >>> drivers/staging/media/ipu3/Kconfig | 15 + >>> drivers/staging/media/ipu3/Makefile | 1 + >>> drivers/staging/media/ipu3/cio2-bridge.c | 448 +++ >> Why does this have to be in drivers/staging/ at all? Why not spend the >> time to fix it up properly and get it merged correctly? It's a very >> small driver, and should be smaller, so it should not be a lot of work >> to do. And it would be faster to do that than to take it through >> staging... > I was just under the impression that that was the process to be honest, > if that's not right I'll just move it directly to drivers/media/ipu3 The IPU3 driver is still in staging (unless I've missed something), so I think this cio2-bridge should stay with it. Hopefully with more users of the IPU3 brought in by this cio2-bridge, that will help gather momentum to get the IPU3 developments required completed and moved into drivers/media. -- Regards -- Kieran
Re: [PATCH v2] software_node: Add support for fwnode_graph*() family of functions
Hi Dan,
On 16/09/2020 14:22, Dan Scally wrote:
> Hi Sakari - thanks for the comments
>
> On 16/09/2020 10:17, Sakari Ailus wrote:
>> Moi Daniel and Heikki,
>>
>> On Wed, Sep 16, 2020 at 12:28:27AM +0100, Daniel Scally wrote:
>>> From: Heikki Krogerus
>>>
>>> This implements the remaining .graph_* callbacks in the
>>> fwnode operations vector for the software nodes. That makes
>>> the fwnode_graph*() functions available in the drivers also
>>> when software nodes are used.
>>>
>>> The implementation tries to mimic the "OF graph" as much as
>>> possible, but there is no support for the "reg" device
>>> property. The ports will need to have the index in their
>>> name which starts with "port" (for example "port0", "port1",
>>> ...) and endpoints will use the index of the software node
>>> that is given to them during creation. The port nodes can
>>> also be grouped under a specially named "ports" subnode,
>>> just like in DT, if necessary.
>>>
>>> The remote-endpoints are reference properties under the
>>> endpoint nodes that are named "remote-endpoint".
>>>
>>> Signed-off-by: Heikki Krogerus
>>> Co-developed-by: Daniel Scally
>>> Signed-off-by: Daniel Scally
>>> ---
>>> changes in v2:
>>> - added software_node_device_is_available
>>> - altered software_node_get_next_child to get references
>>> - altered software_node_get_next_endpoint to release references
>>> to ports and avoid passing invalid combinations of swnodes to
>>> software_node_get_next_child
>>> - altered swnode_graph_find_next_port to release port rather than
>>> old
>>>
>>> drivers/base/swnode.c | 129 +-
>>> 1 file changed, 127 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
>>> index 010828fc785b..d69034b807e3 100644
>>> --- a/drivers/base/swnode.c
>>> +++ b/drivers/base/swnode.c
>>> @@ -363,6 +363,11 @@ static void software_node_put(struct fwnode_handle
>>> *fwnode)
>>> kobject_put(>kobj);
>>> }
>>>
>>> +static bool software_node_device_is_available(const struct fwnode_handle
>>> *fwnode)
>>> +{
>>> + return is_software_node(fwnode);
>> This basically tells whether the device is there. Are there software node
>> based devices, i.e. do you need this?
>>
>> If you do really need this, then I guess this could just return true for
>> now as if you somehow get here, the node is a software node anyway.
>
> I do think its better to include it; I'm targeting using this with
> ipu3-cio2; the cio2_parse_firmware() call there doesn't pass
> FWNODE_GRAPH_DEVICE_DISABLED to fwnode_graph_get_endpoint_by_id() so
> this function does need to exist to be found by that call (or else
> cio2_parse_firmware() would need to pass that flag...but I don't know
> the effect of doing that on devices that aren't using software nodes so
> it didn't seem like a good idea). I can change it to return true though sure
>
>>> +}
>>> +
>>> static bool software_node_property_present(const struct fwnode_handle
>>> *fwnode,
>>>const char *propname)
>>> {
>>> @@ -450,7 +455,7 @@ software_node_get_next_child(const struct fwnode_handle
>>> *fwnode,
>>> c = list_next_entry(c, entry);
>>> else
>>> c = list_first_entry(>children, struct swnode, entry);
>>> - return >fwnode;
>>> + return software_node_get(>fwnode);
>> This looks like a bugfix that probably should or could be backported. Could
>> you make it a separate patch, with a Fixes: tag?
> Yes, sure. That does change how some of the other code would need to
> work though if this patch were applied but not the separated one. Sorry;
> not sure what's the best way to proceed in that case. Should I just note
> that this patch depends on the prior application of the separated one?
I think the assumption is that this individual change to
software_node_property_present() should be in a patch on it's own
preceeding 'this' one.
Running git-blame on drivers/base/swnode.c identifies this line as
previously being added by: 59abd83672f70, so you would add the tag:
Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the
firmware node framework")
to the 'fixing' patch, and that can be backported accordingly.
When patches are sent in a series, the dependency becomes implicit.
You can do this by specifying a range when you do `git format-patch`
If you want to save off the last '2' patches, you can use a range
shorthand of '-2':
for example:
git format-patch -2 -v3 --cover-letter -o patches/swnode
As it's a 'series' we add a cover letter to group them, and that gives a
location to add some free-form text as you wish too.
--
Kieran
>>
>>> }
>>>
>>> static struct fwnode_handle *
>>> @@ -536,9 +541,125 @@ software_node_get_reference_args(const struct
>>> fwnode_handle *fwnode,
>>> return 0;
>>> }
>>>
>>> +static struct fwnode_handle *
>>> +swnode_graph_find_next_port(const struct fwnode_handle
Re: [PATCH 21/29] media: atomisp: Avoid comma separated statements
On 25/08/2020 05:56, Joe Perches wrote:
> Use semicolons and braces.
>
> Signed-off-by: Joe Perches
Reviewed-by: Kieran Bingham
> ---
> drivers/staging/media/atomisp/pci/atomisp_subdev.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index 6ba817f15655..52b9fb18c87f 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -410,8 +410,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
>
> if (atomisp_subdev_format_conversion(isp_sd,
>isp_sd->capture_pad)
> - && crop[pad]->width && crop[pad]->height)
> - crop[pad]->width -= padding_w, crop[pad]->height -=
> padding_h;
> + && crop[pad]->width && crop[pad]->height) {
> + crop[pad]->width -= padding_w;
> + crop[pad]->height -= padding_h;
> + }
>
> /* if subdev type is SOC camera,we do not need to set DVS */
> if (isp->inputs[isp_sd->input_curr].type == SOC_CAMERA)
>
Re: [PATCH 14/29] media: Avoid comma separated statements
On 25/08/2020 05:56, Joe Perches wrote:
> Use semicolons and braces.
>
> Signed-off-by: Joe Perches
> ---
> drivers/media/i2c/msp3400-kthreads.c | 12
> drivers/media/pci/bt8xx/bttv-cards.c | 6 --
> drivers/media/pci/saa7134/saa7134-video.c | 7 +--
> 3 files changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/i2c/msp3400-kthreads.c
> b/drivers/media/i2c/msp3400-kthreads.c
> index d3b0d1c18efd..ac8752c63266 100644
> --- a/drivers/media/i2c/msp3400-kthreads.c
> +++ b/drivers/media/i2c/msp3400-kthreads.c
> @@ -549,8 +549,10 @@ int msp3400c_thread(void *data)
> val = msp_read_dsp(client, 0x1b);
> if (val > 32767)
> val -= 65536;
> - if (val1 < val)
> - val1 = val, max1 = i;
> + if (val1 < val) {
> + val1 = val;
> + max1 = i;
> + }
> dev_dbg_lvl(>dev, 1, msp_debug,
> "carrier1 val: %5d / %s\n", val, cd[i].name);
> }
> @@ -586,8 +588,10 @@ int msp3400c_thread(void *data)
> val = msp_read_dsp(client, 0x1b);
> if (val > 32767)
> val -= 65536;
> - if (val2 < val)
> - val2 = val, max2 = i;
> + if (val2 < val) {
> + val2 = val;
> + max2 = i;
> + }
> dev_dbg_lvl(>dev, 1, msp_debug,
> "carrier2 val: %5d / %s\n", val, cd[i].name);
> }
> diff --git a/drivers/media/pci/bt8xx/bttv-cards.c
> b/drivers/media/pci/bt8xx/bttv-cards.c
> index 16148802dabb..ca20b806e82d 100644
> --- a/drivers/media/pci/bt8xx/bttv-cards.c
> +++ b/drivers/media/pci/bt8xx/bttv-cards.c
> @@ -3934,8 +3934,10 @@ static void osprey_eeprom(struct bttv *btv, const u8
> ee[256])
> if (checksum != ee[21])
> return;
> cardid = BTTV_BOARD_OSPREY1x0_848;
> - for (i = 12; i < 21; i++)
> - serial *= 10, serial += ee[i] - '0';
> + for (i = 12; i < 21; i++) {
> + serial *= 10;
> + serial += ee[i] - '0';
> + }
> }
> } else {
> unsigned short type;
> diff --git a/drivers/media/pci/saa7134/saa7134-video.c
> b/drivers/media/pci/saa7134/saa7134-video.c
> index a8ac94fadc14..f673cda57f30 100644
> --- a/drivers/media/pci/saa7134/saa7134-video.c
> +++ b/drivers/media/pci/saa7134/saa7134-video.c
> @@ -868,8 +868,11 @@ static int buffer_activate(struct saa7134_dev *dev,
> lines_uv = dev->height >> dev->fmt->vshift;
> base2= base + bpl * dev->height;
> base3= base2 + bpl_uv * lines_uv;
> - if (dev->fmt->uvswap)
> - tmp = base2, base2 = base3, base3 = tmp;
> + if (dev->fmt->uvswap) {
> + tmp = base2;
> + base2 = base3;
> + base3 = tmp;
Don't we have any swap macro helpers for this?
Anyway, that would be out of scope of this patch series.
Reviewed-by: Kieran Bingham
> + }
> video_dbg("uv: bpl=%ld lines=%ld base2/3=%ld/%ld\n",
> bpl_uv,lines_uv,base2,base3);
> if (V4L2_FIELD_HAS_BOTH(dev->field)) {
>
Re: [PATCH 08/29] dma-buf: Avoid comma separated statements
Hi Joe,
Nice, I only see three of these on the linux-media list, so I'll only
look at those, but I'm pleased to see this is treewide ;-)
Definitely prefer this.
On 25/08/2020 05:56, Joe Perches wrote:
> Use semicolons and braces.
>
> Signed-off-by: Joe Perches
Reviewed-by: Kieran Bingham
> ---
> drivers/dma-buf/st-dma-fence.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
> index e593064341c8..c8a12d7ad71a 100644
> --- a/drivers/dma-buf/st-dma-fence.c
> +++ b/drivers/dma-buf/st-dma-fence.c
> @@ -471,8 +471,11 @@ static int thread_signal_callback(void *arg)
> dma_fence_signal(f1);
>
> smp_store_mb(cb.seen, false);
> - if (!f2 || dma_fence_add_callback(f2, , simple_callback))
> - miss++, cb.seen = true;
> + if (!f2 ||
> + dma_fence_add_callback(f2, , simple_callback)) {
> + miss++;
> + cb.seen = true;
> + }
>
> if (!t->before)
> dma_fence_signal(f1);
>
Re: [PATCH 1/2] arm64: dts: renesas: r8a774e1: Add FDP1 device nodes
On 27/08/2020 15:53, Lad Prabhakar wrote:
> From: Marian-Cristian Rotariu
>
> Add FDP1 device nodes to R8A774E1 (RZ/G2H) SoC dtsi.
>
> Signed-off-by: Marian-Cristian Rotariu
>
> Signed-off-by: Lad Prabhakar
Reviewed-by: Kieran Bingham
I'd really love to know if people are using this FDP1 with the driver I
added ;-)
> ---
> arch/arm64/boot/dts/renesas/r8a774e1.dtsi | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> index f5909ced7679..34fdb9a0c325 100644
> --- a/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a774e1.dtsi
> @@ -2504,6 +2504,26 @@
> renesas,fcp = <>;
> };
>
> + fdp1@fe94 {
> + compatible = "renesas,fdp1";
> + reg = <0 0xfe94 0 0x2400>;
> + interrupts = ;
> + clocks = < CPG_MOD 119>;
> + power-domains = < R8A774E1_PD_A3VP>;
> + resets = < 119>;
> + renesas,fcp = <>;
> + };
> +
> + fdp1@fe944000 {
> + compatible = "renesas,fdp1";
> + reg = <0 0xfe944000 0 0x2400>;
> + interrupts = ;
> + clocks = < CPG_MOD 118>;
> + power-domains = < R8A774E1_PD_A3VP>;
> + resets = < 118>;
> + renesas,fcp = <>;
> + };
> +
> fcpf0: fcp@fe95 {
> compatible = "renesas,fcpf";
> reg = <0 0xfe95 0 0x200>;
>
Re: [PATCH v3 0/9] media: vimc: Multiple stream support in vimc
Hi Kaaira, Thank you for this series. I have tested it, and indeed it works well, which is great work. I know this has been hard to debug some of the code paths! There are a few bits that are hard for me to understand, with the graph walking/initialisation - so I think either some better comments or refactoring might help there - and Dafna has suggested that there might be a useful helper which will assist too. That needs to be checked though, as I think your 'streamer' needs to have full visibility of the whole pipeline, rather than just a single stream. I wonder if it would help to rename it to make that clearer, as 'vimc_stream' sounds like it deals with only a single stream - but now it deals with multiple - so the naming is a bit confusing. -- Kieran On 19/08/2020 19:04, Kaaira Gupta wrote: > This series adds supoort for two (or more) capture devices to be > connected to the same sensors and run simultaneously. > > Changes since v2: > - This series introduces new patches, namely patch 1, 2, 4, 5, > 7, and 9 to shift multiple captures to operate at a single > thread. > > Kaaira Gupta (7): > media: vimc: Move get_source_entity to vimc-common > media: vimc: Add get_frame callback > media: vimc: Separate starting stream from pipeline initialisation > media: vimc: Separate closing of stream and thread > media: vimc: Dynamically allocate stream struct > media: vimc: Join pipeline if one already exists > media: vimc: Run multiple captures on same thread > > Niklas Söderlund (2): > media: vimc: Add usage count to subdevices > media: vimc: Serialize vimc_streamer_s_stream() > > .../media/test-drivers/vimc/vimc-capture.c| 42 +++- > drivers/media/test-drivers/vimc/vimc-common.c | 14 ++ > drivers/media/test-drivers/vimc/vimc-common.h | 21 +- > .../media/test-drivers/vimc/vimc-debayer.c| 26 ++- > drivers/media/test-drivers/vimc/vimc-scaler.c | 25 +- > drivers/media/test-drivers/vimc/vimc-sensor.c | 17 +- > .../media/test-drivers/vimc/vimc-streamer.c | 213 -- > .../media/test-drivers/vimc/vimc-streamer.h | 2 + > 8 files changed, 271 insertions(+), 89 deletions(-) > -- Regards -- Kieran
Re: [PATCH v3 9/9] media: vimc: Run multiple captures on same thread
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> If multiple captures try to enable stream, start their stream but do not
> initialise the pipeline again. Also, don't start the thread separately.
> Starting their streams will update the use count and their frames would
> be processed by the already running thread.
>
> Signed-off-by: Kaaira Gupta
> ---
> drivers/media/test-drivers/vimc/vimc-streamer.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
> b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index fade37bee26d..880c31759cc0 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -275,13 +275,14 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> return ret;
>
> if (enable) {
> - if (stream->kthread)
> - return 0;
>
> ret = vimc_streamer_stream_start(ved);
> if (ret)
> goto out;
>
> + if (stream->kthread)
> + goto out;
> +
This goto out makes it look like it's an error path. So that probably
warrants a comment along the lines of 'don't reinitialise the pipeline
if it has already started'. ?
I wonder if it's better to handle the pipeline_init during _stream_start
'only' in the code path where it's the first stream ?
Then similarly, the pipeline_terminate would happen on stream_stop
'only' when it's the last stream.
Or I guess that is handled by the refcount ... Hrm it would be nice to
be able to make/keep it symmetrical somehow.
> ret = vimc_streamer_pipeline_init(stream, ved);
> if (ret)
> goto out;
>
--
Regards
--
Kieran
Re: [PATCH v3 8/9] media: vimc: Join pipeline if one already exists
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> Start another capture, if one is already running, by checking for
> existing pipe. If it exists already, don't fail to start second capture,
> instead join it to the already running pipeline.
> Use the same stream struct used by already running capture.
>
> Signed-off-by: Kaaira Gupta
> ---
> drivers/media/test-drivers/vimc/vimc-capture.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c
> b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 73e5bdd17c57..4d20eda9335e 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -247,9 +247,15 @@ static int vimc_cap_start_streaming(struct vb2_queue
> *vq, unsigned int count)
> atomic_inc(>ved.use_count);
> vcap->sequence = 0;
>
> - stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> - kref_init(>refcount);
> - pipe = >pipe;
> + if (vcap->ved.ent->pipe) {
> + pipe = vcap->ved.ent->pipe;
> + stream = container_of(pipe, struct vimc_stream, pipe);
> + kref_get(>refcount);
> + } else {
> + stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> + kref_init(>refcount);
> + pipe = >pipe;
> + }
>
Of course if we move the stream to the sensor entity (which I still
think is a good idea), we'll need a way to easily get from capture
entity to the sensor entity. We could put a reference pointer directly
in the capture ? Or have each entity provide a poitner to it's previous
entity and walk backwards, which would also help on the other code path
that had to do lots of conversions from video or subdev entities or such...
> /* Start the media pipeline */
> ret = media_pipeline_start(entity, pipe);
>
--
Regards
--
Kieran
Re: [PATCH v3 7/9] media: vimc: Dynamically allocate stream struct
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> Multiple streams will share same stream struct if we want them to run on
> same thread. So remove it from vimc_cap struct so that it doesn't get
> destroyed when one of the capture does, and allocate it memory
> dynamically. Use kref with it as it will be used by multiple captures.
>
Is the vimc_stream stuct the context of the streamer? or of each 'stream'?
If it's the streamer - then can't we store this (non-dynamically) as
part of the Sensor node, to avoid kzalloc/freeing it ?
> Signed-off-by: Kaaira Gupta
> ---
> drivers/media/test-drivers/vimc/vimc-capture.c | 15 +++
> drivers/media/test-drivers/vimc/vimc-streamer.c | 17 ++---
> drivers/media/test-drivers/vimc/vimc-streamer.h | 2 ++
> 3 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c
> b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 93418cb5a139..73e5bdd17c57 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -28,7 +28,6 @@ struct vimc_cap_device {
> spinlock_t qlock;
> struct mutex lock;
> u32 sequence;
> - struct vimc_stream stream;
> struct media_pad pad;
> };
>
> @@ -241,19 +240,25 @@ static int vimc_cap_start_streaming(struct vb2_queue
> *vq, unsigned int count)
> {
> struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> struct media_entity *entity = >vdev.entity;
> + struct media_pipeline *pipe = NULL;
> + struct vimc_stream *stream;
> int ret;
>
> atomic_inc(>ved.use_count);
> vcap->sequence = 0;
>
> + stream = kzalloc(sizeof(*stream), GFP_ATOMIC);
> + kref_init(>refcount);
> + pipe = >pipe;
> +
> /* Start the media pipeline */
> - ret = media_pipeline_start(entity, >stream.pipe);
> + ret = media_pipeline_start(entity, pipe);
> if (ret) {
> vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> return ret;
> }
>
> - ret = vimc_streamer_s_stream(>stream, >ved, 1);
> + ret = vimc_streamer_s_stream(stream, >ved, 1);
> if (ret) {
> media_pipeline_stop(entity);
> vimc_cap_return_all_buffers(vcap, VB2_BUF_STATE_QUEUED);
> @@ -270,9 +275,11 @@ static int vimc_cap_start_streaming(struct vb2_queue
> *vq, unsigned int count)
> static void vimc_cap_stop_streaming(struct vb2_queue *vq)
> {
> struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
> + struct media_pipeline *pipe = vcap->ved.ent->pipe;
> + struct vimc_stream *stream = container_of(pipe, struct vimc_stream,
> pipe);
In fact, I see it's stored as part of the 'pipe' so there is one
vimc_stream per pipeline ...
So it could just be a structure on the pipe rather than obtaining a
pointer here.
I think it's probably 'ok' to have one streamer per pipe currently as
the raw input node is not functional, but I also thought that by having
the streamer as part of the sensor entity then there is one streamer
('one thread') per video source ... which would prevent this having to
be changed if someone later deals with the video node that allows
re-processing raw frames ?
> atomic_dec(>ved.use_count);
> - vimc_streamer_s_stream(>stream, >ved, 0);
> + vimc_streamer_s_stream(stream, >ved, 0);
>
> /* Stop the media pipeline */
> media_pipeline_stop(>vdev.entity);
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
> b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index f5c9e2f3bbcb..fade37bee26d 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -20,18 +20,13 @@
> * Erases values of stream struct and terminates the thread
> *
> */
> -static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> +static void vimc_streamer_pipeline_terminate(struct kref *ref)
> {
> - struct vimc_ent_device *ved;
> -
> - while (stream->pipe_size) {
> - stream->pipe_size--;
> - ved = stream->ved_pipeline[stream->pipe_size];
> - stream->ved_pipeline[stream->pipe_size] = NULL;
> - }
> + struct vimc_stream *stream = container_of(ref, struct vimc_stream,
> refcount);
>
> kthread_stop(stream->kthread);
> stream->kthread = NULL;
> + kfree(stream);
> }
>
> /**
> @@ -202,7 +197,7 @@ static int vimc_streamer_pipeline_init(struct vimc_stream
> *stream,
> }
>
> vimc_streamer_stream_terminate(cved);
> - vimc_streamer_pipeline_terminate(stream);
> + kref_put(>refcount, vimc_streamer_pipeline_terminate);
> return -EINVAL;
> }
>
> @@ -298,7 +293,7 @@ int vimc_streamer_s_stream(struct vimc_stream *stream,
> ret = PTR_ERR(stream->kthread);
> dev_err(ved->dev, "kthread_run failed with %d\n", ret);
>
Re: [PATCH v3 5/9] media: vimc: Separate closing of stream and thread
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> Make separate functions for stopping streaming of entities in path of a
> particular stream and stopping thread. This is needed to ensure that
> thread doesn't stop when one device stops streaming in case of multiple
> streams.
>
> Signed-off-by: Kaaira Gupta
> ---
> .../media/test-drivers/vimc/vimc-streamer.c | 82 ---
> 1 file changed, 52 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
> b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index cc40ecabe95a..6b5ea1537952 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -13,29 +13,59 @@
> #include "vimc-streamer.h"
>
> /**
> - * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
> + * vimc_streamer_pipeline_terminate - Terminate the thread
> *
> - * @stream: the pointer to the stream structure with the pipeline to be
> - * disabled.
> + * @stream: the pointer to the stream structure
> *
> - * Calls s_stream to disable the stream in each entity of the pipeline
> + * Erases values of stream struct and terminates the thread
It would help if the function brief described it's purpose rather than
'what it does'. "Erases values of stream struct" is not helpful here, as
that's just a direct read of what happens in the code.
I'm guessing here, but how about:
"Tear down all resources belonging to the pipeline when there are no
longer any active streams being used. This includes stopping the active
processing thread"
But reading my text makes me worry about the ordering that might take
place. The thread should be stopped as soon as the last stream using it
is stopped. Presumably as the 'first thing' that happens to make sure
there is no concurrent processing while the stream is being disabled.
Hopefully there's no race condition ...
> *
> */
> static void vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
> {
> struct vimc_ent_device *ved;
> - struct v4l2_subdev *sd;
>
> while (stream->pipe_size) {
> stream->pipe_size--;
> ved = stream->ved_pipeline[stream->pipe_size];
> stream->ved_pipeline[stream->pipe_size] = NULL;
> + }
>
> - if (!is_media_entity_v4l2_subdev(ved->ent))
> - continue;
> + kthread_stop(stream->kthread);
> + stream->kthread = NULL;
> +}
>
> - sd = media_entity_to_v4l2_subdev(ved->ent);
> - v4l2_subdev_call(sd, video, s_stream, 0);
> +/**
> + * vimc_streamer_stream_terminate - Disable stream in all ved in stream
> + *
> + * @ved: pointer to the ved for which stream needs to be disabled
> + *
> + * Calls s_stream to disable the stream in each entity of the stream
> + *
> + */
> +static void vimc_streamer_stream_terminate(struct vimc_ent_device *ved)
I would call this vimc_streamer_stream_stop to match
vimc_streamer_stream_start() rather than terminate...
> +{
> + struct media_entity *entity = ved->ent;
> + struct video_device *vdev;
> + struct v4l2_subdev *sd;
> +
> + while (entity) {
> + if (is_media_entity_v4l2_subdev(ved->ent)) {
> + sd = media_entity_to_v4l2_subdev(ved->ent);
> + v4l2_subdev_call(sd, video, s_stream, 0);
> + }
> + entity = vimc_get_source_entity(ved->ent);
> + if (!entity)
> + break;
> +
> + if (is_media_entity_v4l2_subdev(entity)) {
> + sd = media_entity_to_v4l2_subdev(entity);
> + ved = v4l2_get_subdevdata(sd);
> + } else {
> + vdev = container_of(entity,
> + struct video_device,
> + entity);
> + ved = video_get_drvdata(vdev);
> + }
It looks like this is walking back through the linked graph, calling
s_stream(0) right?
I wonder if struct vimc_ent_device should have a pointer to the entity
it's connected to, to simplify this. ... presumably this is done
elsewhere too?
Although then that's still walking 'backwards' rather than forwards...
> }
> }
>
> @@ -43,25 +73,25 @@ static void vimc_streamer_pipeline_terminate(struct
> vimc_stream *stream)
> * vimc_streamer_stream_start - Starts streaming for all entities
> * in a stream
> *
> - * @ved:the pointer to the vimc entity initializing the stream
> + * @cved:the pointer to the vimc entity initializing the stream
> *
> * Walks through the entity graph to call vimc_streamer_s_stream()
> * to enable stream in all entities in path of a stream.
> *
> * Return: 0 if success, error code otherwise.
> */
> -static int vimc_streamer_stream_start(struct vimc_stream *stream,
> - struct vimc_ent_device *ved)
> +static
Re: [PATCH v3 4/9] media: vimc: Separate starting stream from pipeline initialisation
Hi Kaaira, Dafna,
On 28/08/2020 21:37, Dafna Hirschfeld wrote:
> Hi,
>
> Am 21.08.20 um 23:01 schrieb Kaaira Gupta:
>> Hi,
>>
>> On Fri, Aug 21, 2020 at 05:11:23PM +0200, Dafna Hirschfeld wrote:
>>>
>>>
>>> Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
Separate the process of initialising pipeline array from starting
streaming for all entities in path of a stream. This is needed because
multiple streams can stream, but only one pipeline object is needed.
Process frames only for those entities in a pipeline which are
streaming. This is known through their use counts.
Signed-off-by: Kaaira Gupta
---
.../media/test-drivers/vimc/vimc-streamer.c | 95
---
1 file changed, 83 insertions(+), 12 deletions(-)
diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
b/drivers/media/test-drivers/vimc/vimc-streamer.c
index c1644d69686d..cc40ecabe95a 100644
--- a/drivers/media/test-drivers/vimc/vimc-streamer.c
+++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
@@ -40,33 +40,30 @@ static void
vimc_streamer_pipeline_terminate(struct vimc_stream *stream)
}
/**
- * vimc_streamer_pipeline_init - Initializes the stream structure
+ * vimc_streamer_stream_start - Starts streaming for all entities
+ * in a stream
*
- * @stream: the pointer to the stream structure to be initialized
* @ved: the pointer to the vimc entity initializing the stream
*
- * Initializes the stream structure. Walks through the entity graph to
- * construct the pipeline used later on the streamer thread.
- * Calls vimc_streamer_s_stream() to enable stream in all entities of
- * the pipeline.
+ * Walks through the entity graph to call vimc_streamer_s_stream()
+ * to enable stream in all entities in path of a stream.
*
* Return: 0 if success, error code otherwise.
*/
-static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
- struct vimc_ent_device *ved)
+static int vimc_streamer_stream_start(struct vimc_stream *stream,
+ struct vimc_ent_device *ved)
{
struct media_entity *entity;
struct video_device *vdev;
struct v4l2_subdev *sd;
+ int stream_size = 0;
int ret = 0;
- stream->pipe_size = 0;
- while (stream->pipe_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
+ while (stream_size < VIMC_STREAMER_PIPELINE_MAX_SIZE) {
if (!ved) {
vimc_streamer_pipeline_terminate(stream);
return -EINVAL;
}
- stream->ved_pipeline[stream->pipe_size++] = ved;
if (is_media_entity_v4l2_subdev(ved->ent)) {
sd = media_entity_to_v4l2_subdev(ved->ent);
@@ -104,6 +101,73 @@ static int vimc_streamer_pipeline_init(struct
vimc_stream *stream,
entity);
ved = video_get_drvdata(vdev);
}
+ stream_size++;
+ }
+
+ vimc_streamer_pipeline_terminate(stream);
+ return -EINVAL;
+}
+
+/**
+ * vimc_streamer_pipeline_init - Initialises pipeline and pipe size
+ *
+ * @stream: the pointer to the stream structure
+ * @ved: the pointer to the vimc entity initializing the stream
pipeline
Which entity is this? Is it the start, or the end of the pipeline? I.e.
the sensor? or the capture ?
+ *
+ * Walks through the entity graph to initialise ved_pipeline and
updates
+ * pipe_size too.
+ *
+ * Return: 0 if success, error code otherwise.
+ */
+static int vimc_streamer_pipeline_init(struct vimc_stream *stream,
+ struct vimc_ent_device *ved)
+{
+ struct media_entity *entity;
+ struct media_device *mdev;
+ struct media_graph graph;
+ struct video_device *vdev;
+ struct v4l2_subdev *sd;
+ int ret;
+
+ entity = ved->ent;
+ mdev = entity->graph_obj.mdev;
+
+ ret = media_graph_walk_init(, mdev);
+ if (ret)
+ return ret;
+
+ media_graph_walk_start(, entity);
+
+ /*
+ * Start pipeline array initialisation from RAW Capture only to
get
+ * entities in the correct order of their frame processing.
+ */
+ if (!strncmp(entity->name, "RGB", 3)) {
>>>
>>> I don't understand this condition, way is it good for?
>>
>> I have explained that later in the reply
Matching on entity names is a bit awkward, as they could be changed I
guess quite easily, and easily missed in this string matching.
I haven't fully understood this code block yet to work out if there's
another way we could do this though, but reading ahead I see
Re: [PATCH] MAINTAINERS: Fix sort order for RDACM20
Hi Mauro,
I see the fixes branch is open now ... could you pick this one up
please? Or do I need to send a pull request?
--
Regards
Kieran
On 16/07/2020 11:25, Kieran Bingham wrote:
> The files maintained as part of the RDACM20 were incorrectly sorted
> while they were added.
>
> Correct the sort-order.
>
> Fixes: 874a93adf972 ("media: i2c: Add RDACM20 driver")
> Signed-off-by: Kieran Bingham
> ---
> MAINTAINERS | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 373e4198d2b1..8bd8d4ab8b5f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14412,9 +14412,9 @@ M:Niklas Söderlund
>
> L: linux-me...@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/media/i2c/imi,rdacm2x-gmsl.yaml
> -F: drivers/media/i2c/rdacm20.c
> F: drivers/media/i2c/max9271.c
> F: drivers/media/i2c/max9271.h
> +F: drivers/media/i2c/rdacm20.c
>
> RDC R-321X SoC
> M: Florian Fainelli
>
--
Regards
--
Kieran
Re: [PATCH v5 9/9] arm64: dts: renesas: salvator: add a connection from adv748x codec (HDMI input) to the R-Car SoC
Hi Alex,
On 18/06/2020 17:32, Kieran Bingham wrote:
> Hi Alex,
>
> On 02/04/2020 19:35, Alex Riesen wrote:
>> As all known variants of the Salvator board have the HDMI decoder
>> chip (the ADV7482) connected to the SSI4 on R-Car SoC, the ADV7482
>> endpoint and the connection definitions are placed in the common board
>> file.
>>
>> For the same reason, the CLK_C clock line and I2C configuration (similar
>> to the ak4613, on the same interface) are added into the common file.
>>
>> Signed-off-by: Alexander Riesen
>>
>> --
>>
>> v5: Add dummy ssi4 node to the rcar sound card in r8a77961, as the
>> devices (Salvator-X 2nd version with R-Car M3 W+) also reference
>> salvator-common.dtsi.
>> Suggested-by: Geert Uytterhoeven
>>
>> v2: Also add ssi4_ctrl pin group in the sound pins. The pins are
>> responsible for SCK4 (sample clock) WS4 and (word boundary input),
>> and are required for SSI audio input over I2S.
>>
>> The adv748x shall provide its own implementation of the output clock
>> (MCLK), connected to the audio_clk_c line of the R-Car SoC.
>>
>> If the frequency of the ADV748x MCLK were fixed, the clock
>> implementation were not necessary, but it does not seem so: the MCLK
>> depends on the value in a speed multiplier register and the input sample
>> rate (48kHz).
>>
>> Remove audio clock C from the clocks of adv7482.
>>
>> The clocks property of the video-receiver node lists the input
>> clocks of the device, which is quite the opposite from the
>> original intention: the adv7482 on Salvator X boards is a
>> provide of the MCLK clock for I2S audio output.
>>
>> Remove old definition of _card.dais and reduce size of changes
>> in the Salvator-X specific device tree source.
>>
>> Declare video-receiver a clock producer, as the adv748x driver
>> implements the master clock used I2S audio output.
>>
>> Suggested-by: Geert Uytterhoeven
>>
>> v2: The driver provides only MCLK clock, not the SCLK and LRCLK,
>> which are part of the I2S protocol.
>>
>> Suggested-by: Laurent Pinchart
>> ---
>> .../boot/dts/renesas/r8a77950-salvator-x.dts | 3 +-
>> arch/arm64/boot/dts/renesas/r8a77961.dtsi | 1 +
>> .../boot/dts/renesas/salvator-common.dtsi | 47 +--
Once again I'm back trying to test this series, and one issue I've had
is that the board I have (r8a77951-salvator-xs.dts) isn't included in
this DT update.
For v6, Should we include the relevant changes to all the following?
arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77951-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77960-salvator-x.dts
arch/arm64/boot/dts/renesas/r8a77965-salvator-x.dts
arch/arm64/boot/dts/renesas/salvator-x.dtsi
And perhaps handle the salvator-xs in a second (yet very similar) patch?
arch/arm64/boot/dts/renesas/r8a77951-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77960-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77961-salvator-xs.dts
arch/arm64/boot/dts/renesas/r8a77965-salvator-xs.dts
arch/arm64/boot/dts/renesas/salvator-xs.dtsi
I think I've added the relevant entries to my dtb, but I haven't
successfully captured audio yet.
I can see the device being listed through arecord:
kbingham@salvator-xs:~$ arecord -l
List of CAPTURE Hardware Devices
card 0: rcarsound [rcar-sound], device 0: rsnd-dai.0-ak4613-hifi
ak4613-hifi-0 []
Subdevices: 0/1
Subdevice #0: subdevice #0
card 0: rcarsound [rcar-sound], device 3: rsnd-dai.3-adv748x-i2s
adv748x.4-0070-3 []
Subdevices: 1/1
Subdevice #0: subdevice #0
But as yet, everything I try to record fails or is empty silence.
Debugging ...
--
Regards
Kieran
>> 3 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> index 2438825c9b22..e16c146808b6 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a77950-salvator-x.dts
>> @@ -146,7 +146,8 @@ {
>> _card {
>> dais = <_port0 /* ak4613 */
>> _port1 /* HDMI0 */
>> -_port2>; /* HDMI1 */
>> +_port2 /* HDMI1 */
>> +_port3>; /* adv7482 hdmi-in */
>
> Ah - that was confusing at first... but HDMI0 and HDMI1 are *outputs*,
> where of course the adv7482 is an input ;-)
>
>
> Otherwise, I can't spot anything else yet so:
>
> Reviewed-by: Kieran Bingham
>
Re: [PATCH 0/2][next] update gdb scripts for lockless printk ringbuffer
Hi Petr, On 21/08/2020 09:55, Jan Kiszka wrote: > On 21.08.20 10:08, Petr Mladek wrote: >> On Fri 2020-08-14 23:31:23, John Ogness wrote: >>> Hi, >>> >>> When we brought in the new lockless printk ringbuffer, we overlooked the gdb >>> scripts. Here are a set of patches to implement gdb support for the new >>> ringbuffer. >>> >>> John Ogness (2): >>> scripts/gdb: add utils.read_ulong() >>> scripts/gdb: update for lockless printk ringbuffer >> >> I am not fluent in the gdb macros and python so I did not try any >> deep review. But both patches work for me: >> >> Tested-by: Petr Mladek >> >> I am going to give it few more days before pushing just in case there is >> another feedback. Before 'pushing' ? What context do you mean here? These patches go through AKPM don't they? Did I miss some update to procedures? >> > > Thanks, perfect! > >> Best Regards, >> Petr >> >> BTW: Are you aware of https://github.com/crash-python/crash-python project? >> The ambition is to implement "crash" features and even more into >> gdb using python extensions. It similar approach like >> scripts/gdb/ >> >> crash-python is being developed sporadically in waves. It is >> mostly during a week that is dedicated for such projects >> at SUSE. It would be great to get more contributors. >> > > The problem with all those out-of-tree kernel debugging projects is that > they are even further away from the changes in upstream they need to > adjust to over and over again. It's already hard for scripts/gdb to keep > up as only few people are aware of the dependencies, and it's always > easy to forget. But it's getting better and better here IMHO. And that > was my idea behind pushing things into upstream. Seconding that, and having also investigated the crash projects in previous work. I really wish there was more alignment to the kernel ;-) /me ponders what happened to the kernel awareness integrations on GDB... I know others have been investigating too, If only I had time ... > Jan > -- -- Kieran
Re: [PATCH v3 2/9] media: vimc: Add get_frame callback
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> In the process of making vimc compatible for multiple streams, we need
> to create a frame passing process such that two different entities can
> get the frame from a common entity. This isn't possible currently without
> calling process_frame twice for the common entity, as process_frames
> returns the frame which gets passed on.
>
> So, to take care of this, add a get_frame callback to vimc device and
> use it to get the frames for an entity from previous entity instead of
> returning and passing the frames as an argument in process_frame.
>
> Signed-off-by: Kaaira Gupta
> ---
> .../media/test-drivers/vimc/vimc-capture.c| 18 +++---
> drivers/media/test-drivers/vimc/vimc-common.h | 7 ---
> .../media/test-drivers/vimc/vimc-debayer.c| 19 ---
> drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++---
> drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +--
> .../media/test-drivers/vimc/vimc-streamer.c | 10 ++
> 6 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c
> b/drivers/media/test-drivers/vimc/vimc-capture.c
> index c63496b17b9a..a8cbb8e4d5ba 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device
> *ved)
> video_unregister_device(>vdev);
> }
>
> -static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
> - const void *frame)
> +static int vimc_cap_process_frame(struct vimc_ent_device *ved)
> {
> struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
> ved);
> struct vimc_cap_buffer *vimc_buf;
> + struct v4l2_subdev *sd;
> + const void *frame;
> void *vbuf;
>
> spin_lock(>qlock);
> @@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct
> vimc_ent_device *ved,
> typeof(*vimc_buf), list);
> if (!vimc_buf) {
> spin_unlock(>qlock);
> - return ERR_PTR(-EAGAIN);
> + return -EAGAIN;
> }
>
> /* Remove this entry from the list */
> @@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct
> vimc_ent_device *ved,
>
> vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
>
> + sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
> + ved = v4l2_get_subdevdata(sd);
> + frame = ved->get_frame(ved);
Hrm, this code block is used in several places throughout this patch,
and it aliases the function parameter ved to a new device which isn't
nice. Not a problem as long as it's not used for the original VED after
of course.
But I wonder if we should instead add a helper into vimc-common.c:
struct vimc_ent_device *vimc_get_source_ved(struct vimc_ent_device *ved)
{
struct media_entity *ent;
struct v4l2_subdev *sd;
ent = vimc_get_source_entity(ved->ent);
if (!ent)
return NULL;
sd = media_entity_to_v4l2_subdev(ent);
return v4l2_get_subdevdata(sd);
}
It might not be necessary though, just an idea. If you like it, it can
be a patch on it's own after the vimc_get_source_entity() moving patch.
But it does show that vimc_get_source_entity() can return NULL which
might have to be checked... though perhaps we 'know' it will always be
valid ...
Also, following the links for each entity, for each frame sounds like
quite a lot of work. I wonder if the active source entity should be
cached in each VED ...
That could be done on top anyway...
Overall, this looks like it will work, so with comments addressed how
you wish,
Reviewed-by: Kieran Bingham
> +
> memcpy(vbuf, frame, vcap->format.sizeimage);
>
> /* Set it as ready */
> vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
> vcap->format.sizeimage);
> vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
> +
> + return 0;
> +}
> +
> +static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
> +{
> return NULL;
> }
>
> @@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct
> vimc_device *vimc,
> vcap->ved.ent = >vdev.entity;
> vcap->ved.process_frame = vimc_cap_process_frame;
> vcap->ved.vdev_get_format = vimc_cap_get_format;
> + vcap->ved.get_frame = vimc_cap_get_frame;
> vcap->ved.dev = vimc->mdev.dev;
>
> /* Initialize the video_device struct */
> diff -
Re: [PATCH v3 1/9] media: vimc: Move get_source_entity to vimc-common
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
> Move the function vimc_get_source_entity() to vimc-common.c to make it
> reusable.
>
> Signed-off-by: Kaaira Gupta
Reviewed-by: Kieran Bingham
> ---
> drivers/media/test-drivers/vimc/vimc-common.c | 14 +++
> drivers/media/test-drivers/vimc/vimc-common.h | 12 ++
> .../media/test-drivers/vimc/vimc-streamer.c | 24 ---
> 3 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.c
> b/drivers/media/test-drivers/vimc/vimc-common.c
> index 0d97b25ce21e..91c8992bb391 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.c
> +++ b/drivers/media/test-drivers/vimc/vimc-common.c
> @@ -417,3 +417,17 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> media_entity_cleanup(>entity);
> return ret;
> }
> +
> +struct media_entity *vimc_get_source_entity(struct media_entity *ent)
> +{
> + struct media_pad *pad;
> + int i;
> +
> + for (i = 0; i < ent->num_pads; i++) {
> + if (ent->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> + continue;
> + pad = media_entity_remote_pad(>pads[i]);
> + return pad ? pad->entity : NULL;
> + }
> + return NULL;
> +}
> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h
> b/drivers/media/test-drivers/vimc/vimc-common.h
> index a289434e75ba..4c580d854007 100644
> --- a/drivers/media/test-drivers/vimc/vimc-common.h
> +++ b/drivers/media/test-drivers/vimc/vimc-common.h
> @@ -230,4 +230,16 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
> */
> int vimc_vdev_link_validate(struct media_link *link);
>
> +/**
> + * vimc_get_source_entity - get the entity connected with the first sink pad
> + *
> + * @ent: reference media_entity
> + *
> + * Helper function that returns the media entity containing the source pad
> + * linked with the first sink pad from the given media entity pad list.
> + *
> + * Return: The source pad or NULL, if it wasn't found.
> + */
> +struct media_entity *vimc_get_source_entity(struct media_entity *ent);
> +
> #endif
> diff --git a/drivers/media/test-drivers/vimc/vimc-streamer.c
> b/drivers/media/test-drivers/vimc/vimc-streamer.c
> index 451a32c0d034..4f8384246042 100644
> --- a/drivers/media/test-drivers/vimc/vimc-streamer.c
> +++ b/drivers/media/test-drivers/vimc/vimc-streamer.c
> @@ -12,30 +12,6 @@
>
> #include "vimc-streamer.h"
>
> -/**
> - * vimc_get_source_entity - get the entity connected with the first sink pad
> - *
> - * @ent: reference media_entity
> - *
> - * Helper function that returns the media entity containing the source pad
> - * linked with the first sink pad from the given media entity pad list.
> - *
> - * Return: The source pad or NULL, if it wasn't found.
> - */
> -static struct media_entity *vimc_get_source_entity(struct media_entity *ent)
> -{
> - struct media_pad *pad;
> - int i;
> -
> - for (i = 0; i < ent->num_pads; i++) {
> - if (ent->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> - continue;
> - pad = media_entity_remote_pad(>pads[i]);
> - return pad ? pad->entity : NULL;
> - }
> - return NULL;
> -}
> -
> /**
> * vimc_streamer_pipeline_terminate - Disable stream in all ved in stream
> *
>
--
Regards
--
Kieran
Re: linux-next: Tree for Aug 13 (drivers/media/i2c/max9286.c)
Hi Randy, On 13/08/2020 19:35, Randy Dunlap wrote: > On 8/12/20 11:58 PM, Stephen Rothwell wrote: >> Hi all, >> >> News: The merge window has opened, so please do not add any v5.10 >> related material to your linux-next included branches until after the >> merge window closes again. >> >> Changes since 20200812: >> > > on x86_64: > > # CONFIG_GPIOLIB is not set > > ../drivers/media/i2c/max9286.c: In function 'max9286_register_gpio': > ../drivers/media/i2c/max9286.c:1033:6: error: 'struct gpio_chip' has no > member named 'of_node' > gpio->of_node = dev->of_node; > Thanks for the report, Sakari has already submitted a fix [0] for this. I assume it just needs a nudge to get picked up via Mauro's tree. [0] https://lore.kernel.org/linux-media/20200803090935.23619-1-sakari.ai...@linux.intel.com/ Thanks -- Kieran
Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
Hi Kaaira, On 31/07/2020 18:22, Kaaira Gupta wrote: > Hi everyone, > > On Wed, Jul 29, 2020 at 05:24:25PM +0200, Dafna Hirschfeld wrote: >> >> >> On 29.07.20 15:27, Kieran Bingham wrote: >>> Hi Dafna, Kaaira, >>> >>> On 29/07/2020 14:16, Dafna Hirschfeld wrote: >>>> >>>> >>>> On 29.07.20 15:05, Kieran Bingham wrote: >>>>> Hi Dafna, >>>>> >>>>> On 28/07/2020 15:00, Dafna Hirschfeld wrote: >>>>>> >>>>>> >>>>>> On 28.07.20 14:07, Dafna Hirschfeld wrote: >>>>>>> Hi >>>>>>> >>>>>>> On 28.07.20 13:39, Kaaira Gupta wrote: >>>>>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote: >>>>>>>>>> Hi all, >>>>>>>>>> >>>>>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc >>>>>>>>>> list. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote: >>>>>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>>> Hi Kaaira, >>>>>>>>>>>> >>>>>>>>>>>> Thanks for your work. >>>>>>>>>>> >>>>>>>>>>> Thanks for yours :D >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote: >>>>>>>>>>>>> This is version 2 of the patch series posted by Niklas for >>>>>>>>>>>>> allowing >>>>>>>>>>>>> multiple streams in VIMC. >>>>>>>>>>>>> The original series can be found here: >>>>>>>>>>>>> https://patchwork.kernel.org/cover/10948831/ >>>>>>>>>>>>> >>>>>>>>>>>>> This series adds support for two (or more) capture devices to be >>>>>>>>>>>>> connected to the same sensors and run simultaneously. Each >>>>>>>>>>>>> capture device >>>>>>>>>>>>> can be started and stopped independent of each other. >>>>>>>>>>>>> >>>>>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once >>>>>>>>>>>>> two >>>>>>>>>>>>> capture devices can be part of the same pipeline. While 3/3 >>>>>>>>>>>>> allows for >>>>>>>>>>>>> two capture devices to be part of the same pipeline and thus >>>>>>>>>>>>> allows for >>>>>>>>>>>>> simultaneously use. >>>>>>> >>>>>>> I wonder if these two patches are enough, since each vimc entity also >>>>>>> have >>>>>>> a 'process_frame' callback, but only one allocated frame. That means >>>>>>> that the 'process_frame' can be called concurrently by two different >>>>>>> streams >>>>>>> on the same frame and cause corruption. >>>>>>> >>>>>> >>>>>> I think we should somehow change the vimc-stream.c code so that we have >>>>>> only >>>>>> one stream process per pipe. So if one capture is already streaming, >>>>>> then the new >>>>>> capture that wants to stream uses the same thread so we don't have two >>>>>> threads >>>>>> both calling 'process_frame'. >>>>> >>>>> >>>>> Yes, I think it looks and sounds like there are two threads running when >>>>> there are two streams. >>>>> >>>>> so in effect, although they 'share a pipe', aren't they in effect just >>>>> sending two separate buffers thr
Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
Hi Dafna, Kaaira, On 29/07/2020 14:16, Dafna Hirschfeld wrote: > > > On 29.07.20 15:05, Kieran Bingham wrote: >> Hi Dafna, >> >> On 28/07/2020 15:00, Dafna Hirschfeld wrote: >>> >>> >>> On 28.07.20 14:07, Dafna Hirschfeld wrote: >>>> Hi >>>> >>>> On 28.07.20 13:39, Kaaira Gupta wrote: >>>>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote: >>>>>> Hi, >>>>>> >>>>>> On 7/27/20 11:31 AM, Kieran Bingham wrote: >>>>>>> Hi all, >>>>>>> >>>>>>> +Dafna for the thread discussion, as she's missed from the to/cc >>>>>>> list. >>>>>>> >>>>>>> >>>>>>> On 24/07/2020 13:21, Kaaira Gupta wrote: >>>>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>>> Hi Kaaira, >>>>>>>>> >>>>>>>>> Thanks for your work. >>>>>>>> >>>>>>>> Thanks for yours :D >>>>>>>> >>>>>>>>> >>>>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote: >>>>>>>>>> This is version 2 of the patch series posted by Niklas for >>>>>>>>>> allowing >>>>>>>>>> multiple streams in VIMC. >>>>>>>>>> The original series can be found here: >>>>>>>>>> https://patchwork.kernel.org/cover/10948831/ >>>>>>>>>> >>>>>>>>>> This series adds support for two (or more) capture devices to be >>>>>>>>>> connected to the same sensors and run simultaneously. Each >>>>>>>>>> capture device >>>>>>>>>> can be started and stopped independent of each other. >>>>>>>>>> >>>>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once >>>>>>>>>> two >>>>>>>>>> capture devices can be part of the same pipeline. While 3/3 >>>>>>>>>> allows for >>>>>>>>>> two capture devices to be part of the same pipeline and thus >>>>>>>>>> allows for >>>>>>>>>> simultaneously use. >>>> >>>> I wonder if these two patches are enough, since each vimc entity also >>>> have >>>> a 'process_frame' callback, but only one allocated frame. That means >>>> that the 'process_frame' can be called concurrently by two different >>>> streams >>>> on the same frame and cause corruption. >>>> >>> >>> I think we should somehow change the vimc-stream.c code so that we have >>> only >>> one stream process per pipe. So if one capture is already streaming, >>> then the new >>> capture that wants to stream uses the same thread so we don't have two >>> threads >>> both calling 'process_frame'. >> >> >> Yes, I think it looks and sounds like there are two threads running when >> there are two streams. >> >> so in effect, although they 'share a pipe', aren't they in effect just >> sending two separate buffers through their stream-path? >> >> If that's the case, then I don't think there's any frame corruption, >> because they would both have grabbed their own frame separately. > > But each entity allocates just one buffer. So the same buffer is used for > both stream. Aha, ok, I hadn't realised there was only a single buffer available in the pipeline for each entity. Indeed there is a risk of corruption in that case. > What for example can happen is that the debayer of one stream can read the > sensor's buffer while the sensor itself writes to the buffer for the other > stream. So, In that case, we have currently got a scenario where each 'stream' really is operating it's own pipe (even though all components are reused). Two questions: Is this acceptable, and we should just use a mutex to ensure the buffers are not corrupted, but essentially each stream is a separate temporal capture? Or B: Should we refactor to make sure that there is a single thread, and the code which calls process_frame on each entity should become aware of the potential for multiple paths at the point of the sensor. I suspect opt
Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
Hi Dafna, On 28/07/2020 15:00, Dafna Hirschfeld wrote: > > > On 28.07.20 14:07, Dafna Hirschfeld wrote: >> Hi >> >> On 28.07.20 13:39, Kaaira Gupta wrote: >>> On Mon, Jul 27, 2020 at 02:54:30PM -0300, Helen Koike wrote: >>>> Hi, >>>> >>>> On 7/27/20 11:31 AM, Kieran Bingham wrote: >>>>> Hi all, >>>>> >>>>> +Dafna for the thread discussion, as she's missed from the to/cc list. >>>>> >>>>> >>>>> On 24/07/2020 13:21, Kaaira Gupta wrote: >>>>>> On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote: >>>>>> Hi, >>>>>> >>>>>>> Hi Kaaira, >>>>>>> >>>>>>> Thanks for your work. >>>>>> >>>>>> Thanks for yours :D >>>>>> >>>>>>> >>>>>>> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote: >>>>>>>> This is version 2 of the patch series posted by Niklas for allowing >>>>>>>> multiple streams in VIMC. >>>>>>>> The original series can be found here: >>>>>>>> https://patchwork.kernel.org/cover/10948831/ >>>>>>>> >>>>>>>> This series adds support for two (or more) capture devices to be >>>>>>>> connected to the same sensors and run simultaneously. Each >>>>>>>> capture device >>>>>>>> can be started and stopped independent of each other. >>>>>>>> >>>>>>>> Patch 1/3 and 2/3 deals with solving the issues that arises once >>>>>>>> two >>>>>>>> capture devices can be part of the same pipeline. While 3/3 >>>>>>>> allows for >>>>>>>> two capture devices to be part of the same pipeline and thus >>>>>>>> allows for >>>>>>>> simultaneously use. >> >> I wonder if these two patches are enough, since each vimc entity also >> have >> a 'process_frame' callback, but only one allocated frame. That means >> that the 'process_frame' can be called concurrently by two different >> streams >> on the same frame and cause corruption. >> > > I think we should somehow change the vimc-stream.c code so that we have > only > one stream process per pipe. So if one capture is already streaming, > then the new > capture that wants to stream uses the same thread so we don't have two > threads > both calling 'process_frame'. Yes, I think it looks and sounds like there are two threads running when there are two streams. so in effect, although they 'share a pipe', aren't they in effect just sending two separate buffers through their stream-path? If that's the case, then I don't think there's any frame corruption, because they would both have grabbed their own frame separately. I don't think that's a good example of the hardware though, as that doesn't reflect what 'should' happen where the TPG runs once to generate a frame at the sensor, which is then read by both the debayer entity and the RAW capture device when there are two streams... So I suspect trying to move to a single thread is desirable, but that might be a fair bit of work also. -- Kieran > The second capture that wants to stream should iterate the topology > downwards until > reaching an entity that already belong to the stream path of the other > streaming capture > and tell the streamer it wants to read the frames this entity > produces. > > Thanks, > Dafna > >> Thanks, >> Dafna >> >>>>>>> >>>>>>> I'm just curious if you are aware of this series? It would >>>>>>> replace the >>>>>>> need for 1/3 and 2/3 of this series right? >>>>>> >>>>>> v3 of this series replaces the need for 1/3, but not the current >>>>>> version >>>>>> (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to >>>>>> keep count of the calls to s_stream. Hence 1/3 becomes relevant >>>>>> again. >>>>> >>>>> So the question really is, how do we best make use of the two current >>>>> series, to achieve our goal of supporting multiple streams. >>>>> >>>>> Having not parsed Dafna's series yet, do we need to combine >>>>> elements of >>>>> both ? Or should we work towards starting with this series and g
Re: [PATCH 4.19 42/86] scripts/gdb: fix lx-symbols gdb.error while loading modules
Hi Stefano,
On 27/07/2020 15:37, Stefano Garzarella wrote:
> On Mon, Jul 27, 2020 at 03:26:42PM +0100, Kieran Bingham wrote:
>> Hi Greg, Sasha,
>>
>> On 27/07/2020 15:04, Greg Kroah-Hartman wrote:
>>> From: Stefano Garzarella
>>>
>>> [ Upstream commit 7359608a271ce81803de148befefd309baf88c76 ]
>>>
>>> Commit ed66f991bb19 ("module: Refactor section attr into bin attribute")
>>> removed the 'name' field from 'struct module_sect_attr' triggering the
>>> following error when invoking lx-symbols:
>>
>>
>> Has ed66f991bb19 ("module: Refactor section attr into bin attribute")
>> been backported to 4.19? It doesn't /sound/ like something that would
>> require backporting unless something else depended up on it, but if it
>> hasn't been ... then *this* patch shouldn't be either...
>>
>> Same for 5.4, and 5.7 that's just come in.
>>
>> This patch will 'apply' cleanly, and not hit any compilation errors, as
>> it only changes python code... so my reason to highlight is in case some
>> automated system picked it up based on those assumptions.
>>
>> If ed66f991bb19 has also been backported, then I'm sorry for the noise ;-)
>>
>
> I had the same doubt, but I just saw that ed66f991bb19 was backported to
> the stable branches (4.19, 5.4 and 5.7), so I think this backport is> correct.
Perfect, thanks for confirming.
--
Kieran
Re: [PATCH v2 0/3] media: vimc: Allow multiple capture devices to use the same sensor
Hi all, +Dafna for the thread discussion, as she's missed from the to/cc list. On 24/07/2020 13:21, Kaaira Gupta wrote: > On Fri, Jul 24, 2020 at 02:15:21PM +0200, Niklas Söderlund wrote: > Hi, > >> Hi Kaaira, >> >> Thanks for your work. > > Thanks for yours :D > >> >> On 2020-07-24 17:32:10 +0530, Kaaira Gupta wrote: >>> This is version 2 of the patch series posted by Niklas for allowing >>> multiple streams in VIMC. >>> The original series can be found here: >>> https://patchwork.kernel.org/cover/10948831/ >>> >>> This series adds support for two (or more) capture devices to be >>> connected to the same sensors and run simultaneously. Each capture device >>> can be started and stopped independent of each other. >>> >>> Patch 1/3 and 2/3 deals with solving the issues that arises once two >>> capture devices can be part of the same pipeline. While 3/3 allows for >>> two capture devices to be part of the same pipeline and thus allows for >>> simultaneously use. >> >> I'm just curious if you are aware of this series? It would replace the >> need for 1/3 and 2/3 of this series right? > > v3 of this series replaces the need for 1/3, but not the current version > (ie v4). v4 of patch 2/5 removes the stream_counter that is needed to > keep count of the calls to s_stream. Hence 1/3 becomes relevant again. So the question really is, how do we best make use of the two current series, to achieve our goal of supporting multiple streams. Having not parsed Dafna's series yet, do we need to combine elements of both ? Or should we work towards starting with this series and get dafna's patches built on top ? Or should patch 1/3 and 3/3 of this series be on top of Dafna's v4 ? (It might be noteworthy to say that Kaaira has reported successful multiple stream operation from /this/ series and her development branch on libcamera). >> 1. >> https://lore.kernel.org/linux-media/20200522075522.6190-1-dafna.hirschf...@collabora.com/ >> >>> >>> Changes since v1: >>> - All three patches rebased on latest media-tree. >>> Patch 3: >>> - Search for an entity with a non-NULL pipe instead of searching >>> for sensor. This terminates the search at output itself. >>> >>> Kaaira Gupta (3): >>> media: vimc: Add usage count to subdevices >>> media: vimc: Serialize vimc_streamer_s_stream() >>> media: vimc: Join pipeline if one already exists >>> >>> .../media/test-drivers/vimc/vimc-capture.c| 35 ++- >>> .../media/test-drivers/vimc/vimc-debayer.c| 8 + >>> drivers/media/test-drivers/vimc/vimc-scaler.c | 8 + >>> drivers/media/test-drivers/vimc/vimc-sensor.c | 9 - >>> .../media/test-drivers/vimc/vimc-streamer.c | 23 +++- >>> 5 files changed, 73 insertions(+), 10 deletions(-) >>> >>> -- >>> 2.17.1 >>> >> >> -- >> Regards, >> Niklas Söderlund -- Regards -- Kieran
Re: [PATCH 4.19 42/86] scripts/gdb: fix lx-symbols gdb.error while loading modules
Hi Greg, Sasha,
On 27/07/2020 15:04, Greg Kroah-Hartman wrote:
> From: Stefano Garzarella
>
> [ Upstream commit 7359608a271ce81803de148befefd309baf88c76 ]
>
> Commit ed66f991bb19 ("module: Refactor section attr into bin attribute")
> removed the 'name' field from 'struct module_sect_attr' triggering the
> following error when invoking lx-symbols:
Has ed66f991bb19 ("module: Refactor section attr into bin attribute")
been backported to 4.19? It doesn't /sound/ like something that would
require backporting unless something else depended up on it, but if it
hasn't been ... then *this* patch shouldn't be either...
Same for 5.4, and 5.7 that's just come in.
This patch will 'apply' cleanly, and not hit any compilation errors, as
it only changes python code... so my reason to highlight is in case some
automated system picked it up based on those assumptions.
If ed66f991bb19 has also been backported, then I'm sorry for the noise ;-)
--
Kieran
>
> (gdb) lx-symbols
> loading vmlinux
> scanning for modules in linux/build
> loading @0xc014f000: linux/build/drivers/net/tun.ko
> Python Exception There is no member named name.:
> Error occurred in Python: There is no member named name.
>
> This patch fixes the issue taking the module name from the 'struct
> attribute'.
>
> Fixes: ed66f991bb19 ("module: Refactor section attr into bin attribute")
> Signed-off-by: Stefano Garzarella
> Signed-off-by: Andrew Morton
> Reviewed-by: Jan Kiszka
> Reviewed-by: Kieran Bingham
> Link: http://lkml.kernel.org/r/20200722102239.313231-1-sgarz...@redhat.com
> Signed-off-by: Linus Torvalds
> Signed-off-by: Sasha Levin
> ---
> scripts/gdb/linux/symbols.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/gdb/linux/symbols.py b/scripts/gdb/linux/symbols.py
> index 4644f1a83b578..aa4f87e3ddb12 100644
> --- a/scripts/gdb/linux/symbols.py
> +++ b/scripts/gdb/linux/symbols.py
> @@ -96,7 +96,7 @@ lx-symbols command."""
> return ""
> attrs = sect_attrs['attrs']
> section_name_to_address = {
> -attrs[n]['name'].string(): attrs[n]['address']
> +attrs[n]['battr']['attr']['name'].string(): attrs[n]['address']
> for n in range(int(sect_attrs['nsections']))}
> args = []
> for section_name in [".data", ".data..read_mostly", ".rodata",
> ".bss",
>
--
--
Kieran
Re: [PATCH v10 2/4] media: i2c: Add MAX9286 driver
Hi Sakari,
On 23/07/2020 23:28, Sakari Ailus wrote:
> Hi Kieran,
>
> On Thu, Jul 16, 2020 at 10:02:24AM +0100, Kieran Bingham wrote:
>> Hi Sakari,
>>
>> This is the output of checkpatch --strict on this driver. Sorry for not
>> detailing this in the commit or cover letter.
>
> No worries.
>
>>
>>> ./patches/gmsl/v10/v10-0001-dt-bindings-media-i2c-Add-bindings-for-Maxim-Int.patch
>>> has style problems, please review.
>>> --
>>> ./patches/gmsl/v10/v10-0002-media-i2c-Add-MAX9286-driver.patch
>>> --
>>> CHECK: Prefer using the BIT macro
>>> #246: FILE: drivers/media/i2c/max9286.c:40:
>>> +#define MAX9286_FSYNCMODE_INT_OUT (1 << 6)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #251: FILE: drivers/media/i2c/max9286.c:45:
>>> +#define MAX9286_FSYNCMETH_SEMI_AUTO(1 << 0)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #262: FILE: drivers/media/i2c/max9286.c:56:
>>> +#define MAX9286_EDC_6BIT_CRC (1 << 5)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #268: FILE: drivers/media/i2c/max9286.c:62:
>>> +#define MAX9286_HVSRC_D14 (1 << 0)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #286: FILE: drivers/media/i2c/max9286.c:80:
>>> +#define MAX9286_DATATYPE_RGB565(1 << 0)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #304: FILE: drivers/media/i2c/max9286.c:98:
>>> +#define MAX9286_I2CSLVSH_469NS_234NS (1 << 5)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #312: FILE: drivers/media/i2c/max9286.c:106:
>>> +#define MAX9286_I2CMSTBT_28KBPS(1 << 2)
>>>
>>> CHECK: Prefer using the BIT macro
>>> #316: FILE: drivers/media/i2c/max9286.c:110:
>>> +#define MAX9286_I2CSLVTO_256US (1 << 0)
>>
>> None of those are appropriate to use the BIT() macro, as they are all
>> entries of a specific field with a shift, such as:
>>
>> #define MAX9286_FSYNCMODE_ECU (3 << 6)
>> #define MAX9286_FSYNCMODE_EXT (2 << 6)
>> #define MAX9286_FSYNCMODE_INT_OUT (1 << 6)
>> #define MAX9286_FSYNCMODE_INT_HIZ (0 << 6)
>>
>> Checkpatch is only picking up on the "1 << x" variant of each entry.
>
> Ideally you should use "1U << x" everywhere. If you happen to have a
> register with 31st bit signifying something, mayhem would follow. So the
> practice is to make all such definitions unsigned.
Just to clarify, because of the location you've put your x, which is not
the variable in the above case.
These definitions are possible field values with a shift (enum << y),
not bit values (1 << x)
They can of course be unsigned though.
Is your statement that you would like to see these as:
#define MAX9286_FSYNCMODE_ECU (3U << 6)
#define MAX9286_FSYNCMODE_EXT (2U << 6)
#define MAX9286_FSYNCMODE_INT_OUT (1U << 6)
#define MAX9286_FSYNCMODE_INT_HIZ (0U << 6)
Or that you would prefer a macro'ised version:
#define FIELD_ENTRY(value, shift) (value U << shift)
Or rather, I could just convert them all to use FIELD_PREP:
#define MAX9286_FSYNCMODE GENMASK(7,6)
#define MAX9286_FSYNCMODE_ECU FIELD_PREP(MAX9286_FSYNCMODE, 3)
#define MAX9286_FSYNCMODE_EXT FIELD_PREP(MAX9286_FSYNCMODE, 2)
#define MAX9286_FSYNCMODE_INT_OUT FIELD_PREP(MAX9286_FSYNCMODE, 1)
#define MAX9286_FSYNCMODE_INT_HIZ FIELD_PREP(MAX9286_FSYNCMODE, 0)
If you want me to change these entries, I suspect moving wholly to use
FIELD_PREP/FIELD_GET throughout the driver would be the best course of
action.
A bit of churn, but I can do that if you wish.
--
Kieran
>>> CHECK: Macro argument reuse 'source' - possible side-effects?
>>> #399: FILE: drivers/media/i2c/max9286.c:193:
>>> +#define for_each_source(priv, source) \
>>> + for ((source) = NULL; ((source) = next_source((priv), (source))); )
>>
>> This warns against possible side effects, but the 're-use' effects are
>> desired ;-)
>>
>> If you'd prefer this macro to be re-written please let me know.
>
> Works for me. Some warnigns are just not useful. I bet quite a few macros
> elsewhere in the kernel would trigger this.
I think we'll just leave this one ;-)
>>> CHECK: Lines should not end with a '('
>>> #1372: FILE: drivers/media/i2c/max9286.c:1166:
>>> + ret = v4l2_fw
Re: [PATCH] scripts/gdb: fix lx-symbols 'gdb.error' while loading modules
Hi All,
On 23/07/2020 11:25, Jan Kiszka wrote:
> On 22.07.20 12:22, Stefano Garzarella wrote:
>> Commit ed66f991bb19 ("module: Refactor section attr into bin
>> attribute") removed the 'name' field from 'struct module_sect_attr'
>> triggering the following error when invoking lx-symbols:
>>
>> (gdb) lx-symbols
>> loading vmlinux
>> scanning for modules in linux/build
>> loading @0xc014f000: linux/build/drivers/net/tun.ko
>> Python Exception There is no member named name.:
>> Error occurred in Python: There is no member named name.
>>
>> This patch fixes the issue taking the module name from the 'struct
>> attribute'.
>>
It might not be needed if this gets in to v5.8 in time, but perhaps:
Fixes: ed66f991bb19 ("module: Refactor section attr into bin attribute")
>> Signed-off-by: Stefano Garzarella
Otherwise, also looks fine to me.
Reviewed-by: Kieran Bingham
>> ---
>> scripts/gdb/linux/symbols.py | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/gdb/linux/symbols.py b/scripts/gdb/linux/symbols.py
>> index be984aa29b75..1be9763cf8bb 100644
>> --- a/scripts/gdb/linux/symbols.py
>> +++ b/scripts/gdb/linux/symbols.py
>> @@ -96,7 +96,7 @@ lx-symbols command."""
>> return ""
>> attrs = sect_attrs['attrs']
>> section_name_to_address = {
>> - attrs[n]['name'].string(): attrs[n]['address']
>> + attrs[n]['battr']['attr']['name'].string():
>> attrs[n]['address']
>> for n in range(int(sect_attrs['nsections']))}
>> args = []
>> for section_name in [".data", ".data..read_mostly",
>> ".rodata", ".bss",
>>
>
> Reviewed-by: Jan Kiszka
>
> Thanks,
> Jan
>
--
--
Kieran
