linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: linux-renesas-soc@vger.kernel.org, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	"Hyun Kwon" <hyunk@xilinx.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Jacopo Mondi" <jacopo+renesas@jmondi.org>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Subject: Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver
Date: Mon, 18 May 2020 12:45:18 +0100	[thread overview]
Message-ID: <930009cd-d887-752a-4f1f-567c795101ee@ideasonboard.com> (raw)
In-Reply-To: <20200516215103.GA857@valkosipuli.retiisi.org.uk>

Hi Sakari,

There are only fairly minor comments here, fix ups will be included in a
v10.

Is there anything major blocking integration?

Regards

Kieran



On 16/05/2020 22:51, Sakari Ailus wrote:
> Hi Kieran,
> 
> Thanks for the update.
> 
> On Tue, May 12, 2020 at 04:51:03PM +0100, Kieran Bingham wrote:
> 
> ...
> 
>> +static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>> +				  struct v4l2_subdev_pad_config *cfg,
>> +				  struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	if (code->pad || code->index > 0)
>> +		return -EINVAL;
>> +
>> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> 
> Why UYVY8_2X8 and not UYVY8_1X16? In general, the single sample / pixel
> variant of the format is generally used on the serial busses. This choice
> was made when serial busses were introduced.

Ok - I presume this doesn't really have much effect anyway, they just
have to match for the transmitter/receiver?

But it makes sense to me, so I'll update to the 1x16 variant.

