Hi Andrew,
>> + struct ptp_clock *clock;
>> + struct aq_ptp_s *self;
>
> I find the use of self in this code quite confusing. It does not
> appear to have a clear meaning. It can be a aq_ring_s, aq_nic_c,
> aq_hw_s, aq_vec_s.
>
> Looking at this code i always have to figure out what self is. Could
> you not just use struct aq_ptp_s aq_ptp consistently in the code?
Agreed,
>> +
>> + self = kzalloc(sizeof(*self), GFP_KERNEL);
>
> Using devm_kzalloc() will make your clean up easier.
>> +
>> + kfree(self);
>
> kfree() is happy to take a NULL pointer. But this could all go away
> with devm_kzalloc().
You are probably right, that'll be easier,
>> +static int hw_atl_b0_adj_sys_clock(struct aq_hw_s *self, s64 delta)
>> +{
>> + ptp_clk_offset += delta;
>> +
>> + return 0;
>> +}
>
> Does this work when i have a box with 42 NICs in it? Does not each NIC
> need its own clock offset? Just seeing code like this causes alarm
> bells. So if it is correct, i would expect some sort of comment to
> prevent those alarm bells.
No comment is needed, that is obviously a per-device variable,
Thanks for catching this!
Will fix that, as well as kbuild robot discovered issues.
Regards,
Igor
