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

  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>
---
 MAINTAINERS                   |   7 +
 drivers/media/i2c/Kconfig     |   9 +
 drivers/media/i2c/Makefile    |   1 +
 drivers/media/i2c/cat24c208.c | 421 ++++++++++++++++++++++++++++++++++
 4 files changed, 438 insertions(+)
 create mode 100644 drivers/media/i2c/cat24c208.c
  

Hi Andy,

On 28/07/2022 14:02, Andy Shevchenko wrote:
> 
> 
> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
> 
>     From: Jonathan Selnes <jonathansb1@gmail.com <mailto:jonathansb1@gmail.com>>
> 
>     Support reading and writing the EDID EEPROM through the
>     v4l2 API.
> 
> 
> 
> Why the normal way of representing as a memory (we have framework and drivers) can’t work?

Because support for EDID for video sinks is already part of the media subsystem (V4L2).
Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
support without the video receiver. It belongs in drivers/media in any case since EDIDs
are closely tied to media.

> 
> Moreover, this driver seems limited in support of variety of the eeprom chips.

Not quite sure what you mean. The cat24c208 is what this was developed for and
the only one we have.

Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
standard, which a normal EEPROM doesn't. So these devices are specifically made
for this use-case.

Regards,

	Hans

> 
>  
> 
> 
>     Signed-off-by: Jonathan Selnes <jonathansb1@gmail.com <mailto:jonathansb1@gmail.com>>
>     Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>>
>     Signed-off-by: Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>>
>     ---
>      MAINTAINERS                   |   7 +
>      drivers/media/i2c/Kconfig     |   9 +
>      drivers/media/i2c/Makefile    |   1 +
>      drivers/media/i2c/cat24c208.c | 421 ++++++++++++++++++++++++++++++++++
>      4 files changed, 438 insertions(+)
>      create mode 100644 drivers/media/i2c/cat24c208.c
> 
>     diff --git a/MAINTAINERS b/MAINTAINERS
>     index 0a1d3d2b42bc..97d7ead555ac 100644
>     --- a/MAINTAINERS
>     +++ b/MAINTAINERS
>     @@ -14632,6 +14632,13 @@ S:     Maintained
>      T:     git git://linuxtv.org/media_tree.git <http://linuxtv.org/media_tree.git>
>      F:     drivers/media/i2c/ov9734.c
> 
>     +ON SEMICONDUCTOR CAT24C208 EDID EEPROM DRIVER
>     +M:     Hans Verkuil <hverkuil-cisco@xs4all.nl <mailto:hverkuil-cisco@xs4all.nl>>
>     +L:     linux-media@vger.kernel.org <mailto:linux-media@vger.kernel.org>
>     +S:     Maintained
>     +F:     Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
>     +F:     drivers/media/i2c/cat24c208*
>     +
>      ONENAND FLASH DRIVER
>      M:     Kyungmin Park <kyungmin.park@samsung.com <mailto:kyungmin.park@samsung.com>>
>      L:     linux-mtd@lists.infradead.org <mailto:linux-mtd@lists.infradead.org>
>     diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>     index fae2baabb773..10d437f79a35 100644
>     --- a/drivers/media/i2c/Kconfig
>     +++ b/drivers/media/i2c/Kconfig
>     @@ -1529,6 +1529,15 @@ endmenu
>      menu "Miscellaneous helper chips"
>             visible if !MEDIA_HIDE_ANCILLARY_SUBDRV
> 
>     +config VIDEO_CAT24C208
>     +       tristate "ON Semiconductor cat24c208 EDID EEPROM"
>     +       depends on VIDEO_DEV && I2C
>     +       help
>     +         Support for the ON Semiconductor CAT24C208 Dual Port EDID EEPROM.
>     +
>     +         To compile this driver as a module, choose M here: the
>     +         module will be called cat24c208.
>     +
>      config VIDEO_I2C
>             tristate "I2C transport video support"
>             depends on VIDEO_DEV && I2C
>     diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>     index 3e1696963e7f..ae5a88204892 100644
>     --- a/drivers/media/i2c/Makefile
>     +++ b/drivers/media/i2c/Makefile
>     @@ -35,6 +35,7 @@ obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
>      obj-$(CONFIG_VIDEO_HI556) += hi556.o
>      obj-$(CONFIG_VIDEO_HI846) += hi846.o
>      obj-$(CONFIG_VIDEO_HI847) += hi847.o
>     +obj-$(CONFIG_VIDEO_CAT24C208) += cat24c208.o
>      obj-$(CONFIG_VIDEO_I2C) += video-i2c.o
>      obj-$(CONFIG_VIDEO_IMX208) += imx208.o
>      obj-$(CONFIG_VIDEO_IMX214) += imx214.o
>     diff --git a/drivers/media/i2c/cat24c208.c b/drivers/media/i2c/cat24c208.c
>     new file mode 100644
>     index 000000000000..b56e5f829a8d
>     --- /dev/null
>     +++ b/drivers/media/i2c/cat24c208.c
>     @@ -0,0 +1,421 @@
>     +// SPDX-License-Identifier: GPL-2.0-only
>     +/*
>     + * cat24c208 - HDMI i2c controlled EEPROM from ON Semiconductor or Catalyst Semiconductor
>     + *
>     + * cat24c208.c - Support for i2c based DDC EEPROM
>     + *
>     + * Copyright (C) 2021-2022 Cisco Systems, Inc. and/or its affiliates. All rights reserved.
>     + */
>     +
>     +/*
>     + * REF_01 - ON Semiconductor, cat24c208, Datasheet, URL : https://www.onsemi.com/pdf/datasheet/cat24c208-d.pdf <https://www.onsemi.com/pdf/datasheet/cat24c208-d.pdf>
>     + *     Revision 7, May 2018
>     + */
>     +
>     +#include <linux/i2c.h>
>     +#include <linux/module.h>
>     +#include <linux/mutex.h>
>     +#include <linux/of_device.h>
>     +#include <linux/regmap.h>
>     +#include <linux/slab.h>
>     +#include <linux/videodev2.h>
>     +#include <linux/kernel.h>
>     +
>     +#include <media/v4l2-common.h>
>     +#include <media/v4l2-device.h>
>     +#include <media/v4l2-event.h>
>     +#include <media/v4l2-fh.h>
>     +#include <media/v4l2-ioctl.h>
>     +
>     +MODULE_DESCRIPTION("cat24c208 EDID EEPROM driver");
>     +MODULE_AUTHOR("Jonathan Selnes Bognaes <jonathansb1@gmail.com <mailto:jonathansb1@gmail.com>>");
>     +MODULE_LICENSE("GPL");
>     +
>     +/*
>     + * CAT24C208 setup
>     + */
>     +#define BYTES_PER_BLOCK                128
>     +#define EDID_EXT_FLAG_BIT      126
>     +#define MAX_WRITE_BYTES                16
>     +#define NUM_BLOCKS             4
>     +#define NUM_CLIENTS            3
>     +#define CONFIG_NB_BIT          BIT(0)
>     +#define CONFIG_AB0_BIT         BIT(1)
>     +#define CONFIG_AB1_BIT         BIT(2)
>     +#define CONFIG_WE_BIT          BIT(3)
>     +
>     +/*
>     + * From 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
>     +
>     +/*
>     + * CAT24C208 addresses
>     + */
>     +#define CONFIG_I2C_ADDR                0x31
>     +#define EEPROM_I2C_ADDR                0x50
>     +#define SEGMENT_I2C_ADDR       0x30
>     +
>     +struct cat24c208_state {
>     +       struct i2c_client *i2c_clients[NUM_CLIENTS];
>     +       // V4L2 ioctl serialization
>     +       struct mutex lock;
>     +
>     +       struct v4l2_device v4l2_dev;
>     +       struct video_device vdev;
>     +
>     +       u8 edid_blocks;         // edid length can vary, one block = 128 bytes
>     +       u8 edid[BYTES_PER_BLOCK * NUM_BLOCKS]; // actual active edid data
>     +};
>     +
>     +static const struct v4l2_file_operations cat24c208_fops = {
>     +       .owner          = THIS_MODULE,
>     +       .open           = v4l2_fh_open,
>     +       .release        = v4l2_fh_release,
>     +       .unlocked_ioctl = video_ioctl2,
>     +};
>     +
>     +static int cat24c208_seg_write(struct cat24c208_state *state, u8 *data, u16 len, u8 seg)
>     +{
>     +       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];
>     +
>     +       struct i2c_msg msg[] = {
>     +               {
>     +                       .addr = seg_client->addr,       // Segment
>     +                       .buf = &seg,
>     +                       .len = 1,
>     +                       .flags = 0,
>     +               },
>     +               {
>     +                       .addr = data_client->addr,      // write data
>     +                       .buf = data,
>     +                       .len = len,
>     +                       .flags = 0,
>     +               },
>     +       };
>     +       int err;
>     +
>     +       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);
>     +       return err < 0 ? err : 0;
>     +}
>     +
>     +static int cat24c208_edid_read(struct cat24c208_state *state, u8 *data, u8 seg, u8 offset, u16 len)
>     +{
>     +       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];
>     +       int err;
>     +
>     +       len *= BYTES_PER_BLOCK;
>     +       if (seg) {
>     +               struct i2c_msg msg[] = {
>     +                       {
>     +                               .addr = seg_client->addr,       // Segment
>     +                               .buf = &seg,
>     +                               .len = 1,
>     +                               .flags = 0,
>     +                       },
>     +                       {
>     +                               .addr = data_client->addr,      // read data
>     +                               .buf = data,
>     +                               .len = len,
>     +                               .flags = I2C_M_RD,
>     +                       },
>     +               };
>     +               err = i2c_transfer(dev_client->adapter, msg, ARRAY_SIZE(msg));
>     +       } else {
>     +               struct i2c_msg msg[] = {
>     +                       {
>     +                               .addr = data_client->addr,      // set offset
>     +                               .buf = &offset,
>     +                               .len = 1,
>     +                               .flags = 0,
>     +                       },
>     +                       {
>     +                               .addr = data_client->addr,      // read data
>     +                               .buf = data,
>     +                               .len = len,
>     +                               .flags = I2C_M_RD,
>     +                       },
>     +               };
>     +               err = i2c_transfer(dev_client->adapter, msg, ARRAY_SIZE(msg));
>     +       }
>     +
>     +       if (err < 0)
>     +               dev_err(&dev_client->dev, "Reading of EDID failed\n");
>     +       return err < 0 ? err : 0;
>     +}
>     +
>     +static int cat24c208_set_config(struct i2c_client *client)
>     +{
>     +       u8 buf[2] = { 0, CONFIG_NB_BIT };
>     +       struct i2c_msg msg = {
>     +               .addr = client->addr,
>     +               .buf = buf,
>     +               .len = sizeof(buf),
>     +               .flags = 0,
>     +       };
>     +       int err;
>     +
>     +       err = i2c_transfer(client->adapter, &msg, 1);
>     +       if (err < 0)
>     +               dev_err(&client->dev, "Could not set config register\n");
>     +       usleep_range(WRITE_CYCLE_TIME_US, 2 * WRITE_CYCLE_TIME_US);
>     +       return err < 0 ? err : 0;
>     +}
>     +
>     +static bool cat24c208_is_valid_edid(const u8 *block)
>     +{
>     +       static const u8 header_pattern[] = {
>     +               0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00
>     +       };
>     +
>     +       return !memcmp(block, header_pattern, sizeof(header_pattern));
>     +}
>     +
>     +static int cat24c208_set_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>     +{
>     +       struct cat24c208_state *state = video_drvdata(file);
>     +       u8 buf[MAX_WRITE_BYTES + 1];
>     +       int err;
>     +       int seg;
>     +       int i;
>     +
>     +       memset(edid->reserved, 0, sizeof(edid->reserved));
>     +
>     +       if (edid->pad)
>     +               return -EINVAL;
>     +
>     +       if (edid->blocks > NUM_BLOCKS) {
>     +               edid->blocks = NUM_BLOCKS;
>     +               return -E2BIG;
>     +       }
>     +
>     +       if (edid->start_block)
>     +               return -EINVAL;
>     +
>     +       state->edid_blocks = edid->blocks;
>     +       memcpy(state->edid, edid->edid, state->edid_blocks * BYTES_PER_BLOCK);
>     +
>     +       /* Write EDID to EEPROM */
>     +       for (i = 0; i < edid->blocks * BYTES_PER_BLOCK; i = i + MAX_WRITE_BYTES) {
>     +               if (i >= 2 * BYTES_PER_BLOCK) {
>     +                       seg = 1;
>     +                       buf[0] = i - BYTES_PER_BLOCK * 2;
>     +               } else {
>     +                       seg = 0;
>     +                       buf[0] = i;
>     +               }
>     +
>     +               memcpy(buf + 1, &edid->edid[i], MAX_WRITE_BYTES);
>     +               err = cat24c208_seg_write(state, buf, MAX_WRITE_BYTES + 1, seg);
>     +               if (err) {
>     +                       dev_err(&state->i2c_clients[0]->dev,
>     +                               "Could not write EDID to EEPROM, i: %d\n", i);
>     +                       return err;
>     +               }
>     +       }
>     +
>     +       return 0;
>     +}
>     +
>     +static int cat24c208_get_edid(struct file *file, void *fh, struct v4l2_edid *edid)
>     +{
>     +       struct cat24c208_state *state = video_drvdata(file);
>     +
>     +       memset(edid->reserved, 0, sizeof(edid->reserved));
>     +
>     +       if (edid->pad != 0)
>     +               return -EINVAL;
>     +
>     +       if (edid->start_block == 0 && edid->blocks == 0) {
>     +               edid->blocks = state->edid_blocks;
>     +               return 0;
>     +       }
>     +
>     +       if (state->edid_blocks == 0)
>     +               return -ENODATA;
>     +
>     +       if (edid->start_block >= state->edid_blocks)
>     +               return -EINVAL;
>     +
>     +       if (edid->start_block + edid->blocks > state->edid_blocks)
>     +               edid->blocks = state->edid_blocks - edid->start_block;
>     +
>     +       memcpy(edid->edid, state->edid + edid->start_block * BYTES_PER_BLOCK,
>     +              edid->blocks * BYTES_PER_BLOCK);
>     +
>     +       return 0;
>     +}
>     +
>     +static int cat24c208_get_input(struct file *file, void *priv, unsigned int *i)
>     +{
>     +       *i = 0;
>     +       return 0;
>     +}
>     +
>     +static int cat24c208_set_input(struct file *file, void *priv, unsigned int i)
>     +{
>     +       return i > 0 ? -EINVAL : 0;
>     +}
>     +
>     +static int cat24c208_enum_input(struct file *file, void *priv,
>     +                               struct v4l2_input *inp)
>     +{
>     +       if (inp->index)
>     +               return -EINVAL;
>     +       strscpy(inp->name, "HDMI", sizeof(inp->name));
>     +       inp->capabilities = 0;
>     +       inp->type = V4L2_INPUT_TYPE_CAMERA;
>     +       return 0;
>     +}
>     +
>     +static int cat24c208_querycap(struct file *file,
>     +                             void *priv, struct v4l2_capability *cap)
>     +{
>     +       strscpy(cap->driver, "cat24c208", sizeof(cap->driver));
>     +       strscpy(cap->card, "cat24c208 EDID EEPROM", sizeof(cap->card));
>     +       return 0;
>     +}
>     +
>     +static const struct v4l2_ioctl_ops cat24c208_ioctl_ops = {
>     +       .vidioc_querycap        = cat24c208_querycap,
>     +       .vidioc_g_edid          = cat24c208_get_edid,
>     +       .vidioc_s_edid          = cat24c208_set_edid,
>     +       .vidioc_g_input         = cat24c208_get_input,
>     +       .vidioc_s_input         = cat24c208_set_input,
>     +       .vidioc_enum_input      = cat24c208_enum_input,
>     +};
>     +
>     +static void cat24c208_release(struct video_device *vdev)
>     +{
>     +       struct cat24c208_state *state = video_get_drvdata(vdev);
>     +
>     +       v4l2_device_unregister(&state->v4l2_dev);
>     +       mutex_destroy(&state->lock);
>     +       kfree(state);
>     +}
>     +
>     +static int cat24c208_probe(struct i2c_client *client)
>     +{
>     +       struct cat24c208_state *state;
>     +       struct v4l2_device *v4l2_dev;
>     +       int blocks;
>     +       int ret;
>     +
>     +       state = kzalloc(sizeof(*state), GFP_KERNEL);
>     +       if (!state)
>     +               return -ENOMEM;
>     +
>     +       state->i2c_clients[0] = client;
>     +       state->i2c_clients[1] = i2c_new_ancillary_device(client, "eeprom", EEPROM_I2C_ADDR);
>     +       if (IS_ERR(state->i2c_clients[1])) {
>     +               ret = PTR_ERR(state->i2c_clients[1]);
>     +               goto free_state;
>     +       }
>     +       state->i2c_clients[2] = i2c_new_ancillary_device(client, "segment", SEGMENT_I2C_ADDR);
>     +       if (IS_ERR(state->i2c_clients[2])) {
>     +               ret = PTR_ERR(state->i2c_clients[2]);
>     +               goto unreg_i2c_first;
>     +       }
>     +
>     +       ret = cat24c208_set_config(client);
>     +       if (ret)
>     +               goto unreg_i2c_all;
>     +
>     +       if (cat24c208_edid_read(state, state->edid, 0, 0, 2) >= 0 &&
>     +           cat24c208_is_valid_edid(state->edid)) {
>     +               unsigned int i;
>     +
>     +               blocks = 1 + state->edid[126];
>     +               state->edid_blocks = blocks;
>     +               for (i = 2; i < blocks; i += 2) {
>     +                       if (cat24c208_edid_read(state, state->edid + i * BYTES_PER_BLOCK,
>     +                                               i / 2, 0, (i + 1 >= blocks ? 1 : 2))) {
>     +                               state->edid_blocks = i;
>     +                               break;
>     +                       }
>     +               }
>     +       }
>     +
>     +       v4l2_dev = &state->v4l2_dev;
>     +       strscpy(v4l2_dev->name, "cat24c208", sizeof(v4l2_dev->name));
>     +       ret = v4l2_device_register(&client->dev, v4l2_dev);
>     +       if (ret) {
>     +               dev_err(&client->dev, "v4l2_device_register failed: %d\n", ret);
>     +               goto unreg_i2c_all;
>     +       }
>     +
>     +       mutex_init(&state->lock);
>     +
>     +       snprintf(state->vdev.name <http://vdev.name>, sizeof(state->vdev.name <http://vdev.name>),
>     +                "cat24c208 %d-%d", client->adapter->nr, client->addr);
>     +
>     +       state->vdev.v4l2_dev = v4l2_dev;
>     +       state->vdev.fops = &cat24c208_fops;
>     +       state->vdev.ioctl_ops = &cat24c208_ioctl_ops;
>     +       state->vdev.lock = &state->lock;
>     +       state->vdev.release = cat24c208_release;
>     +       state->vdev.device_caps = V4L2_CAP_EDID_MEMORY;
>     +
>     +       video_set_drvdata(&state->vdev, state);
>     +       i2c_set_clientdata(client, state);
>     +       ret = video_register_device(&state->vdev, VFL_TYPE_VIDEO, -1);
>     +       if (ret != 0) {
>     +               dev_err(&client->dev, "Video registering failed: %d\n", ret);
>     +               goto unreg_v4l2_dev;
>     +       }
>     +       return 0;
>     +
>     +unreg_v4l2_dev:
>     +       v4l2_device_unregister(&state->v4l2_dev);
>     +unreg_i2c_all:
>     +       i2c_unregister_device(state->i2c_clients[2]);
>     +unreg_i2c_first:
>     +       i2c_unregister_device(state->i2c_clients[1]);
>     +free_state:
>     +       kfree(state);
>     +       return ret;
>     +}
>     +
>     +static int cat24c208_remove(struct i2c_client *client)
>     +{
>     +       struct cat24c208_state *state = i2c_get_clientdata(client);
>     +
>     +       i2c_unregister_device(state->i2c_clients[1]);
>     +       i2c_unregister_device(state->i2c_clients[2]);
>     +       video_unregister_device(&state->vdev);
>     +
>     +       return 0;
>     +}
>     +
>     +static const struct of_device_id cat24c208_of_match[] = {
>     +       { .compatible = "onnn,cat24c208"},
>     +       {}
>     +};
>     +MODULE_DEVICE_TABLE(of, cat24c208_of_match);
>     +
>     +static struct i2c_driver cat24c208_driver = {
>     +       .driver = {
>     +               .name   = "cat24c208",
>     +               .of_match_table = of_match_ptr(cat24c208_of_match),
>     +       },
>     +       .probe_new      = cat24c208_probe,
>     +       .remove         = cat24c208_remove,
>     +};
>     +module_i2c_driver(cat24c208_driver);
>     -- 
>     2.37.1
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
  

