On 01/08/2022 16:57, Andy Shevchenko wrote:
> 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?
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
