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