On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 28/07/2022 14:02, Andy Shevchenko wrote:
> > On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:

> >     Support reading and writing the EDID EEPROM through the
> >     v4l2 API.
> >
> > Why the normal way of representing as a memory (we have framework and drivers) can’t work?
>
> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
> support without the video receiver. It belongs in drivers/media in any case since EDIDs
> are closely tied to media.

It's fine. From the Linux perspective we do not reduplicate the
drivers that are done by other frameworks, right?

> > Moreover, this driver seems limited in support of variety of the eeprom chips.
>
> Not quite sure what you mean. The cat24c208 is what this was developed for and
> the only one we have.
>
> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
> standard, which a normal EEPROM doesn't. So these devices are specifically made
> for this use-case.

What is the difference from a programming interface?
Can the nvmem driver(s) be reused (at24?)?

...

> >      drivers/media/i2c/cat24c208.c | 421 ++++++++++++++++++++++++++++++++++

It really seems silly to me to add so many LoCs for the existing
drivers and perhaps we need to extend the nvmem to support EDID rather
than copying everything again?

Note, I can be well mistaken by not understanding some underlying
issues, perhaps there is some documentation to read...
  

Hi Andy,

On 28/07/2022 22:56, Andy Shevchenko wrote:
> On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 28/07/2022 14:02, Andy Shevchenko wrote:
>>> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
> 
>>>     Support reading and writing the EDID EEPROM through the
>>>     v4l2 API.
>>>
>>> Why the normal way of representing as a memory (we have framework and drivers) can’t work?
>>
>> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
>> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
>> support without the video receiver. It belongs in drivers/media in any case since EDIDs
>> are closely tied to media.
> 
> It's fine. From the Linux perspective we do not reduplicate the
> drivers that are done by other frameworks, right?
> 
>>> Moreover, this driver seems limited in support of variety of the eeprom chips.
>>
>> Not quite sure what you mean. The cat24c208 is what this was developed for and
>> the only one we have.
>>
>> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
>> standard, which a normal EEPROM doesn't. So these devices are specifically made
>> for this use-case.
> 
> What is the difference from a programming interface?
> Can the nvmem driver(s) be reused (at24?)?

