Hi Andy, Thanks for your review. On Wed, 2024-04-10 at 19:00 +0300, Andy Shevchenko wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On Wed, Apr 10, 2024 at 1:40 PM Zhi Mao <zhi.mao@xxxxxxxxxxxx> > wrote: > > > > Add a V4L2 sub-device driver for Giantec GT97xx VCM. > > ... > > > +/* > > + * Ring control and Power control register > > + * Bit[1] RING_EN > > + * 0: Direct mode > > + * 1: AAC mode (ringing control mode) > > + * Bit[0] PD > > + * 0: Normal operation mode > > + * 1: Power down mode > > + * gt97xx requires waiting time of Topr after PD reset takes > place. > > + */ > > +#define GT97XX_RING_PD_CONTROL_REG CCI_REG8(0x02) > > > +#define GT97XX_PD_MODE_OFF 0x00 > > Okay, this seems to be bitmapped, but do you really need to have this > separate definition? > > > +#define GT97XX_PD_MODE_EN BIT(0) > > +#define GT97XX_AAC_MODE_EN BIT(1) > > ... > > > +#define GT97XX_TVIB_MS_BASE10 (64 - 1) > > Should it be (BIT(6) - 1) ? > > ... > > > +#define GT97XX_AAC_MODE_DEFAULT 2 > > +#define GT97XX_AAC_TIME_DEFAULT 0x20 > > Some are decimal, some are hexadecimal, but look to me like > bitfields. > Some "aac-mode/aac-timing/clock-presc" control function were removed, so these related defines were also removed. > ... > > > +#define GT97XX_MAX_FOCUS_POS (1024 - 1) > > (BIT(10) - 1) ? > fixed in patch:v1. > ... > > > +enum vcm_giantec_reg_desc { > > + GT_IC_INFO_REG, > > + GT_RING_PD_CONTROL_REG, > > + GT_MSB_ADDR_REG, > > + GT_AAC_PRESC_REG, > > + GT_AAC_TIME_REG, > > > + GT_MAX_REG, > > No comma for the terminator. > fixed in patch:v1. > > +}; > > ... > > > +static u32 gt97xx_find_ot_multi(u32 aac_mode_param) > > +{ > > + u32 cur_ot_multi_base100 = 70; > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) { > > + if (aac_mode_ot_multi[i].aac_mode_enum == > aac_mode_param) { > > + cur_ot_multi_base100 = > > > + aac_mode_ot_multi[i].ot_multi_base100 > ; > > + } > > No break / return here? fixed in patch:v1. > > > + } > > + > > + return cur_ot_multi_base100; > > +} > > + > > +static u32 gt97xx_find_dividing_rate(u32 presc_param) > > Same comments as per above function. > > ... > > > +static inline u32 gt97xx_cal_move_delay(u32 aac_mode_param, u32 > presc_param, > > + u32 aac_timing_param) > > Why inline? > > ... > > > + return tvib_us * ot_multi_base100 / 100; > > HECTO ? > > ... > > > + val = ((unsigned char)read_val & ~mask) | (val & mask); > > Why casting? > Some "aac-mode/aac-timing/clock-presc" related control function were removed. > ... > > > +static int gt97xx_power_on(struct device *dev) > > +{ > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > + struct gt97xx *gt97xx = sd_to_gt97xx(sd); > > + int ret; > > + > > + ret = > regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names), > > + gt97xx->supplies); > > + if (ret < 0) { > > + dev_err(dev, "failed to enable regulators\n"); > > > + return ret; > > Dup. fixed in patch:v1. > > > + } > > + > > + return ret; > > +} > > + > > +static int gt97xx_power_off(struct device *dev) > > +{ > > + struct v4l2_subdev *sd = dev_get_drvdata(dev); > > + struct gt97xx *gt97xx = sd_to_gt97xx(sd); > > + int ret; > > + > > + ret = > regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names), > > + gt97xx->supplies); > > + if (ret < 0) { > > + dev_err(dev, "failed to disable regulators\n"); > > > + return ret; > > Ditto. fixed in patch:v1. > > > + } > > + > > + return ret; > > +} > > ... > > > +static int gt97xx_open(struct v4l2_subdev *sd, struct > v4l2_subdev_fh *fh) > > +{ > > + return pm_runtime_resume_and_get(sd->dev); > > +} > > + > > +static int gt97xx_close(struct v4l2_subdev *sd, struct > v4l2_subdev_fh *fh) > > +{ > > + return pm_runtime_put(sd->dev); > > +} > > Hmm... Shouldn't v4l2 take care about these (PM calls)? > Accoring to disscussion in another thread, there is not a good mechanism at present, so I keep this method. > ... > > > + gt97xx->chip = of_device_get_match_data(dev); > > device_get_match_data() > > ... > > > + fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac- > mode", > > + >97xx->aac_mode); > > No, use device_property_read_u32() directly. > > ... > > > + fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock- > presc", > > + >97xx->clock_presc); > > Ditto. > > ... > > > + fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac- > timing", > > + >97xx->aac_timing); > > Ditto. > Some "aac-mode/aac-timing/clock-presc" related control function were removed. > ... > > > + /*power on and Initialize hw*/ > > Missing spaces (and capitalisation?). > fixed in patch:v1. > ... > > > + ret = gt97xx_runtime_resume(dev); > > + if (ret < 0) { > > + dev_err(dev, "failed to power on: %d\n", ret); > > Use dev_err_probe() to match the style of the messages. > fixed in patch:v1. > > + goto err_clean_entity; > > + } > > ... > > > + ret = v4l2_async_register_subdev(>97xx->sd); > > + if (ret < 0) { > > + dev_err(dev, "failed to register V4L2 subdev: %d", > ret); > > Ditto. fixed in patch:v1. > > > + goto err_power_off; > > + } > > -- > With Best Regards, > Andy Shevchenko