On Fri, 2022-07-29 at 17:51 +0200, Andy Shevchenko wrote:
> 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.
>
OK
> ...
>
> > 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?
OK
>
> > 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.
OK
>
> > + * 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?
yes
>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/videodev2.h>
>
> > +#include <linux/kernel.h>
>
> Perhaps keep it ordered?
OK
>
> ...
>
> > +/*
> > + * From the datasheet:
>
> Maybe add an URL to the Datasheet?
There already is a link to the datasheet in the source file further up,
no need to add another.
>
> > + * 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?
OK
>
> ...
>
> > + 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)
The i2c transfer may still have written some data, and we need to wait
for the EEPROM to update.
>
> > + 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)
OK
>
> ...
>
> > + static const u8 header_pattern[] = {
> > + 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>
> Keeping a comma at the end is good anyway.
This header pattern is fixed to 8 bytes, and will never be more than 8
bytes. So I don't think think the added comma is necessary.
>
> > + };
>
> ...
>
> > + state = kzalloc(sizeof(*state), GFP_KERNEL);
> > + if (!state)
> > + return -ENOMEM;
>
> devm_kzalloc() ?
This will fail if the device is forcibly unloaded while some
application has the device node open.
>
> ...
>
> > + blocks = 1 + state->edid[126];
>
> Magic index.
Ack
>
> ...
>
> > + .of_match_table = of_match_ptr(cat24c208_of_match),
>
> of_match_ptr() brings more harm than help.
OK
>