No. EDID EEPROM devices are specific to storing EDIDs: they have two i2c
ports, one connected to (typically) the HDMI bus (DDC lines) allowing a
video source to read the EDID, the other is connected to the SoC to write to
and configure the device. The HDMI bus side has two i2c addresses (reading the
EEPROM and to write to the segment address for EDIDs > 256 bytes), the SoC
side has three i2c addresses: to configure the behavior, the segment address,
and to write the EDID from the SoC.

So it is a much more complex device than a regular eeprom, and it really
is dedicated to EDIDs only.

Also note that the V4L2 API is already used to get/set EDIDs, everything is
in place for supporting that, including support for parsing EDIDs for the
physical address, which is something that is needed if this is combined with
HDMI CEC hardware. It's not implemented in this driver since it is not
needed in our use-case, but that might change in the future.

And by using the V4L2 API you can use v4l2-ctl --get-edid and --set-edid
out of the box, using the standard API for EDIDs.

Regards,

	Hans

> 
> ...
> 
>>>      drivers/media/i2c/cat24c208.c | 421 ++++++++++++++++++++++++++++++++++
> 
> It really seems silly to me to add so many LoCs for the existing
> drivers and perhaps we need to extend the nvmem to support EDID rather
> than copying everything again?
> 
> Note, I can be well mistaken by not understanding some underlying
> issues, perhaps there is some documentation to read...
>
  

