Hi Laurent,
On Sat, Oct 28, 2023 at 06:18:58PM +0300, Laurent Pinchart wrote:
> 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 ?
I'd presume that at some point the -Iinclude/uapi will be cleaned up and
then the only option remains to include it from there directly. Why not to
do it already now?
>
> > > > > + 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.
If this is only done in order to *upgrade* the firmware, it's certainly not
a concern IMO.
>
> > > > > +
> > > > > + 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.
If you leave it using OF-only API now in a driver that is not bound to OF
in any way, someone moving it to fwnode later may not be able to test it on
OF, increasing the likelihood something breaks. So use fwnode API where you
can now, and we'll address that one call later on.
>
> > > > > + /* 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.
Ack.
--
Regards,
Sakari Ailus
