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", ®);
> > > > + 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