On Fri, Jul 29, 2022 at 9:21 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 28/07/2022 22:56, Andy Shevchenko wrote:
> > On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 28/07/2022 14:02, Andy Shevchenko wrote:
> >>> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
> >
> >>>     Support reading and writing the EDID EEPROM through the
> >>>     v4l2 API.
> >>>
> >>> Why the normal way of representing as a memory (we have framework and drivers) can’t work?
> >>
> >> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
> >> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
> >> support without the video receiver. It belongs in drivers/media in any case since EDIDs
> >> are closely tied to media.
> >
> > It's fine. From the Linux perspective we do not reduplicate the
> > drivers that are done by other frameworks, right?
> >
> >>> Moreover, this driver seems limited in support of variety of the eeprom chips.
> >>
> >> Not quite sure what you mean. The cat24c208 is what this was developed for and
> >> the only one we have.
> >>
> >> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
> >> standard, which a normal EEPROM doesn't. So these devices are specifically made
> >> for this use-case.
> >
> > What is the difference from a programming interface?
> > Can the nvmem driver(s) be reused (at24?)?
>
> No. EDID EEPROM devices are specific to storing EDIDs: they have two i2c
> ports, one connected to (typically) the HDMI bus (DDC lines) allowing a
> video source to read the EDID, the other is connected to the SoC to write to
> and configure the device. The HDMI bus side has two i2c addresses (reading the
> EEPROM and to write to the segment address for EDIDs > 256 bytes), the SoC
> side has three i2c addresses: to configure the behavior, the segment address,
> and to write the EDID from the SoC.
>
> So it is a much more complex device than a regular eeprom, and it really
> is dedicated to EDIDs only.

Thanks for the explanation, but it's still unclear what the
differences are in the programming interface there. Perhaps you may
simply register a platform device in this driver and reuse the rest
from at24?

> Also note that the V4L2 API is already used to get/set EDIDs, everything is
> in place for supporting that, including support for parsing EDIDs for the
> physical address, which is something that is needed if this is combined with
> HDMI CEC hardware. It's not implemented in this driver since it is not
> needed in our use-case, but that might change in the future.
>
> And by using the V4L2 API you can use v4l2-ctl --get-edid and --set-edid
> out of the box, using the standard API for EDIDs.

Bonus question: we have cat24c04/cat24c05 are recognized by at24
already, are they different to cat24c08?
  

Hi Andy,

