mbox series

[0/2] media: i2c: Add support for GT97xx VCM

Message ID 20240410104002.1197-1-zhi.mao@mediatek.com
Headers show
Series media: i2c: Add support for GT97xx VCM | expand

Message

Zhi Mao (毛智) April 10, 2024, 10:40 a.m. UTC 
This series add YAML DT binding and V4L2 sub-device driver for Giantec's GT9768&GT9769.
GT9768&GT9769 is a 10-bit DAC with 100mA output current sink capability, designed
for voice coil motor(VCM) with I2C control bus.

This driver supports:
 - support pm runtime function for suspend/resume
 - support camera lens focus position by V4L2_CID_FOCUS_ABSOLUTE CMD
 - used in camera features on ChromeOS application

This series is based on linux-next, tag: next-20240409

Thanks

Zhi Mao (2):
  media: dt-bindings: i2c: add Giantec GT97xx VCM driver
  media: i2c: Add GT97xx VCM driver

 .../bindings/media/i2c/giantec,gt97xx.yaml    |  91 +++
 drivers/media/i2c/Kconfig                     |  13 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/gt97xx.c                    | 640 ++++++++++++++++++
 4 files changed, 745 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/giantec,gt97xx.yaml
 create mode 100644 drivers/media/i2c/gt97xx.c

Comments

Andy Shevchenko April 10, 2024, 4 p.m. UTC | #1 
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.

...

> +#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",
> +                                &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 = 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(&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
Sakari Ailus April 12, 2024, 9:38 a.m. UTC | #2 
Hi Andy, Zhi,

On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > +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)?

Ideally yes. We don't have a good mechanism for this at the moment as the
lens isn't part of the image pipeline. Non-data links may be used for this
in the future but that's not implemented yet.
Andy Shevchenko April 12, 2024, 1:43 p.m. UTC | #3 
On Fri, Apr 12, 2024 at 12:39 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > > +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)?
>
> Ideally yes. We don't have a good mechanism for this at the moment as the
> lens isn't part of the image pipeline. Non-data links may be used for this
> in the future but that's not implemented yet.

Aren't you using devlinks? It was designed exactly to make sure that
the PM chain of calls goes in the correct order.
Sakari Ailus April 15, 2024, 7:25 a.m. UTC | #4 
Hi Andy,

On Fri, Apr 12, 2024 at 04:43:43PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 12, 2024 at 12:39 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > On Wed, Apr 10, 2024 at 07:00:02PM +0300, Andy Shevchenko wrote:
> > > > +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)?
> >
> > Ideally yes. We don't have a good mechanism for this at the moment as the
> > lens isn't part of the image pipeline. Non-data links may be used for this
> > in the future but that's not implemented yet.
> 
> Aren't you using devlinks? It was designed exactly to make sure that
> the PM chain of calls goes in the correct order.

Device links are already used by the IPU bridge, but in the other
direction: the VCM requires the sensor to be powered up in this case.
Zhi Mao (毛智) April 20, 2024, 1:48 a.m. UTC | #5 
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",
> > +                                &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