| Message ID | 20221208104006.316606-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | i2c-atr and FPDLink | expand |
On 08/12/2022 12:39, Tomi Valkeinen wrote: > Hi, > > You can find v4 of the series from: > > https://lore.kernel.org/all/20221101132032.1542416-1-tomi.valkeinen@ideasonboard.com/ > > You can find a longer introduction of the series in that version's cover > letter. > > There has been a lot of changes to the DT bindings and the i2c-atr code in this > version, but they are all fixes and cleanups, no architectural changes. The > FPDLink drivers have not been changed, except to reflect the changes in the > DT. > > I will send a diff between v4 and v5 to give a better idea of the changes. diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml index 1e0d66704968..3a5b34c6bb64 100644 --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub913.yaml @@ -9,7 +9,7 @@ title: Texas Instruments DS90UB913 FPD-Link 3 Serializer maintainers: - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> -description: | +description: The TI DS90UB913 is an FPD-Link 3 video serializer for parallel video. properties: @@ -38,23 +38,21 @@ properties: $ref: /schemas/graph.yaml#/properties/ports properties: - port@0: $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false description: CSI-2 input port properties: endpoint: $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false port@1: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link 3 output port - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - i2c: $ref: /schemas/i2c/i2c-controller.yaml# unevaluatedProperties: false @@ -106,15 +104,11 @@ examples: #address-cells = <1>; #size-cells = <0>; - sensor@37 { - compatible = "ovti,ov10635"; - reg = <0x37>; + sensor@48 { + compatible = "aptina,mt9v111"; + reg = <0x48>; clocks = <&fixed_clock>; - clock-names = "xvclk"; - - reset-gpios = <&gpio4 17 GPIO_ACTIVE_HIGH>; - powerdown-gpios = <&gpio5 11 GPIO_ACTIVE_HIGH>; port { sensor_out: endpoint { diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml index 355f7d6a19fe..fd7d25d93e2c 100644 --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub953.yaml @@ -9,7 +9,7 @@ title: Texas Instruments DS90UB953 FPD-Link 3 Serializer maintainers: - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> -description: | +description: The TI DS90UB953 is an FPD-Link 3 video serializer for MIPI CSI-2. properties: @@ -30,23 +30,21 @@ properties: $ref: /schemas/graph.yaml#/properties/ports properties: - port@0: $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false description: CSI-2 input port properties: endpoint: $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false port@1: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link 3 output port - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - i2c: $ref: /schemas/i2c/i2c-controller.yaml# unevaluatedProperties: false @@ -97,17 +95,11 @@ examples: #address-cells = <1>; #size-cells = <0>; - sensor@21 { - compatible = "sony,imx390"; - reg = <0x21>; - - clocks = <&clk_cam_27M>; - clock-names = "inck"; + sensor@1a { + compatible = "sony,imx274"; + reg = <0x1a>; - xclr-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; - error0-gpios = <&serializer 1 GPIO_ACTIVE_HIGH>; - error1-gpios = <&serializer 2 GPIO_ACTIVE_HIGH>; - comready-gpios = <&serializer 3 GPIO_ACTIVE_HIGH>; + reset-gpios = <&serializer 0 GPIO_ACTIVE_LOW>; port { sensor_out: endpoint { diff --git a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml index 4456d9b3e2c7..d8b5e219d420 100644 --- a/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ti,ds90ub960.yaml @@ -9,7 +9,7 @@ title: Texas Instruments DS90UB9XX Family FPD-Link Deserializer Hubs maintainers: - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> -description: | +description: The TI DS90UB9XX devices are FPD-Link video deserializers with I2C and GPIO forwarding. @@ -24,10 +24,6 @@ properties: description: i2c addresses for the deserializer and the serializers - reg-names: - items: - - const: main - clocks: maxItems: 1 description: @@ -45,7 +41,12 @@ properties: i2c-alias-pool: $ref: /schemas/types.yaml#/definitions/uint16-array description: - i2c alias pool for remote devices. + i2c alias pool is a pool of i2c addresses on the main i2c bus that can be + used to access the remote peripherals. The addresses must be available, + not used by any other peripheral. Each remote peripheral is assigned an + alias from the pool, and transactions to that address will be forwarded + to the remote peripheral, with the address translated to the remote + peripheral's real address. links: type: object @@ -58,7 +59,7 @@ properties: '#size-cells': const: 0 - manual-strobe: + ti,manual-strobe: type: boolean description: Enable manual strobe position and EQ level @@ -73,12 +74,12 @@ properties: maxItems: 1 i2c-alias: - description: | + description: The i2c address used for the serializer. Transactions to this address on the i2c bus where the deserializer resides are forwarded to the serializer. - rx-mode: + ti,rx-mode: $ref: /schemas/types.yaml#/definitions/uint32 enum: - 0 # RAW10 @@ -88,23 +89,23 @@ properties: - 4 # CSI2 NON-SYNC description: FPD-Link Input Mode - cdr-mode: + ti,cdr-mode: $ref: /schemas/types.yaml#/definitions/uint32 enum: - 0 # FPD3 - 1 # FPD4 description: FPD-Link CDR Mode - strobe-pos: + ti,strobe-pos: $ref: /schemas/types.yaml#/definitions/int32 minimum: -13 maximum: 13 - description: Manual strobe position, from -13 to 13 + description: Manual strobe position - eq-level: + ti,eq-level: $ref: /schemas/types.yaml#/definitions/uint32 maximum: 14 - description: Manual EQ level, from 0 to 14 + description: Manual EQ level serializer: type: object @@ -113,7 +114,7 @@ properties: required: - reg - i2c-alias - - rx-mode + - ti,rx-mode - serializer ports: @@ -121,65 +122,51 @@ properties: properties: port@0: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link input 0 - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - port@1: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link input 1 - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - port@2: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link input 2 - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - port@3: - $ref: /schemas/graph.yaml#/$defs/port-base + $ref: /schemas/graph.yaml#/properties/port + unevaluatedProperties: false description: FPD-Link input 3 - properties: - endpoint: - $ref: /schemas/media/video-interfaces.yaml# - port@4: $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false description: CSI-2 Output 0 properties: endpoint: $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false properties: - clock-lanes: - maxItems: 1 - data-lanes: minItems: 1 maxItems: 4 port@5: $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false description: CSI-2 Output 1 properties: endpoint: $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false properties: - clock-lanes: - maxItems: 1 - data-lanes: minItems: 1 maxItems: 4 @@ -204,9 +191,7 @@ examples: deser@3d { compatible = "ti,ds90ub960-q1"; - - reg-names = "main"; - reg = <0x3d>; + reg = <0x3d>; clock-names = "refclk"; clocks = <&fixed_clock>; @@ -225,19 +210,15 @@ examples: ub960_fpd3_1_in: endpoint { remote-endpoint = <&ub953_1_out>; - - rx-mode = <0>; }; }; - /* Port 0, Camera 1 */ + /* Port 1, Camera 1 */ port@1 { reg = <1>; ub960_fpd3_2_in: endpoint { remote-endpoint = <&ub913_2_out>; - - rx-mode = <0>; }; }; @@ -245,7 +226,6 @@ examples: port@4 { reg = <4>; ds90ub960_0_csi_out: endpoint { - clock-lanes = <0>; data-lanes = <1 2 3 4>; link-frequencies = /bits/ 64 <800000000>; remote-endpoint = <&csi2_phy0>; @@ -257,13 +237,13 @@ examples: #address-cells = <1>; #size-cells = <0>; - /* Link 0 has DS90UB953 serializer and IMX390 sensor */ + /* Link 0 has DS90UB953 serializer and IMX274 sensor */ link@0 { reg = <0>; - i2c-alias = <68>; + i2c-alias = <0x44>; - rx-mode = <3>; + ti,rx-mode = <3>; serializer1: serializer { compatible = "ti,ds90ub953-q1"; @@ -280,7 +260,6 @@ examples: port@0 { reg = <0>; ub953_1_in: endpoint { - clock-lanes = <0>; data-lanes = <1 2 3 4>; remote-endpoint = <&sensor_1_out>; }; @@ -299,17 +278,11 @@ examples: #address-cells = <1>; #size-cells = <0>; - sensor@21 { - compatible = "sony,imx390"; - reg = <0x21>; - - clocks = <&clk_cam_27M>; - clock-names = "inck"; + sensor@1a { + compatible = "sony,imx274"; + reg = <0x1a>; - xclr-gpios = <&serializer1 0 GPIO_ACTIVE_LOW>; - error0-gpios = <&serializer1 1 GPIO_ACTIVE_HIGH>; - error1-gpios = <&serializer1 2 GPIO_ACTIVE_HIGH>; - comready-gpios = <&serializer1 3 GPIO_ACTIVE_HIGH>; + reset-gpios = <&serializer1 0 GPIO_ACTIVE_LOW>; port { sensor_1_out: endpoint { @@ -321,13 +294,13 @@ examples: }; }; /* End of link@0 */ - /* Link 1 has DS90UB913 serializer and OV10635 sensor */ + /* Link 1 has DS90UB913 serializer and MT9V111 sensor */ link@1 { reg = <1>; - i2c-alias = <69>; + i2c-alias = <0x45>; - rx-mode = <0>; + ti,rx-mode = <0>; serializer2: serializer { compatible = "ti,ds90ub913a-q1"; @@ -364,22 +337,15 @@ examples: #address-cells = <1>; #size-cells = <0>; - sensor@30 { - compatible = "ovti,ov10635"; - reg = <0x30>; + sensor@48 { + compatible = "aptina,mt9v111"; + reg = <0x48>; clocks = <&serializer2>; - clock-names = "xvclk"; - - powerdown-gpios = <&serializer2 0 GPIO_ACTIVE_HIGH>; port { sensor_2_out: endpoint { remote-endpoint = <&ub913_2_in>; - hsync-active = <1>; - vsync-active = <1>; - pclk-sample = <0>; - bus-width = <10>; }; }; }; diff --git a/Documentation/i2c/index.rst b/Documentation/i2c/index.rst index 3d177d4ec1d2..aaf33d1315f4 100644 --- a/Documentation/i2c/index.rst +++ b/Documentation/i2c/index.rst @@ -16,9 +16,9 @@ Introduction instantiating-devices busses/index i2c-topology + muxes/i2c-atr muxes/i2c-mux-gpio i2c-sysfs - muxes/i2c-atr Writing device drivers ====================== diff --git a/MAINTAINERS b/MAINTAINERS index 357de12cbca6..4c375e30f951 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9569,7 +9569,8 @@ S: Maintained F: drivers/i2c/i2c-core-acpi.c I2C ADDRESS TRANSLATOR (ATR) -M: Luca Ceresoli <luca@lucaceresoli.net> +M: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> +R: Luca Ceresoli <luca.ceresoli@bootlin.com> L: linux-i2c@vger.kernel.org S: Maintained F: drivers/i2c/i2c-atr.c diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c index 5636bbd03b09..1d3b25a6550f 100644 --- a/drivers/i2c/i2c-atr.c +++ b/drivers/i2c/i2c-atr.c @@ -7,14 +7,17 @@ * Originally based on i2c-mux.c */ +#include <linux/fwnode.h> #include <linux/i2c-atr.h> #include <linux/i2c.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/mutex.h> -#include <linux/of.h> #include <linux/slab.h> +#define ATR_MAX_ADAPTERS 99 /* Just a sanity limit */ +#define ATR_MAX_SYMLINK_LEN 16 /* Longest name is 10 chars: "channel-99" */ + /** * struct i2c_atr_cli2alias_pair - Hold the alias assigned to a client. * @node: List node @@ -85,9 +88,11 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs, /* Ensure we have enough room to save the original addresses */ if (unlikely(chan->orig_addrs_size < num)) { - void *new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), - GFP_KERNEL); - if (new_buf == NULL) + u16 *new_buf; + + new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), + GFP_KERNEL); + if (!new_buf) return -ENOMEM; kfree(chan->orig_addrs); @@ -118,7 +123,7 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs, * * @see i2c_atr_map_msgs() */ -static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg msgs[], +static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs, int num) { int i; @@ -127,7 +132,7 @@ static void i2c_atr_unmap_msgs(struct i2c_atr_chan *chan, struct i2c_msg msgs[], msgs[i].addr = chan->orig_addrs[i]; } -static int i2c_atr_master_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], +static int i2c_atr_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num) { struct i2c_atr_chan *chan = adap->algo_data; @@ -236,8 +241,8 @@ static int i2c_atr_attach_client(struct i2c_adapter *adapter, struct i2c_atr_chan *chan = adapter->algo_data; struct i2c_atr *atr = chan->atr; struct i2c_atr_cli2alias_pair *c2a; - u16 alias_id = 0; - int ret = 0; + u16 alias_id; + int ret; c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); if (!c2a) @@ -311,7 +316,7 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id, struct i2c_adapter *parent = atr->parent; struct device *dev = atr->dev; struct i2c_atr_chan *chan; - char *symlink_name; + char symlink_name[ATR_MAX_SYMLINK_LEN]; int ret; if (chan_id >= atr->max_adapters) { @@ -370,24 +375,26 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id, if (ret) { dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", chan_id, ret); - goto err_add_adapter; + goto err_mutex_destroy; } - symlink_name = kasprintf(GFP_KERNEL, "channel-%u", chan_id); - - WARN(sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"), - "can't create symlink to atr device\n"); - WARN(sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name), - "can't create symlink for channel %u\n", chan_id); + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", + chan->chan_id); - kfree(symlink_name); + ret = sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"); + if (ret) + dev_warn(dev, "can't create symlink to atr device\n"); + ret = sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name); + if (ret) + dev_warn(dev, "can't create symlink for channel %u\n", chan_id); dev_dbg(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); atr->adapter[chan_id] = &chan->adap; return 0; -err_add_adapter: +err_mutex_destroy: + fwnode_handle_put(dev_fwnode(&chan->adap.dev)); mutex_destroy(&chan->orig_addrs_lock); kfree(chan); return ret; @@ -402,14 +409,14 @@ EXPORT_SYMBOL_GPL(i2c_atr_add_adapter); */ void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) { - char symlink_name[20]; + char symlink_name[ATR_MAX_SYMLINK_LEN]; struct i2c_adapter *adap = atr->adapter[chan_id]; struct i2c_atr_chan *chan = adap->algo_data; - struct fwnode_handle *fwnode = adap->dev.fwnode; + struct fwnode_handle *fwnode = dev_fwnode(&adap->dev); struct device *dev = atr->dev; - if (atr->adapter[chan_id] == NULL) { + if (!atr->adapter[chan_id]) { dev_err(dev, "Adapter %d does not exist\n", chan_id); return; } @@ -418,8 +425,8 @@ void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) atr->adapter[chan_id] = NULL; - snprintf(symlink_name, sizeof(symlink_name), - "channel-%u", chan->chan_id); + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", + chan->chan_id); sysfs_remove_link(&dev->kobj, symlink_name); sysfs_remove_link(&chan->adap.dev.kobj, "atr_device"); @@ -449,16 +456,15 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev, const struct i2c_atr_ops *ops, int max_adapters) { struct i2c_atr *atr; - size_t atr_size; - if (!ops || !ops->attach_client || !ops->detach_client) + if (max_adapters > ATR_MAX_ADAPTERS) return ERR_PTR(-EINVAL); - atr_size = struct_size(atr, adapter, max_adapters); - if (atr_size == SIZE_MAX) - return ERR_PTR(-EOVERFLOW); + if (!ops || !ops->attach_client || !ops->detach_client) + return ERR_PTR(-EINVAL); - atr = devm_kzalloc(dev, atr_size, GFP_KERNEL); + atr = devm_kzalloc(dev, struct_size(atr, adapter, max_adapters), + GFP_KERNEL); if (!atr) return ERR_PTR(-ENOMEM); diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig index 1563fa0abf0e..b24f89f58807 100644 --- a/drivers/media/i2c/Kconfig +++ b/drivers/media/i2c/Kconfig @@ -1603,7 +1603,12 @@ menu "Video serializers and deserializers" config VIDEO_DS90UB960 tristate "TI DS90UB960 Deserializer" - depends on OF_GPIO + depends on OF && I2C && VIDEO_DEV + select MEDIA_CONTROLLER + select VIDEO_V4L2_SUBDEV_API + select V4L2_FWNODE + select REGMAP_I2C + select OF_GPIO select I2C_ATR help Device driver for the Texas Instruments DS90UB960 @@ -1611,12 +1616,26 @@ config VIDEO_DS90UB960 config VIDEO_DS90UB913 tristate "TI DS90UB913 Serializer" + depends on OF && I2C && VIDEO_DEV + select MEDIA_CONTROLLER + select VIDEO_V4L2_SUBDEV_API + select V4L2_FWNODE + select REGMAP_I2C + select OF_GPIO + select I2C_ATR help Device driver for the Texas Instruments DS90UB913 FPD-Link III Serializer. config VIDEO_DS90UB953 tristate "TI DS90UB953 Serializer" + depends on OF && I2C && VIDEO_DEV + select MEDIA_CONTROLLER + select VIDEO_V4L2_SUBDEV_API + select V4L2_FWNODE + select REGMAP_I2C + select OF_GPIO + select I2C_ATR help Device driver for the Texas Instruments DS90UB953 FPD-Link III Serializer. diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c index c653474b5efc..fef704ee5529 100644 --- a/drivers/media/i2c/ds90ub960.c +++ b/drivers/media/i2c/ds90ub960.c @@ -2253,7 +2253,6 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, unsigned int i; unsigned int nport; u8 fwd_ctl; - u32 active_tx_mask = 0; struct { u32 num_streams; u8 pixel_dt; @@ -2283,8 +2282,6 @@ static int ub960_configure_ports_for_streaming(struct ub960_data *priv, if (!txport) return -EINVAL; - active_tx_mask |= BIT(ub960_pad_to_port(priv, route->source_pad)); - rx_data[nport].tx_port = ub960_pad_to_port(priv, route->source_pad); rx_data[nport].num_streams++; @@ -3346,7 +3343,7 @@ static int ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, u32 eq_level; unsigned int nport = rxport->nport; - ret = of_property_read_u32(np, "cdr-mode", &cdr_mode); + ret = of_property_read_u32(np, "ti,cdr-mode", &cdr_mode); if (ret == -EINVAL) { cdr_mode = RXPORT_CDR_FPD3; } else if (ret < 0) { @@ -3367,7 +3364,7 @@ static int ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, rxport->cdr_mode = cdr_mode; - ret = of_property_read_u32(np, "rx-mode", &rx_mode); + ret = of_property_read_u32(np, "ti,rx-mode", &rx_mode); if (ret < 0) { dev_err(dev, "Missing RX port RX mode: %d\n", ret); return ret; @@ -3387,10 +3384,10 @@ static int ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, rxport->eq.aeq.eq_level_min = UB960_MIN_EQ_LEVEL; rxport->eq.aeq.eq_level_max = UB960_MAX_EQ_LEVEL; - ret = of_property_read_s32(np, "strobe-pos", &strobe_pos); + ret = of_property_read_s32(np, "ti,strobe-pos", &strobe_pos); if (ret) { if (ret != -EINVAL) { - dev_err(dev, "Failed to read 'strobe-pos': %d\n", ret); + dev_err(dev, "Failed to read 'ti,strobe-pos': %d\n", ret); return ret; } } else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS || @@ -3400,17 +3397,17 @@ static int ub960_parse_dt_rxport_link_properties(struct ub960_data *priv, // NOTE: ignored unless global manual strobe pos is set rxport->eq.strobe_pos = strobe_pos; if (!priv->strobe.manual) - dev_warn(dev, "'strobe-pos' ignored as 'manual-strobe' not set\n"); + dev_warn(dev, "'ti,strobe-pos' ignored as 'ti,manual-strobe' not set\n"); } - ret = of_property_read_u32(np, "eq-level", &eq_level); + ret = of_property_read_u32(np, "ti,eq-level", &eq_level); if (ret) { if (ret != -EINVAL) { - dev_err(dev, "Failed to read 'eq-level': %d\n", ret); + dev_err(dev, "Failed to read 'ti,eq-level': %d\n", ret); return ret; } } else if (eq_level > UB960_MAX_EQ_LEVEL) { - dev_err(dev, "Illegal 'eq-level' value: %d\n", eq_level); + dev_err(dev, "Illegal 'ti,eq-level' value: %d\n", eq_level); } else { rxport->eq.manual_eq = true; rxport->eq.manual.eq_level = eq_level; @@ -3566,7 +3563,7 @@ static int ub960_parse_dt_rxports(struct ub960_data *priv) priv->strobe.min = 2; priv->strobe.max = 3; - priv->strobe.manual = of_property_read_bool(links_np, "manual-strobe"); + priv->strobe.manual = of_property_read_bool(links_np, "ti,manual-strobe"); for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) { struct device_node *link_np; diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h index 19ac2f1db96b..044c87c5b336 100644 --- a/include/linux/i2c-atr.h +++ b/include/linux/i2c-atr.h @@ -16,6 +16,7 @@ struct device; struct i2c_atr; +struct fwnode_handle; /** * struct i2c_atr_ops - Callbacks from ATR to the device driver. @@ -41,7 +42,7 @@ struct i2c_atr_ops { }; /** - * Helper to add I2C ATR features to a device driver. + * struct i2c_atr - Represents the I2C ATR instance */ struct i2c_atr { /* private: internal use only */ @@ -53,10 +54,11 @@ struct i2c_atr { void *priv; struct i2c_algorithm algo; + /* lock for the I2C bus segment (see struct i2c_lock_operations) */ struct mutex lock; int max_adapters; - struct i2c_adapter *adapter[0]; + struct i2c_adapter *adapter[]; }; struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
On Thu, Dec 08, 2022 at 12:42:13PM +0200, Tomi Valkeinen wrote: > On 08/12/2022 12:39, Tomi Valkeinen wrote: ... > +#include <linux/fwnode.h> > #include <linux/i2c-atr.h> > #include <linux/i2c.h> > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/mutex.h> > -#include <linux/of.h> > #include <linux/slab.h> + Blank line here? > +#define ATR_MAX_ADAPTERS 99 /* Just a sanity limit */ > +#define ATR_MAX_SYMLINK_LEN 16 /* Longest name is 10 chars: "channel-99" */ ... > + u16 *new_buf; > + > + new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), > + GFP_KERNEL); new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); ? > + if (!new_buf) > return -ENOMEM; ... > struct i2c_atr_cli2alias_pair *c2a; > - u16 alias_id = 0; > - int ret = 0; > + u16 alias_id; > + int ret; Is it mangled or it's missing blank line here? > c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); > if (!c2a) ... > struct device; > struct i2c_atr; > +struct fwnode_handle; Order? ... > /** > - * Helper to add I2C ATR features to a device driver. > + * struct i2c_atr - Represents the I2C ATR instance > */ This is incomplete. Have you run kernel doc validator against this file? > struct i2c_atr { > };
On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: > From: Luca Ceresoli <luca@lucaceresoli.net> > > An adapter might need to know when a new device is about to be > added. This will soon bee needed to implement an "I2C address > translator" (ATR for short), a device that propagates I2C transactions > with a different slave address (an "alias" address). An ATR driver > needs to know when a slave is being added to find a suitable alias and > program the device translation map. > > Add an attach/detach callback pair to allow adapter drivers to be > notified of clients being added and removed. ... > + if (adap->attach_ops && > + adap->attach_ops->attach_client && > + adap->attach_ops->attach_client(adap, info, client) != 0) > + goto out_remove_swnode; With a temporary variable it becomes better ... *ops = adap->attach_ops; if (ops && ops->attach_client && ops->attach_client(adap, info, client)) Also notice drop of unneeded ' != 0' part. > status = device_register(&client->dev); > if (status) > - goto out_remove_swnode; > + goto out_detach_client; > > dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n", > client->name, dev_name(&client->dev)); > > return client; > > +out_detach_client: > + if (adap->attach_ops && adap->attach_ops->detach_client) > + adap->attach_ops->detach_client(adap, client); In the similar way. ... > + if (adap->attach_ops && > + adap->attach_ops->detach_client) > + adap->attach_ops->detach_client(adap, client); In the similar way.
On Thu, Dec 08, 2022 at 12:40:00PM +0200, Tomi Valkeinen wrote: > From: Luca Ceresoli <luca@lucaceresoli.net> > > An ATR is a device that looks similar to an i2c-mux: it has an I2C > slave "upstream" port and N master "downstream" ports, and forwards > transactions from upstream to the appropriate downstream port. But is > is different in that the forwarded transaction has a different slave > address. The address used on the upstream bus is called the "alias" > and is (potentially) different from the physical slave address of the > downstream chip. > > Add a helper file (just like i2c-mux.c for a mux or switch) to allow > implementing ATR features in a device driver. The helper takes care or > adapter creation/destruction and translates addresses at each transaction. Besides comments given against diff between series versions, see below. ... > +static int i2c_atr_attach_client(struct i2c_adapter *adapter, > + const struct i2c_board_info *info, > + const struct i2c_client *client) > +{ > + struct i2c_atr_chan *chan = adapter->algo_data; > + struct i2c_atr *atr = chan->atr; > + struct i2c_atr_cli2alias_pair *c2a; > + u16 alias_id; > + int ret; > + > + c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); > + if (!c2a) > + return -ENOMEM; > + > + ret = atr->ops->attach_client(atr, chan->chan_id, info, client, > + &alias_id); > + if (ret) > + goto err_free; > + if (alias_id == 0) { > + ret = -EINVAL; I'm wondering why attach_client can't return this error and provide a guarantee that if no error, the alias_id is never be 0? > + goto err_free; > + } > + > + c2a->client = client; > + c2a->alias = alias_id; > + list_add(&c2a->node, &chan->alias_list); > + > + return 0; > + > +err_free: > + kfree(c2a); > + return ret; > +} ... > + if (bus_handle) { > + device_set_node(&chan->adap.dev, fwnode_handle_get(bus_handle)); I believe the correct way, while above still works, is device_set_node(&chan->adap.dev, bus_handle); fwnode_handle_get(dev_fwnode(&chan->adap.dev)); But I agree that this looks a bit verbose. And... > + } else { > + struct fwnode_handle *atr_node; > + struct fwnode_handle *child; > + u32 reg; > + > + atr_node = device_get_named_child_node(dev, "i2c-atr"); > + > + fwnode_for_each_child_node(atr_node, child) { > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) > + continue; > + if (chan_id == reg) > + break; > + } > + > + device_set_node(&chan->adap.dev, child); ...OTOH, you set node with bumped reference here. So I leave all this to the maintainers. > + fwnode_handle_put(atr_node); > + } > + ret = i2c_add_adapter(&chan->adap); > + if (ret) { > + dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", > + chan_id, ret); > + goto err_mutex_destroy; > + } > + > + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", > + chan->chan_id); > + > + ret = sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"); > + if (ret) > + dev_warn(dev, "can't create symlink to atr device\n"); > + ret = sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name); > + if (ret) > + dev_warn(dev, "can't create symlink for channel %u\n", chan_id); > + > + dev_dbg(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); > + > + atr->adapter[chan_id] = &chan->adap; > + return 0; > + > +err_mutex_destroy: Now it's a bit misleading, wouldn't be better err_put_fwnode: ? > + fwnode_handle_put(dev_fwnode(&chan->adap.dev)); > + mutex_destroy(&chan->orig_addrs_lock); > + kfree(chan); > + return ret; > +} > +EXPORT_SYMBOL_GPL(i2c_atr_add_adapter); Wondering if we may put this into namespace from day 1. ... > +/** > + * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by > + * i2c_atr_del_adapter(). > + * @atr: The I2C ATR > + * @chan_id: Index of the `adapter to be removed (0 .. max_adapters-1) > + */ > +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) > +{ > + char symlink_name[ATR_MAX_SYMLINK_LEN]; > + > + struct i2c_adapter *adap = atr->adapter[chan_id]; > + struct i2c_atr_chan *chan = adap->algo_data; > + struct fwnode_handle *fwnode = dev_fwnode(&adap->dev); > + struct device *dev = atr->dev; > + if (!atr->adapter[chan_id]) { Isn't it the same as if (!adap) ? > + dev_err(dev, "Adapter %d does not exist\n", chan_id); > + return; > + } > + > + dev_dbg(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap)); > + > + atr->adapter[chan_id] = NULL; > + > + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", > + chan->chan_id); > + sysfs_remove_link(&dev->kobj, symlink_name); > + sysfs_remove_link(&chan->adap.dev.kobj, "atr_device"); > + > + i2c_del_adapter(adap); > + fwnode_handle_put(fwnode); > + mutex_destroy(&chan->orig_addrs_lock); > + kfree(chan->orig_addrs); > + kfree(chan); > +} ... > +struct i2c_atr { > + /* private: internal use only */ What is private? The entire structure? Then why it's defined in the include/linux/? Can't you make it opaque? > + struct i2c_adapter *parent; > + struct device *dev; > + const struct i2c_atr_ops *ops; > + > + void *priv; > + > + struct i2c_algorithm algo; > + /* lock for the I2C bus segment (see struct i2c_lock_operations) */ > + struct mutex lock; > + int max_adapters; > + > + struct i2c_adapter *adapter[]; > +}; ... > +static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data) > +{ > + atr->priv = data; > +} > + > +static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr) > +{ > + return atr->priv; > +} The function names are misleading, because I would think this is about driver data that has been set. I would rather use name like i2c_atr_get_priv() i2c_atr_set_priv()
Hi Andy, On 08/12/2022 14:26, Andy Shevchenko wrote: > On Thu, Dec 08, 2022 at 12:42:13PM +0200, Tomi Valkeinen wrote: >> On 08/12/2022 12:39, Tomi Valkeinen wrote: > > ... > >> +#include <linux/fwnode.h> >> #include <linux/i2c-atr.h> >> #include <linux/i2c.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/mutex.h> >> -#include <linux/of.h> >> #include <linux/slab.h> > > + Blank line here? > There is a blank line there. >> +#define ATR_MAX_ADAPTERS 99 /* Just a sanity limit */ >> +#define ATR_MAX_SYMLINK_LEN 16 /* Longest name is 10 chars: "channel-99" */ > > ... > >> + u16 *new_buf; >> + >> + new_buf = kmalloc_array(num, sizeof(chan->orig_addrs[0]), >> + GFP_KERNEL); > > new_buf = kmalloc_array(num, sizeof(*new_buf), GFP_KERNEL); > > ? Yes, I think that looks better here. >> + if (!new_buf) >> return -ENOMEM; > > ... > >> struct i2c_atr_cli2alias_pair *c2a; >> - u16 alias_id = 0; >> - int ret = 0; >> + u16 alias_id; >> + int ret; > > Is it mangled or it's missing blank line here? Also here there is a blank line. Is the mail somehow mangled on your side? On lore it looks fine: https://lore.kernel.org/all/c5eac6a6-f44b-ddd0-d27b-ccbe01498ae9@ideasonboard.com/ >> c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); >> if (!c2a) > > ... > >> struct device; >> struct i2c_atr; >> +struct fwnode_handle; > > Order? Yep, I'll fix. > ... > >> /** >> - * Helper to add I2C ATR features to a device driver. >> + * struct i2c_atr - Represents the I2C ATR instance >> */ > > This is incomplete. Have you run kernel doc validator against this file? What's kernel doc validator? Do you mean that it's incomplete and kernel doc doesn't work correctly, or that there should be more information here? I don't get any errors/warnings from any tool I have used. But I agree it looks a bit odd with only the name of the struct in the doc. Tomi
On Thu, Dec 08, 2022 at 04:40:58PM +0200, Tomi Valkeinen wrote: > On 08/12/2022 14:26, Andy Shevchenko wrote: > > On Thu, Dec 08, 2022 at 12:42:13PM +0200, Tomi Valkeinen wrote: > > > On 08/12/2022 12:39, Tomi Valkeinen wrote: ... > > > /** > > > - * Helper to add I2C ATR features to a device driver. > > > + * struct i2c_atr - Represents the I2C ATR instance > > > */ > > > > This is incomplete. Have you run kernel doc validator against this file? > > What's kernel doc validator? Do you mean that it's incomplete and kernel doc > doesn't work correctly, or that there should be more information here? > > I don't get any errors/warnings from any tool I have used. But I agree it > looks a bit odd with only the name of the struct in the doc. ... $ scripts/kernel-doc -none -v include/linux/i2c.h include/linux/i2c.h:79: warning: No description found for return value of 'i2c_master_recv' include/linux/i2c.h:94: warning: No description found for return value of 'i2c_master_recv_dmasafe' include/linux/i2c.h:109: warning: No description found for return value of 'i2c_master_send' include/linux/i2c.h:124: warning: No description found for return value of 'i2c_master_send_dmasafe' 4 warnings
On Thu, Dec 08, 2022 at 05:57:23PM +0200, Andy Shevchenko wrote: > On Thu, Dec 08, 2022 at 04:40:58PM +0200, Tomi Valkeinen wrote: > > On 08/12/2022 14:26, Andy Shevchenko wrote: > > > On Thu, Dec 08, 2022 at 12:42:13PM +0200, Tomi Valkeinen wrote: > > > > On 08/12/2022 12:39, Tomi Valkeinen wrote: ... > > > > /** > > > > - * Helper to add I2C ATR features to a device driver. > > > > + * struct i2c_atr - Represents the I2C ATR instance > > > > */ > > > > > > This is incomplete. Have you run kernel doc validator against this file? > > > > What's kernel doc validator? Do you mean that it's incomplete and kernel doc > > doesn't work correctly, or that there should be more information here? > > > > I don't get any errors/warnings from any tool I have used. But I agree it > > looks a bit odd with only the name of the struct in the doc. > > ... > > $ scripts/kernel-doc -none -v include/linux/i2c.h > include/linux/i2c.h:79: warning: No description found for return value of 'i2c_master_recv' > include/linux/i2c.h:94: warning: No description found for return value of 'i2c_master_recv_dmasafe' > include/linux/i2c.h:109: warning: No description found for return value of 'i2c_master_send' > include/linux/i2c.h:124: warning: No description found for return value of 'i2c_master_send_dmasafe' > 4 warnings Note, this is just example against existing file, not your case.
Hi Andy, On 08/12/2022 14:53, Andy Shevchenko wrote: > On Thu, Dec 08, 2022 at 12:40:00PM +0200, Tomi Valkeinen wrote: >> From: Luca Ceresoli <luca@lucaceresoli.net> >> >> An ATR is a device that looks similar to an i2c-mux: it has an I2C >> slave "upstream" port and N master "downstream" ports, and forwards >> transactions from upstream to the appropriate downstream port. But is >> is different in that the forwarded transaction has a different slave >> address. The address used on the upstream bus is called the "alias" >> and is (potentially) different from the physical slave address of the >> downstream chip. >> >> Add a helper file (just like i2c-mux.c for a mux or switch) to allow >> implementing ATR features in a device driver. The helper takes care or >> adapter creation/destruction and translates addresses at each transaction. > > Besides comments given against diff between series versions, see below. > > ... > >> +static int i2c_atr_attach_client(struct i2c_adapter *adapter, >> + const struct i2c_board_info *info, >> + const struct i2c_client *client) >> +{ >> + struct i2c_atr_chan *chan = adapter->algo_data; >> + struct i2c_atr *atr = chan->atr; >> + struct i2c_atr_cli2alias_pair *c2a; >> + u16 alias_id; >> + int ret; >> + >> + c2a = kzalloc(sizeof(*c2a), GFP_KERNEL); >> + if (!c2a) >> + return -ENOMEM; >> + >> + ret = atr->ops->attach_client(atr, chan->chan_id, info, client, >> + &alias_id); >> + if (ret) >> + goto err_free; > >> + if (alias_id == 0) { >> + ret = -EINVAL; > > I'm wondering why attach_client can't return this error and provide a guarantee > that if no error, the alias_id is never be 0? I think that's a valid point. I see no reason to check for alias_id == 0 here. >> + goto err_free; >> + } >> + >> + c2a->client = client; >> + c2a->alias = alias_id; >> + list_add(&c2a->node, &chan->alias_list); >> + >> + return 0; >> + >> +err_free: >> + kfree(c2a); >> + return ret; >> +} > > ... > >> + if (bus_handle) { >> + device_set_node(&chan->adap.dev, fwnode_handle_get(bus_handle)); > > I believe the correct way, while above still works, is > > device_set_node(&chan->adap.dev, bus_handle); > fwnode_handle_get(dev_fwnode(&chan->adap.dev)); Hmm, why is that correct? Shouldn't you give device_set_node() an fwnode that has been referenced? > But I agree that this looks a bit verbose. And... > >> + } else { >> + struct fwnode_handle *atr_node; >> + struct fwnode_handle *child; >> + u32 reg; >> + >> + atr_node = device_get_named_child_node(dev, "i2c-atr"); >> + >> + fwnode_for_each_child_node(atr_node, child) { >> + ret = fwnode_property_read_u32(child, "reg", ®); >> + if (ret) >> + continue; >> + if (chan_id == reg) >> + break; >> + } >> + >> + device_set_node(&chan->adap.dev, child); > > ...OTOH, you set node with bumped reference here. So I leave all this to > the maintainers. > >> + fwnode_handle_put(atr_node); >> + } > >> + ret = i2c_add_adapter(&chan->adap); >> + if (ret) { >> + dev_err(dev, "failed to add atr-adapter %u (error=%d)\n", >> + chan_id, ret); >> + goto err_mutex_destroy; >> + } >> + >> + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", >> + chan->chan_id); >> + >> + ret = sysfs_create_link(&chan->adap.dev.kobj, &dev->kobj, "atr_device"); >> + if (ret) >> + dev_warn(dev, "can't create symlink to atr device\n"); >> + ret = sysfs_create_link(&dev->kobj, &chan->adap.dev.kobj, symlink_name); >> + if (ret) >> + dev_warn(dev, "can't create symlink for channel %u\n", chan_id); >> + >> + dev_dbg(dev, "Added ATR child bus %d\n", i2c_adapter_id(&chan->adap)); >> + >> + atr->adapter[chan_id] = &chan->adap; >> + return 0; >> + >> +err_mutex_destroy: > > Now it's a bit misleading, wouldn't be better > > err_put_fwnode: > > ? Yes. >> + fwnode_handle_put(dev_fwnode(&chan->adap.dev)); >> + mutex_destroy(&chan->orig_addrs_lock); >> + kfree(chan); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(i2c_atr_add_adapter); > > Wondering if we may put this into namespace from day 1. Right, that's something I didn't look at all for v5. I have not heard anyone else commenting about the NS, though. I could have a look at it anyway, just to learn about NSes. > ... > >> +/** >> + * i2c_atr_del_adapter - Remove a child ("downstream") I2C bus added by >> + * i2c_atr_del_adapter(). >> + * @atr: The I2C ATR >> + * @chan_id: Index of the `adapter to be removed (0 .. max_adapters-1) >> + */ >> +void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id) >> +{ >> + char symlink_name[ATR_MAX_SYMLINK_LEN]; >> + >> + struct i2c_adapter *adap = atr->adapter[chan_id]; >> + struct i2c_atr_chan *chan = adap->algo_data; >> + struct fwnode_handle *fwnode = dev_fwnode(&adap->dev); >> + struct device *dev = atr->dev; > >> + if (!atr->adapter[chan_id]) { > > Isn't it the same as > > if (!adap) > > ? Yes. > >> + dev_err(dev, "Adapter %d does not exist\n", chan_id); >> + return; >> + } >> + >> + dev_dbg(dev, "Removing ATR child bus %d\n", i2c_adapter_id(adap)); >> + >> + atr->adapter[chan_id] = NULL; >> + >> + snprintf(symlink_name, sizeof(symlink_name), "channel-%u", >> + chan->chan_id); >> + sysfs_remove_link(&dev->kobj, symlink_name); >> + sysfs_remove_link(&chan->adap.dev.kobj, "atr_device"); >> + >> + i2c_del_adapter(adap); >> + fwnode_handle_put(fwnode); >> + mutex_destroy(&chan->orig_addrs_lock); >> + kfree(chan->orig_addrs); >> + kfree(chan); >> +} > > ... > >> +struct i2c_atr { >> + /* private: internal use only */ > > What is private? The entire structure? Then why it's defined in > the include/linux/? Can't you make it opaque? Good point, I see no reason to keep this in the public header. i2c_atr_set/get_clientdata used it, but I can move their implementations into the .c file. >> + struct i2c_adapter *parent; >> + struct device *dev; >> + const struct i2c_atr_ops *ops; >> + >> + void *priv; >> + >> + struct i2c_algorithm algo; >> + /* lock for the I2C bus segment (see struct i2c_lock_operations) */ >> + struct mutex lock; >> + int max_adapters; >> + >> + struct i2c_adapter *adapter[]; >> +}; > > ... > >> +static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data) >> +{ >> + atr->priv = data; >> +} >> + >> +static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr) >> +{ >> + return atr->priv; >> +} > > The function names are misleading, because I would think this is about driver > data that has been set. > > I would rather use name like > > i2c_atr_get_priv() > i2c_atr_set_priv() Indeed, set_clientdata is probably wrong. But i2c_atr_set_priv() sounds like it's private to the i2c-atr itself. Maybe i2c_atr_set_driver_data? Tomi
On 08/12/2022 17:58, Andy Shevchenko wrote: > On Thu, Dec 08, 2022 at 05:57:23PM +0200, Andy Shevchenko wrote: >> On Thu, Dec 08, 2022 at 04:40:58PM +0200, Tomi Valkeinen wrote: >>> On 08/12/2022 14:26, Andy Shevchenko wrote: >>>> On Thu, Dec 08, 2022 at 12:42:13PM +0200, Tomi Valkeinen wrote: >>>>> On 08/12/2022 12:39, Tomi Valkeinen wrote: > > ... > >>>>> /** >>>>> - * Helper to add I2C ATR features to a device driver. >>>>> + * struct i2c_atr - Represents the I2C ATR instance >>>>> */ >>>> >>>> This is incomplete. Have you run kernel doc validator against this file? >>> >>> What's kernel doc validator? Do you mean that it's incomplete and kernel doc >>> doesn't work correctly, or that there should be more information here? >>> >>> I don't get any errors/warnings from any tool I have used. But I agree it >>> looks a bit odd with only the name of the struct in the doc. >> >> ... >> >> $ scripts/kernel-doc -none -v include/linux/i2c.h >> include/linux/i2c.h:79: warning: No description found for return value of 'i2c_master_recv' >> include/linux/i2c.h:94: warning: No description found for return value of 'i2c_master_recv_dmasafe' >> include/linux/i2c.h:109: warning: No description found for return value of 'i2c_master_send' >> include/linux/i2c.h:124: warning: No description found for return value of 'i2c_master_send_dmasafe' >> 4 warnings > > Note, this is just example against existing file, not your case. Yes, I tested with scripts/kernel-doc, it doesn't give any warnings about i2c-atr.h. Tomi
On 08/12/2022 14:30, Andy Shevchenko wrote: > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: >> From: Luca Ceresoli <luca@lucaceresoli.net> >> >> An adapter might need to know when a new device is about to be >> added. This will soon bee needed to implement an "I2C address >> translator" (ATR for short), a device that propagates I2C transactions >> with a different slave address (an "alias" address). An ATR driver >> needs to know when a slave is being added to find a suitable alias and >> program the device translation map. >> >> Add an attach/detach callback pair to allow adapter drivers to be >> notified of clients being added and removed. > > ... > >> + if (adap->attach_ops && >> + adap->attach_ops->attach_client && >> + adap->attach_ops->attach_client(adap, info, client) != 0) >> + goto out_remove_swnode; > > With a temporary variable it becomes better > Ok. Tomi
On Thu, Dec 08, 2022 at 06:01:19PM +0200, Tomi Valkeinen wrote: > On 08/12/2022 14:53, Andy Shevchenko wrote: > > On Thu, Dec 08, 2022 at 12:40:00PM +0200, Tomi Valkeinen wrote: ... > > > + if (bus_handle) { > > > + device_set_node(&chan->adap.dev, fwnode_handle_get(bus_handle)); > > > > I believe the correct way, while above still works, is > > > > device_set_node(&chan->adap.dev, bus_handle); > > fwnode_handle_get(dev_fwnode(&chan->adap.dev)); > > Hmm, why is that correct? Shouldn't you give device_set_node() an fwnode > that has been referenced? You take a reference on the adap->dev and not on input. It's just a logical, But as I said your variant still works. > > But I agree that this looks a bit verbose. And... > > > > > + } else { > > > + struct fwnode_handle *atr_node; > > > + struct fwnode_handle *child; > > > + u32 reg; > > > + > > > + atr_node = device_get_named_child_node(dev, "i2c-atr"); > > > + > > > + fwnode_for_each_child_node(atr_node, child) { > > > + ret = fwnode_property_read_u32(child, "reg", ®); > > > + if (ret) > > > + continue; > > > + if (chan_id == reg) > > > + break; > > > + } > > > + > > > + device_set_node(&chan->adap.dev, child); > > > > ...OTOH, you set node with bumped reference here. So I leave all this to > > the maintainers. > > > > > + fwnode_handle_put(atr_node); > > > + } ... > > > +static inline void i2c_atr_set_clientdata(struct i2c_atr *atr, void *data) > > > +{ > > > + atr->priv = data; > > > +} > > > + > > > +static inline void *i2c_atr_get_clientdata(struct i2c_atr *atr) > > > +{ > > > + return atr->priv; > > > +} > > > > The function names are misleading, because I would think this is about driver > > data that has been set. > > > > I would rather use name like > > > > i2c_atr_get_priv() > > i2c_atr_set_priv() > > Indeed, set_clientdata is probably wrong. But i2c_atr_set_priv() sounds like > it's private to the i2c-atr itself. Maybe i2c_atr_set_driver_data? Works for me.
Hi Tomi and Luca, Thank you for the patch. On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: > From: Luca Ceresoli <luca@lucaceresoli.net> > > An adapter might need to know when a new device is about to be > added. This will soon bee needed to implement an "I2C address > translator" (ATR for short), a device that propagates I2C transactions > with a different slave address (an "alias" address). An ATR driver > needs to know when a slave is being added to find a suitable alias and > program the device translation map. > > Add an attach/detach callback pair to allow adapter drivers to be > notified of clients being added and removed. This may be a stupid question, but couldn't you instead use the BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ? > Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/i2c/i2c-core-base.c | 18 +++++++++++++++++- > include/linux/i2c.h | 16 ++++++++++++++++ > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index b4edf10e8fd0..c8bc71b1db82 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -966,15 +966,23 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf > } > } > > + if (adap->attach_ops && > + adap->attach_ops->attach_client && > + adap->attach_ops->attach_client(adap, info, client) != 0) > + goto out_remove_swnode; > + > status = device_register(&client->dev); > if (status) > - goto out_remove_swnode; > + goto out_detach_client; > > dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n", > client->name, dev_name(&client->dev)); > > return client; > > +out_detach_client: > + if (adap->attach_ops && adap->attach_ops->detach_client) > + adap->attach_ops->detach_client(adap, client); > out_remove_swnode: > device_remove_software_node(&client->dev); > out_err_put_of_node: > @@ -996,9 +1004,17 @@ EXPORT_SYMBOL_GPL(i2c_new_client_device); > */ > void i2c_unregister_device(struct i2c_client *client) > { > + struct i2c_adapter *adap; > + > if (IS_ERR_OR_NULL(client)) > return; > > + adap = client->adapter; > + > + if (adap->attach_ops && > + adap->attach_ops->detach_client) > + adap->attach_ops->detach_client(adap, client); > + > if (client->dev.of_node) { > of_node_clear_flag(client->dev.of_node, OF_POPULATED); > of_node_put(client->dev.of_node); > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index f7c49bbdb8a1..9a385b6de388 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -584,6 +584,21 @@ struct i2c_lock_operations { > void (*unlock_bus)(struct i2c_adapter *adapter, unsigned int flags); > }; > > +/** > + * struct i2c_attach_operations - callbacks to notify client attach/detach > + * @attach_client: Notify of new client being attached > + * @detach_client: Notify of new client being detached > + * > + * Both ops are optional. > + */ > +struct i2c_attach_operations { > + int (*attach_client)(struct i2c_adapter *adapter, > + const struct i2c_board_info *info, > + const struct i2c_client *client); > + void (*detach_client)(struct i2c_adapter *adapter, > + const struct i2c_client *client); > +}; > + > /** > * struct i2c_timings - I2C timing information > * @bus_freq_hz: the bus frequency in Hz > @@ -726,6 +741,7 @@ struct i2c_adapter { > > /* data fields that are valid for all devices */ > const struct i2c_lock_operations *lock_ops; > + const struct i2c_attach_operations *attach_ops; > struct rt_mutex bus_lock; > struct rt_mutex mux_lock; >
Hi Tomi, Thank you for the patch. On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: > Add driver for TI DS90UB913 FPDLink-3 Serializer. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/i2c/Kconfig | 13 + > drivers/media/i2c/Makefile | 2 +- > drivers/media/i2c/ds90ub913.c | 892 ++++++++++++++++++++++++++++++++++ > 3 files changed, 906 insertions(+), 1 deletion(-) > create mode 100644 drivers/media/i2c/ds90ub913.c > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index a23f723b89b5..ff5847aed5ae 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -1614,6 +1614,19 @@ config VIDEO_DS90UB960 > Device driver for the Texas Instruments DS90UB960 > FPD-Link III Deserializer > > +config VIDEO_DS90UB913 > + tristate "TI DS90UB913 Serializer" > + depends on OF && I2C && VIDEO_DEV > + select MEDIA_CONTROLLER > + select VIDEO_V4L2_SUBDEV_API > + select V4L2_FWNODE > + select REGMAP_I2C > + select OF_GPIO > + select I2C_ATR > + help > + Device driver for the Texas Instruments DS90UB913 > + FPD-Link III Serializer. > + > endmenu > > endif # VIDEO_DEV > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index 2735b00437bb..376886f2ded6 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -143,4 +143,4 @@ obj-$(CONFIG_VIDEO_VS6624) += vs6624.o > obj-$(CONFIG_VIDEO_WM8739) += wm8739.o > obj-$(CONFIG_VIDEO_WM8775) += wm8775.o > obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o > - > +obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o Alphabetical order please. > diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c > new file mode 100644 > index 000000000000..6001a622e622 > --- /dev/null > +++ b/drivers/media/i2c/ds90ub913.c > @@ -0,0 +1,892 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for the Texas Instruments DS90UB913 video serializer > + * > + * Based on a driver from Luca Ceresoli <luca@lucaceresoli.net> > + * > + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> > + * Copyright (c) 2022 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/gpio/driver.h> > +#include <linux/i2c-atr.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/regmap.h> > + > +#include <media/i2c/ds90ub9xx.h> > +#include <media/v4l2-subdev.h> > + > +#define UB913_PAD_SINK 0 > +#define UB913_PAD_SOURCE 1 > + > +/* > + * UB913 has 4 gpios, but gpios 3 and 4 are reserved for external oscillator > + * mode. Thus we only support 2 gpios for now. > + */ > +#define UB913_NUM_GPIOS 2 > + > +#define UB913_REG_RESET_CTL 0x01 > +#define UB913_REG_RESET_CTL_DIGITAL_RESET_1 BIT(1) > +#define UB913_REG_RESET_CTL_DIGITAL_RESET_0 BIT(0) > + > +#define UB913_REG_GENERAL_CFG 0x03 > +#define UB913_REG_MODE_SEL 0x05 > + > +#define UB913_REG_CRC_ERRORS_LSB 0x0a > +#define UB913_REG_CRC_ERRORS_MSB 0x0b > + > +#define UB913_REG_GENERAL_STATUS 0x0c > + > +#define UB913_REG_GPIO_CFG(n) (0x0d + (n)) > +#define UB913_REG_GPIO_CFG_ENABLE(n) BIT(0 + (n) * 4) > +#define UB913_REG_GPIO_CFG_DIR_INPUT(n) BIT(1 + (n) * 4) > +#define UB913_REG_GPIO_CFG_REMOTE_EN(n) BIT(2 + (n) * 4) > +#define UB913_REG_GPIO_CFG_OUT_VAL(n) BIT(3 + (n) * 4) > +#define UB913_REG_GPIO_CFG_MASK(n) (0xf << ((n) * 4)) > + > +#define UB913_REG_SCL_HIGH_TIME 0x11 > +#define UB913_REG_SCL_LOW_TIME 0x12 > + > +#define UB913_REG_PLL_OVR 0x35 > + > +struct ub913_data { > + struct i2c_client *client; > + struct regmap *regmap; > + struct clk *clkin; > + > + u32 gpio_func[UB913_NUM_GPIOS]; > + > + struct gpio_chip gpio_chip; > + char gpio_chip_name[64]; > + > + struct v4l2_subdev sd; > + struct media_pad pads[2]; > + > + struct v4l2_async_notifier notifier; > + > + struct v4l2_subdev *source_sd; > + > + u64 enabled_source_streams; > + > + struct device_node *tx_ep_np; > + > + struct clk_hw *clkout_clk_hw; > + > + struct ds90ub9xx_platform_data *plat_data; > + > + /* Have we succefully called i2c_atr_add_adapter() */ > + bool has_i2c_adapter; > +}; > + > +static inline struct ub913_data *sd_to_ub913(struct v4l2_subdev *sd) > +{ > + return container_of(sd, struct ub913_data, sd); > +} > + > +static int ub913_read(const struct ub913_data *priv, u8 reg, u8 *val) > +{ > + unsigned int v; > + int ret; > + > + ret = regmap_read(priv->regmap, reg, &v); > + if (ret < 0) { > + dev_err(&priv->client->dev, > + "Cannot read register 0x%02x: %d!\n", reg, ret); > + return ret; > + } > + > + *val = v; > + return 0; > +} > + > +static int ub913_write(const struct ub913_data *priv, u8 reg, u8 val) > +{ > + int ret; > + > + ret = regmap_write(priv->regmap, reg, val); > + if (ret < 0) > + dev_err(&priv->client->dev, > + "Cannot write register 0x%02x: %d!\n", reg, ret); > + > + return ret; > +} > + > +/* > + * GPIO chip > + */ > +static int ub913_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) > +{ > + return GPIO_LINE_DIRECTION_OUT; > +} > + > +static int ub913_gpio_direction_out(struct gpio_chip *gc, unsigned int offset, > + int value) > +{ > + struct ub913_data *priv = gpiochip_get_data(gc); > + unsigned int reg_idx; > + unsigned int field_idx; > + int ret; > + > + reg_idx = offset / 2; > + field_idx = offset % 2; > + > + ret = regmap_update_bits( > + priv->regmap, UB913_REG_GPIO_CFG(reg_idx), > + UB913_REG_GPIO_CFG_MASK(field_idx), > + UB913_REG_GPIO_CFG_ENABLE(field_idx) | > + (value ? UB913_REG_GPIO_CFG_OUT_VAL(field_idx) : 0)); > + > + return ret; > +} > + > +static void ub913_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) > +{ > + ub913_gpio_direction_out(gc, offset, value); > +} > + > +static int ub913_gpio_of_xlate(struct gpio_chip *gc, > + const struct of_phandle_args *gpiospec, > + u32 *flags) > +{ > + if (flags) > + *flags = gpiospec->args[1]; > + > + return gpiospec->args[0]; > +} > + > +static int ub913_gpiochip_probe(struct ub913_data *priv) > +{ > + struct device *dev = &priv->client->dev; > + struct gpio_chip *gc = &priv->gpio_chip; > + int ret; > + > + /* Initialize GPIOs 0 and 1 to local control, tri-state */ > + ub913_write(priv, UB913_REG_GPIO_CFG(0), 0); > + > + scnprintf(priv->gpio_chip_name, sizeof(priv->gpio_chip_name), "%s", > + dev_name(dev)); > + > + gc->label = priv->gpio_chip_name; > + gc->parent = dev; > + gc->owner = THIS_MODULE; > + gc->base = -1; > + gc->can_sleep = 1; > + gc->ngpio = UB913_NUM_GPIOS; > + gc->get_direction = ub913_gpio_get_direction; > + gc->direction_output = ub913_gpio_direction_out; > + gc->set = ub913_gpio_set; > + gc->of_xlate = ub913_gpio_of_xlate; > + gc->of_node = priv->client->dev.of_node; > + gc->of_gpio_n_cells = 2; > + > + ret = gpiochip_add_data(gc, priv); > + if (ret) { > + dev_err(dev, "Failed to add GPIOs: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static void ub913_gpiochip_remove(struct ub913_data *priv) > +{ > + gpiochip_remove(&priv->gpio_chip); > +} > + > +static int ub913_parse_dt(struct ub913_data *priv) I would have moved this just before ub913_probe(). > +{ > + struct device_node *np = priv->client->dev.of_node; > + struct device *dev = &priv->client->dev; > + int ret; > + > + if (!np) { > + dev_err(dev, "OF: no device tree node!\n"); > + return -ENOENT; > + } > + > + /* optional, if absent all GPIO pins are unused */ > + ret = of_property_read_u32_array(np, "gpio-functions", priv->gpio_func, > + ARRAY_SIZE(priv->gpio_func)); > + if (ret && ret != -EINVAL) > + dev_err(dev, "DT: invalid gpio-functions property (%d)", ret); > + > + return 0; > +} > + > +static const struct regmap_config ub913_regmap_config = { > + .name = "ds90ub913", > + .reg_bits = 8, > + .val_bits = 8, > + .reg_format_endian = REGMAP_ENDIAN_DEFAULT, > + .val_format_endian = REGMAP_ENDIAN_DEFAULT, > +}; > + > +/* > + * V4L2 > + */ > + > +static int ub913_enable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + struct media_pad *remote_pad; > + u64 sink_streams; > + int ret; > + > + if (streams_mask & priv->enabled_source_streams) > + return -EALREADY; > + > + sink_streams = v4l2_subdev_state_xlate_streams( > + state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); > + > + remote_pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); > + > + ret = v4l2_subdev_enable_streams(priv->source_sd, remote_pad->index, > + sink_streams); > + if (ret) > + return ret; > + > + priv->enabled_source_streams |= streams_mask; > + > + return 0; > +} > + > +static int ub913_disable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + struct media_pad *remote_pad; > + int ret; > + u64 sink_streams; > + > + if ((streams_mask & priv->enabled_source_streams) != streams_mask) > + return -EALREADY; > + > + sink_streams = v4l2_subdev_state_xlate_streams( > + state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); > + > + remote_pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); > + > + ret = v4l2_subdev_disable_streams(priv->source_sd, remote_pad->index, > + sink_streams); > + if (ret) > + return ret; > + > + priv->enabled_source_streams &= ~streams_mask; > + > + return 0; > +} > + > +static int _ub913_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_krouting *routing) > +{ > + const struct v4l2_mbus_framefmt format = { static > + .width = 640, > + .height = 480, > + .code = MEDIA_BUS_FMT_UYVY8_2X8, > + .field = V4L2_FIELD_NONE, > + .colorspace = V4L2_COLORSPACE_SRGB, > + .ycbcr_enc = V4L2_YCBCR_ENC_601, > + .quantization = V4L2_QUANTIZATION_LIM_RANGE, > + .xfer_func = V4L2_XFER_FUNC_SRGB, > + }; > + int ret; > + > + /* > + * Note: we can only support up to V4L2_FRAME_DESC_ENTRY_MAX, until > + * frame desc is made dynamically allocated. > + */ > + > + if (routing->num_routes > V4L2_FRAME_DESC_ENTRY_MAX) > + return -EINVAL; > + > + ret = v4l2_subdev_routing_validate(sd, routing, > + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); > + if (ret) > + return ret; > + > + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int ub913_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + enum v4l2_subdev_format_whence which, > + struct v4l2_subdev_krouting *routing) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + > + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && priv->enabled_source_streams) > + return -EBUSY; > + > + return _ub913_set_routing(sd, state, routing); > +} > + > +static int ub913_get_source_frame_desc(struct ub913_data *priv, > + struct v4l2_mbus_frame_desc *desc) > +{ > + struct media_pad *pad; > + int ret; > + > + pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); > + if (!pad) > + return -EPIPE; > + > + ret = v4l2_subdev_call(priv->source_sd, pad, get_frame_desc, pad->index, > + desc); > + if (ret) > + return ret; > + > + return 0; I would inline this in the caller. > +} > + > +static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + const struct v4l2_subdev_krouting *routing; > + struct v4l2_mbus_frame_desc source_fd; > + struct v4l2_subdev_route *route; > + struct v4l2_subdev_state *state; > + int ret = 0; No need to initialize this to 0. > + > + if (pad != 1) /* first tx pad */ if (pad != UB913_PAD_SOURCE) and drop the comment. > + return -EINVAL; > + > + ret = ub913_get_source_frame_desc(priv, &source_fd); > + if (ret) > + return ret; > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + > + routing = &state->routing; > + > + memset(fd, 0, sizeof(*fd)); > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL; > + > + for_each_active_route(routing, route) { > + unsigned int j; Anything wrong with 'i' ? > + > + if (route->source_pad != pad) > + continue; > + > + for (j = 0; j < source_fd.num_entries; ++j) > + if (source_fd.entry[j].stream == route->sink_stream) > + break; > + > + if (j == source_fd.num_entries) { > + dev_err(&priv->client->dev, > + "Failed to find stream from source frame desc\n"); > + ret = -EPIPE; > + goto out; > + } > + > + fd->entry[fd->num_entries].stream = route->source_stream; > + > + fd->entry[fd->num_entries].flags = > + V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; Shouldn't this be set only if set in the source frame descriptor ? > + fd->entry[fd->num_entries].length = source_fd.entry[j].length; > + fd->entry[fd->num_entries].pixelcode = > + source_fd.entry[j].pixelcode; > + > + fd->num_entries++; > + } > + > +out: > + v4l2_subdev_unlock_state(state); > + > + return ret; > +} > + > +static int ub913_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_format *format) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + struct v4l2_mbus_framefmt *fmt; > + > + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && > + priv->enabled_source_streams) > + return -EBUSY; > + > + /* No transcoding, source and sink formats must match. */ > + if (format->pad == 1) if (format->pad == UB913_PAD_SOURCE) > + return v4l2_subdev_get_fmt(sd, state, format); > + > + /* Set sink format */ > + fmt = v4l2_subdev_state_get_stream_format(state, format->pad, > + format->stream); > + if (!fmt) > + return -EINVAL; > + > + *fmt = format->format; > + > + /* Propagate to source format */ > + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, > + format->stream); > + if (!fmt) > + return -EINVAL; > + > + *fmt = format->format; > + > + return 0; > +} > + > +static int ub913_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_route routes[] = { > + { > + .sink_pad = 0, .sink_pad = UB913_PAD_SINK, > + .sink_stream = 0, > + .source_pad = 1, .source_pad = UB913_PAD_SOURCE, > + .source_stream = 0, > + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + }, > + }; > + > + struct v4l2_subdev_krouting routing = { > + .num_routes = ARRAY_SIZE(routes), > + .routes = routes, > + }; > + > + return _ub913_set_routing(sd, state, &routing); > +} > + > +static int ub913_log_status(struct v4l2_subdev *sd) > +{ > + struct ub913_data *priv = sd_to_ub913(sd); > + struct device *dev = &priv->client->dev; > + u8 v, v1, v2; > + > + ub913_read(priv, UB913_REG_MODE_SEL, &v); > + dev_info(dev, "MODE_SEL %#x\n", v); > + > + ub913_read(priv, UB913_REG_CRC_ERRORS_LSB, &v1); > + ub913_read(priv, UB913_REG_CRC_ERRORS_MSB, &v2); > + dev_info(dev, "CRC errors %u\n", v1 | (v2 << 8)); > + > + ub913_read(priv, UB913_REG_GENERAL_STATUS, &v); > + dev_info(dev, "GENERAL_STATUS %#x\n", v); > + > + ub913_read(priv, UB913_REG_PLL_OVR, &v); > + dev_info(dev, "PLL_OVR %#x\n", v); > + > + /* clear CRC errors */ > + ub913_read(priv, UB913_REG_GENERAL_CFG, &v); > + ub913_write(priv, UB913_REG_GENERAL_CFG, v | BIT(5)); > + ub913_write(priv, UB913_REG_GENERAL_CFG, v); > + > + return 0; > +} > + > +static const struct v4l2_subdev_core_ops ub913_subdev_core_ops = { > + .log_status = ub913_log_status, > +}; > + > +static const struct v4l2_subdev_pad_ops ub913_pad_ops = { > + .enable_streams = ub913_enable_streams, > + .disable_streams = ub913_disable_streams, > + .set_routing = ub913_set_routing, > + .get_frame_desc = ub913_get_frame_desc, > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = ub913_set_fmt, > + .init_cfg = ub913_init_cfg, > +}; > + > +static const struct v4l2_subdev_ops ub913_subdev_ops = { > + .core = &ub913_subdev_core_ops, > + .pad = &ub913_pad_ops, > +}; > + > +static const struct media_entity_operations ub913_entity_ops = { > + .link_validate = v4l2_subdev_link_validate, > +}; > + > +static int ub913_notify_bound(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *source_subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct ub913_data *priv = sd_to_ub913(notifier->sd); > + struct device *dev = &priv->client->dev; > + unsigned int src_pad; > + int ret; > + > + dev_dbg(dev, "Bind %s\n", source_subdev->name); I'd drop this message. > + > + ret = media_entity_get_fwnode_pad(&source_subdev->entity, > + source_subdev->fwnode, > + MEDIA_PAD_FL_SOURCE); > + if (ret < 0) { > + dev_err(dev, "Failed to find pad for %s\n", > + source_subdev->name); > + return ret; > + } > + > + priv->source_sd = source_subdev; > + src_pad = ret; > + > + ret = media_create_pad_link(&source_subdev->entity, src_pad, > + &priv->sd.entity, 0, &priv->sd.entity, UB913_PAD_SINK, > + MEDIA_LNK_FL_ENABLED | > + MEDIA_LNK_FL_IMMUTABLE); > + if (ret) { > + dev_err(dev, "Unable to link %s:%u -> %s:0\n", > + source_subdev->name, src_pad, priv->sd.name); > + return ret; > + } > + > + dev_dbg(dev, "Bound %s:%u\n", source_subdev->name, src_pad); > + > + dev_dbg(dev, "All subdevs bound\n"); I'd drop this message. > + > + return 0; > +} > + > +static void ub913_notify_unbind(struct v4l2_async_notifier *notifier, > + struct v4l2_subdev *source_subdev, > + struct v4l2_async_subdev *asd) > +{ > + struct ub913_data *priv = sd_to_ub913(notifier->sd); > + struct device *dev = &priv->client->dev; > + > + dev_dbg(dev, "Unbind %s\n", source_subdev->name); > +} This is a no-op so you can drop it. > + > +static const struct v4l2_async_notifier_operations ub913_notify_ops = { > + .bound = ub913_notify_bound, > + .unbind = ub913_notify_unbind, > +}; > + > +static int ub913_v4l2_notifier_register(struct ub913_data *priv) > +{ > + struct device *dev = &priv->client->dev; > + struct v4l2_async_subdev *asd; > + struct device_node *ep_node; > + int ret; > + > + dev_dbg(dev, "register async notif\n"); > + > + ep_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); > + if (!ep_node) { > + dev_err(dev, "No graph endpoint\n"); > + return -ENODEV; > + } > + > + v4l2_async_nf_init(&priv->notifier); > + > + asd = v4l2_async_nf_add_fwnode_remote(&priv->notifier, > + of_fwnode_handle(ep_node), > + struct v4l2_async_subdev); > + > + of_node_put(ep_node); > + > + if (IS_ERR(asd)) { > + dev_err(dev, "Failed to add subdev: %ld", PTR_ERR(asd)); > + v4l2_async_nf_cleanup(&priv->notifier); > + return PTR_ERR(asd); > + } > + > + priv->notifier.ops = &ub913_notify_ops; > + > + ret = v4l2_async_subdev_nf_register(&priv->sd, &priv->notifier); > + if (ret) { > + dev_err(dev, "Failed to register subdev_notifier"); > + v4l2_async_nf_cleanup(&priv->notifier); > + return ret; > + } > + > + return 0; > +} > + > +static void ub913_v4l2_nf_unregister(struct ub913_data *priv) > +{ > + struct device *dev = &priv->client->dev; > + > + dev_dbg(dev, "Unregister async notif\n"); > + > + v4l2_async_nf_unregister(&priv->notifier); > + v4l2_async_nf_cleanup(&priv->notifier); > +} > + > +static int ub913_register_clkout(struct ub913_data *priv) > +{ > + struct device *dev = &priv->client->dev; > + const char *name; > + int ret; > + > + name = kasprintf(GFP_KERNEL, "ds90ub913.%s.clk_out", dev_name(dev)); > + > + priv->clkout_clk_hw = devm_clk_hw_register_fixed_factor(dev, name, > + __clk_get_name(priv->clkin), 0, 1, 2); > + > + kfree(name); > + > + if (IS_ERR(priv->clkout_clk_hw)) > + return dev_err_probe(dev, PTR_ERR(priv->clkout_clk_hw), > + "Cannot register clkout hw\n"); > + > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, > + priv->clkout_clk_hw); > + if (ret) > + return dev_err_probe(dev, ret, > + "Cannot add OF clock provider\n"); > + > + return 0; > +} > + > +static int ub913_i2c_master_init(struct ub913_data *priv) > +{ > + /* i2c fast mode */ > + u32 scl_high = 600 + 300; /* high period + rise time, ns */ > + u32 scl_low = 1300 + 300; /* low period + fall time, ns */ > + unsigned long ref; > + int ret; > + > + ref = clk_get_rate(priv->clkin) / 2; > + > + scl_high = div64_u64((u64)scl_high * ref, 1000000000); > + scl_low = div64_u64((u64)scl_low * ref, 1000000000); > + > + ret = ub913_write(priv, UB913_REG_SCL_HIGH_TIME, scl_high); > + if (ret) > + return ret; > + > + ret = ub913_write(priv, UB913_REG_SCL_LOW_TIME, scl_low); > + if (ret) > + return ret; > + > + return 0; > +} > + > +static int ub913_add_i2c_adapter(struct ub913_data *priv) > +{ > + struct device *dev = &priv->client->dev; > + struct fwnode_handle *i2c_handle; > + int ret; > + > + i2c_handle = device_get_named_child_node(dev, "i2c"); > + if (!i2c_handle) > + return 0; > + > + ret = i2c_atr_add_adapter(priv->plat_data->atr, priv->plat_data->port, > + i2c_handle); > + > + fwnode_handle_put(i2c_handle); > + > + if (ret) > + return ret; > + > + priv->has_i2c_adapter = true; > + > + return 0; > +} > + > +static void ub913_remove_i2c_adapter(struct ub913_data *priv) > +{ > + if (priv->has_i2c_adapter) > + i2c_atr_del_adapter(priv->plat_data->atr, > + priv->plat_data->port); Could i2c_atr_del_adapter() be a no-op if no adapter for the port has been added ? You could then drop the has_i2c_adapter field. I'm also wondering if the struct device of the DS90UB913 could be passed instead of the port, to avoid passing the port throught ds90ub9xx_platform_data. > +} > + > +static int ub913_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct ub913_data *priv; > + int ret; > + u8 v; > + bool mode_override; > + u8 mode; > + > + dev_dbg(dev, "probing, addr 0x%02x\n", client->addr); > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->client = client; > + > + priv->plat_data = dev_get_platdata(&client->dev); > + if (!priv->plat_data) { > + dev_err(dev, "Platform data missing\n"); > + return -ENODEV; > + } > + > + priv->regmap = devm_regmap_init_i2c(client, &ub913_regmap_config); > + if (IS_ERR(priv->regmap)) { > + dev_err(dev, "Failed to init regmap\n"); > + return PTR_ERR(priv->regmap); > + } > + > + /* ub913 can also work without ext clock, but that is not supported */ > + priv->clkin = devm_clk_get(dev, "clkin"); > + if (IS_ERR(priv->clkin)) { > + ret = PTR_ERR(priv->clkin); > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "Cannot get CLKIN (%d)", ret); > + return ret; > + } > + > + ret = ub913_parse_dt(priv); > + if (ret) > + return ret; > + > + ret = ub913_read(priv, UB913_REG_MODE_SEL, &v); > + if (ret) > + return ret; > + > + if (!(v & BIT(4))) { Please add a mcro for this. Same for other magic bits in the driver. > + dev_err(dev, "Mode value not stabilized\n"); > + return -ENODEV; > + } > + > + mode_override = v & BIT(5); > + mode = v & 0xf; > + > + dev_dbg(dev, "mode from %s: %#x\n", > + mode_override ? "reg" : "deserializer", mode); > + > + ret = ub913_i2c_master_init(priv); > + if (ret) { > + dev_err(dev, "i2c master init failed: %d\n", ret); > + return ret; > + } > + > + ret = ub913_gpiochip_probe(priv); > + if (ret) { > + dev_err(dev, "Failed to init gpiochip\n"); > + return ret; > + } > + > + ret = ub913_register_clkout(priv); > + if (ret) { > + dev_err(dev, "Failed to register clkout\n"); > + goto err_gpiochip_remove; > + } > + > + v4l2_i2c_subdev_init(&priv->sd, priv->client, &ub913_subdev_ops); > + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > + priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > + priv->sd.entity.ops = &ub913_entity_ops; > + > + priv->pads[0].flags = MEDIA_PAD_FL_SINK; > + priv->pads[1].flags = MEDIA_PAD_FL_SOURCE; > + > + ret = media_entity_pads_init(&priv->sd.entity, 2, priv->pads); > + if (ret) { > + dev_err(dev, "Failed to init pads\n"); > + goto err_gpiochip_remove; > + } > + > + priv->tx_ep_np = of_graph_get_endpoint_by_regs(dev->of_node, 1, 0); > + if (priv->tx_ep_np) > + priv->sd.fwnode = of_fwnode_handle(priv->tx_ep_np); > + > + ret = v4l2_subdev_init_finalize(&priv->sd); > + if (ret) > + goto err_entity_cleanup; > + > + ret = ub913_v4l2_notifier_register(priv); > + if (ret) { > + dev_err(dev, "v4l2 subdev notifier register failed: %d\n", ret); > + goto err_free_state; > + } > + > + ret = v4l2_async_register_subdev(&priv->sd); > + if (ret) { > + dev_err(dev, "v4l2_async_register_subdev error: %d\n", ret); > + goto err_unreg_notif; > + } > + > + ret = ub913_add_i2c_adapter(priv); > + if (ret) { > + dev_err(dev, "failed to add remote i2c adapter\n"); > + goto err_unreg_async_subdev; > + } > + > + dev_dbg(dev, "Successfully probed\n"); > + > + return 0; > + > +err_unreg_async_subdev: > + v4l2_async_unregister_subdev(&priv->sd); > +err_unreg_notif: > + ub913_v4l2_nf_unregister(priv); > +err_free_state: > + v4l2_subdev_cleanup(&priv->sd); > +err_entity_cleanup: > + if (priv->tx_ep_np) > + of_node_put(priv->tx_ep_np); > + > + media_entity_cleanup(&priv->sd.entity); > +err_gpiochip_remove: > + ub913_gpiochip_remove(priv); > + > + return ret; > +} > + > +static void ub913_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct ub913_data *priv = sd_to_ub913(sd); > + > + dev_dbg(&client->dev, "Removing\n"); > + > + ub913_remove_i2c_adapter(priv); > + > + v4l2_async_unregister_subdev(&priv->sd); > + > + ub913_v4l2_nf_unregister(priv); > + > + v4l2_subdev_cleanup(&priv->sd); > + > + if (priv->tx_ep_np) > + of_node_put(priv->tx_ep_np); > + > + media_entity_cleanup(&priv->sd.entity); > + > + ub913_gpiochip_remove(priv); > +} > + > +static const struct i2c_device_id ub913_id[] = { { "ds90ub913a-q1", 0 }, {} }; > +MODULE_DEVICE_TABLE(i2c, ub913_id); > + > +#ifdef CONFIG_OF > +static const struct of_device_id ub913_dt_ids[] = { > + { .compatible = "ti,ds90ub913a-q1", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ub913_dt_ids); > +#endif > + > +static struct i2c_driver ds90ub913_driver = { > + .probe_new = ub913_probe, > + .remove = ub913_remove, > + .id_table = ub913_id, > + .driver = { > + .name = "ds90ub913a", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ub913_dt_ids), > + }, > +}; > + > +module_i2c_driver(ds90ub913_driver); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Texas Instruments DS90UB913 serializer driver"); > +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); > +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>"); > -- > 2.34.1 >
Hi Laurent, On 11/12/2022 20:33, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: >> Add driver for TI DS90UB913 FPDLink-3 Serializer. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/i2c/Kconfig | 13 + >> drivers/media/i2c/Makefile | 2 +- >> drivers/media/i2c/ds90ub913.c | 892 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 906 insertions(+), 1 deletion(-) >> create mode 100644 drivers/media/i2c/ds90ub913.c >> >> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig >> index a23f723b89b5..ff5847aed5ae 100644 >> --- a/drivers/media/i2c/Kconfig >> +++ b/drivers/media/i2c/Kconfig >> @@ -1614,6 +1614,19 @@ config VIDEO_DS90UB960 >> Device driver for the Texas Instruments DS90UB960 >> FPD-Link III Deserializer >> >> +config VIDEO_DS90UB913 >> + tristate "TI DS90UB913 Serializer" >> + depends on OF && I2C && VIDEO_DEV >> + select MEDIA_CONTROLLER >> + select VIDEO_V4L2_SUBDEV_API >> + select V4L2_FWNODE >> + select REGMAP_I2C >> + select OF_GPIO >> + select I2C_ATR >> + help >> + Device driver for the Texas Instruments DS90UB913 >> + FPD-Link III Serializer. >> + >> endmenu >> >> endif # VIDEO_DEV >> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile >> index 2735b00437bb..376886f2ded6 100644 >> --- a/drivers/media/i2c/Makefile >> +++ b/drivers/media/i2c/Makefile >> @@ -143,4 +143,4 @@ obj-$(CONFIG_VIDEO_VS6624) += vs6624.o >> obj-$(CONFIG_VIDEO_WM8739) += wm8739.o >> obj-$(CONFIG_VIDEO_WM8775) += wm8775.o >> obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o >> - >> +obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o > > Alphabetical order please. Ok. >> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c >> new file mode 100644 >> index 000000000000..6001a622e622 >> --- /dev/null >> +++ b/drivers/media/i2c/ds90ub913.c >> @@ -0,0 +1,892 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Driver for the Texas Instruments DS90UB913 video serializer >> + * >> + * Based on a driver from Luca Ceresoli <luca@lucaceresoli.net> >> + * >> + * Copyright (c) 2019 Luca Ceresoli <luca@lucaceresoli.net> >> + * Copyright (c) 2022 Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/clk.h> >> +#include <linux/delay.h> >> +#include <linux/gpio/driver.h> >> +#include <linux/i2c-atr.h> >> +#include <linux/i2c.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_graph.h> >> +#include <linux/regmap.h> >> + >> +#include <media/i2c/ds90ub9xx.h> >> +#include <media/v4l2-subdev.h> >> + >> +#define UB913_PAD_SINK 0 >> +#define UB913_PAD_SOURCE 1 >> + >> +/* >> + * UB913 has 4 gpios, but gpios 3 and 4 are reserved for external oscillator >> + * mode. Thus we only support 2 gpios for now. >> + */ >> +#define UB913_NUM_GPIOS 2 >> + >> +#define UB913_REG_RESET_CTL 0x01 >> +#define UB913_REG_RESET_CTL_DIGITAL_RESET_1 BIT(1) >> +#define UB913_REG_RESET_CTL_DIGITAL_RESET_0 BIT(0) >> + >> +#define UB913_REG_GENERAL_CFG 0x03 >> +#define UB913_REG_MODE_SEL 0x05 >> + >> +#define UB913_REG_CRC_ERRORS_LSB 0x0a >> +#define UB913_REG_CRC_ERRORS_MSB 0x0b >> + >> +#define UB913_REG_GENERAL_STATUS 0x0c >> + >> +#define UB913_REG_GPIO_CFG(n) (0x0d + (n)) >> +#define UB913_REG_GPIO_CFG_ENABLE(n) BIT(0 + (n) * 4) >> +#define UB913_REG_GPIO_CFG_DIR_INPUT(n) BIT(1 + (n) * 4) >> +#define UB913_REG_GPIO_CFG_REMOTE_EN(n) BIT(2 + (n) * 4) >> +#define UB913_REG_GPIO_CFG_OUT_VAL(n) BIT(3 + (n) * 4) >> +#define UB913_REG_GPIO_CFG_MASK(n) (0xf << ((n) * 4)) >> + >> +#define UB913_REG_SCL_HIGH_TIME 0x11 >> +#define UB913_REG_SCL_LOW_TIME 0x12 >> + >> +#define UB913_REG_PLL_OVR 0x35 >> + >> +struct ub913_data { >> + struct i2c_client *client; >> + struct regmap *regmap; >> + struct clk *clkin; >> + >> + u32 gpio_func[UB913_NUM_GPIOS]; >> + >> + struct gpio_chip gpio_chip; >> + char gpio_chip_name[64]; >> + >> + struct v4l2_subdev sd; >> + struct media_pad pads[2]; >> + >> + struct v4l2_async_notifier notifier; >> + >> + struct v4l2_subdev *source_sd; >> + >> + u64 enabled_source_streams; >> + >> + struct device_node *tx_ep_np; >> + >> + struct clk_hw *clkout_clk_hw; >> + >> + struct ds90ub9xx_platform_data *plat_data; >> + >> + /* Have we succefully called i2c_atr_add_adapter() */ >> + bool has_i2c_adapter; >> +}; >> + >> +static inline struct ub913_data *sd_to_ub913(struct v4l2_subdev *sd) >> +{ >> + return container_of(sd, struct ub913_data, sd); >> +} >> + >> +static int ub913_read(const struct ub913_data *priv, u8 reg, u8 *val) >> +{ >> + unsigned int v; >> + int ret; >> + >> + ret = regmap_read(priv->regmap, reg, &v); >> + if (ret < 0) { >> + dev_err(&priv->client->dev, >> + "Cannot read register 0x%02x: %d!\n", reg, ret); >> + return ret; >> + } >> + >> + *val = v; >> + return 0; >> +} >> + >> +static int ub913_write(const struct ub913_data *priv, u8 reg, u8 val) >> +{ >> + int ret; >> + >> + ret = regmap_write(priv->regmap, reg, val); >> + if (ret < 0) >> + dev_err(&priv->client->dev, >> + "Cannot write register 0x%02x: %d!\n", reg, ret); >> + >> + return ret; >> +} >> + >> +/* >> + * GPIO chip >> + */ >> +static int ub913_gpio_get_direction(struct gpio_chip *gc, unsigned int offset) >> +{ >> + return GPIO_LINE_DIRECTION_OUT; >> +} >> + >> +static int ub913_gpio_direction_out(struct gpio_chip *gc, unsigned int offset, >> + int value) >> +{ >> + struct ub913_data *priv = gpiochip_get_data(gc); >> + unsigned int reg_idx; >> + unsigned int field_idx; >> + int ret; >> + >> + reg_idx = offset / 2; >> + field_idx = offset % 2; >> + >> + ret = regmap_update_bits( >> + priv->regmap, UB913_REG_GPIO_CFG(reg_idx), >> + UB913_REG_GPIO_CFG_MASK(field_idx), >> + UB913_REG_GPIO_CFG_ENABLE(field_idx) | >> + (value ? UB913_REG_GPIO_CFG_OUT_VAL(field_idx) : 0)); >> + >> + return ret; >> +} >> + >> +static void ub913_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) >> +{ >> + ub913_gpio_direction_out(gc, offset, value); >> +} >> + >> +static int ub913_gpio_of_xlate(struct gpio_chip *gc, >> + const struct of_phandle_args *gpiospec, >> + u32 *flags) >> +{ >> + if (flags) >> + *flags = gpiospec->args[1]; >> + >> + return gpiospec->args[0]; >> +} >> + >> +static int ub913_gpiochip_probe(struct ub913_data *priv) >> +{ >> + struct device *dev = &priv->client->dev; >> + struct gpio_chip *gc = &priv->gpio_chip; >> + int ret; >> + >> + /* Initialize GPIOs 0 and 1 to local control, tri-state */ >> + ub913_write(priv, UB913_REG_GPIO_CFG(0), 0); >> + >> + scnprintf(priv->gpio_chip_name, sizeof(priv->gpio_chip_name), "%s", >> + dev_name(dev)); >> + >> + gc->label = priv->gpio_chip_name; >> + gc->parent = dev; >> + gc->owner = THIS_MODULE; >> + gc->base = -1; >> + gc->can_sleep = 1; >> + gc->ngpio = UB913_NUM_GPIOS; >> + gc->get_direction = ub913_gpio_get_direction; >> + gc->direction_output = ub913_gpio_direction_out; >> + gc->set = ub913_gpio_set; >> + gc->of_xlate = ub913_gpio_of_xlate; >> + gc->of_node = priv->client->dev.of_node; >> + gc->of_gpio_n_cells = 2; >> + >> + ret = gpiochip_add_data(gc, priv); >> + if (ret) { >> + dev_err(dev, "Failed to add GPIOs: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void ub913_gpiochip_remove(struct ub913_data *priv) >> +{ >> + gpiochip_remove(&priv->gpio_chip); >> +} >> + >> +static int ub913_parse_dt(struct ub913_data *priv) > > I would have moved this just before ub913_probe(). Sure. >> +{ >> + struct device_node *np = priv->client->dev.of_node; >> + struct device *dev = &priv->client->dev; >> + int ret; >> + >> + if (!np) { >> + dev_err(dev, "OF: no device tree node!\n"); >> + return -ENOENT; >> + } >> + >> + /* optional, if absent all GPIO pins are unused */ >> + ret = of_property_read_u32_array(np, "gpio-functions", priv->gpio_func, >> + ARRAY_SIZE(priv->gpio_func)); >> + if (ret && ret != -EINVAL) >> + dev_err(dev, "DT: invalid gpio-functions property (%d)", ret); >> + >> + return 0; >> +} >> + >> +static const struct regmap_config ub913_regmap_config = { >> + .name = "ds90ub913", >> + .reg_bits = 8, >> + .val_bits = 8, >> + .reg_format_endian = REGMAP_ENDIAN_DEFAULT, >> + .val_format_endian = REGMAP_ENDIAN_DEFAULT, >> +}; >> + >> +/* >> + * V4L2 >> + */ >> + >> +static int ub913_enable_streams(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + struct media_pad *remote_pad; >> + u64 sink_streams; >> + int ret; >> + >> + if (streams_mask & priv->enabled_source_streams) >> + return -EALREADY; >> + >> + sink_streams = v4l2_subdev_state_xlate_streams( >> + state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); >> + >> + remote_pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); >> + >> + ret = v4l2_subdev_enable_streams(priv->source_sd, remote_pad->index, >> + sink_streams); >> + if (ret) >> + return ret; >> + >> + priv->enabled_source_streams |= streams_mask; >> + >> + return 0; >> +} >> + >> +static int ub913_disable_streams(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, u32 pad, >> + u64 streams_mask) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + struct media_pad *remote_pad; >> + int ret; >> + u64 sink_streams; >> + >> + if ((streams_mask & priv->enabled_source_streams) != streams_mask) >> + return -EALREADY; >> + >> + sink_streams = v4l2_subdev_state_xlate_streams( >> + state, UB913_PAD_SOURCE, UB913_PAD_SINK, &streams_mask); >> + >> + remote_pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); >> + >> + ret = v4l2_subdev_disable_streams(priv->source_sd, remote_pad->index, >> + sink_streams); >> + if (ret) >> + return ret; >> + >> + priv->enabled_source_streams &= ~streams_mask; >> + >> + return 0; >> +} >> + >> +static int _ub913_set_routing(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, >> + struct v4l2_subdev_krouting *routing) >> +{ >> + const struct v4l2_mbus_framefmt format = { > > static Yep. >> + .width = 640, >> + .height = 480, >> + .code = MEDIA_BUS_FMT_UYVY8_2X8, >> + .field = V4L2_FIELD_NONE, >> + .colorspace = V4L2_COLORSPACE_SRGB, >> + .ycbcr_enc = V4L2_YCBCR_ENC_601, >> + .quantization = V4L2_QUANTIZATION_LIM_RANGE, >> + .xfer_func = V4L2_XFER_FUNC_SRGB, >> + }; >> + int ret; >> + >> + /* >> + * Note: we can only support up to V4L2_FRAME_DESC_ENTRY_MAX, until >> + * frame desc is made dynamically allocated. >> + */ >> + >> + if (routing->num_routes > V4L2_FRAME_DESC_ENTRY_MAX) >> + return -EINVAL; >> + >> + ret = v4l2_subdev_routing_validate(sd, routing, >> + V4L2_SUBDEV_ROUTING_ONLY_1_TO_1); >> + if (ret) >> + return ret; >> + >> + ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int ub913_set_routing(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, >> + enum v4l2_subdev_format_whence which, >> + struct v4l2_subdev_krouting *routing) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + >> + if (which == V4L2_SUBDEV_FORMAT_ACTIVE && priv->enabled_source_streams) >> + return -EBUSY; >> + >> + return _ub913_set_routing(sd, state, routing); >> +} >> + >> +static int ub913_get_source_frame_desc(struct ub913_data *priv, >> + struct v4l2_mbus_frame_desc *desc) >> +{ >> + struct media_pad *pad; >> + int ret; >> + >> + pad = media_pad_remote_pad_first(&priv->pads[UB913_PAD_SINK]); >> + if (!pad) >> + return -EPIPE; >> + >> + ret = v4l2_subdev_call(priv->source_sd, pad, get_frame_desc, pad->index, >> + desc); >> + if (ret) >> + return ret; >> + >> + return 0; > > I would inline this in the caller. Well... Having it separate doesn't provide too much benefit, but it does make it a bit cleaner as we don't need to play with the 'unsigned int pad' and the 'struct media_pad *pad' in the same function. >> +} >> + >> +static int ub913_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, >> + struct v4l2_mbus_frame_desc *fd) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + const struct v4l2_subdev_krouting *routing; >> + struct v4l2_mbus_frame_desc source_fd; >> + struct v4l2_subdev_route *route; >> + struct v4l2_subdev_state *state; >> + int ret = 0; > > No need to initialize this to 0. Yep. >> + >> + if (pad != 1) /* first tx pad */ > > if (pad != UB913_PAD_SOURCE) > > and drop the comment. Yep. >> + return -EINVAL; >> + >> + ret = ub913_get_source_frame_desc(priv, &source_fd); >> + if (ret) >> + return ret; >> + >> + state = v4l2_subdev_lock_and_get_active_state(sd); >> + >> + routing = &state->routing; >> + >> + memset(fd, 0, sizeof(*fd)); >> + >> + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL; >> + >> + for_each_active_route(routing, route) { >> + unsigned int j; > > Anything wrong with 'i' ? I'm pretty sure it was reserved for the route iteration, before we had for_each_active_route() =). >> + >> + if (route->source_pad != pad) >> + continue; >> + >> + for (j = 0; j < source_fd.num_entries; ++j) >> + if (source_fd.entry[j].stream == route->sink_stream) >> + break; >> + >> + if (j == source_fd.num_entries) { >> + dev_err(&priv->client->dev, >> + "Failed to find stream from source frame desc\n"); >> + ret = -EPIPE; >> + goto out; >> + } >> + >> + fd->entry[fd->num_entries].stream = route->source_stream; >> + >> + fd->entry[fd->num_entries].flags = >> + V4L2_MBUS_FRAME_DESC_FL_LEN_MAX; > > Shouldn't this be set only if set in the source frame descriptor ? Yes... I seem to be doing the same in the other ub9xx drivers. I'm not sure where this came originally. >> + fd->entry[fd->num_entries].length = source_fd.entry[j].length; >> + fd->entry[fd->num_entries].pixelcode = >> + source_fd.entry[j].pixelcode; >> + >> + fd->num_entries++; >> + } >> + >> +out: >> + v4l2_subdev_unlock_state(state); >> + >> + return ret; >> +} >> + >> +static int ub913_set_fmt(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state, >> + struct v4l2_subdev_format *format) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + struct v4l2_mbus_framefmt *fmt; >> + >> + if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && >> + priv->enabled_source_streams) >> + return -EBUSY; >> + >> + /* No transcoding, source and sink formats must match. */ >> + if (format->pad == 1) > > if (format->pad == UB913_PAD_SOURCE) Yep. >> + return v4l2_subdev_get_fmt(sd, state, format); >> + >> + /* Set sink format */ >> + fmt = v4l2_subdev_state_get_stream_format(state, format->pad, >> + format->stream); >> + if (!fmt) >> + return -EINVAL; >> + >> + *fmt = format->format; >> + >> + /* Propagate to source format */ >> + fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad, >> + format->stream); >> + if (!fmt) >> + return -EINVAL; >> + >> + *fmt = format->format; >> + >> + return 0; >> +} >> + >> +static int ub913_init_cfg(struct v4l2_subdev *sd, >> + struct v4l2_subdev_state *state) >> +{ >> + struct v4l2_subdev_route routes[] = { >> + { >> + .sink_pad = 0, > > .sink_pad = UB913_PAD_SINK, > >> + .sink_stream = 0, >> + .source_pad = 1, > > .source_pad = UB913_PAD_SOURCE, Yep. >> + .source_stream = 0, >> + .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE, >> + }, >> + }; >> + >> + struct v4l2_subdev_krouting routing = { >> + .num_routes = ARRAY_SIZE(routes), >> + .routes = routes, >> + }; >> + >> + return _ub913_set_routing(sd, state, &routing); >> +} >> + >> +static int ub913_log_status(struct v4l2_subdev *sd) >> +{ >> + struct ub913_data *priv = sd_to_ub913(sd); >> + struct device *dev = &priv->client->dev; >> + u8 v, v1, v2; >> + >> + ub913_read(priv, UB913_REG_MODE_SEL, &v); >> + dev_info(dev, "MODE_SEL %#x\n", v); >> + >> + ub913_read(priv, UB913_REG_CRC_ERRORS_LSB, &v1); >> + ub913_read(priv, UB913_REG_CRC_ERRORS_MSB, &v2); >> + dev_info(dev, "CRC errors %u\n", v1 | (v2 << 8)); >> + >> + ub913_read(priv, UB913_REG_GENERAL_STATUS, &v); >> + dev_info(dev, "GENERAL_STATUS %#x\n", v); >> + >> + ub913_read(priv, UB913_REG_PLL_OVR, &v); >> + dev_info(dev, "PLL_OVR %#x\n", v); >> + >> + /* clear CRC errors */ >> + ub913_read(priv, UB913_REG_GENERAL_CFG, &v); >> + ub913_write(priv, UB913_REG_GENERAL_CFG, v | BIT(5)); >> + ub913_write(priv, UB913_REG_GENERAL_CFG, v); >> + >> + return 0; >> +} >> + >> +static const struct v4l2_subdev_core_ops ub913_subdev_core_ops = { >> + .log_status = ub913_log_status, >> +}; >> + >> +static const struct v4l2_subdev_pad_ops ub913_pad_ops = { >> + .enable_streams = ub913_enable_streams, >> + .disable_streams = ub913_disable_streams, >> + .set_routing = ub913_set_routing, >> + .get_frame_desc = ub913_get_frame_desc, >> + .get_fmt = v4l2_subdev_get_fmt, >> + .set_fmt = ub913_set_fmt, >> + .init_cfg = ub913_init_cfg, >> +}; >> + >> +static const struct v4l2_subdev_ops ub913_subdev_ops = { >> + .core = &ub913_subdev_core_ops, >> + .pad = &ub913_pad_ops, >> +}; >> + >> +static const struct media_entity_operations ub913_entity_ops = { >> + .link_validate = v4l2_subdev_link_validate, >> +}; >> + >> +static int ub913_notify_bound(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *source_subdev, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct ub913_data *priv = sd_to_ub913(notifier->sd); >> + struct device *dev = &priv->client->dev; >> + unsigned int src_pad; >> + int ret; >> + >> + dev_dbg(dev, "Bind %s\n", source_subdev->name); > > I'd drop this message. Why is that? Do we get this easily from the v4l2 core? These debug prints in the bind/unbind process have been valuable for me. >> + >> + ret = media_entity_get_fwnode_pad(&source_subdev->entity, >> + source_subdev->fwnode, >> + MEDIA_PAD_FL_SOURCE); >> + if (ret < 0) { >> + dev_err(dev, "Failed to find pad for %s\n", >> + source_subdev->name); >> + return ret; >> + } >> + >> + priv->source_sd = source_subdev; >> + src_pad = ret; >> + >> + ret = media_create_pad_link(&source_subdev->entity, src_pad, >> + &priv->sd.entity, 0, > > &priv->sd.entity, UB913_PAD_SINK, Yep. >> + MEDIA_LNK_FL_ENABLED | >> + MEDIA_LNK_FL_IMMUTABLE); >> + if (ret) { >> + dev_err(dev, "Unable to link %s:%u -> %s:0\n", >> + source_subdev->name, src_pad, priv->sd.name); >> + return ret; >> + } >> + >> + dev_dbg(dev, "Bound %s:%u\n", source_subdev->name, src_pad); >> + >> + dev_dbg(dev, "All subdevs bound\n"); > > I'd drop this message. > >> + >> + return 0; >> +} >> + >> +static void ub913_notify_unbind(struct v4l2_async_notifier *notifier, >> + struct v4l2_subdev *source_subdev, >> + struct v4l2_async_subdev *asd) >> +{ >> + struct ub913_data *priv = sd_to_ub913(notifier->sd); >> + struct device *dev = &priv->client->dev; >> + >> + dev_dbg(dev, "Unbind %s\n", source_subdev->name); >> +} > > This is a no-op so you can drop it. This has been useful for development, but, yes, perhaps it's time to drop it. >> + >> +static const struct v4l2_async_notifier_operations ub913_notify_ops = { >> + .bound = ub913_notify_bound, >> + .unbind = ub913_notify_unbind, >> +}; >> + >> +static int ub913_v4l2_notifier_register(struct ub913_data *priv) >> +{ >> + struct device *dev = &priv->client->dev; >> + struct v4l2_async_subdev *asd; >> + struct device_node *ep_node; >> + int ret; >> + >> + dev_dbg(dev, "register async notif\n"); >> + >> + ep_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0); >> + if (!ep_node) { >> + dev_err(dev, "No graph endpoint\n"); >> + return -ENODEV; >> + } >> + >> + v4l2_async_nf_init(&priv->notifier); >> + >> + asd = v4l2_async_nf_add_fwnode_remote(&priv->notifier, >> + of_fwnode_handle(ep_node), >> + struct v4l2_async_subdev); >> + >> + of_node_put(ep_node); >> + >> + if (IS_ERR(asd)) { >> + dev_err(dev, "Failed to add subdev: %ld", PTR_ERR(asd)); >> + v4l2_async_nf_cleanup(&priv->notifier); >> + return PTR_ERR(asd); >> + } >> + >> + priv->notifier.ops = &ub913_notify_ops; >> + >> + ret = v4l2_async_subdev_nf_register(&priv->sd, &priv->notifier); >> + if (ret) { >> + dev_err(dev, "Failed to register subdev_notifier"); >> + v4l2_async_nf_cleanup(&priv->notifier); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static void ub913_v4l2_nf_unregister(struct ub913_data *priv) >> +{ >> + struct device *dev = &priv->client->dev; >> + >> + dev_dbg(dev, "Unregister async notif\n"); >> + >> + v4l2_async_nf_unregister(&priv->notifier); >> + v4l2_async_nf_cleanup(&priv->notifier); >> +} >> + >> +static int ub913_register_clkout(struct ub913_data *priv) >> +{ >> + struct device *dev = &priv->client->dev; >> + const char *name; >> + int ret; >> + >> + name = kasprintf(GFP_KERNEL, "ds90ub913.%s.clk_out", dev_name(dev)); >> + >> + priv->clkout_clk_hw = devm_clk_hw_register_fixed_factor(dev, name, >> + __clk_get_name(priv->clkin), 0, 1, 2); >> + >> + kfree(name); >> + >> + if (IS_ERR(priv->clkout_clk_hw)) >> + return dev_err_probe(dev, PTR_ERR(priv->clkout_clk_hw), >> + "Cannot register clkout hw\n"); >> + >> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, >> + priv->clkout_clk_hw); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "Cannot add OF clock provider\n"); >> + >> + return 0; >> +} >> + >> +static int ub913_i2c_master_init(struct ub913_data *priv) >> +{ >> + /* i2c fast mode */ >> + u32 scl_high = 600 + 300; /* high period + rise time, ns */ >> + u32 scl_low = 1300 + 300; /* low period + fall time, ns */ >> + unsigned long ref; >> + int ret; >> + >> + ref = clk_get_rate(priv->clkin) / 2; >> + >> + scl_high = div64_u64((u64)scl_high * ref, 1000000000); >> + scl_low = div64_u64((u64)scl_low * ref, 1000000000); >> + >> + ret = ub913_write(priv, UB913_REG_SCL_HIGH_TIME, scl_high); >> + if (ret) >> + return ret; >> + >> + ret = ub913_write(priv, UB913_REG_SCL_LOW_TIME, scl_low); >> + if (ret) >> + return ret; >> + >> + return 0; >> +} >> + >> +static int ub913_add_i2c_adapter(struct ub913_data *priv) >> +{ >> + struct device *dev = &priv->client->dev; >> + struct fwnode_handle *i2c_handle; >> + int ret; >> + >> + i2c_handle = device_get_named_child_node(dev, "i2c"); >> + if (!i2c_handle) >> + return 0; >> + >> + ret = i2c_atr_add_adapter(priv->plat_data->atr, priv->plat_data->port, >> + i2c_handle); >> + >> + fwnode_handle_put(i2c_handle); >> + >> + if (ret) >> + return ret; >> + >> + priv->has_i2c_adapter = true; >> + >> + return 0; >> +} >> + >> +static void ub913_remove_i2c_adapter(struct ub913_data *priv) >> +{ >> + if (priv->has_i2c_adapter) >> + i2c_atr_del_adapter(priv->plat_data->atr, >> + priv->plat_data->port); > > Could i2c_atr_del_adapter() be a no-op if no adapter for the port has > been added ? You could then drop the has_i2c_adapter field. I'm also Yes, I think that'd be fine. > wondering if the struct device of the DS90UB913 could be passed instead > of the port, to avoid passing the port throught > ds90ub9xx_platform_data. Interesting thought. That would limit the number of remote i2c busses to one, though. Not a problem for FPD-Link, but I wonder if that's assuming too much for the future users. Then again, this is an in-kernel API so we could extend it later if needed. So I'll try this out and see if I hit any issues. >> +} >> + >> +static int ub913_probe(struct i2c_client *client) >> +{ >> + struct device *dev = &client->dev; >> + struct ub913_data *priv; >> + int ret; >> + u8 v; >> + bool mode_override; >> + u8 mode; >> + >> + dev_dbg(dev, "probing, addr 0x%02x\n", client->addr); >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->client = client; >> + >> + priv->plat_data = dev_get_platdata(&client->dev); >> + if (!priv->plat_data) { >> + dev_err(dev, "Platform data missing\n"); >> + return -ENODEV; >> + } >> + >> + priv->regmap = devm_regmap_init_i2c(client, &ub913_regmap_config); >> + if (IS_ERR(priv->regmap)) { >> + dev_err(dev, "Failed to init regmap\n"); >> + return PTR_ERR(priv->regmap); >> + } >> + >> + /* ub913 can also work without ext clock, but that is not supported */ >> + priv->clkin = devm_clk_get(dev, "clkin"); >> + if (IS_ERR(priv->clkin)) { >> + ret = PTR_ERR(priv->clkin); >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "Cannot get CLKIN (%d)", ret); >> + return ret; >> + } >> + >> + ret = ub913_parse_dt(priv); >> + if (ret) >> + return ret; >> + >> + ret = ub913_read(priv, UB913_REG_MODE_SEL, &v); >> + if (ret) >> + return ret; >> + >> + if (!(v & BIT(4))) { > > Please add a mcro for this. Same for other magic bits in the driver. Sure. >> + dev_err(dev, "Mode value not stabilized\n"); >> + return -ENODEV; >> + } >> + >> + mode_override = v & BIT(5); >> + mode = v & 0xf; >> + >> + dev_dbg(dev, "mode from %s: %#x\n", >> + mode_override ? "reg" : "deserializer", mode); >> + >> + ret = ub913_i2c_master_init(priv); >> + if (ret) { >> + dev_err(dev, "i2c master init failed: %d\n", ret); >> + return ret; >> + } >> + >> + ret = ub913_gpiochip_probe(priv); >> + if (ret) { >> + dev_err(dev, "Failed to init gpiochip\n"); >> + return ret; >> + } >> + >> + ret = ub913_register_clkout(priv); >> + if (ret) { >> + dev_err(dev, "Failed to register clkout\n"); >> + goto err_gpiochip_remove; >> + } >> + >> + v4l2_i2c_subdev_init(&priv->sd, priv->client, &ub913_subdev_ops); >> + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; >> + priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; >> + priv->sd.entity.ops = &ub913_entity_ops; >> + >> + priv->pads[0].flags = MEDIA_PAD_FL_SINK; >> + priv->pads[1].flags = MEDIA_PAD_FL_SOURCE; >> + >> + ret = media_entity_pads_init(&priv->sd.entity, 2, priv->pads); >> + if (ret) { >> + dev_err(dev, "Failed to init pads\n"); >> + goto err_gpiochip_remove; >> + } >> + >> + priv->tx_ep_np = of_graph_get_endpoint_by_regs(dev->of_node, 1, 0); >> + if (priv->tx_ep_np) >> + priv->sd.fwnode = of_fwnode_handle(priv->tx_ep_np); >> + >> + ret = v4l2_subdev_init_finalize(&priv->sd); >> + if (ret) >> + goto err_entity_cleanup; >> + >> + ret = ub913_v4l2_notifier_register(priv); >> + if (ret) { >> + dev_err(dev, "v4l2 subdev notifier register failed: %d\n", ret); >> + goto err_free_state; >> + } >> + >> + ret = v4l2_async_register_subdev(&priv->sd); >> + if (ret) { >> + dev_err(dev, "v4l2_async_register_subdev error: %d\n", ret); >> + goto err_unreg_notif; >> + } >> + >> + ret = ub913_add_i2c_adapter(priv); >> + if (ret) { >> + dev_err(dev, "failed to add remote i2c adapter\n"); >> + goto err_unreg_async_subdev; >> + } >> + >> + dev_dbg(dev, "Successfully probed\n"); >> + >> + return 0; >> + >> +err_unreg_async_subdev: >> + v4l2_async_unregister_subdev(&priv->sd); >> +err_unreg_notif: >> + ub913_v4l2_nf_unregister(priv); >> +err_free_state: >> + v4l2_subdev_cleanup(&priv->sd); >> +err_entity_cleanup: >> + if (priv->tx_ep_np) >> + of_node_put(priv->tx_ep_np); >> + >> + media_entity_cleanup(&priv->sd.entity); >> +err_gpiochip_remove: >> + ub913_gpiochip_remove(priv); >> + >> + return ret; >> +} >> + >> +static void ub913_remove(struct i2c_client *client) >> +{ >> + struct v4l2_subdev *sd = i2c_get_clientdata(client); >> + struct ub913_data *priv = sd_to_ub913(sd); >> + >> + dev_dbg(&client->dev, "Removing\n"); >> + >> + ub913_remove_i2c_adapter(priv); >> + >> + v4l2_async_unregister_subdev(&priv->sd); >> + >> + ub913_v4l2_nf_unregister(priv); >> + >> + v4l2_subdev_cleanup(&priv->sd); >> + >> + if (priv->tx_ep_np) >> + of_node_put(priv->tx_ep_np); >> + >> + media_entity_cleanup(&priv->sd.entity); >> + >> + ub913_gpiochip_remove(priv); >> +} >> + >> +static const struct i2c_device_id ub913_id[] = { { "ds90ub913a-q1", 0 }, {} }; >> +MODULE_DEVICE_TABLE(i2c, ub913_id); >> + >> +#ifdef CONFIG_OF >> +static const struct of_device_id ub913_dt_ids[] = { >> + { .compatible = "ti,ds90ub913a-q1", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, ub913_dt_ids); >> +#endif >> + >> +static struct i2c_driver ds90ub913_driver = { >> + .probe_new = ub913_probe, >> + .remove = ub913_remove, >> + .id_table = ub913_id, >> + .driver = { >> + .name = "ds90ub913a", >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(ub913_dt_ids), >> + }, >> +}; >> + >> +module_i2c_driver(ds90ub913_driver); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("Texas Instruments DS90UB913 serializer driver"); >> +MODULE_AUTHOR("Luca Ceresoli <luca@lucaceresoli.net>"); >> +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>"); >> -- >> 2.34.1 >> >
On 14/12/2022 08:29, Tomi Valkeinen wrote: >> wondering if the struct device of the DS90UB913 could be passed instead >> of the port, to avoid passing the port throught >> ds90ub9xx_platform_data. > > Interesting thought. That would limit the number of remote i2c busses to > one, though. Not a problem for FPD-Link, but I wonder if that's assuming > too much for the future users. Then again, this is an in-kernel API so > we could extend it later if needed. So I'll try this out and see if I > hit any issues. Right, so the issue with this one would be that it would prevent a single device uses. E.g. a single chip which acts as an ATR (similar to i2c-mux chips), i.e. it contains both the main and the remote i2c busses. Tomi
Hi Laurent, On 11/12/2022 20:33, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: >> Add driver for TI DS90UB913 FPDLink-3 Serializer. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/i2c/Kconfig | 13 + >> drivers/media/i2c/Makefile | 2 +- >> drivers/media/i2c/ds90ub913.c | 892 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 906 insertions(+), 1 deletion(-) >> create mode 100644 drivers/media/i2c/ds90ub913.c Many of your comments here also apply to the ub953 driver. I'll fix those. Tomi
Hello Laurent, thanks for the feedback and apologies for the delayed reply. On Sun, 11 Dec 2022 18:55:39 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Tomi and Luca, > > Thank you for the patch. > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: > > From: Luca Ceresoli <luca@lucaceresoli.net> > > > > An adapter might need to know when a new device is about to be > > added. This will soon bee needed to implement an "I2C address > > translator" (ATR for short), a device that propagates I2C transactions > > with a different slave address (an "alias" address). An ATR driver > > needs to know when a slave is being added to find a suitable alias and > > program the device translation map. > > > > Add an attach/detach callback pair to allow adapter drivers to be > > notified of clients being added and removed. > > This may be a stupid question, but couldn't you instead use the > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ? I'm not sure they would be the correct tool for this task. Bus notifiers inform about new events on the 'struct bus_type, i.e. any event on the global i2c bus type. In the i2c world this means being notified about new _adapters_, which is exactly what drivers/i2c/i2c-dev.c does. Here, however, we need to be informed about new _clients_ being added under a specific adapter. I'm not sure whether the bus notifiers can inform about new clients in addition of new adapters, but they at least seem unable to provide per-adapter notification. Does that seem correct? Best regards,
On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote: > On Sun, 11 Dec 2022 18:55:39 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: ... > > This may be a stupid question, but couldn't you instead use the > > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ? > > I'm not sure they would be the correct tool for this task. Bus > notifiers inform about new events on the 'struct bus_type, i.e. any > event on the global i2c bus type. In the i2c world this means being > notified about new _adapters_, which is exactly what > drivers/i2c/i2c-dev.c does. > > Here, however, we need to be informed about new _clients_ being added > under a specific adapter. This is for example exactly what ACPI integration in I2C framework does. But... > I'm not sure whether the bus notifiers can > inform about new clients in addition of new adapters, but they at least > seem unable to provide per-adapter notification. ...personally I don't like notifiers, they looks like overkill for this task. > Does that seem correct?
On Mon, Dec 19, 2022 at 11:48:51AM +0200, Andy Shevchenko wrote: > On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote: > > On Sun, 11 Dec 2022 18:55:39 +0200 Laurent Pinchart wrote: > > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: > > ... > > > > This may be a stupid question, but couldn't you instead use the > > > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ? > > > > I'm not sure they would be the correct tool for this task. Bus > > notifiers inform about new events on the 'struct bus_type, i.e. any > > event on the global i2c bus type. In the i2c world this means being > > notified about new _adapters_, which is exactly what > > drivers/i2c/i2c-dev.c does. > > > > Here, however, we need to be informed about new _clients_ being added > > under a specific adapter. > > This is for example exactly what ACPI integration in I2C framework does. But... > > > I'm not sure whether the bus notifiers can > > inform about new clients in addition of new adapters, but they at least > > seem unable to provide per-adapter notification. > > ...personally I don't like notifiers, they looks like overkill for this task. But isn't this patch essentially implementing a custom notification system ? If we need notifiers, why would it be better to add an ad-hoc API for I2C instead of using bus notifiers ? > > Does that seem correct?
Hi Tomi, On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: > On 14/12/2022 08:29, Tomi Valkeinen wrote: > > >> wondering if the struct device of the DS90UB913 could be passed instead > >> of the port, to avoid passing the port throught > >> ds90ub9xx_platform_data. > > > > Interesting thought. That would limit the number of remote i2c busses to > > one, though. Not a problem for FPD-Link, but I wonder if that's assuming > > too much for the future users. Then again, this is an in-kernel API so > > we could extend it later if needed. So I'll try this out and see if I > > hit any issues. > > Right, so the issue with this one would be that it would prevent a > single device uses. E.g. a single chip which acts as an ATR (similar to > i2c-mux chips), i.e. it contains both the main and the remote i2c busses. I don't think I understand this, sorry.
Hi Tomi, On Wed, Dec 14, 2022 at 08:29:48AM +0200, Tomi Valkeinen wrote: > On 11/12/2022 20:33, Laurent Pinchart wrote: > > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: > >> Add driver for TI DS90UB913 FPDLink-3 Serializer. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> drivers/media/i2c/Kconfig | 13 + > >> drivers/media/i2c/Makefile | 2 +- > >> drivers/media/i2c/ds90ub913.c | 892 ++++++++++++++++++++++++++++++++++ > >> 3 files changed, 906 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/media/i2c/ds90ub913.c [snip] > >> diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c > >> new file mode 100644 > >> index 000000000000..6001a622e622 > >> --- /dev/null > >> +++ b/drivers/media/i2c/ds90ub913.c > >> @@ -0,0 +1,892 @@ [snip] > >> +static int ub913_notify_bound(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *source_subdev, > >> + struct v4l2_async_subdev *asd) > >> +{ > >> + struct ub913_data *priv = sd_to_ub913(notifier->sd); > >> + struct device *dev = &priv->client->dev; > >> + unsigned int src_pad; > >> + int ret; > >> + > >> + dev_dbg(dev, "Bind %s\n", source_subdev->name); > > > > I'd drop this message. > > Why is that? Do we get this easily from the v4l2 core? These debug > prints in the bind/unbind process have been valuable for me. Because debug messages are not meant to be a tracing infrastructure, and because, if we want to keep this message, it would be best handled in the v4l2-async core instead of being duplicated across drivers. Same for the messages at the end of the function. > >> + > >> + ret = media_entity_get_fwnode_pad(&source_subdev->entity, > >> + source_subdev->fwnode, > >> + MEDIA_PAD_FL_SOURCE); > >> + if (ret < 0) { > >> + dev_err(dev, "Failed to find pad for %s\n", > >> + source_subdev->name); > >> + return ret; > >> + } > >> + > >> + priv->source_sd = source_subdev; > >> + src_pad = ret; > >> + > >> + ret = media_create_pad_link(&source_subdev->entity, src_pad, > >> + &priv->sd.entity, 0, > > > > &priv->sd.entity, UB913_PAD_SINK, > > Yep. > > >> + MEDIA_LNK_FL_ENABLED | > >> + MEDIA_LNK_FL_IMMUTABLE); > >> + if (ret) { > >> + dev_err(dev, "Unable to link %s:%u -> %s:0\n", > >> + source_subdev->name, src_pad, priv->sd.name); > >> + return ret; > >> + } > >> + > >> + dev_dbg(dev, "Bound %s:%u\n", source_subdev->name, src_pad); > >> + > >> + dev_dbg(dev, "All subdevs bound\n"); > > > > I'd drop this message. > > > >> + > >> + return 0; > >> +} > >> + > >> +static void ub913_notify_unbind(struct v4l2_async_notifier *notifier, > >> + struct v4l2_subdev *source_subdev, > >> + struct v4l2_async_subdev *asd) > >> +{ > >> + struct ub913_data *priv = sd_to_ub913(notifier->sd); > >> + struct device *dev = &priv->client->dev; > >> + > >> + dev_dbg(dev, "Unbind %s\n", source_subdev->name); > >> +} > > > > This is a no-op so you can drop it. > > This has been useful for development, but, yes, perhaps it's time to > drop it. > > >> + > >> +static const struct v4l2_async_notifier_operations ub913_notify_ops = { > >> + .bound = ub913_notify_bound, > >> + .unbind = ub913_notify_unbind, > >> +}; [snip]
On 26/12/2022 18:56, Laurent Pinchart wrote: > Hi Tomi, > > On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: >> On 14/12/2022 08:29, Tomi Valkeinen wrote: >> >>>> wondering if the struct device of the DS90UB913 could be passed instead >>>> of the port, to avoid passing the port throught >>>> ds90ub9xx_platform_data. >>> >>> Interesting thought. That would limit the number of remote i2c busses to >>> one, though. Not a problem for FPD-Link, but I wonder if that's assuming >>> too much for the future users. Then again, this is an in-kernel API so >>> we could extend it later if needed. So I'll try this out and see if I >>> hit any issues. >> >> Right, so the issue with this one would be that it would prevent a >> single device uses. E.g. a single chip which acts as an ATR (similar to >> i2c-mux chips), i.e. it contains both the main and the remote i2c busses. > > I don't think I understand this, sorry. What you are suggesting above means that we'd have a separate device for each port of the ATR. Which is fine in our current case, as the i2c master busses are behind separate remote devices. But if you consider a case similar to i2c-mux, where we have a single chip with the slave bus and, say, 4 master busses. We would probably have only a single device for that. Tomi
On Mon, Dec 26, 2022 at 06:54:37PM +0200, Laurent Pinchart wrote: > On Mon, Dec 19, 2022 at 11:48:51AM +0200, Andy Shevchenko wrote: > > On Mon, Dec 19, 2022 at 09:51:43AM +0100, Luca Ceresoli wrote: > > > On Sun, 11 Dec 2022 18:55:39 +0200 Laurent Pinchart wrote: > > > > On Thu, Dec 08, 2022 at 12:39:59PM +0200, Tomi Valkeinen wrote: ... > > > > This may be a stupid question, but couldn't you instead use the > > > > BUS_NOTIFY_ADD_DEVICE and BUS_NOTIFY_DEL_DEVICE bus notifiers ? > > > > > > I'm not sure they would be the correct tool for this task. Bus > > > notifiers inform about new events on the 'struct bus_type, i.e. any > > > event on the global i2c bus type. In the i2c world this means being > > > notified about new _adapters_, which is exactly what > > > drivers/i2c/i2c-dev.c does. > > > > > > Here, however, we need to be informed about new _clients_ being added > > > under a specific adapter. > > > > This is for example exactly what ACPI integration in I2C framework does. But... > > > > > I'm not sure whether the bus notifiers can > > > inform about new clients in addition of new adapters, but they at least > > > seem unable to provide per-adapter notification. > > > > ...personally I don't like notifiers, they looks like overkill for this task. > > But isn't this patch essentially implementing a custom notification > system ? If we need notifiers, why would it be better to add an ad-hoc > API for I2C instead of using bus notifiers ? Notifiers (as implemented in the Linux kernel) have some drawbacks IIUC. For example, it's not easy to pass the data over it. Another example is the context (you wouldn't know when the notifier be called and if it can hold some locks or not). That said, each case should be discussed individually despite the generic approach being present (i.o.w. do not consider notifiers as a silver bullet in _any_ notification scheme). > > > Does that seem correct?
On Mon, Dec 26, 2022 at 07:01:11PM +0200, Laurent Pinchart wrote: > On Wed, Dec 14, 2022 at 08:29:48AM +0200, Tomi Valkeinen wrote: > > On 11/12/2022 20:33, Laurent Pinchart wrote: > > > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: ... > > >> + dev_dbg(dev, "Bind %s\n", source_subdev->name); > > > > > > I'd drop this message. +1 here. > > Why is that? Do we get this easily from the v4l2 core? These debug > > prints in the bind/unbind process have been valuable for me. > > Because debug messages are not meant to be a tracing infrastructure, and > because, if we want to keep this message, it would be best handled in > the v4l2-async core instead of being duplicated across drivers. Same for > the messages at the end of the function. I don't think v4l2 needs debug prints. If we consider the above case, the ftrace already provides that. If we consider something specific to v4l2 to trace only critical parts, then trace events should be implemented.
On Tue, Dec 27, 2022 at 10:09:43PM +0200, Andy Shevchenko wrote: > On Mon, Dec 26, 2022 at 07:01:11PM +0200, Laurent Pinchart wrote: > > On Wed, Dec 14, 2022 at 08:29:48AM +0200, Tomi Valkeinen wrote: > > > On 11/12/2022 20:33, Laurent Pinchart wrote: > > > > On Thu, Dec 08, 2022 at 12:40:05PM +0200, Tomi Valkeinen wrote: > > ... > > > > >> + dev_dbg(dev, "Bind %s\n", source_subdev->name); > > > > > > > > I'd drop this message. > > +1 here. > > > > Why is that? Do we get this easily from the v4l2 core? These debug > > > prints in the bind/unbind process have been valuable for me. > > > > Because debug messages are not meant to be a tracing infrastructure, and > > because, if we want to keep this message, it would be best handled in > > the v4l2-async core instead of being duplicated across drivers. Same for > > the messages at the end of the function. > > I don't think v4l2 needs debug prints. If we consider the above case, the > ftrace already provides that. If we consider something specific to v4l2 to > trace only critical parts, then trace events should be implemented. V4L2 has no support for trace events, and I agree that they would be useful. It doesn't mean that dev_dbg() is entirely useless though, and sometimes the line between the two isn't very clear. In any case, for this very specific debug message, I don't think it should be printed in individual drivers, but it should be handled in the V4L2 core, either as a debug message or a trace event.
Hi Tomi, On Mon, Dec 26, 2022 at 09:25:34PM +0200, Tomi Valkeinen wrote: > On 26/12/2022 18:56, Laurent Pinchart wrote: > > On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: > >> On 14/12/2022 08:29, Tomi Valkeinen wrote: > >> > >>>> wondering if the struct device of the DS90UB913 could be passed instead > >>>> of the port, to avoid passing the port throught > >>>> ds90ub9xx_platform_data. > >>> > >>> Interesting thought. That would limit the number of remote i2c busses to > >>> one, though. Not a problem for FPD-Link, but I wonder if that's assuming > >>> too much for the future users. Then again, this is an in-kernel API so > >>> we could extend it later if needed. So I'll try this out and see if I > >>> hit any issues. > >> > >> Right, so the issue with this one would be that it would prevent a > >> single device uses. E.g. a single chip which acts as an ATR (similar to > >> i2c-mux chips), i.e. it contains both the main and the remote i2c busses. > > > > I don't think I understand this, sorry. > > What you are suggesting above means that we'd have a separate device for > each port of the ATR. Which is fine in our current case, as the i2c > master busses are behind separate remote devices. > > But if you consider a case similar to i2c-mux, where we have a single > chip with the slave bus and, say, 4 master busses. We would probably > have only a single device for that. Hmmm... Yes you're right, it won't work in that case. Maybe we could have two functions, the existing i2c_atr_add_adapter(), and another one that wraps it ? It would be nice if we could get rid of the platform data for the UB913 and UB953 drivers.
On 04/01/2023 15:55, Laurent Pinchart wrote: > Hi Tomi, > > On Mon, Dec 26, 2022 at 09:25:34PM +0200, Tomi Valkeinen wrote: >> On 26/12/2022 18:56, Laurent Pinchart wrote: >>> On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: >>>> On 14/12/2022 08:29, Tomi Valkeinen wrote: >>>> >>>>>> wondering if the struct device of the DS90UB913 could be passed instead >>>>>> of the port, to avoid passing the port throught >>>>>> ds90ub9xx_platform_data. >>>>> >>>>> Interesting thought. That would limit the number of remote i2c busses to >>>>> one, though. Not a problem for FPD-Link, but I wonder if that's assuming >>>>> too much for the future users. Then again, this is an in-kernel API so >>>>> we could extend it later if needed. So I'll try this out and see if I >>>>> hit any issues. >>>> >>>> Right, so the issue with this one would be that it would prevent a >>>> single device uses. E.g. a single chip which acts as an ATR (similar to >>>> i2c-mux chips), i.e. it contains both the main and the remote i2c busses. >>> >>> I don't think I understand this, sorry. >> >> What you are suggesting above means that we'd have a separate device for >> each port of the ATR. Which is fine in our current case, as the i2c >> master busses are behind separate remote devices. >> >> But if you consider a case similar to i2c-mux, where we have a single >> chip with the slave bus and, say, 4 master busses. We would probably >> have only a single device for that. > > Hmmm... Yes you're right, it won't work in that case. Maybe we could > have two functions, the existing i2c_atr_add_adapter(), and another one > that wraps it ? It would be nice if we could get rid of the platform > data for the UB913 and UB953 drivers. I wouldn't mind that at all, but we already have the bc_rate there. And I have a feeling that we might need more if we implement more features. And we also have the atr pointer there. Or do you think that could be dropped also? In your mail above you only mention the port, but maybe the deser could register the serializer device and port to the ATR, and then the ser could just use its device pointer instead of atr & port. Tomi
On Wed, Jan 04, 2023 at 04:13:17PM +0200, Tomi Valkeinen wrote: > On 04/01/2023 15:55, Laurent Pinchart wrote: > > Hi Tomi, > > > > On Mon, Dec 26, 2022 at 09:25:34PM +0200, Tomi Valkeinen wrote: > >> On 26/12/2022 18:56, Laurent Pinchart wrote: > >>> On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: > >>>> On 14/12/2022 08:29, Tomi Valkeinen wrote: > >>>> > >>>>>> wondering if the struct device of the DS90UB913 could be passed instead > >>>>>> of the port, to avoid passing the port throught > >>>>>> ds90ub9xx_platform_data. > >>>>> > >>>>> Interesting thought. That would limit the number of remote i2c busses to > >>>>> one, though. Not a problem for FPD-Link, but I wonder if that's assuming > >>>>> too much for the future users. Then again, this is an in-kernel API so > >>>>> we could extend it later if needed. So I'll try this out and see if I > >>>>> hit any issues. > >>>> > >>>> Right, so the issue with this one would be that it would prevent a > >>>> single device uses. E.g. a single chip which acts as an ATR (similar to > >>>> i2c-mux chips), i.e. it contains both the main and the remote i2c busses. > >>> > >>> I don't think I understand this, sorry. > >> > >> What you are suggesting above means that we'd have a separate device for > >> each port of the ATR. Which is fine in our current case, as the i2c > >> master busses are behind separate remote devices. > >> > >> But if you consider a case similar to i2c-mux, where we have a single > >> chip with the slave bus and, say, 4 master busses. We would probably > >> have only a single device for that. > > > > Hmmm... Yes you're right, it won't work in that case. Maybe we could > > have two functions, the existing i2c_atr_add_adapter(), and another one > > that wraps it ? It would be nice if we could get rid of the platform > > data for the UB913 and UB953 drivers. > > I wouldn't mind that at all, but we already have the bc_rate there. And > I have a feeling that we might need more if we implement more features. Indeed. I feel that platform data is a bit of a hack here, but maybe it's not that bad. > And we also have the atr pointer there. Or do you think that could be > dropped also? In your mail above you only mention the port, but maybe > the deser could register the serializer device and port to the ATR, and > then the ser could just use its device pointer instead of atr & port. I was wondering if we could drop the atr pointer too, yes. I'm not sure how, and there's no urgency to fix this. My main concern is that new drivers should ideally not be forced to use platform data just for ATR support, if they don't use it already for something else.
On 04/01/2023 17:32, Laurent Pinchart wrote: > On Wed, Jan 04, 2023 at 04:13:17PM +0200, Tomi Valkeinen wrote: >> On 04/01/2023 15:55, Laurent Pinchart wrote: >>> Hi Tomi, >>> >>> On Mon, Dec 26, 2022 at 09:25:34PM +0200, Tomi Valkeinen wrote: >>>> On 26/12/2022 18:56, Laurent Pinchart wrote: >>>>> On Wed, Dec 14, 2022 at 08:36:47AM +0200, Tomi Valkeinen wrote: >>>>>> On 14/12/2022 08:29, Tomi Valkeinen wrote: >>>>>> >>>>>>>> wondering if the struct device of the DS90UB913 could be passed instead >>>>>>>> of the port, to avoid passing the port throught >>>>>>>> ds90ub9xx_platform_data. >>>>>>> >>>>>>> Interesting thought. That would limit the number of remote i2c busses to >>>>>>> one, though. Not a problem for FPD-Link, but I wonder if that's assuming >>>>>>> too much for the future users. Then again, this is an in-kernel API so >>>>>>> we could extend it later if needed. So I'll try this out and see if I >>>>>>> hit any issues. >>>>>> >>>>>> Right, so the issue with this one would be that it would prevent a >>>>>> single device uses. E.g. a single chip which acts as an ATR (similar to >>>>>> i2c-mux chips), i.e. it contains both the main and the remote i2c busses. >>>>> >>>>> I don't think I understand this, sorry. >>>> >>>> What you are suggesting above means that we'd have a separate device for >>>> each port of the ATR. Which is fine in our current case, as the i2c >>>> master busses are behind separate remote devices. >>>> >>>> But if you consider a case similar to i2c-mux, where we have a single >>>> chip with the slave bus and, say, 4 master busses. We would probably >>>> have only a single device for that. >>> >>> Hmmm... Yes you're right, it won't work in that case. Maybe we could >>> have two functions, the existing i2c_atr_add_adapter(), and another one >>> that wraps it ? It would be nice if we could get rid of the platform >>> data for the UB913 and UB953 drivers. >> >> I wouldn't mind that at all, but we already have the bc_rate there. And >> I have a feeling that we might need more if we implement more features. > > Indeed. I feel that platform data is a bit of a hack here, but maybe > it's not that bad. > >> And we also have the atr pointer there. Or do you think that could be >> dropped also? In your mail above you only mention the port, but maybe >> the deser could register the serializer device and port to the ATR, and >> then the ser could just use its device pointer instead of atr & port. > > I was wondering if we could drop the atr pointer too, yes. I'm not sure > how, and there's no urgency to fix this. My main concern is that new > drivers should ideally not be forced to use platform data just for ATR > support, if they don't use it already for something else. Good point. However, we don't know who will use ATR or how ATR is going to be used. Using it the same way i2c-mux is used, there's no problem and platform data is not needed. Using it in this split manner we do with FPDLink does bring up the problem. And using i2c-mux in the split manner would also bring up the same problem. So maybe there's some neat solution out there that would solve the issue for both i2c-atr and i2c-mux, but it escapes me at the moment. Tomi