Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Mon, 4 Dec 2023 09:30:24 +0000
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Tommaso Merciai <tomm.merciai@gmail.com>
Cc:     laurent.pinchart@ideasonboard.com, martin.hecht@avnet.eu,
        michael.roeder@avnet.eu, linuxfancy@googlegroups.com,
        mhecht73@gmail.com, christophe.jaillet@wanadoo.fr,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Hans de Goede <hdegoede@redhat.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>,
        Gerald Loacker <gerald.loacker@wolfvision.net>,
        Nicholas Roth <nicholas@rothemail.net>,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Daniel Scally <djrscally@gmail.com>,
        Bingbu Cao <bingbu.cao@intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-kernel@vger.kernel.org, linux-media@vger.kernel.org
Subject: Re: [PATCH v14 3/3] media: i2c: Add support for alvium camera
Message-ID: <ZW2cMPpbDN8xrOM9@kekkonen.localdomain>
References: <20231124093011.2095073-1-tomm.merciai@gmail.com>
 <20231124093011.2095073-4-tomm.merciai@gmail.com>
 <ZWpJcS7aJmnRm1CB@kekkonen.localdomain>
 <ZWr5my7SVKE2HPTZ@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZWr5my7SVKE2HPTZ@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Precedence: bulk

Hi Tommaso,