On 29/07/2022 14:00, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 9:21 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 28/07/2022 22:56, Andy Shevchenko wrote:
>>> On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> On 28/07/2022 14:02, Andy Shevchenko wrote:
>>>>> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
>>>
>>>>>     Support reading and writing the EDID EEPROM through the
>>>>>     v4l2 API.
>>>>>
>>>>> Why the normal way of representing as a memory (we have framework and drivers) can’t work?
>>>>
>>>> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
>>>> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
>>>> support without the video receiver. It belongs in drivers/media in any case since EDIDs
>>>> are closely tied to media.
>>>
>>> It's fine. From the Linux perspective we do not reduplicate the
>>> drivers that are done by other frameworks, right?
>>>
>>>>> Moreover, this driver seems limited in support of variety of the eeprom chips.
>>>>
>>>> Not quite sure what you mean. The cat24c208 is what this was developed for and
>>>> the only one we have.
>>>>
>>>> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
>>>> standard, which a normal EEPROM doesn't. So these devices are specifically made
>>>> for this use-case.
>>>
>>> What is the difference from a programming interface?
>>> Can the nvmem driver(s) be reused (at24?)?
>>
>> No. EDID EEPROM devices are specific to storing EDIDs: they have two i2c
>> ports, one connected to (typically) the HDMI bus (DDC lines) allowing a
>> video source to read the EDID, the other is connected to the SoC to write to
>> and configure the device. The HDMI bus side has two i2c addresses (reading the
>> EEPROM and to write to the segment address for EDIDs > 256 bytes), the SoC
>> side has three i2c addresses: to configure the behavior, the segment address,
>> and to write the EDID from the SoC.
>>
>> So it is a much more complex device than a regular eeprom, and it really
>> is dedicated to EDIDs only.
> 
> Thanks for the explanation, but it's still unclear what the
> differences are in the programming interface there. Perhaps you may
> simply register a platform device in this driver and reuse the rest
> from at24?

No, it's really different from a regular eeprom.

> 
>> Also note that the V4L2 API is already used to get/set EDIDs, everything is
>> in place for supporting that, including support for parsing EDIDs for the
>> physical address, which is something that is needed if this is combined with
>> HDMI CEC hardware. It's not implemented in this driver since it is not
>> needed in our use-case, but that might change in the future.
>>
>> And by using the V4L2 API you can use v4l2-ctl --get-edid and --set-edid
>> out of the box, using the standard API for EDIDs.
> 
> Bonus question: we have cat24c04/cat24c05 are recognized by at24
> already, are they different to cat24c08?
> 

Yes, they are different.

Regards,

	Hans
  

On Fri, Jul 29, 2022 at 2:11 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 29/07/2022 14:00, Andy Shevchenko wrote:
> > On Fri, Jul 29, 2022 at 9:21 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 28/07/2022 22:56, Andy Shevchenko wrote:
> >>> On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>>> On 28/07/2022 14:02, Andy Shevchenko wrote:
> >>>>> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
> >>>
> >>>>>     Support reading and writing the EDID EEPROM through the
> >>>>>     v4l2 API.
> >>>>>
> >>>>> Why the normal way of representing as a memory (we have framework and drivers) can’t work?
> >>>>
> >>>> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
> >>>> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
> >>>> support without the video receiver. It belongs in drivers/media in any case since EDIDs
> >>>> are closely tied to media.
> >>>
> >>> It's fine. From the Linux perspective we do not reduplicate the
> >>> drivers that are done by other frameworks, right?
> >>>
> >>>>> Moreover, this driver seems limited in support of variety of the eeprom chips.
> >>>>
> >>>> Not quite sure what you mean. The cat24c208 is what this was developed for and
> >>>> the only one we have.
> >>>>
> >>>> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
> >>>> standard, which a normal EEPROM doesn't. So these devices are specifically made
> >>>> for this use-case.
> >>>
> >>> What is the difference from a programming interface?
> >>> Can the nvmem driver(s) be reused (at24?)?
> >>
> >> No. EDID EEPROM devices are specific to storing EDIDs: they have two i2c
> >> ports, one connected to (typically) the HDMI bus (DDC lines) allowing a
> >> video source to read the EDID, the other is connected to the SoC to write to
> >> and configure the device. The HDMI bus side has two i2c addresses (reading the
> >> EEPROM and to write to the segment address for EDIDs > 256 bytes), the SoC
> >> side has three i2c addresses: to configure the behavior, the segment address,
> >> and to write the EDID from the SoC.
> >>
> >> So it is a much more complex device than a regular eeprom, and it really
> >> is dedicated to EDIDs only.
> >
> > Thanks for the explanation, but it's still unclear what the
> > differences are in the programming interface there. Perhaps you may
> > simply register a platform device in this driver and reuse the rest
> > from at24?
>
> No, it's really different from a regular eeprom.
>
> >> Also note that the V4L2 API is already used to get/set EDIDs, everything is
> >> in place for supporting that, including support for parsing EDIDs for the
> >> physical address, which is something that is needed if this is combined with
> >> HDMI CEC hardware. It's not implemented in this driver since it is not
> >> needed in our use-case, but that might change in the future.
> >>
> >> And by using the V4L2 API you can use v4l2-ctl --get-edid and --set-edid
> >> out of the box, using the standard API for EDIDs.
> >
> > Bonus question: we have cat24c04/cat24c05 are recognized by at24
> > already, are they different to cat24c08?
> >
>
> Yes, they are different.

Thanks for your patience and elaboration, I got it.

Would this driver be used only by v4l2? Or potentially some other
hardware may need it (DRM?)?
  

