From: "Zhi Mao (毛智)" <zhi.mao@mediatek.com>
To: "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>
Cc: "heiko@sntech.de" <heiko@sntech.de>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"laurent.pinchart+renesas@ideasonboard.com"
<laurent.pinchart+renesas@ideasonboard.com>,
"yunkec@chromium.org" <yunkec@chromium.org>,
"linux-mediatek@lists.infradead.org"
<linux-mediatek@lists.infradead.org>,
"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
"hdegoede@redhat.com" <hdegoede@redhat.com>,
"bingbu.cao@intel.com" <bingbu.cao@intel.com>,
"paul.elder@ideasonboard.com" <paul.elder@ideasonboard.com>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"mchehab@kernel.org" <mchehab@kernel.org>,
"Shengnan Wang (王圣男)" <shengnan.wang@mediatek.com>,
"Yaya Chang (張雅清)" <Yaya.Chang@mediatek.com>,
"p.zabel@pengutronix.de" <p.zabel@pengutronix.de>,
"alain.volmat@foss.st.com" <alain.volmat@foss.st.com>,
"conor+dt@kernel.org" <conor+dt@kernel.org>,
"sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
"robh@kernel.org" <robh@kernel.org>,
"hverkuil-cisco@xs4all.nl" <hverkuil-cisco@xs4all.nl>,
"tomi.valkeinen@ideasonboard.com"
<tomi.valkeinen@ideasonboard.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
"mehdi.djait@bootlin.com" <mehdi.djait@bootlin.com>,
"krzk+dt@kernel.org" <krzk+dt@kernel.org>,
"angelogioacchino.delregno@collabora.com"
<angelogioacchino.delregno@collabora.com>,
"10572168@qq.com" <10572168@qq.com>
Subject: Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver
Date: Sat, 20 Apr 2024 01:48:55 +0000 [thread overview]
Message-ID: <34869abbbcce4fc82878aaafbbf59f7e52f06b8f.camel@mediatek.com> (raw)
In-Reply-To: <CAHp75VfF0pbrKXjWZg7sTr-T=_CbjP+deFQP-VLCGX8ooahctg@mail.gmail.com>
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@mediatek.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",
> > + >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
prev parent reply other threads:[~2024-04-20 1:49 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-10 10:40 [PATCH 0/2] media: i2c: Add support for GT97xx VCM Zhi Mao
2024-04-10 10:40 ` [PATCH 1/2] media: dt-bindings: i2c: add Giantec GT97xx VCM driver Zhi Mao
2024-04-10 11:27 ` Conor Dooley
2024-04-10 11:41 ` Sakari Ailus
2024-04-10 20:52 ` Rob Herring
2024-04-11 2:04 ` Zhi Mao (毛智)
2024-04-11 6:05 ` Krzysztof Kozlowski
2024-04-10 10:40 ` [PATCH 2/2] media: i2c: Add " Zhi Mao
2024-04-10 16:00 ` Andy Shevchenko
2024-04-12 9:38 ` Sakari Ailus
2024-04-12 13:43 ` Andy Shevchenko
2024-04-15 7:25 ` Sakari Ailus
2024-04-20 1:48 ` Zhi Mao (毛智) [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34869abbbcce4fc82878aaafbbf59f7e52f06b8f.camel@mediatek.com \
--to=zhi.mao@mediatek.com \
--cc=10572168@qq.com \
--cc=Yaya.Chang@mediatek.com \
--cc=alain.volmat@foss.st.com \
--cc=andy.shevchenko@gmail.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=bingbu.cao@intel.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=hdegoede@redhat.com \
--cc=heiko@sntech.de \
--cc=hverkuil-cisco@xs4all.nl \
--cc=krzk+dt@kernel.org \
--cc=laurent.pinchart+renesas@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=mehdi.djait@bootlin.com \
--cc=p.zabel@pengutronix.de \
--cc=paul.elder@ideasonboard.com \
--cc=robh@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=shengnan.wang@mediatek.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=yunkec@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).