| 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
| ||
|
Message-ID: <CAEth8oG-FjR+TDE_bd94i3ODaVBJGAuouXyoTrSPtfrM+OOJPg@mail.gmail.com>
Date: Fri, 4 Oct 2024 14:30:34 +0800
From: Kate Hsuan <hpa@...hat.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Hans de Goede <hdegoede@...hat.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: Add t4ka3 camera sensor driver
Hi Christophe,
Thank you for reviewing.
On Thu, Oct 3, 2024 at 5:03 AM Christophe JAILLET
<christophe.jaillet@...adoo.fr> wrote:
>
> Le 02/10/2024 à 11:30, Kate Hsuan a écrit :
> > Add the t4ka3 driver from:
> > https://github.com/kitakar5525/surface3-atomisp-cameras.git
> >
> > With many cleanups / changes (almost a full rewrite) to make it suitable
> > for upstream:
> >
> > * Remove the VCM and VCM-OTP support, the mainline kernel models VCMs and
> > calibration data eeproms as separate v4l2-subdev-s.
> >
> > * Remove the integration-factor t4ka3_get_intg_factor() support and IOCTL,
> > this provided info to userspace through an atomisp private IOCTL.
> >
> > * Turn atomisp specific exposure/gain IOCTL into standard v4l2 controls.
> >
> > * Use normal ACPI power-management in combination with runtime-pm support
> > instead of atomisp specific GMIN power-management code.
> >
> > * Turn into a standard V4L2 sensor driver using
> > v4l2_async_register_subdev_sensor().
> >
> > * Add vblank, hblank, and link-freq controls; drop get_frame_interval().
> >
> > * Use CCI register helpers.
> >
> > * Calculate values for modes instead of using fixed register-value lists,
> > allowing arbritrary modes.
> >
> > * Add get_selection() and set_selection() support
> >
> > * Add a CSI2 bus configuration check
> >
> > This been tested on a Xiaomi Mipad2 tablet which has a T4KA3 sensor with
> > DW9761 VCM as back sensor.
> >
> > Co-developed-by: Hans de Goede <hdegoede@...hat.com>
> > Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> > Signed-off-by: Kate Hsuan <hpa@...hat.com>
> > ---
>
> Hi,
>
> a few comments, should it help.
>
> > +static int t4ka3_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > + struct t4ka3_data *sensor = to_t4ka3_sensor(sd);
> > + int ret;
> > +
> > + mutex_lock(&sensor->lock);
> > +
> > + if (sensor->streaming == enable) {
> > + dev_warn(sensor->dev, "Stream already %s\n", enable ? "started" : "stopped");
> > + goto error_unlock;
> > + }
> > +
> > + if (enable) {
> > + ret = pm_runtime_get_sync(sensor->sd.dev);
> > + if (ret) {
> > + dev_err(sensor->dev, "power-up err.\n");
> > + goto error_unlock;
> > + }
> > +
> > + cci_multi_reg_write(sensor->regmap, t4ka3_init_config,
> > + ARRAY_SIZE(t4ka3_init_config), &ret);
> > + /* enable group hold */
> > + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 1, &ret);
> > + cci_multi_reg_write(sensor->regmap, t4ka3_pre_mode_set_regs,
> > + ARRAY_SIZE(t4ka3_pre_mode_set_regs), &ret);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + ret = t4ka3_set_mode(sensor);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + ret = cci_multi_reg_write(sensor->regmap, t4ka3_post_mode_set_regs,
> > + ARRAY_SIZE(t4ka3_post_mode_set_regs), NULL);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + /* Restore value of all ctrls */
> > + ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + /* disable group hold */
> > + cci_write(sensor->regmap, T4KA3_REG_PARAM_HOLD, 0, &ret);
> > + cci_write(sensor->regmap, T4KA3_REG_STREAM, 1, &ret);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + sensor->streaming = 1;
> > + } else {
> > + ret = cci_write(sensor->regmap, T4KA3_REG_STREAM, 0, NULL);
> > + if (ret)
> > + goto error_powerdown;
> > +
> > + ret = pm_runtime_put(sensor->sd.dev);
> > + if (ret)
> > + goto error_unlock;
> > +
> > + sensor->streaming = 0;
> > + }
> > +
> > + mutex_unlock(&sensor->lock);
> > + return ret;
> > +
> > +error_powerdown:
> > + ret = pm_runtime_put(sensor->sd.dev);
>
> I think that the "ret = " should be removed here.
Okay, make sense.
>
> > +error_unlock:
> > + mutex_unlock(&sensor->lock);
> > + return ret;
> > +}
>
> ...
>
> > +static int t4ka3_probe(struct i2c_client *client)
> > +{
> > + struct t4ka3_data *sensor;
> > + int ret;
> > +
> > + /* allocate sensor device & init sub device */
> > + sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> > + if (!sensor)
> > + return -ENOMEM;
> > +
> > + sensor->dev = &client->dev;
> > +
> > + ret = t4ka3_check_hwcfg(sensor);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_init(&sensor->lock);
> > +
> > + sensor->link_freq[0] = T4KA3_LINK_FREQ;
> > + sensor->mode.crop = t4ka3_default_crop;
> > + t4ka3_fill_format(sensor, &sensor->mode.fmt, T4KA3_ACTIVE_WIDTH, T4KA3_ACTIVE_HEIGHT);
> > + t4ka3_calc_mode(sensor);
> > +
> > + v4l2_i2c_subdev_init(&(sensor->sd), client, &t4ka3_ops);
> > + sensor->sd.internal_ops = &t4ka3_internal_ops;
> > +
> > + sensor->powerdown_gpio = devm_gpiod_get(&client->dev, "powerdown",
> > + GPIOD_OUT_HIGH);
> > + if (IS_ERR(sensor->powerdown_gpio))
> > + return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown_gpio),
> > + "getting powerdown GPIO\n");
> > +
> > + sensor->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > + GPIOD_OUT_HIGH);
> > + if (IS_ERR(sensor->reset_gpio))
> > + return dev_err_probe(&client->dev, PTR_ERR(sensor->reset_gpio),
> > + "getting reset GPIO\n");
> > +
> > + pm_runtime_set_suspended(&client->dev);
> > + pm_runtime_enable(&client->dev);
> > + pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> > + pm_runtime_use_autosuspend(&client->dev);
> > +
> > + sensor->regmap = devm_cci_regmap_init_i2c(client, 16);
> > + if (IS_ERR(sensor->regmap))
> > + return PTR_ERR(sensor->regmap);
>
> I thing this should goto err_pm_runtime;
Okay, I'll get the regmap information before setting up runtime pm.
So, probe() can return without diable the pm.
>
> > +
> > + ret = t4ka3_s_config(&sensor->sd);
> > + if (ret)
> > + goto err_pm_runtime;
> > +
> > + sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > + sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > +
> > + ret = t4ka3_init_controls(sensor);
> > + if (ret)
> > + goto err_controls;
> > +
> > + ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> > + if (ret)
> > + goto err_controls;
> > +
> > + ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> > + if (ret)
> > + goto err_media_entity;
> > +
> > + return 0;
> > +
> > +err_media_entity:
> > + media_entity_cleanup(&sensor->sd.entity);
> > +err_controls:
> > + v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> > +err_pm_runtime:
> > + pm_runtime_disable(&client->dev);
> > + return ret;
> > +}
> > +
> > +static struct acpi_device_id t4ka3_acpi_match[] = {
> > + { "XMCC0003" },
> > + {},
>
> No need for ending comma after terminator.
Okay.
>
> > +};
>
> ...
>
> CJ
>
I'll propose a v2 patch to include the improvements.
--
BR,
Kate
Powered by blists - more mailing lists