Hi Tomi,
On Mon, Jan 09, 2023 at 11:40:43AM +0200, Tomi Valkeinen wrote:
> On 08/01/2023 06:06, Laurent Pinchart wrote:
> > On Thu, Jan 05, 2023 at 04:03:06PM +0200, Tomi Valkeinen wrote:
> >> Add driver for TI DS90UB913 FPD-Link III Serializer.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/media/i2c/Kconfig | 13 +
> >> drivers/media/i2c/Makefile | 2 +-
> >> drivers/media/i2c/ds90ub913.c | 871 ++++++++++++++++++++++++++++++++++
> >> 3 files changed, 885 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..0a60afb09cd3
> >> --- /dev/null
> >> +++ b/drivers/media/i2c/ds90ub913.c
> >> @@ -0,0 +1,871 @@
[snip]
> >> +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);
> >
> > %#02x ? Same below.
>
> Ok.
>
> >> +
> >> + ub913_read(priv, UB913_REG_CRC_ERRORS_LSB, &v1);
> >> + ub913_read(priv, UB913_REG_CRC_ERRORS_MSB, &v2);
> >
> > Looks racy, but if it's for debugging only, I suppose it's fine.
>
> Well, nothing we can do about that in SW. In any case, I think for the
> user the value is either "none", "just a few", "a lot", so maybe the
> racyness doesn't matter.
It could be improved in software:
do {
ub913_read(priv, UB913_REG_CRC_ERRORS_MSB, &msb);
ub913_read(priv, UB913_REG_CRC_ERRORS_LSB, &lsb);
ub913_read(priv, UB913_REG_CRC_ERRORS_MSB, &msb2);
} while (msb1 != msb2);
but I think it's overkill.
> >> + 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 | UB913_REG_GENERAL_CFG_CRC_ERR_RESET);
> >
> > Line wrap.
>
> Ok.
>
> >> + ub913_write(priv, UB913_REG_GENERAL_CFG, v);
> >
> > Move this just after reading the number of CRC errors to avoid dropping
> > some errors.
>
> Ok.
>
> >> +
> >> + return 0;
> >> +}
[snip]
> >> +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;
> >> +
> >> + 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 */
> >
> > Maybe "not supported by the driver yet." to make it clear it could be
> > added ?
>
> Ok.
>
> >> + 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);
> >
> > Use dev_err_probe().
>
> Ok.
>
> >> + 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 & UB913_REG_MODE_SEL_MODE_UP_TO_DATE)) {
> >> + dev_err(dev, "Mode value not stabilized\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + mode_override = v & UB913_REG_MODE_SEL_MODE_OVERRIDE;
> >> + mode = v & 0xf;
> >
> > A macro for the 0xf would be nice.
>
> Ok.
>
> >> +
> >> + 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;
> >> + }
> >> +
> >> + ub913_read(priv, UB913_REG_GENERAL_CFG, &v);
> >> + v &= ~UB913_REG_GENERAL_CFG_PCLK_RISING;
> >> + v |= priv->pclk_polarity ? UB913_REG_GENERAL_CFG_PCLK_RISING : 0;
> >> + ub913_write(priv, UB913_REG_GENERAL_CFG, v);
> >
> > We're completely missing power management, but I suppose that can be
> > done later.
>
> Yes. I'm not sure how that would be implemented. The serializer and the
> whole camera module depends on the deserializer. In most cases both the
> power and the communication comes from the deserializer over the
> FPD-Link cable. I'm not sure if there's much the serializer can do alone
> wrt. the power management.
>
> Hmm, do we need a full bus structure for the FPD-Link after all, so that
> we get power management features? Although that would mean also the
> other peripherals on the camera module should somehow be involved, as we
> can't turn off the deserializer and the serializer without somehow being
> permitted by the other peripherals (like sensor).
I suppose time will tell :-)
> > Should this be grouped with the UB913_REG_MODE_SEL check above, and
> > maybe moved to a hardware init function ?
>
> Yes, I can try to restructure this a bit. I guess if we add a hw init
> function, also the ub913_i2c_master_init() would be called from there.
>
> >> +
> >> + 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);
> >
> > Can we meaningfully continue with tx_ep_np is NULL, or should that be an
> > error ?
>
> The matching part of v4l2 is not quite clear to me. I believe I took
> this part from some other driver. The driver doesn't need the tx_ep_np,
> afaiu this is only to help with the subdev connection matching. Is it
> possible the matching could happen some other way than via fwnode?
In general yes, in practice we require DT so we will never match through
another mean.
> That said... We require DT, so I think that means the tx_ep_np must be
> there. If it's not, something is wrong, and we'd better fail. So, I
> think I can handle !tx_ep_np as an error.
Sounds good to me.
> >> +
> >> + 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;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_unreg_async_subdev:
> >> + v4l2_async_unregister_subdev(&priv->sd);
> >> +err_unreg_notif:
> >> + ub913_v4l2_nf_unregister(priv);
> >> +err_free_state:
> >
> > I'd name this err_subdev_cleanup.
>
> Yep.
>
> >> + v4l2_subdev_cleanup(&priv->sd);
> >> +err_entity_cleanup:
> >> + if (priv->tx_ep_np)
> >> + of_node_put(priv->tx_ep_np);
> >
> > of_node_put() is a no-op when called with NULL, you can drop the check.
> > Same below.
>
> Ok.
>
> >> +
> >> + 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);
> >> +
> >> + i2c_atr_del_adapter(priv->plat_data->atr,
> >> + priv->plat_data->port);
> >> +
> >> + 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);
> >> +}
--
Regards,
Laurent Pinchart
