On 15/02/2024 21:44, Umang Jain wrote:
> From: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>
> Add a v4l2 subdevice driver for the Sony IMX283 image sensor.
>
> The IMX283 is a 20MP Diagonal 15.86 mm (Type 1) CMOS Image Sensor with
> Square Pixel for Color Cameras.
>
...
> +
> +static int imx283_power_off(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct imx283 *imx283 = to_imx283(sd);
> +
> + gpiod_set_value_cansleep(imx283->reset_gpio, 0);
That's odd. It is reset GPIO, so 0 means deassert means "stop resetting
device". You have wrong handling of the GPIO.
> + regulator_bulk_disable(ARRAY_SIZE(imx283_supply_name), imx283->supplies);
> + clk_disable_unprepare(imx283->xclk);
> +
> + return 0;
> +}
> +
> +static int imx283_get_regulators(struct imx283 *imx283)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(imx283_supply_name); i++)
> + imx283->supplies[i].supply = imx283_supply_name[i];
> +
> + return devm_regulator_bulk_get(imx283->dev,
> + ARRAY_SIZE(imx283_supply_name),
> + imx283->supplies);
> +}
...
> +static const struct of_device_id imx283_dt_ids[] = {
> + { .compatible = "sony,imx283", },
> + { /* sentinel */ }
> +};
This goes next to driver structure. You also miss module device table.
> +
> +static int imx283_parse_endpoint(struct imx283 *imx283)
> +{
> + struct fwnode_handle *fwnode = dev_fwnode(imx283->dev);
> + struct v4l2_fwnode_endpoint bus_cfg = {
> + .bus_type = V4L2_MBUS_CSI2_DPHY
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + if (!fwnode)
> + return -ENXIO;
> +
> + ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
> + if (!ep) {
> + dev_err(imx283->dev, "Failed to get next endpoint\n");
> + return -ENXIO;
> + }
> +
> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> + fwnode_handle_put(ep);
> + if (ret)
> + return ret;
> +
> + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> + dev_err(imx283->dev,
> + "number of CSI2 data lanes %d is not supported\n",
> + bus_cfg.bus.mipi_csi2.num_data_lanes);
> + ret = -EINVAL;
> + goto done_endpoint_free;
> + }
> +
> + ret = v4l2_link_freq_to_bitmap(imx283->dev, bus_cfg.link_frequencies,
> + bus_cfg.nr_of_link_frequencies,
> + link_frequencies, ARRAY_SIZE(link_frequencies),
> + &imx283->link_freq_bitmap);
> +
> +done_endpoint_free:
> + v4l2_fwnode_endpoint_free(&bus_cfg);
> +
> + return ret;
> +};
> +
> +static int imx283_probe(struct i2c_client *client)
> +{
> + struct imx283 *imx283;
> + int ret;
> + unsigned int i;
> + unsigned int xclk_freq;
> +
> + imx283 = devm_kzalloc(&client->dev, sizeof(*imx283), GFP_KERNEL);
> + if (!imx283)
> + return -ENOMEM;
> +
> + imx283->dev = &client->dev;
> +
> + v4l2_i2c_subdev_init(&imx283->sd, client, &imx283_subdev_ops);
> +
> + imx283->cci = devm_cci_regmap_init_i2c(client, 16);
> + if (IS_ERR(imx283->cci)) {
> + ret = PTR_ERR(imx283->cci);
> + dev_err(imx283->dev, "failed to initialize CCI: %d\n", ret);
> + return ret;
> + }
> +
> + /* Get system clock (xclk) */
> + imx283->xclk = devm_clk_get(imx283->dev, NULL);
> + if (IS_ERR(imx283->xclk)) {
> + dev_err(imx283->dev, "failed to get xclk\n");
Syntax is: return dev_err_probe()
> + return PTR_ERR(imx283->xclk);
> + }
> +
> + xclk_freq = clk_get_rate(imx283->xclk);
> + for (i = 0; i < ARRAY_SIZE(imx283_frequencies); i++) {
> + if (xclk_freq == imx283_frequencies[i].mhz) {
> + imx283->freq = &imx283_frequencies[i];
> + break;
> + }
> + }
> + if (!imx283->freq) {
> + dev_err(imx283->dev, "xclk frequency unsupported: %d Hz\n", xclk_freq);
> + return -EINVAL;
> + }
> +
> + ret = imx283_get_regulators(imx283);
> + if (ret) {
> + dev_err(imx283->dev, "failed to get regulators\n");
> + return ret;
return dev_err_probe()
> + }
> +
> + ret = imx283_parse_endpoint(imx283);
> + if (ret) {
> + dev_err(imx283->dev, "failed to parse endpoint configuration\n");
> + return ret;
> + }
> +
> + /* Request optional enable pin */
> + imx283->reset_gpio = devm_gpiod_get_optional(imx283->dev, "reset",
> + GPIOD_OUT_HIGH);
No, that's not allowed in your binding. Don't add undocumented properties.
> +
> + mutex_init(&imx283->mutex);
> +
> + /*
> + * The sensor must be powered for imx283_identify_module()
> + * to be able to read the CHIP_ID register
> + */
> + ret = imx283_power_on(imx283->dev);
> + if (ret)
> + return ret;
> +
Best regards,
Krzysztof
