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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Fri, Oct 27, 2023 at 02:50:09PM +0000, Sakari Ailus wrote:
> 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)?

There are some occurences of '#include <uapi/' in drivers/ (I counted
338), but why is that better ?

> > > > +	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.

That's what I've now done, yes.

> > > > +	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?

I'll try to ask and get more information. As firmware update is an
uncommon and not time-sensitive operation, I'd rather be cautious here
and not over-optimize.

> > > > +
> > > > +	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.

I understand, but if the conversion is not complete, it's not very
valuable. I have no objection against using the fwnode API in the
driver, but I'll let someone else handle it when and if needed.

> > > > +	/* 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.

I think it's easier to keep it mandatory.

-- 
Regards,

Laurent Pinchart