Re: [PATCH v1 1/2] media: i2c: et8ek8: et8ek8_driver: add support for crc configuration via device tree

[Alex Tran]

On Mon, Nov 10, 2025 at 12:14 AM Sakari Ailus
<sakari.ailus@xxxxxxxxxxxxxxx> wrote:
>
> Hi Alex,
>
> Thanks for the patch.
>
> On Sat, Nov 08, 2025 at 03:29:23PM -0800, Alex Tran wrote:
> > Retrieve the configuration for CRC from the device tree instead
> > using the hard coded USE_CRC directive. If there is an issue
> > retrieving the endpoint node or the CRC property then the default
> > of 1 is used and the driver does not fail to maintain backward
> > compatibility.
>
> Is there a need for making this configurable? I have to say I can't recall
> the specifics of CCP2 but presumably the receiver side would need to be
> aligned with this as well, requiring such configuration on both sides.
>
> If you want to pursue this further, please also cc me to the DT binding
> patch.
>
> >
> > Signed-off-by: Alex Tran <alex.t.tran@xxxxxxxxx>
> > ---
> >  drivers/media/i2c/et8ek8/et8ek8_driver.c | 49 +++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > index 2cb7b7187..4ef92359c 100644
> > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c
> > @@ -29,6 +29,7 @@
> >  #include <media/media-entity.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> > +#include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> >  #include "et8ek8_reg.h"
> > @@ -45,6 +46,7 @@ struct et8ek8_sensor {
> >       struct regulator *vana;
> >       struct clk *ext_clk;
> >       u32 xclk_freq;
> > +     u32 use_crc;
> >
> >       u16 version;
> >
> > @@ -130,8 +132,6 @@ static struct et8ek8_gain {
> >
> >  #define ET8EK8_I2C_DELAY     3       /* msec delay b/w accesses */
> >
> > -#define USE_CRC                      1
> > -
> >  /*
> >   * Register access helpers
> >   *
> > @@ -844,20 +844,16 @@ static int et8ek8_power_on(struct et8ek8_sensor *sensor)
> >       if (rval)
> >               goto out;
> >
> > -#ifdef USE_CRC
> >       rval = et8ek8_i2c_read_reg(client, ET8EK8_REG_8BIT, 0x1263, &val);
> >       if (rval)
> >               goto out;
> > -#if USE_CRC /* TODO get crc setting from DT */
> > -     val |= BIT(4);
> > -#else
> > -     val &= ~BIT(4);
> > -#endif
> > +     if (sensor->use_crc)
> > +             val |= BIT(4);
> > +     else
> > +             val &= ~BIT(4);
> >       rval = et8ek8_i2c_write_reg(client, ET8EK8_REG_8BIT, 0x1263, val);
> >       if (rval)
> >               goto out;
> > -#endif
> > -
> >  out:
> >       if (rval)
> >               et8ek8_power_off(sensor);
> > @@ -1396,6 +1392,34 @@ static int __maybe_unused et8ek8_resume(struct device *dev)
> >       return __et8ek8_set_power(sensor, true);
> >  }
> >
> > +static int et8ek8_parse_fwnode(struct device *dev, struct et8ek8_sensor *sensor)
> > +{
> > +     struct v4l2_fwnode_endpoint bus_cfg = {
> > +             .bus_type = V4L2_MBUS_CCP2,
> > +     };
> > +     struct fwnode_handle *ep;
> > +     int ret;
> > +
> > +     ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> > +                                          FWNODE_GRAPH_ENDPOINT_NEXT);
> > +     if (!ep) {
> > +             dev_warn(dev, "could not get endpoint node\n");
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +     if (ret) {
> > +             dev_warn(dev, "parsing endpoint node failed\n");
> > +             goto done;
> > +     }
> > +
> > +     fwnode_property_read_u32(ep, "crc", &sensor->use_crc);
> > +done:
> > +     v4l2_fwnode_endpoint_free(&bus_cfg);
> > +     fwnode_handle_put(ep);
> > +     return ret;
> > +}
> > +
> >  static int et8ek8_probe(struct i2c_client *client)
> >  {
> >       struct et8ek8_sensor *sensor;
> > @@ -1406,6 +1430,11 @@ static int et8ek8_probe(struct i2c_client *client)
> >       if (!sensor)
> >               return -ENOMEM;
> >
> > +     sensor->use_crc = 1;
> > +     ret = et8ek8_parse_fwnode(dev, sensor);
> > +     if (ret)
> > +             dev_warn(dev, "parsing endpoint failed\n");
> > +
> >       sensor->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> >       if (IS_ERR(sensor->reset)) {
> >               dev_dbg(&client->dev, "could not request reset gpio\n");
>
> --
> Kind regards,
>
> Sakari Ailus

Thanks for the review. It seems like I missed adjusting the change on
the receiver side in omap3isp: isp.c
since the crc is still being defaulted to 1. I'll go ahead and add the
corresponding device tree parsing for the
crc property on the receiver side as well, so both the sensor and ISP
endpoints can be configured consistently.

The threading for this patch series was broken. I'll send in v2 as a 3
patch series with them
properly linked.

Best,
--
Alex Tran