Re: [PATCH v2 0/5] Add the cat24c208 EDID EEPROM driver + new EDID capability

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Wed, Aug 03, 2022 at 11:19:31AM +0200, Hans Verkuil wrote:
> On 03/08/2022 11:14, Laurent Pinchart wrote:
> > Hi Erling,
> > 
> > (CC'ing the linux-i2c mailing list and Srinivas, the maintainer of the
> > nvmem framework)
> > 
> > Thank you for the patches.
> > 
> > On Wed, Aug 03, 2022 at 09:58:45AM +0200, Erling Ljunggren wrote:
> >> This series adds support for the standalone cat24c208 EDID EEPROM i2c device.
> >> Usually EDID support is part of an HDMI receiver, but this is a standalone EEPROM.
> >>
> >> Note that EEPROMs for EDIDs are not regular EEPROM devices, these are dual port
> >> devices that follow the VESA E-DDC standard.
> >>
> >> Since this is a standalone device that does not capture any video a new
> >> V4L2_CAP_EDID_MEMORY capability is introduced to represent such devices.
> >> Note that such a device doesn't have to be an EEPROM, it can also be
> >> implemented using a microcontroller, for example. Hence the use of the generic
> >> word 'MEMORY'.
> > 
> > Why can't this be exposed as a nvmem device, like other types of eeproms
> > here ?
> 
> Because it isn't. Same discussion I had with Andy: it's not a regular eeprom but
> a dedicated eeprom for EDIDs. And we have support for that already in V4L2. It's
> specific to receivers as well, so the only subsystem it belongs to is V4L2.

Why does that matter ? It's still an NVMEM device that stores 512 bytes
of data, even if the I2C interface to read/write it is different than
other types of standard EEPROMs.

> See the discussion I had with Andy on this:
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20220728114050.2400475-5-hljunggr@cisco.com/
> 
> >> The new capability uses the free bit 0x00000008. But we are running out of
> >> capability bits: only 0x40000000 and 0x00000008 are free at the moment.
> >>
> >> There is one other capability V4L2_CAP_ASYNCIO (0x02000000) that is not used
> >> at all, it was never implemented. Wouldn't it be better to define
> >> V4L2_CAP_ASYNCIO to 0, mark it as obsolete, and instead reuse it for this
> >> V4L2_CAP_EDID_MEMORY capability?
> >>
> >> v2:
> >>  - fix dt binding example
> >>  - rename i2c client variables in data struct
> >>  - fix include: of_device.h -> mod_devicetable.h
> >>  - Sorted makefile
> >>  - used define EDID_OFFSET_EXT_FLAG instead of magic number
> >>  - removed of_match_ptr
> >>  - added bus_info
> >>  - remove unneeded headers
> >>  - add depends on OF to Kconfig
> >>
> >> Erling Ljunggren (4):
> >>   media: videodev2.h: add V4L2_CAP_EDID_MEMORY
> >>   media: docs: Add V4L2_CAP_EDID_MEMORY
> >>   dt-bindings: media: add cat24c208 bindings
> >>   media: v4l2-dev: handle V4L2_CAP_EDID_MEMORY
> >>
> >> Jonathan Selnes (1):
> >>   media: i2c: cat24c208: driver for the cat24c208 EDID EEPROM
> >>
> >>  .../bindings/media/i2c/onnn,cat24c208.yaml    |  40 ++
> >>  .../userspace-api/media/v4l/biblio.rst        |  11 +
> >>  .../media/v4l/vidioc-querycap.rst             |   7 +
> >>  .../media/videodev2.h.rst.exceptions          |   1 +
> >>  MAINTAINERS                                   |   7 +
> >>  drivers/media/i2c/Kconfig                     |   9 +
> >>  drivers/media/i2c/Makefile                    |   1 +
> >>  drivers/media/i2c/cat24c208.c                 | 421 ++++++++++++++++++
> >>  drivers/media/v4l2-core/v4l2-dev.c            |   8 +
> >>  include/uapi/linux/videodev2.h                |   1 +
> >>  10 files changed, 506 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/media/i2c/onnn,cat24c208.yaml
> >>  create mode 100644 drivers/media/i2c/cat24c208.c

-- 
Regards,

Laurent Pinchart