>> +
>> +	return 0;
>> +}
>> +
>> +static struct v4l2_mbus_framefmt *
>> +max9286_get_pad_format(struct max9286_priv *priv,
>> +		       struct v4l2_subdev_pad_config *cfg,
>> +		       unsigned int pad, u32 which)
>> +{
>> +	switch (which) {
>> +	case V4L2_SUBDEV_FORMAT_TRY:
>> +		return v4l2_subdev_get_try_format(&priv->sd, cfg, pad);
>> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
>> +		return &priv->fmt[pad];
>> +	default:
>> +		return NULL;
>> +	}
>> +}
>> +
>> +static int max9286_set_fmt(struct v4l2_subdev *sd,
>> +			   struct v4l2_subdev_pad_config *cfg,
>> +			   struct v4l2_subdev_format *format)
>> +{
>> +	struct max9286_priv *priv = sd_to_max9286(sd);
>> +	struct v4l2_mbus_framefmt *cfg_fmt;
>> +
>> +	if (format->pad >= MAX9286_SRC_PAD)
>> +		return -EINVAL;
> 
> You can remove these checks; it's been already done by the caller.
> 

Ok.


> ...
> 
>> +static int max9286_parse_dt(struct max9286_priv *priv)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	struct device_node *i2c_mux;
>> +	struct device_node *node = NULL;
>> +	unsigned int i2c_mux_mask = 0;
>> +
>> +	of_node_get(dev->of_node);
>> +	i2c_mux = of_find_node_by_name(dev->of_node, "i2c-mux");
>> +	if (!i2c_mux) {
>> +		dev_err(dev, "Failed to find i2c-mux node\n");
>> +		of_node_put(dev->of_node);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Identify which i2c-mux channels are enabled */
>> +	for_each_child_of_node(i2c_mux, node) {
>> +		u32 id = 0;
>> +
>> +		of_property_read_u32(node, "reg", &id);
>> +		if (id >= MAX9286_NUM_GMSL)
>> +			continue;
>> +
>> +		if (!of_device_is_available(node)) {
>> +			dev_dbg(dev, "Skipping disabled I2C bus port %u\n", id);
>> +			continue;
>> +		}
>> +
>> +		i2c_mux_mask |= BIT(id);
>> +	}
>> +	of_node_put(node);
>> +	of_node_put(i2c_mux);
>> +
>> +	/* Parse the endpoints */
>> +	for_each_endpoint_of_node(dev->of_node, node) {
>> +		struct max9286_source *source;
>> +		struct of_endpoint ep;
>> +
>> +		of_graph_parse_endpoint(node, &ep);
>> +		dev_dbg(dev, "Endpoint %pOF on port %d",
>> +			ep.local_node, ep.port);
>> +
>> +		if (ep.port > MAX9286_NUM_GMSL) {
>> +			dev_err(dev, "Invalid endpoint %s on port %d",
>> +				of_node_full_name(ep.local_node), ep.port);
>> +			continue;
>> +		}
>> +
>> +		/* For the source endpoint just parse the bus configuration. */
>> +		if (ep.port == MAX9286_SRC_PAD) {
>> +			struct v4l2_fwnode_endpoint vep = {
>> +				.bus_type = V4L2_MBUS_CSI2_DPHY
>> +			};
>> +			int ret;
>> +
>> +			ret = v4l2_fwnode_endpoint_parse(
>> +					of_fwnode_handle(node), &vep);
>> +			if (ret) {
>> +				of_node_put(node);
>> +				of_node_put(dev->of_node);
>> +				return ret;
>> +			}
>> +
>> +			if (vep.bus_type != V4L2_MBUS_CSI2_DPHY) {
> 
> This won't happen, the bus type will stay if you set it to a non-zero
> value.


Ok - I'll remove this check.


> 
>> +				dev_err(dev,
>> +					"Media bus %u type not supported\n",
>> +					vep.bus_type);
>> +				v4l2_fwnode_endpoint_free(&vep);
>> +				of_node_put(node);
>> +				of_node_put(dev->of_node);
>> +				return -EINVAL;
>> +			}
>> +
>> +			priv->csi2_data_lanes =
>> +				vep.bus.mipi_csi2.num_data_lanes;
>> +			v4l2_fwnode_endpoint_free(&vep);
> 
> No need to call this unless you use v4l2_fwnode_endpoint_alloc_parse().
> 
> And as you don't, you also won't know which frequencies are known to be
> safe to use. That said, perhaps where this device is used having a random
> frequency on that bus could not be an issue. Perhaps.

Does this generate a range? or a list of static supported frequencies?

We configure the pixel clock based upon the number of cameras connected,
and their pixel rates etc ...

Are you saying that the frequency of this clock should be validated to
be a specific range? or are you talking about a different frequency?


For now I'll remove the v4l2_fwnode_endpoint_alloc_parse().



>> +
>> +			continue;
>> +		}
>> +
>> +		/* Skip if the corresponding GMSL link is unavailable. */
>> +		if (!(i2c_mux_mask & BIT(ep.port)))
>> +			continue;
>> +
>> +		if (priv->sources[ep.port].fwnode) {
>> +			dev_err(dev,
>> +				"Multiple port endpoints are not supported: %d",
>> +				ep.port);
>> +
>> +			continue;
>> +		}
>> +
>> +		source = &priv->sources[ep.port];
>> +		source->fwnode = fwnode_graph_get_remote_endpoint(
>> +						of_fwnode_handle(node));
>> +		if (!source->fwnode) {
>> +			dev_err(dev,
>> +				"Endpoint %pOF has no remote endpoint connection\n",
>> +				ep.local_node);
>> +
>> +			continue;
>> +		}
>> +
>> +		priv->source_mask |= BIT(ep.port);
>> +		priv->nsources++;
>> +	}
>> +	of_node_put(node);
>> +	of_node_put(dev->of_node);
>> +
>> +	priv->route_mask = priv->source_mask;
>> +
>> +	return 0;
>> +}
> 


  reply	other threads:[~2020-05-18 11:45 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12 15:51 [PATCH v9 0/4] MAX9286 GMSL Support (+RDACM20) Kieran Bingham
2020-05-12 15:51 ` [PATCH v9 1/4] dt-bindings: media: i2c: Add bindings for Maxim Integrated MAX9286 Kieran Bingham
2020-05-12 22:33   ` Rob Herring
2020-05-12 22:36   ` Rob Herring
2020-05-13 12:28     ` Jacopo Mondi
2020-05-12 15:51 ` [PATCH v9 2/4] media: i2c: Add MAX9286 driver Kieran Bingham
2020-05-12 17:40   ` Manivannan Sadhasivam
2020-05-14  9:53     ` Kieran Bingham
2020-05-14  9:59       ` Manivannan Sadhasivam
2020-05-14 10:05         ` Kieran Bingham
2020-05-12 18:17   ` Manivannan Sadhasivam
2020-05-14 10:02     ` Kieran Bingham
2020-05-14 10:13       ` Manivannan Sadhasivam
2020-05-14 11:47         ` Kieran Bingham
2020-05-14 11:59           ` Geert Uytterhoeven
2020-05-14 12:48             ` Manivannan Sadhasivam
2020-05-14 13:50               ` Kieran Bingham
2020-05-14 11:47         ` Kieran Bingham
2020-05-14 12:27           ` Kieran Bingham
2020-05-14 12:54             ` Manivannan Sadhasivam
2020-05-16 21:51   ` Sakari Ailus
2020-05-18 11:45     ` Kieran Bingham [this message]
2020-05-18 12:38       ` Jacopo Mondi
2020-05-18 13:09         ` Kieran Bingham
2020-05-19  8:10       ` Sakari Ailus
2020-05-19  8:55         ` Kieran Bingham
2020-05-19  9:03           ` Sakari Ailus
2020-05-20  0:50       ` Laurent Pinchart
2020-05-26  8:54         ` Sakari Ailus
2020-05-12 15:51 ` [PATCH v9 3/4] dt-bindings: media: i2c: Add bindings for IMI RDACM2x Kieran Bingham
2020-05-12 15:51 ` [PATCH v9 4/4] media: i2c: Add RDACM20 driver Kieran Bingham
2020-05-16 22:36   ` Sakari Ailus
2020-05-18  7:35     ` Jacopo Mondi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=930009cd-d887-752a-4f1f-567c795101ee@ideasonboard.com \
    --to=kieran.bingham+renesas@ideasonboard.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hverkuil@xs4all.nl \
    --cc=hyunk@xilinx.com \
    --cc=jacopo+renesas@jmondi.org \
    --cc=jacopo@jmondi.org \
    --cc=kieran.bingham@ideasonboard.com \
    --cc=laurent.pinchart+renesas@ideasonboard.com \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=mchehab@kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=niklas.soderlund@ragnatech.se \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@iki.fi \
    --subject='Re: [PATCH v9 2/4] media: i2c: Add MAX9286 driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).