Re: [PATCH 4/5] media: i2c: cat24c208: driver for the cat24c208 EDID EEPROM

["Erling Ljunggren (hljunggr)" <hljunggr@cisco.com>]

On Fri, 2022-07-29 at 17:51 +0200, Andy Shevchenko wrote:
> On Thu, Jul 28, 2022 at 1:53 PM Erling Ljunggren <hljunggr@cisco.com>
> wrote:
> > 
> > From: Jonathan Selnes <jonathansb1@gmail.com>
> > 
> > Support reading and writing the EDID EEPROM through the
> > v4l2 API.
> > 
> > Signed-off-by: Jonathan Selnes <jonathansb1@gmail.com>
> > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
> 
> 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
>