Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

media: dt-bindings: i2c: Add Sony IMX500 #6194

Open
wants to merge 1 commit into
base: rpi-6.6.y
Choose a base branch
from

Conversation

roliver-rpi
Copy link
Contributor

Add YAML device tree binding for the Sony IMX500 CMOS image sensor / CNN inference engine. Also, add a MAINTAINERS entry.

Add YAML device tree binding for the Sony IMX500 CMOS image sensor /
CNN inference engine.  Also, add a MAINTAINERS entry.

Signed-off-by: Richard Oliver <[email protected]>
Copy link
Contributor

@6by9 6by9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to love bindings :-/

unevaluatedProperties: false
properties:
data-lanes:
items:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your description states it supports 2 or 4 lanes, but this only allows 2.

          data-lanes:
            oneOf:
              - items:
                  - const: 1
                  - const: 2
                  - const: 3
                  - const: 4
              - items:
                  - const: 1
                  - const: 2

- const: 2
clock-noncontinuous: true
link-frequencies: true
clock-lanes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clock-lanes is normally skipped when there is no support for lane reordering.

items:
- const: 1
- const: 2
clock-noncontinuous: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst not incorrect to be in the binding, just check whether you are actually selecting non-continuous clock, or and whether it is mandatory (I'm never clear in bindings whether this "true" means supported or must be set).

I've just been looking at imx477 and found that the sensor is configured for continuous clocks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference, #6208 for adding continuous clock mode selection to imx477.

#size-cells = <0>;

sensor@0 {
compatible = "sony,imx500";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've not pushed the driver yet, but doesn't this instantiate a second instance of the driver? How do the 2 get associated, or do they deliberately not?

I'm thinking of what happens if you have 2 attached to the system. How do you control which SPI and I2C device are linked, or does it not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently refactoring the driver but I will push it soon.

I'm doing something very similar to the s5c73m3 driver:

ret = s5c73m3_register_spi_driver(state);

int s5c73m3_register_spi_driver(struct s5c73m3 *state)

I'll have to examine the consequences of having 2 devices attached to the the system more closely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So AIUI that will gate registering the SPI driver part until the I2C bit has loaded, which is necessary as the i2c part allocates the common state structure.

However I don't see how that will work with two (it may not be applicable for that driver).
s5c73m3_register_spi_driver will get called when the first I2C device gets probed, but that will allow both SPI instances to probe, and the container_of(spi->dev.driver, ...) will point back to the one I2C instance.
When the second I2C device probes, spi_register_driver will get called for a second time, but I sort of hope it falls down the "Error: Driver '%s' is already registered, aborting" error path at https://elixir.bootlin.com/linux/latest/source/drivers/base/driver.c#L241

I don't know exactly what it would look like, but I would have expected a form not dissimilar to the lens-focus or flash DT links that you get from an image sensor to lens or flash devices. (Those get handled by the common v4l2-fwnode code, whereas this would be driver specific).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants