Hi Christophe, Thank you for reviewing. On Thu, Oct 3, 2024 at 5:03 AM Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> 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@xxxxxxxxxx> > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx> > > --- > > 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
