On Thu, Jul 28, 2022 at 1:53 PM Erling Ljunggren <hljunggr@xxxxxxxxx> wrote:
>
> From: Jonathan Selnes <jonathansb1@xxxxxxxxx>
>
> Support reading and writing the EDID EEPROM through the
> v4l2 API.
>
> Signed-off-by: Jonathan Selnes <jonathansb1@xxxxxxxxx>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Signed-off-by: Erling Ljunggren <hljunggr@xxxxxxxxx>
Wondering if the last two people don't do any development, otherwise
Co-developed-by would be appreciated.
...
> obj-$(CONFIG_VIDEO_HI556) += hi556.o
> obj-$(CONFIG_VIDEO_HI846) += hi846.o
> obj-$(CONFIG_VIDEO_HI847) += hi847.o
> +obj-$(CONFIG_VIDEO_CAT24C208) += cat24c208.o
Perhaps more sorted?
> obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
> obj-$(CONFIG_VIDEO_IMX208) += imx208.o
> obj-$(CONFIG_VIDEO_IMX214) += imx214.o
...
> +/*
> + * cat24c208 - HDMI i2c controlled EEPROM from ON Semiconductor or Catalyst Semiconductor
Here...
> + * cat24c208.c - Support for i2c based DDC EEPROM
...and here and in general putting filename into the file is not a
good idea in the long term. For example, this can be expanded in the
future to support more EDID EEPROMs, and if we want to rename, this
adds an additional burden.
> + * Copyright (C) 2021-2022 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
> + */
...
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_device.h>
Why? Who is the user of this?
Perhaps you meant mod_devicetable.h, which is currently missed?
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>
> +#include <linux/kernel.h>
Perhaps keep it ordered?
...
> +/*
> + * From the datasheet:
Maybe add an URL to the Datasheet?
> + * The write cycle time is the time from a valid stop condition of a write
> + * sequence to the end of the internal program/erase cycle. During the write
> + * cycle, the bus interface circuits are disabled, SDA is allowed to remain
> + * high, and the device does not respond to its slave address.
> + */
> +#define WRITE_CYCLE_TIME_US 5000
...
> + struct i2c_client *dev_client = state->i2c_clients[0];
> + struct i2c_client *data_client = state->i2c_clients[1];
> + struct i2c_client *seg_client = state->i2c_clients[2];
Why not have those clients named accordingly in the data struct,
instead of indexing them?
...
> + if (seg)
> + err = i2c_transfer(dev_client->adapter, msg, ARRAY_SIZE(msg));
> + else
> + err = i2c_transfer(dev_client->adapter, &msg[1], 1);
> + if (err < 0)
> + dev_err(&dev_client->dev, "Writing to 0x%x failed (segment %d)\n",
> + data_client->addr, seg);
> + usleep_range(WRITE_CYCLE_TIME_US, 2 * WRITE_CYCLE_TIME_US);
Sleep even in case of error? Is it required?
(Same Q per other similar places)
> + return err < 0 ? err : 0;
Hence here...
...
> + if (err < 0)
> + dev_err(&dev_client->dev, "Reading of EDID failed\n");
> + return err < 0 ? err : 0;
...and here we can avoid a duplication test for error code being set, right?
(Same suggestion per other similar cases)
...
> + static const u8 header_pattern[] = {
> + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
Keeping a comma at the end is good anyway.
> + };
...
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
devm_kzalloc() ?
...
> + blocks = 1 + state->edid[126];
Magic index.
...
> + .of_match_table = of_match_ptr(cat24c208_of_match),
of_match_ptr() brings more harm than help.
--
With Best Regards,
Andy Shevchenko
