Hi! > I'm sorry to keep you waiting.
Thanks for comments.
> > + struct gpio_desc *gpio_reset;
> > + struct gpio_desc *gpio_cabledet;
> > +
> > + uint32_t src_caps[8];
>
> Use u32 instead of uint32_t.
Will replace globally.
> > +static int anx7688_reg_read(struct anx7688 *anx7688, u8 reg_addr)
> > +{
> > + int ret;
> > +
> > + ret = i2c_smbus_read_byte_data(anx7688->client, reg_addr);
> > + if (ret < 0)
> > + dev_err(anx7688->dev, "i2c read failed at 0x%x (%d)\n",
> > + reg_addr, ret);
>
> dev_dbg instead.
I'd prefer not to. i2c functions should not really fail, and if they
do, we want the error log. This is not debugging, this is i2c failing.
> > +static void anx7688_power_enable(struct anx7688 *anx7688)
> > +{
> > + gpiod_set_value(anx7688->gpio_reset, 1);
> > + gpiod_set_value(anx7688->gpio_enable, 1);
> > +
> > + /* wait for power to stabilize and release reset */
> > + msleep(10);
>
> So is it okay that the sleep may take up to 20ms?
I don't see how that would be a problem.
> > + gpiod_set_value(anx7688->gpio_reset, 0);
> > + udelay(2);
>
> Use usleep_range() instead.
Can do, but it makes no difference.
> > + gpiod_set_value(anx7688->gpio_reset, 1);
> > + msleep(5);
>
> The same question here, is it a problem if the sleep ends up taking
> 20ms?
Again, I expect that to be ok.
> > + ret = anx7688_tcpc_reg_read(anx7688, ANX7688_TCPC_REG_INTERFACE_SEND);
> > + if (ret) {
> > + dev_err(anx7688->dev,
> > + "failed to send pd packet (tx buffer full)\n");
>
> One line should be enought for that one.
That makes it go over 80 columns, but yes, can be one line.
> > + // wait until the message is processed (30ms max)
> > + for (i = 0; i < 300; i++) {
> > + ret = anx7688_tcpc_reg_read(anx7688,
> > ANX7688_TCPC_REG_INTERFACE_SEND);
> > + if (ret <= 0)
> > + return ret;
> > +
> > + udelay(100);
> > + }
> > +
> > + dev_err(anx7688->dev, "timeout waiting for the message queue flush\n");
>
> Maybe dev_dbg for this too.
Let's not hide these. If they happen, we can downgrade them, but they
should not.
> > + /* wait till the firmware is loaded (typically ~30ms) */
> > + for (i = 0; i < 100; i++) {
> > + ret = anx7688_reg_read(anx7688,
> > ANX7688_REG_EEPROM_LOAD_STATUS0);
> > +
> > + if (ret >= 0 && (ret & ANX7688_EEPROM_FW_LOADED) ==
> > ANX7688_EEPROM_FW_LOADED) {
> > + dev_dbg(anx7688->dev, "eeprom0 = 0x%02x\n", ret);
> > + dev_info(anx7688->dev, "fw loaded after %d ms\n", i *
> > 10);
>
> Debugging information. Use dev_dbg.
Ok.
> > + set_bit(ANX7688_F_FW_FAILED, anx7688->flags);
> > + dev_err(anx7688->dev, "boot firmware load failed (you may need to flash
> > FW to anx7688 first)\n");
> > + ret = -ETIMEDOUT;
> > + goto err_vconoff;
> > +
> > +fw_loaded:
>
> This label looks a bit messy to me. You could also move that firmware
> loading wait to its own function.
Ok, let me try to refactor this.
> > +static int anx7688_handle_pd_message_response(struct anx7688 *anx7688,
> > + u8 to_cmd, u8 resp)
> > +{
...
> > + return 0;
> > +}
>
> Noise. Drop this whole function. If you need this kind of information,
> then please consider trace points, or just use some debugfs trick like
> what we have in drivers/usb/typec/tcpm/tcpm.c and few other drivers.
Ok.
> > + switch (cmd) {
> > + case ANX7688_OCM_MSG_PWR_SRC_CAP:
> > + dev_info(anx7688->dev, "received SRC_CAP\n");
>
> Noise.
Ok, let me convert these to dev_dbg.
> > +
> > + if (len % 4 != 0) {
> > + dev_warn(anx7688->dev, "received invalid sized PDO
> > array\n");
> > + break;
> > + }
> > +
> > + /* the partner is PD capable */
> > + anx7688->pd_capable = true;
> > +
> > + for (i = 0; i < len / 4; i++) {
> > + pdo = le32_to_cpu(pdos[i]);
> > +
> > + if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> > + unsigned int voltage = pdo_fixed_voltage(pdo);
> > + unsigned int max_curr = pdo_max_current(pdo);
> > +
> > + dev_info(anx7688->dev, "SRC_CAP PDO_FIXED (%umV
> > %umA)\n", voltage, max_curr);
>
> Noise.
>
> > + } else if (pdo_type(pdo) == PDO_TYPE_BATT) {
> > + unsigned int min_volt = pdo_min_voltage(pdo);
> > + unsigned int max_volt = pdo_max_voltage(pdo);
> > + unsigned int max_pow = pdo_max_power(pdo);
> > +
> > + dev_info(anx7688->dev, "SRC_CAP PDO_BATT
> > (%umV-%umV %umW)\n", min_volt, max_volt, max_pow);
>
> Noise. That line also really should be split in two.
>
> I'm stopping my review here. This driver is too noisy. All dev_info
> calls need to be dropped. If the driver is working correctly then it
> needs to quiet.
>
> Most of those prints are useful for debugging only, so I think similar
> debugfs log like the one tcpm.c uses could be a good idea for them
> since you already use debugfs in this driver in any case.
Ok, let me convert the non-error ones to dev_dbg() and split the long
lines. Debug needs to be enabled, so it should not bother anyone, and
it is easier than refactoring driver to use debugfs.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
signature.asc
Description: PGP signature
