[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHp75VfF0pbrKXjWZg7sTr-T=_CbjP+deFQP-VLCGX8ooahctg@mail.gmail.com>
Date: Wed, 10 Apr 2024 19:00:02 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Zhi Mao <zhi.mao@...iatek.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
Heiko Stuebner <heiko@...ech.de>, Sakari Ailus <sakari.ailus@...ux.intel.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>, Hans de Goede <hdegoede@...hat.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, Alain Volmat <alain.volmat@...s.st.com>,
Paul Elder <paul.elder@...asonboard.com>, Mehdi Djait <mehdi.djait@...tlin.com>,
Bingbu Cao <bingbu.cao@...el.com>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org,
shengnan.wang@...iatek.com, yaya.chang@...iatek.com, yunkec@...omium.org,
10572168@...com
Subject: Re: [PATCH 2/2] media: i2c: Add GT97xx VCM driver
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.
..
> +#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 = 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?
> + }
> +
> + 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?
..
> +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.
> + }
> +
> + 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.
> + }
> +
> + 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)?
..
> + 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.
..
> + /*power on and Initialize hw*/
Missing spaces (and capitalisation?).
..
> + 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.
> + 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.
> + goto err_power_off;
> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists