Re: [Qemu-devel] [PATCH 05/17] timer: generalize Dallas/Maxim RTC i2c devices

[Michael Davidsaver]

On 12/06/2017 05:14 AM, David Gibson wrote:
> On Sun, Dec 03, 2017 at 03:15:10PM -0600, Michael Davidsaver wrote:
>> On 11/29/2017 11:13 PM, David Gibson wrote:
>>> On Sun, Nov 26, 2017 at 03:59:03PM -0600, Michael Davidsaver wrote:
>>>> Support for: ds1307, ds1337, ds1338, ds1339,
>>>> ds1340, ds1375, ds1388, and ds3231.
>>>>
>>>> Tested with ds1338 and ds1375.
>>>>
>>>> Signed-off-by: Michael Davidsaver <address@hidden>
>>>
>>> I certainly like the idea of consolidating this code, but reviewing to
>>> see that the new code really is a generalization of the old is
>>> something I won't have time for for a while.
>>>
>>> Also, hw/timer is not within my purview so it'll probably need to go
>>> another path to merge.
>>
>> Could you be a bit more explicit about what, if anything, I need to do
>> to move this forward?
> 
> Ugh.. that's pretty tough, since ds1338 doesn't have an activate
> maintainer.  You can look at the git history for some possible
> candidates of people to ask about it, but it hasn't been touched much
> in quite a while.
> 
> One approach that could help is to re-order so that your testing
> rework goes before the change to ds1338.  If your new generalization
> can pass the same set of tests as the original ds1338 code, that's at
> least a good start on being convincing that it's a true superset of
> the previous functionality.

A slight wrinkle is that my testing found two bugs with the original
ds1338 model (also one in my new code).  The two don't seem
significant practically.  It actually isn't possible to switch to
12-hour mode.  And there is an obscure off by one situation possible
with wday_offset and timezones.  Of course most guests use 24-hour mode
and ignore the RTC day-of-week.

The upshot of this is that several test cases now fail when run against 
ds1338.c.

My recommendation would be to start by looking at the test code.
This could be compared against guest code.  When I send the next
iteration I'll include some links.

> The other approach is to do the rework in a rather longer series of
> patches.  Start by simply moving ds1338.c, then do a mechanical
> replacement of the names within it, then start generalizing and
> altering.  That's a lot of work for you, but it makes it much easier
> to review each step

I know from my first foray into QEMU land that this is preferable for review.
Unfortunately, I expanded from my previous ds1375 model instead of the ds1338.

signature.asc
Description: OpenPGP digital signature