On 29/07/2022 16:47, Andy Shevchenko wrote:
> On Fri, Jul 29, 2022 at 2:11 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 29/07/2022 14:00, Andy Shevchenko wrote:
>>> On Fri, Jul 29, 2022 at 9:21 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> On 28/07/2022 22:56, Andy Shevchenko wrote:
>>>>> On Thu, Jul 28, 2022 at 3:23 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>>>> On 28/07/2022 14:02, Andy Shevchenko wrote:
>>>>>>> On Thursday, July 28, 2022, Erling Ljunggren <hljunggr@cisco.com <mailto:hljunggr@cisco.com>> wrote:
>>>>>
>>>>>>>     Support reading and writing the EDID EEPROM through the
>>>>>>>     v4l2 API.
>>>>>>>
>>>>>>> Why the normal way of representing as a memory (we have framework and drivers) can’t work?
>>>>>>
>>>>>> Because support for EDID for video sinks is already part of the media subsystem (V4L2).
>>>>>> Normally it is integrated into an HDMI receiver, but in this case it is just the EDID
>>>>>> support without the video receiver. It belongs in drivers/media in any case since EDIDs
>>>>>> are closely tied to media.
>>>>>
>>>>> It's fine. From the Linux perspective we do not reduplicate the
>>>>> drivers that are done by other frameworks, right?
>>>>>
>>>>>>> Moreover, this driver seems limited in support of variety of the eeprom chips.
>>>>>>
>>>>>> Not quite sure what you mean. The cat24c208 is what this was developed for and
>>>>>> the only one we have.
>>>>>>
>>>>>> Note that an EDID EEPROM != a regular EEPROM: it has to support the VESA E-DDC
>>>>>> standard, which a normal EEPROM doesn't. So these devices are specifically made
>>>>>> for this use-case.
>>>>>
>>>>> What is the difference from a programming interface?
>>>>> Can the nvmem driver(s) be reused (at24?)?
>>>>
>>>> No. EDID EEPROM devices are specific to storing EDIDs: they have two i2c
>>>> ports, one connected to (typically) the HDMI bus (DDC lines) allowing a
>>>> video source to read the EDID, the other is connected to the SoC to write to
>>>> and configure the device. The HDMI bus side has two i2c addresses (reading the
>>>> EEPROM and to write to the segment address for EDIDs > 256 bytes), the SoC
>>>> side has three i2c addresses: to configure the behavior, the segment address,
>>>> and to write the EDID from the SoC.
>>>>
>>>> So it is a much more complex device than a regular eeprom, and it really
>>>> is dedicated to EDIDs only.
>>>
>>> Thanks for the explanation, but it's still unclear what the
>>> differences are in the programming interface there. Perhaps you may
>>> simply register a platform device in this driver and reuse the rest
>>> from at24?
>>
>> No, it's really different from a regular eeprom.
>>
>>>> Also note that the V4L2 API is already used to get/set EDIDs, everything is
>>>> in place for supporting that, including support for parsing EDIDs for the
>>>> physical address, which is something that is needed if this is combined with
>>>> HDMI CEC hardware. It's not implemented in this driver since it is not
>>>> needed in our use-case, but that might change in the future.
>>>>
>>>> And by using the V4L2 API you can use v4l2-ctl --get-edid and --set-edid
>>>> out of the box, using the standard API for EDIDs.
>>>
>>> Bonus question: we have cat24c04/cat24c05 are recognized by at24
>>> already, are they different to cat24c08?
>>>
>>
>> Yes, they are different.
> 
> Thanks for your patience and elaboration, I got it.
> 
> Would this driver be used only by v4l2? Or potentially some other
> hardware may need it (DRM?)?

Only V4L2: an EDID describes the capabilities of a video sink (e.g. a display),
so it is specific to video receivers, and that's the domain of V4L2.

Regards,

	Hans
  

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.

...

>  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.
  

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
>
  

On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
<hljunggr@cisco.com> wrote:
> 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:

...

> > > +       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.

But in an error case you will leave EEPROM in an erroneous state?

...

> > > +       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.

It's minor, so up to you, folks.

> > > +       };

...

> > > +       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.

I'm not sure how it's related. Can you elaborate a bit, please?

If you try to forcibly unload the device (driver) when it's open and
it somehow succeeds, that will be a sign of lifetime issues in the
code.
  

On 01/08/2022 16:57, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
> <hljunggr@cisco.com> wrote:
>> 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:
> 
> ...
> 
>>>> +       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.
> 
> But in an error case you will leave EEPROM in an erroneous state?

Yes, if this happens you likely have a hardware error anyway. It's not
worth it trying to be smart in that case. For that matter, I don't know
what a smart solution would be.

> 
> ...
> 
>>>> +       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.
> 
> It's minor, so up to you, folks.
> 
>>>> +       };
> 
> ...
> 
>>>> +       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.
> 
> I'm not sure how it's related. Can you elaborate a bit, please?
> 
> If you try to forcibly unload the device (driver) when it's open and
> it somehow succeeds, that will be a sign of lifetime issues in the
> code.
> 

Not with rmmod but using the unbind facility. For new media drivers we generally
want to avoid using devm_*alloc, it causes more problems than it solves.

It's unlikely to be an issue here, but I'd rather keep it as-is.

Regards,

	Hans
  

On Mon, Aug 1, 2022 at 8:35 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 01/08/2022 16:57, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
> > <hljunggr@cisco.com> wrote:
> >> 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:

...

> >>>> +       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.
> >
> > I'm not sure how it's related. Can you elaborate a bit, please?
> >
> > If you try to forcibly unload the device (driver) when it's open and
> > it somehow succeeds, that will be a sign of lifetime issues in the
> > code.
>
> Not with rmmod but using the unbind facility.

And what is the difference? The device driver core calls the same, no?

> For new media drivers we generally
> want to avoid using devm_*alloc, it causes more problems than it solves.

I think it's because people don't think much about the lifetime of
objects. I don't think devm is an issue here.

> It's unlikely to be an issue here, but I'd rather keep it as-is.

OK.
  

On 8/2/22 10:42, Andy Shevchenko wrote:
> On Mon, Aug 1, 2022 at 8:35 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 01/08/2022 16:57, Andy Shevchenko wrote:
>>> On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
>>> <hljunggr@cisco.com> wrote:
>>>> 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:
> 
> ...
> 
>>>>>> +       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.
>>>
>>> I'm not sure how it's related. Can you elaborate a bit, please?
>>>
>>> If you try to forcibly unload the device (driver) when it's open and
>>> it somehow succeeds, that will be a sign of lifetime issues in the
>>> code.
>>
>> Not with rmmod but using the unbind facility.
> 
> And what is the difference? The device driver core calls the same, no?

rmmod when the /dev/videoX is open won't work (device is busy), whereas
unbind *will* work, and it is really the same as a USB unplug.

> 
>> For new media drivers we generally
>> want to avoid using devm_*alloc, it causes more problems than it solves.
> 
> I think it's because people don't think much about the lifetime of
> objects. I don't think devm is an issue here.

Yes, it is: unbind will call the driver remove() function, and after that
function all memory allocated with devm_*alloc will be freed immediately.

But if an application still has a filehandle open and was possibly even in
the middle of an ioctl call, then having the memory removed instantaneously
is a really bad thing.

Hotpluggable devices in general definitely should not use it. I'm not a fan
of devm_*alloc anymore.

Regards,

	Hans

> 
>> It's unlikely to be an issue here, but I'd rather keep it as-is.
> 
> OK.
>
  

On Tue, Aug 2, 2022 at 11:06 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 8/2/22 10:42, Andy Shevchenko wrote:
> > On Mon, Aug 1, 2022 at 8:35 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 01/08/2022 16:57, Andy Shevchenko wrote:
> >>> On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
> >>> <hljunggr@cisco.com> wrote:
> >>>> 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:

...

> >>>>>> +       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.
> >>>
> >>> I'm not sure how it's related. Can you elaborate a bit, please?
> >>>
> >>> If you try to forcibly unload the device (driver) when it's open and
> >>> it somehow succeeds, that will be a sign of lifetime issues in the
> >>> code.
> >>
> >> Not with rmmod but using the unbind facility.
> >
> > And what is the difference? The device driver core calls the same, no?
>
> rmmod when the /dev/videoX is open won't work (device is busy), whereas
> unbind *will* work, and it is really the same as a USB unplug.

Seems there are no guards against that.