On Sat, Dec 02, 2023 at 10:32:11AM +0100, Tommaso Merciai wrote:
> Hi Sakari,
> Thanks for your comments.
> 
> On Fri, Dec 01, 2023 at 09:00:33PM +0000, Sakari Ailus wrote:
> > Hi Tommaso,
> > 
> > A few more comments below...
> > 
> > On Fri, Nov 24, 2023 at 10:30:07AM +0100, Tommaso Merciai wrote:
> > 
> > ...
> > 
> > > +static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> > > +{
> > > +	struct device *dev = &alvium->i2c_client->dev;
> > > +	struct alvium_bcrm_vers *v;
> > > +	u64 val;
> > > +	int ret;
> > > +
> > > +	ret = alvium_read(alvium, REG_BCRM_VERSION_R, &val, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	v = (struct alvium_bcrm_vers *)&val;
> > 
> > You're still reading the entire struct using a single read. :-( This won't
> > work on a BE machine as while the struct fields are in the same memory
> > locations, the respective data in a single 64-bit value is not.
> 
> What about splitting REG_BCRM_VERSION_R in:
> 
> #define REG_BCRM_MINOR_VERSION_R			CCI_REG16(0x0000)
> #define REG_BCRM_MAJOR_VERSION_R			CCI_REG16(0x0002)
> 
> and REG_BCRM_DEVICE_FIRMWARE_VERSION_R in:
> 
> #define REG_BCRM_DEVICE_FW_SPEC_VERSION_R		REG_BCRM_V4L2_8BIT(0x0010)
> #define REG_BCRM_DEVICE_FW_MAJOR_VERSION_R		REG_BCRM_V4L2_8BIT(0x0011)
> #define REG_BCRM_DEVICE_FW_MINOR_VERSION_R		REG_BCRM_V4L2_16BIT(0x0012)
> #define REG_BCRM_DEVICE_FW_PATCH_VERSION_R		REG_BCRM_V4L2_32BIT(0x0014)
> 
> 
> Then reading those values as a single values as you suggest:
> 
> static int alvium_get_bcrm_vers(struct alvium_dev *alvium)
> {
> 	struct device *dev = &alvium->i2c_client->dev;
> 	u64 min, maj;
> 	int ret = 0;
> 
> 	ret = alvium_read(alvium, REG_BCRM_MINOR_VERSION_R, &min, &ret);
> 	ret = alvium_read(alvium, REG_BCRM_MAJOR_VERSION_R, &maj, &ret);
> 	if (ret)
> 		return ret;
> 
> 	dev_info(dev, "bcrm version: %llu.%llu\n", min, maj);
> 
> 	return 0;
> }
> 
> static int alvium_get_fw_version(struct alvium_dev *alvium)
> {
> 	struct device *dev = &alvium->i2c_client->dev;
> 	u64 spec, maj, min, pat;
> 	int ret = 0;
> 
> 	ret = alvium_read(alvium, REG_BCRM_DEVICE_FW_SPEC_VERSION_R, &spec, &ret);
> 	ret = alvium_read(alvium, REG_BCRM_DEVICE_FW_MAJOR_VERSION_R, &maj, &ret);
> 	ret = alvium_read(alvium, REG_BCRM_DEVICE_FW_MINOR_VERSION_R, &min, &ret);
> 	ret = alvium_read(alvium, REG_BCRM_DEVICE_FW_PATCH_VERSION_R, &pat, &ret);
> 	if (ret)
> 		return ret;
> 
> 	dev_info(dev, "fw version: %llu.%llu.%llu.%llu\n", spec, maj, min, pat);
> 
> 	return 0;
> }
> 
> Then I'm going to remove alvium_bcrm_vers and alvium_fw_vers.
> And alvium_is_alive became: 
> 
> static int alvium_is_alive(struct alvium_dev *alvium)
> {
> 	u64 bcrm, hbeat;
> 	int ret = 0;
> 
> 	alvium_read(alvium, REG_BCRM_MINOR_VERSION_R, &bcrm, &ret);
> 	alvium_read(alvium, REG_BCRM_HEARTBEAT_RW, &hbeat, &ret);
> 	if (ret)
> 		return ret;
> 
> 	return hbeat;
> }
> 
> What do you think? Let me know.
> (Maybe is this that you are trying to explain me but I haven't catch,
> sorry) :)

Yes. The above code looks good to me.

> 
> 
> > 
> > > +
> > > +	dev_info(dev, "bcrm version: %u.%u\n", v->minor, v->major);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int alvium_get_fw_version(struct alvium_dev *alvium)
> > > +{
> > > +	struct device *dev = &alvium->i2c_client->dev;
> > > +	struct alvium_fw_vers *fw_v;
> > > +	u64 val;
> > > +	int ret;
> > > +
> > > +	ret = alvium_read(alvium, REG_BCRM_DEVICE_FIRMWARE_VERSION_R, &val, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	fw_v = (struct alvium_fw_vers *)&val;
> > 
> > Same here.
> > 
> > > +
> > > +	dev_info(dev, "fw version: %u.%u.%u.%u\n", fw_v->special, fw_v->major,
> > > +		 fw_v->minor, fw_v->patch);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int alvium_get_bcrm_addr(struct alvium_dev *alvium)
> > > +{
> > > +	u64 val;
> > > +	int ret;
> > > +
> > > +	ret = alvium_read(alvium, REG_BCRM_REG_ADDR_R, &val, NULL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	alvium->bcrm_addr = val;
> > > +
> > > +	return 0;
> > > +}
> > 
> > ...
> > 
> > > +static int alvium_setup_mipi_fmt(struct alvium_dev *alvium)
> > > +{
> > > +	unsigned int avail_fmt_cnt = 0;
> > > +	unsigned int fmt = 0;
> > > +	size_t sz = 0;
> > > +
> > > +	alvium->alvium_csi2_fmt = NULL;
> > 
> > This seems to be unnnecessary: the field is assigned below without using it
> > (obviously).
> 
> Ok, I will remove this in v15.
> 
> > 
> > > +
> > > +	/* calculate fmt array size */
> > > +	for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
> > > +		if (!alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit])
> > > +			continue;
> > > +
> > > +		if ((!alvium_csi2_fmts[fmt].is_raw) ||
> > > +		    (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit]))
> > > +			sz++;
> > > +	}
> > > +
> > > +	/* init alvium_csi2_fmt array */
> > > +	alvium->alvium_csi2_fmt_n = sz;
> > > +	alvium->alvium_csi2_fmt =
> > > +		kmalloc_array(sz, sizeof(struct alvium_pixfmt), GFP_KERNEL);
> > > +	if (!alvium->alvium_csi2_fmt)
> > > +		return -ENOMEM;
> > > +
> > > +	/* Create the alvium_csi2 fmt array from formats available */
> > > +	for (fmt = 0; fmt < ALVIUM_NUM_SUPP_MIPI_DATA_FMT; fmt++) {
> > > +		if (!alvium->is_mipi_fmt_avail[alvium_csi2_fmts[fmt].fmt_av_bit])
> > > +			continue;
> > > +
> > > +		if ((!alvium_csi2_fmts[fmt].is_raw) ||
> > > +		    (alvium->is_bay_avail[alvium_csi2_fmts[fmt].bay_av_bit])) {
> > > +			alvium->alvium_csi2_fmt[avail_fmt_cnt] = alvium_csi2_fmts[fmt];
> > > +			avail_fmt_cnt++;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > ...
> > 
> > > +static const struct alvium_pixfmt *
> > > +alvium_code_to_pixfmt(struct alvium_dev *alvium, u32 code)
> > > +{
> > > +	const struct alvium_pixfmt *formats = alvium->alvium_csi2_fmt;
> > 
> > I'd use alvium->alvium_csi2_fmt and not add a local variable. Up to you.
> 
> Ok also for me.
> 
> > 
> > > +	unsigned int i;
> > > +
> > > +	for (i = 0; formats[i].code; ++i)
> > > +		if (formats[i].code == code)
> > > +			return &formats[i];
> > > +
> > > +	return &formats[0];
> > > +}
> > > +
> > > +static int alvium_set_mode(struct alvium_dev *alvium,
> > > +			   struct v4l2_subdev_state *state)
> > > +{
> > > +	struct v4l2_mbus_framefmt *fmt;
> > > +	struct v4l2_rect *crop;
> > > +	int ret;
> > > +
> > > +	crop = v4l2_subdev_state_get_crop(state, 0);
> > > +	fmt = v4l2_subdev_state_get_format(state, 0);
> > > +
> > > +	v4l_bound_align_image(&fmt->width, alvium->img_min_width,
> > > +			      alvium->img_max_width, 0,
> > > +			      &fmt->height, alvium->img_min_height,
> > > +			      alvium->img_max_height, 0, 0);
> > > +
> > > +	/* alvium don't accept negative crop left/top */
> > > +	crop->left = clamp((u32)max(0, crop->left), alvium->min_offx,
> > > +			   (u32)(alvium->img_max_width - fmt->width));
> > > +	crop->top = clamp((u32)max(0, crop->top), alvium->min_offy,
> > > +			  (u32)(alvium->img_max_height - fmt->height));
> > > +
> > > +	ret = alvium_set_img_width(alvium, fmt->width);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = alvium_set_img_height(alvium, fmt->height);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = alvium_set_img_offx(alvium, crop->left);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = alvium_set_img_offy(alvium, crop->top);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> 
> I'm going to rebase v15 on top of your master branch.
> My plan is moving alvium_init_cfg now alvium_init_state into
> v4l2_subdev_internal_ops.

Ack, thanks!

-- 
Regards,

Sakari Ailus