Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Oct 2022 18:07:16 +0000
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Jacopo Mondi <jacopo@jmondi.org>
Cc:     Mikhail Rudenko <mike.rudenko@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Shawn Tu <shawnx.tu@intel.com>,
        Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Mike Pagano <mpagano@gentoo.org>,
        Randy Dunlap <rdunlap@infradead.org>,
        Marek Vasut <marex@denx.de>, Jimmy Su <jimmy.su@intel.com>,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] media: i2c: add support for OV4689
Message-ID: <Y1gl1FMAjhXCfCmk@paasikivi.fi.intel.com>
References: <20221022162042.14113-1-mike.rudenko@gmail.com>
 <20221022162042.14113-3-mike.rudenko@gmail.com>
 <20221025130958.bnedjlkm6kmiluoe@uno.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221025130958.bnedjlkm6kmiluoe@uno.localdomain>
Precedence: bulk

Hi Jacopo,

On Tue, Oct 25, 2022 at 03:09:58PM +0200, Jacopo Mondi wrote:
> > +static int ov4689_configure_regulators(struct ov4689 *ov4689)
> > +{
> > +	unsigned int supplies_count = ARRAY_SIZE(ov4689_supply_names);

No need for a temporary variable.

> > +	unsigned int i;
> > +
> > +	for (i = 0; i < supplies_count; i++)
> > +		ov4689->supplies[i].supply = ov4689_supply_names[i];
> > +
> > +	return devm_regulator_bulk_get(&ov4689->client->dev, supplies_count,
> > +				       ov4689->supplies);
> > +}
> > +
> > +static u64 ov4689_check_link_frequency(struct v4l2_fwnode_endpoint *ep)
> > +{
> > +	unsigned int freqs_count = ARRAY_SIZE(link_freq_menu_items);
> > +	const u64 *freqs = link_freq_menu_items;
> > +	unsigned int i, j;
> > +
> > +	for (i = 0; i < freqs_count; i++) {

Ditto.

> > +		for (j = 0; j < ep->nr_of_link_frequencies; j++)
> > +			if (freqs[i] == ep->link_frequencies[j])
> > +				return freqs[i];
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ov4689_check_hwcfg(struct device *dev)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +	struct v4l2_fwnode_endpoint bus_cfg = {
> > +		.bus_type = V4L2_MBUS_CSI2_DPHY,
> > +	};
> > +	struct fwnode_handle *endpoint;
> > +	int ret;
> > +
> > +	endpoint = fwnode_graph_get_next_endpoint(fwnode, NULL);
> > +	if (!endpoint)
> > +		return -EINVAL;
> > +
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> > +	fwnode_handle_put(endpoint);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (bus_cfg.bus.mipi_csi2.num_data_lanes != OV4689_LANES) {
> > +		dev_err(dev, "Only a 4-lane CSI2 config is supported");
> > +		ret = -EINVAL;
> > +		goto out_free_bus_cfg;
> > +	}
> > +
> > +	if (!bus_cfg.nr_of_link_frequencies) {
> > +		dev_err(dev, "No link frequencies defined\n");
> > +		ret = -EINVAL;
> > +		goto out_free_bus_cfg;
> > +	}
> 
> As the driver has a single supported freq I wonder if it is required
> to have it mandatory. I got contradictory feedbacks in the past, so
> whatever you have here I guess it's fine (same reasoning goes for dts,
> if there's only one accepted item, does it need to be made mandatory
> ?)

This check could indeed be removed, the one below already handles the case.

The driver can be amended in the future to support additional frequencies.
It probably requires more code here, too...

I've got this in my tree, feel free to send a follow-up patch.

> 
> Nits apart, the driver looks sane
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks!

-- 
Regards,

Sakari Ailus