linux-kernel - Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <34869abbbcce4fc82878aaafbbf59f7e52f06b8f.camel@mediatek.com>
Date: Sat, 20 Apr 2024 01:48:55 +0000
From: Zhi Mao (毛智) <zhi.mao@...iatek.com>
To: "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>
CC: "heiko@...ech.de" <heiko@...ech.de>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "laurent.pinchart+renesas@...asonboard.com"
	<laurent.pinchart+renesas@...asonboard.com>, "yunkec@...omium.org"
	<yunkec@...omium.org>, "linux-mediatek@...ts.infradead.org"
	<linux-mediatek@...ts.infradead.org>, "linux-media@...r.kernel.org"
	<linux-media@...r.kernel.org>, "hdegoede@...hat.com" <hdegoede@...hat.com>,
	"bingbu.cao@...el.com" <bingbu.cao@...el.com>, "paul.elder@...asonboard.com"
	<paul.elder@...asonboard.com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "mchehab@...nel.org" <mchehab@...nel.org>,
	Shengnan Wang (王圣男)
	<shengnan.wang@...iatek.com>, Yaya Chang (張雅清)
	<Yaya.Chang@...iatek.com>, "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
	"alain.volmat@...s.st.com" <alain.volmat@...s.st.com>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "sakari.ailus@...ux.intel.com"
	<sakari.ailus@...ux.intel.com>, "robh@...nel.org" <robh@...nel.org>,
	"hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
	"tomi.valkeinen@...asonboard.com" <tomi.valkeinen@...asonboard.com>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "matthias.bgg@...il.com"
	<matthias.bgg@...il.com>, "mehdi.djait@...tlin.com"
	<mehdi.djait@...tlin.com>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"angelogioacchino.delregno@...labora.com"
	<angelogioacchino.delregno@...labora.com>, "10572168@...com"
	<10572168@...com>
Subject: Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver

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@...iatek.com>
> 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",
> > +                                &gt97xx->aac_mode);
> 
> No, use device_property_read_u32() directly.
> 
> ...
> 
> > +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,clock-
> presc",
> > +                                &gt97xx->clock_presc);
> 
> Ditto.
> 
> ...
> 
> > +       fwnode_property_read_u32(dev_fwnode(dev), "giantec,aac-
> timing",
> > +                                &gt97xx->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(&gt97xx->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