Received-SPF: pass (google.com: domain of linux-kernel+bounces-138945-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240410104002.1197-1-zhi.mao@mediatek.com> <20240410104002.1197-3-zhi.mao@mediatek.com>
In-Reply-To: <20240410104002.1197-3-zhi.mao@mediatek.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Wed, 10 Apr 2024 19:00:02 +0300
Message-ID: <CAHp75VfF0pbrKXjWZg7sTr-T=_CbjP+deFQP-VLCGX8ooahctg@mail.gmail.com>
Subject: Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver
To: Zhi Mao <zhi.mao@mediatek.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, 
	Matthias Brugger <matthias.bgg@gmail.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, 
	Philipp Zabel <p.zabel@pengutronix.de>, 
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>, 
	Heiko Stuebner <heiko@sntech.de>, Sakari Ailus <sakari.ailus@linux.intel.com>, 
	Hans Verkuil <hverkuil-cisco@xs4all.nl>, Hans de Goede <hdegoede@redhat.com>, 
	Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>, Alain Volmat <alain.volmat@foss.st.com>, 
	Paul Elder <paul.elder@ideasonboard.com>, Mehdi Djait <mehdi.djait@bootlin.com>, 
	Bingbu Cao <bingbu.cao@intel.com>, linux-media@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, 
	shengnan.wang@mediatek.com, yaya.chang@mediatek.com, yunkec@chromium.org, 
	10572168@qq.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 10, 2024 at 1:40=E2=80=AFPM Zhi Mao <zhi.mao@mediatek.com> wrot=
e:
>
> 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.

..

> +#define GT97XX_MAX_FOCUS_POS (1024 - 1)

(BIT(10) - 1) ?

..

> +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.

> +};

..

> +static u32 gt97xx_find_ot_multi(u32 aac_mode_param)
> +{
> +       u32 cur_ot_multi_base100 =3D 70;
> +       unsigned int i;
> +
> +       for (i =3D 0; i < ARRAY_SIZE(aac_mode_ot_multi); i++) {
> +               if (aac_mode_ot_multi[i].aac_mode_enum =3D=3D aac_mode_pa=
ram) {
> +                       cur_ot_multi_base100 =3D
> +                               aac_mode_ot_multi[i].ot_multi_base100;
> +               }

No break / return here?

> +       }
> +
> +       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_pa=
ram,
> +                                       u32 aac_timing_param)

Why inline?

..

> +       return tvib_us * ot_multi_base100 / 100;

HECTO ?

..

> +       val =3D ((unsigned char)read_val & ~mask) | (val & mask);

Why casting?

..

> +static int gt97xx_power_on(struct device *dev)
> +{
> +       struct v4l2_subdev *sd =3D dev_get_drvdata(dev);
> +       struct gt97xx *gt97xx =3D sd_to_gt97xx(sd);
> +       int ret;
> +
> +       ret =3D regulator_bulk_enable(ARRAY_SIZE(gt97xx_supply_names),
> +                                   gt97xx->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable regulators\n");

> +               return ret;

Dup.

> +       }
> +
> +       return ret;
> +}
> +
> +static int gt97xx_power_off(struct device *dev)
> +{
> +       struct v4l2_subdev *sd =3D dev_get_drvdata(dev);
> +       struct gt97xx *gt97xx =3D sd_to_gt97xx(sd);
> +       int ret;
> +
> +       ret =3D regulator_bulk_disable(ARRAY_SIZE(gt97xx_supply_names),
> +                                    gt97xx->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to disable regulators\n");

> +               return ret;

Ditto.

> +       }
> +
> +       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 *f=
h)
> +{
> +       return pm_runtime_put(sd->dev);
> +}

Hmm... Shouldn't v4l2 take care about these (PM calls)?

..

> +       gt97xx->chip =3D 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.

..

> +       /*power on and Initialize hw*/

Missing spaces (and capitalisation?).

..

> +       ret =3D 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.

> +               goto err_clean_entity;
> +       }

..

> +       ret =3D v4l2_async_register_subdev(&gt97xx->sd);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to register V4L2 subdev: %d", ret);

Ditto.

> +               goto err_power_off;
> +       }

--
With Best Regards,
Andy Shevchenko