> >> For new media drivers we generally
> >> want to avoid using devm_*alloc, it causes more problems than it solves.
> >
> > I think it's because people don't think much about the lifetime of
> > objects. I don't think devm is an issue here.
>
> Yes, it is: unbind will call the driver remove() function, and after that
> function all memory allocated with devm_*alloc will be freed immediately.

Yes.

> But if an application still has a filehandle open and was possibly even in
> the middle of an ioctl call, then having the memory removed instantaneously
> is a really bad thing.

True.

> Hotpluggable devices in general definitely should not use it. I'm not a fan
> of devm_*alloc anymore.

You are blaming the wrong man here, i.e. devm. The problem as I stated
above is developers who do not understand (pay attention to) the
lifetime of the objects.
  

On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 2, 2022 at 11:06 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > On 8/2/22 10:42, Andy Shevchenko wrote:
> > > On Mon, Aug 1, 2022 at 8:35 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > >> On 01/08/2022 16:57, Andy Shevchenko wrote:
> > >>> On Mon, Aug 1, 2022 at 3:07 PM Erling Ljunggren (hljunggr)
> > >>> <hljunggr@cisco.com> wrote:
> > >>>> 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:
>
> ...
>
> > >>>>>> +       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.
> > >>>
> > >>> I'm not sure how it's related. Can you elaborate a bit, please?
> > >>>
> > >>> If you try to forcibly unload the device (driver) when it's open and
> > >>> it somehow succeeds, that will be a sign of lifetime issues in the
> > >>> code.
> > >>
> > >> Not with rmmod but using the unbind facility.
> > >
> > > And what is the difference? The device driver core calls the same, no?
> >
> > rmmod when the /dev/videoX is open won't work (device is busy), whereas
> > unbind *will* work, and it is really the same as a USB unplug.
>
> Seems there are no guards against that.
>
> > >> For new media drivers we generally
> > >> want to avoid using devm_*alloc, it causes more problems than it solves.
> > >
> > > I think it's because people don't think much about the lifetime of
> > > objects. I don't think devm is an issue here.
> >
> > Yes, it is: unbind will call the driver remove() function, and after that
> > function all memory allocated with devm_*alloc will be freed immediately.
>
> Yes.
>
> > But if an application still has a filehandle open and was possibly even in
> > the middle of an ioctl call, then having the memory removed instantaneously
> > is a really bad thing.
>
> True.
>
> > Hotpluggable devices in general definitely should not use it. I'm not a fan
> > of devm_*alloc anymore.
>
> You are blaming the wrong man here, i.e. devm. The problem as I stated
> above is developers who do not understand (pay attention to) the
> lifetime of the objects.

That said, the devm has nothing to do with the driver still being
problematic for the scenario you described, no?
  

On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

...

> > You are blaming the wrong man here, i.e. devm. The problem as I stated
> > above is developers who do not understand (pay attention to) the
> > lifetime of the objects.
>
> That said, the devm has nothing to do with the driver still being
> problematic for the scenario you described, no?

And the cleanest (at the first glance) solution is to make v4l2 to fix
this bug by suppressing unbind attributes when the device is opened
for all v4l2 subdev drivers, and restore it back when it's closed.
  

On 8/2/22 14:26, Andy Shevchenko wrote:
> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
>>> You are blaming the wrong man here, i.e. devm. The problem as I stated
>>> above is developers who do not understand (pay attention to) the
>>> lifetime of the objects.
>>
>> That said, the devm has nothing to do with the driver still being
>> problematic for the scenario you described, no?
> 
> And the cleanest (at the first glance) solution is to make v4l2 to fix
> this bug by suppressing unbind attributes when the device is opened
> for all v4l2 subdev drivers, and restore it back when it's closed.
> 

Why would we do that? The patch works in the scenario that I described:
the memory is freed in the struct video_device release() callback, which
is called when the last reference to the video node goes away. This is
standard V4L2 framework behavior that works great in the case of a unbind.

Without devm_kzalloc it works fine, even when unbind is called. With
devm_kzalloc the unbind attributes would have to be suppressed. I see no
reason for that as media maintainer.

Regards,

	Hans
  

On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 8/2/22 14:26, Andy Shevchenko wrote:
> > On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:

...

> >>> You are blaming the wrong man here, i.e. devm. The problem as I stated
> >>> above is developers who do not understand (pay attention to) the
> >>> lifetime of the objects.
> >>
> >> That said, the devm has nothing to do with the driver still being
> >> problematic for the scenario you described, no?
> >
> > And the cleanest (at the first glance) solution is to make v4l2 to fix
> > this bug by suppressing unbind attributes when the device is opened
> > for all v4l2 subdev drivers, and restore it back when it's closed.
>
> Why would we do that? The patch works in the scenario that I described:
> the memory is freed in the struct video_device release() callback, which
> is called when the last reference to the video node goes away. This is
> standard V4L2 framework behavior that works great in the case of a unbind.
>
> Without devm_kzalloc it works fine, even when unbind is called. With
> devm_kzalloc the unbind attributes would have to be suppressed. I see no
> reason for that as media maintainer.

I'm not sure anymore that we are talking about the same thing.

Your driver allocates memory with kzalloc() in ->probe() and frees it
in ->remove(). How is this different from the lifetime of a devm:ed
object? If what you said is true, than driver is still problematic,
since ->remove() frees this memory the very same way at unbind call.
  

On 8/2/22 14:49, Andy Shevchenko wrote:
> On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 8/2/22 14:26, Andy Shevchenko wrote:
>>> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
> 
> ...
> 
>>>>> You are blaming the wrong man here, i.e. devm. The problem as I stated
>>>>> above is developers who do not understand (pay attention to) the
>>>>> lifetime of the objects.
>>>>
>>>> That said, the devm has nothing to do with the driver still being
>>>> problematic for the scenario you described, no?
>>>
>>> And the cleanest (at the first glance) solution is to make v4l2 to fix
>>> this bug by suppressing unbind attributes when the device is opened
>>> for all v4l2 subdev drivers, and restore it back when it's closed.
>>
>> Why would we do that? The patch works in the scenario that I described:
>> the memory is freed in the struct video_device release() callback, which
>> is called when the last reference to the video node goes away. This is
>> standard V4L2 framework behavior that works great in the case of a unbind.
>>
>> Without devm_kzalloc it works fine, even when unbind is called. With
>> devm_kzalloc the unbind attributes would have to be suppressed. I see no
>> reason for that as media maintainer.
> 
> I'm not sure anymore that we are talking about the same thing.
> 
> Your driver allocates memory with kzalloc() in ->probe() and frees it
> in ->remove(). How is this different from the lifetime of a devm:ed
> object? If what you said is true, than driver is still problematic,
> since ->remove() frees this memory the very same way at unbind call.

No, it is not freed in remove(). remove() calls video_unregister_device(),
and that calls cat24c208_release() when the last user of /dev/videoX closes
its filehandle. And it is cat24c208_release() that finally frees the memory.

