[PATCH v4 3/3] media: i2c: Add driver for THine THP7312

[Sakari Ailus]

Hi Laurent,

On Fri, Oct 27, 2023 at 03:45:29PM +0300, Laurent Pinchart wrote:

...

> > > +#include <linux/clk.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/firmware.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/iopoll.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mtd/spi-nor.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/thp7312.h>
> > 
> > uapi/linux/thp7321.h ?
> 
> Is that needed ?

It's a UAPI header. Wouldn't it be reasonable to include it that way
(instead of relying on searching include/uapi as well)?

> > > +	struct {
> > > +		struct v4l2_ctrl *noise_reduction_auto;
> > > +		struct v4l2_ctrl *noise_reduction_absolute;
> > > +	};
> > > +
> > > +	const char *fw_name;
> > > +	u8 *fw_data;
> > > +	size_t fw_size;
> > > +
> > > +	u8 fw_major_version;
> > > +	u8 fw_minor_version;
> > > +
> > > +	/* Lock to protect fw_cancel */
> > > +	struct mutex fw_lock;
> > > +	struct fw_upload *fwl;
> > > +	bool fw_cancel;
> > 
> > Arranging this right after fw_* would save some memory.
> 
> After what ? I assume you mean fw_*_version ? It would, but it would
> feel a bit out of place. I'll see what I can do.

Yes. There doesn't seem to be any firm ordering here either. Up to you.

...

> > > +	val = ((conv_lanes[3] & 0x03) << 6) |
> > > +	      ((conv_lanes[2] & 0x03) << 4) |
> > > +	      ((conv_lanes[1] & 0x03) << 2) |
> > > +	       (conv_lanes[0] & 0x03);
> > 
> > You could construct val in the loop and drop conv_lanes altogether.
> > 
> > I.e.
> > 
> > 		val |= (i & 0x03) << ((lanes[i] - 1) * 2);
> > 
> > And assign val to 0 in declaration.
> 
> I think I'll compute it at probe time and cache it instead.

If you don't need anything else in the endpoint, you could move it out of
the device context struct.

> > > +	for (rate = mode->rates; rate->fps; ++rate, --index) {
> > > +		if (!index) {
> > > +			fie->interval.numerator = 1;
> > > +			fie->interval.denominator = rate->fps;
> > 
> > Maybe a newline here?
> 
> If that makes you happy :-)

Newlines are great (when they are at the right places)!

> > > +	case V4L2_CID_THP7312_NOISE_REDUCTION_AUTO:
> > > +	case V4L2_CID_THP7312_NOISE_REDUCTION_ABSOLUTE:
> > > +		/* Ignore the manually set value if auto has been set */
> > > +		value = thp7312->noise_reduction_auto->val
> > > +		      ? 0 : 0x80 | (thp7312->noise_reduction_absolute->val & 0x7f);
> > 
> > "?" should be on the preceding line.
> 
> Isn't that a matter of coding style preference ?

Yes, indeed, and I recall GNU coding style is shunned upon here. :-)

> 
> > > +
> > > +		cci_write(thp7312->regmap, THP7312_REG_NOISE_REDUCTION, value,
> > > +			  &ret);
> > > +		break;
> > > +
> > > +	case V4L2_CID_AUTO_WHITE_BALANCE:
> > > +		value = ctrl->val ? THP7312_WB_MODE_AUTO : THP7312_WB_MODE_MANUAL;
> > 
> > I'd do this in the call, up to you.
> 
> Only if you allow lines longer than 80 columns ;-)

I don't think you need longer lines for that, do you?

> 
> > > +
> > > +		cci_write(thp7312->regmap, THP7312_REG_WB_MODE, value, &ret);
> > > +		break;
> > > +

...

> > > +static enum fw_upload_err thp7312_fw_write_to_flash(struct thp7312_device *thp7312,
> > > +						    u32 dest, u32 write_size)
> > > +{
> > > +	u8 command[sizeof(thp7312_cmd_write_ram_to_flash) + 6];
> > > +	static const u32 cmd_size = sizeof(thp7312_cmd_write_ram_to_flash);
> > > +	u64 val;
> > > +	int ret;
> > > +
> > > +	memcpy(command, thp7312_cmd_write_ram_to_flash, cmd_size);
> > > +
> > > +	command[cmd_size] = (dest & 0xff0000) >> 16;
> > > +	command[cmd_size + 1] = (dest & 0x00ff00) >> 8;
> > > +	command[cmd_size + 2] = (dest & 0x0000ff);
> > > +	command[cmd_size + 3] = ((write_size - 1) & 0xff0000) >> 16;
> > > +	command[cmd_size + 4] = ((write_size - 1) & 0x00ff00) >> 8;
> > > +	command[cmd_size + 5] = ((write_size - 1) & 0x0000ff);
> > > +
> > > +	ret = thp7312_write_buf(thp7312, command, sizeof(command));
> > > +	if (ret < 0)
> > > +		return FW_UPLOAD_ERR_RW_ERROR;
> > > +
> > > +	usleep_range(8000000, 8100000);
> > 
> > I guess there's time to make some tea here?
> 
> For a flash infusion, gong fu style, probably.
> 
> We don't have much documentation about the exact values of the delays
> that are needed, and why :-(

I have even less documentation (none) on this device. Is polling an option,
as you're reading a register to verify the operation was successful?

> 
> > > +
> > > +	ret = cci_read(thp7312->regmap, THP7312_REG_FW_VERIFY_RESULT, &val,
> > > +		       NULL);
> > > +	if (ret < 0)
> > > +		return FW_UPLOAD_ERR_RW_ERROR;
> > > +
> > > +	return val ?  FW_UPLOAD_ERR_HW_ERROR : FW_UPLOAD_ERR_NONE;
> > > +}

...

> > > +	/*
> > > +	 * Register a device for the sensor, to support usage of the regulator
> > > +	 * API.
> > > +	 */
> > > +	sensor->dev = kzalloc(sizeof(*sensor->dev), GFP_KERNEL);
> > > +	if (!sensor->dev)
> > > +		return -ENOMEM;
> > > +
> > > +	sensor->dev->parent = dev;
> > > +	sensor->dev->of_node = of_node_get(sensor->of_node);
> > 
> > This device could well find its way to a non-OF system. Could you use the
> > fwnode property API instead?
> 
> I'm pretty sure there will be problems if someone was using this driver
> on an ACPI-based system, so trying to pretend it's supported without
> being able to test it may not be the best use of development time. I'll
> try, but if I hit any issue, I'll keep using the OF-specific functions
> in the next version.

I'd suggest to use OF functions if there's no corresponding fwnode function
available. The intention is they cover the same scope, so it is likely
something that's missing will be added sooner or later.

> > > +	/* Retrieve the sensor index from the reg property. */
> > > +	ret = of_property_read_u32(node, "reg", &reg);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "'reg' property missing in sensor node\n");
> > 
> > Shouldn't you assume it's zero instead?
> 
> The property is mandatory.

You could also make it optional as that appears to be the general practice.
Up to you.

-- 
Kind regards,

Sakari Ailus