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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

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.

-- 
With Best Regards,
Andy Shevchenko