Regards,

	Hans
  

On Tue, Aug 2, 2022 at 2:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> On 8/2/22 14:49, Andy Shevchenko wrote:
> > On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 8/2/22 14:26, Andy Shevchenko wrote:
> >>> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
> >>> <andy.shevchenko@gmail.com> wrote:
> >>>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
> >>>> <andy.shevchenko@gmail.com> wrote:

...

> >>>>> You are blaming the wrong man here, i.e. devm. The problem as I stated
> >>>>> above is developers who do not understand (pay attention to) the
> >>>>> lifetime of the objects.
> >>>>
> >>>> That said, the devm has nothing to do with the driver still being
> >>>> problematic for the scenario you described, no?
> >>>
> >>> And the cleanest (at the first glance) solution is to make v4l2 to fix
> >>> this bug by suppressing unbind attributes when the device is opened
> >>> for all v4l2 subdev drivers, and restore it back when it's closed.
> >>
> >> Why would we do that? The patch works in the scenario that I described:
> >> the memory is freed in the struct video_device release() callback, which
> >> is called when the last reference to the video node goes away. This is
> >> standard V4L2 framework behavior that works great in the case of a unbind.
> >>
> >> Without devm_kzalloc it works fine, even when unbind is called. With
> >> devm_kzalloc the unbind attributes would have to be suppressed. I see no
> >> reason for that as media maintainer.
> >
> > I'm not sure anymore that we are talking about the same thing.
> >
> > Your driver allocates memory with kzalloc() in ->probe() and frees it
> > in ->remove(). How is this different from the lifetime of a devm:ed
> > object? If what you said is true, than driver is still problematic,
> > since ->remove() frees this memory the very same way at unbind call.
>
> No, it is not freed in remove(). remove() calls video_unregister_device(),
> and that calls cat24c208_release() when the last user of /dev/videoX closes
> its filehandle. And it is cat24c208_release() that finally frees the memory.

Okay, I got it.

So, if you unbind the driver the state will be stale and still being
accessible by the user (with outdated info). Now, what happens if you
unbind and try to bind back, while the user is slow enough to close
the device file?

Also it's still racy against any i2c calls done via IOCTLs, because
you have unregistered I2C devices.
  

On Tue, Aug 2, 2022 at 6:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Aug 2, 2022 at 2:58 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > On 8/2/22 14:49, Andy Shevchenko wrote:
> > > On Tue, Aug 2, 2022 at 2:45 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> > >> On 8/2/22 14:26, Andy Shevchenko wrote:
> > >>> On Tue, Aug 2, 2022 at 2:23 PM Andy Shevchenko
> > >>> <andy.shevchenko@gmail.com> wrote:
> > >>>> On Tue, Aug 2, 2022 at 2:21 PM Andy Shevchenko
> > >>>> <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > >>>>> You are blaming the wrong man here, i.e. devm. The problem as I stated
> > >>>>> above is developers who do not understand (pay attention to) the
> > >>>>> lifetime of the objects.
> > >>>>
> > >>>> That said, the devm has nothing to do with the driver still being
> > >>>> problematic for the scenario you described, no?
> > >>>
> > >>> And the cleanest (at the first glance) solution is to make v4l2 to fix
> > >>> this bug by suppressing unbind attributes when the device is opened
> > >>> for all v4l2 subdev drivers, and restore it back when it's closed.
> > >>
> > >> Why would we do that? The patch works in the scenario that I described:
> > >> the memory is freed in the struct video_device release() callback, which
> > >> is called when the last reference to the video node goes away. This is
> > >> standard V4L2 framework behavior that works great in the case of a unbind.
> > >>
> > >> Without devm_kzalloc it works fine, even when unbind is called. With
> > >> devm_kzalloc the unbind attributes would have to be suppressed. I see no
> > >> reason for that as media maintainer.
> > >
> > > I'm not sure anymore that we are talking about the same thing.
> > >
> > > Your driver allocates memory with kzalloc() in ->probe() and frees it
> > > in ->remove(). How is this different from the lifetime of a devm:ed
> > > object? If what you said is true, than driver is still problematic,
> > > since ->remove() frees this memory the very same way at unbind call.
> >
> > No, it is not freed in remove(). remove() calls video_unregister_device(),
> > and that calls cat24c208_release() when the last user of /dev/videoX closes
> > its filehandle. And it is cat24c208_release() that finally frees the memory.
>
> Okay, I got it.
>
> So, if you unbind the driver the state will be stale and still being
> accessible by the user (with outdated info). Now, what happens if you
> unbind and try to bind back, while the user is slow enough to close
> the device file?
>
> Also it's still racy against any i2c calls done via IOCTLs, because
> you have unregistered I2C devices.

As far as I understand current design, you may not call anything but
video_device_unregister() in the subdevice's ->remove(), or be very
careful on what resources can be used via IOCTLs.
  

Hi Erling,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v5.19]
[cannot apply to next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Erling-Ljunggren/Add-the-cat24c208-EDID-EEPROM-driver-new-EDID-capability/20220728-194524
base:   git://linuxtv.org/media_tree.git master
config: s390-randconfig-c005-20220802 (https://download.01.org/0day-ci/archive/20220803/202208030925.bp514QvB-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/94ba9bb463937ad05b92c6be56a552894b386774
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Erling-Ljunggren/Add-the-cat24c208-EDID-EEPROM-driver-new-EDID-capability/20220728-194524
        git checkout 94ba9bb463937ad05b92c6be56a552894b386774
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/gpu/drm/ drivers/media/i2c/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/media/i2c/cat24c208.c:19:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/media/i2c/cat24c208.c:19:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/media/i2c/cat24c208.c:19:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/media/i2c/cat24c208.c:407:34: warning: unused variable 'cat24c208_of_match' [-Wunused-const-variable]
   static const struct of_device_id cat24c208_of_match[] = {
                                    ^
   13 warnings generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for DRM_DP_AUX_BUS
   Depends on [n]: HAS_IOMEM [=y] && DRM [=y] && OF [=n]
   Selected by [y]:
   - DRM_MSM [=y] && HAS_IOMEM [=y] && DRM [=y] && (ARCH_QCOM || SOC_IMX5 || COMPILE_TEST [=y]) && COMMON_CLK [=y] && IOMMU_SUPPORT [=y] && (QCOM_OCMEM [=n] || QCOM_OCMEM [=n]=n) && (QCOM_LLCC [=y] || QCOM_LLCC [=y]=n) && (QCOM_COMMAND_DB [=n] || QCOM_COMMAND_DB [=n]=n)


vim +/cat24c208_of_match +407 drivers/media/i2c/cat24c208.c

   406	
 > 407	static const struct of_device_id cat24c208_of_match[] = {
   408		{ .compatible = "onnn,cat24c208"},
   409		{}
   410	};
   411	MODULE_DEVICE_TABLE(of, cat24c208_of_match);
   412