diff mbox series

[v2,10/11] media: i2c: max9286: Configure bus width from device tree

Message ID 20220101182806.19311-11-laurent.pinchart+renesas@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: max9286: Small new features | expand

Commit Message

Laurent Pinchart Jan. 1, 2022, 6:28 p.m. UTC
The GMSL serial data bus width is normally selected through the BWS pin.
On some systems, the pin may not be wired to the correct value. Support
overriding the bus width by software, using the value specified in the
device tree.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

Comments

Jacopo Mondi Jan. 9, 2022, 10:56 a.m. UTC | #1
Hi Laurent

On Sat, Jan 01, 2022 at 08:28:05PM +0200, Laurent Pinchart wrote:
> The GMSL serial data bus width is normally selected through the BWS pin.
> On some systems, the pin may not be wired to the correct value. Support
> overriding the bus width by software, using the value specified in the
> device tree.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index d88a4d8e63ab..07ebb01640a1 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -90,6 +90,11 @@
>  /* Register 0x1b */
>  #define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
>  #define MAX9286_ENEQ(n)			(1 << (n))
> +/* Register 0x1c */
> +#define MAX9286_HIGHIMM(n)		BIT((n) + 4)
> +#define MAX9286_I2CSEL			BIT(2)
> +#define MAX9286_HIBW			BIT(1)
> +#define MAX9286_BWS			BIT(0)
>  /* Register 0x27 */
>  #define MAX9286_LOCKED			BIT(7)
>  /* Register 0x31 */
> @@ -182,6 +187,7 @@ struct max9286_priv {
>  	u32 init_rev_chan_mv;
>  	u32 rev_chan_mv;
>  	u8 i2c_mstbt;
> +	u32 bus_width;
>
>  	struct v4l2_ctrl_handler ctrls;
>  	struct v4l2_ctrl *pixelrate_ctrl;
> @@ -1159,6 +1165,23 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_set_video_format(priv, &max9286_default_format);
>  	max9286_set_fsync_period(priv);
>
> +	if (priv->bus_width) {
> +		int val;
> +
> +		val = max9286_read(priv, 0x1c);
> +		if (val < 0)
> +			return val;
> +
> +		val &= ~(MAX9286_HIBW | MAX9286_BWS);
> +
> +		if (priv->bus_width == 27)
> +			val |= MAX9286_HIBW;
> +		else if (priv->bus_width == 32)
> +			val |= MAX9286_BWS;
> +
> +		max9286_write(priv, 0x1c, val);
> +	}
> +
>  	/*
>  	 * The overlap window seems to provide additional validation by tracking
>  	 * the delay between vsync and frame sync, generating an error if the
> @@ -1429,6 +1452,19 @@ static int max9286_parse_dt(struct max9286_priv *priv)
>  	}
>  	of_node_put(node);
>
> +	of_property_read_u32(dev->of_node, "maxim,bus-width", &priv->bus_width);
> +	switch (priv->bus_width) {
> +	case 0:

Can you add a comment that in this case we rely on HW configuration ?
Looking at this with the above function in the same patch makes it
clear, but it might get lost when merged.

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j


> +	case 24:
> +	case 27:
> +	case 32:
> +		break;
> +	default:
> +		dev_err(dev, "Invalid %s value %u\n", "maxim,bus-width",
> +			priv->bus_width);
> +		return -EINVAL;
> +	}
> +
>  	of_property_read_u32(dev->of_node, "maxim,i2c-clock-frequency",
>  			     &i2c_clk_freq);
>  	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Jan. 9, 2022, 11:32 p.m. UTC | #2
Hi Jacopo,

On Sun, Jan 09, 2022 at 11:56:34AM +0100, Jacopo Mondi wrote:
> On Sat, Jan 01, 2022 at 08:28:05PM +0200, Laurent Pinchart wrote:
> > The GMSL serial data bus width is normally selected through the BWS pin.
> > On some systems, the pin may not be wired to the correct value. Support
> > overriding the bus width by software, using the value specified in the
> > device tree.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index d88a4d8e63ab..07ebb01640a1 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -90,6 +90,11 @@
> >  /* Register 0x1b */
> >  #define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
> >  #define MAX9286_ENEQ(n)			(1 << (n))
> > +/* Register 0x1c */
> > +#define MAX9286_HIGHIMM(n)		BIT((n) + 4)
> > +#define MAX9286_I2CSEL			BIT(2)
> > +#define MAX9286_HIBW			BIT(1)
> > +#define MAX9286_BWS			BIT(0)
> >  /* Register 0x27 */
> >  #define MAX9286_LOCKED			BIT(7)
> >  /* Register 0x31 */
> > @@ -182,6 +187,7 @@ struct max9286_priv {
> >  	u32 init_rev_chan_mv;
> >  	u32 rev_chan_mv;
> >  	u8 i2c_mstbt;
> > +	u32 bus_width;
> >
> >  	struct v4l2_ctrl_handler ctrls;
> >  	struct v4l2_ctrl *pixelrate_ctrl;
> > @@ -1159,6 +1165,23 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	max9286_set_video_format(priv, &max9286_default_format);
> >  	max9286_set_fsync_period(priv);
> >
> > +	if (priv->bus_width) {
> > +		int val;
> > +
> > +		val = max9286_read(priv, 0x1c);
> > +		if (val < 0)
> > +			return val;
> > +
> > +		val &= ~(MAX9286_HIBW | MAX9286_BWS);
> > +
> > +		if (priv->bus_width == 27)
> > +			val |= MAX9286_HIBW;
> > +		else if (priv->bus_width == 32)
> > +			val |= MAX9286_BWS;
> > +
> > +		max9286_write(priv, 0x1c, val);
> > +	}
> > +
> >  	/*
> >  	 * The overlap window seems to provide additional validation by tracking
> >  	 * the delay between vsync and frame sync, generating an error if the
> > @@ -1429,6 +1452,19 @@ static int max9286_parse_dt(struct max9286_priv *priv)
> >  	}
> >  	of_node_put(node);
> >
> > +	of_property_read_u32(dev->of_node, "maxim,bus-width", &priv->bus_width);
> > +	switch (priv->bus_width) {
> > +	case 0:
> 
> Can you add a comment that in this case we rely on HW configuration ?
> Looking at this with the above function in the same patch makes it
> clear, but it might get lost when merged.

I'll add

		/*
		 * The property isn't specified in the device tree, the driver
		 * will keep the default value selected by the BWS pin.
		 */

> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> > +	case 24:
> > +	case 27:
> > +	case 32:
> > +		break;
> > +	default:
> > +		dev_err(dev, "Invalid %s value %u\n", "maxim,bus-width",
> > +			priv->bus_width);
> > +		return -EINVAL;
> > +	}
> > +
> >  	of_property_read_u32(dev->of_node, "maxim,i2c-clock-frequency",
> >  			     &i2c_clk_freq);
> >  	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {
diff mbox series

Patch

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index d88a4d8e63ab..07ebb01640a1 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -90,6 +90,11 @@ 
 /* Register 0x1b */
 #define MAX9286_SWITCHIN(n)		(1 << ((n) + 4))
 #define MAX9286_ENEQ(n)			(1 << (n))
+/* Register 0x1c */
+#define MAX9286_HIGHIMM(n)		BIT((n) + 4)
+#define MAX9286_I2CSEL			BIT(2)
+#define MAX9286_HIBW			BIT(1)
+#define MAX9286_BWS			BIT(0)
 /* Register 0x27 */
 #define MAX9286_LOCKED			BIT(7)
 /* Register 0x31 */
@@ -182,6 +187,7 @@  struct max9286_priv {
 	u32 init_rev_chan_mv;
 	u32 rev_chan_mv;
 	u8 i2c_mstbt;
+	u32 bus_width;
 
 	struct v4l2_ctrl_handler ctrls;
 	struct v4l2_ctrl *pixelrate_ctrl;
@@ -1159,6 +1165,23 @@  static int max9286_setup(struct max9286_priv *priv)
 	max9286_set_video_format(priv, &max9286_default_format);
 	max9286_set_fsync_period(priv);
 
+	if (priv->bus_width) {
+		int val;
+
+		val = max9286_read(priv, 0x1c);
+		if (val < 0)
+			return val;
+
+		val &= ~(MAX9286_HIBW | MAX9286_BWS);
+
+		if (priv->bus_width == 27)
+			val |= MAX9286_HIBW;
+		else if (priv->bus_width == 32)
+			val |= MAX9286_BWS;
+
+		max9286_write(priv, 0x1c, val);
+	}
+
 	/*
 	 * The overlap window seems to provide additional validation by tracking
 	 * the delay between vsync and frame sync, generating an error if the
@@ -1429,6 +1452,19 @@  static int max9286_parse_dt(struct max9286_priv *priv)
 	}
 	of_node_put(node);
 
+	of_property_read_u32(dev->of_node, "maxim,bus-width", &priv->bus_width);
+	switch (priv->bus_width) {
+	case 0:
+	case 24:
+	case 27:
+	case 32:
+		break;
+	default:
+		dev_err(dev, "Invalid %s value %u\n", "maxim,bus-width",
+			priv->bus_width);
+		return -EINVAL;
+	}
+
 	of_property_read_u32(dev->of_node, "maxim,i2c-clock-frequency",
 			     &i2c_clk_freq);
 	for (i = 0; i < ARRAY_SIZE(max9286_i2c_speeds); ++i) {