linux-kernel - Re: [PATCH RFC V1 2/3] rtc: da9063: Add DA9062 RTC capability to DA9063 RTC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Mon, 20 Jul 2015 23:50:45 +0200
From:	Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:	"Opensource [Steve Twiss]" <stwiss.opensource@...semi.com>
Cc:	Alessandro Zummo <a.zummo@...ertech.it>,
	Lee Jones <lee.jones@...aro.org>,
	DEVICETREE <devicetree@...r.kernel.org>,
	LINUXINPUT <linux-input@...r.kernel.org>,
	LINUXKERNEL <linux-kernel@...r.kernel.org>,
	RTCLINUX <rtc-linux@...glegroups.com>,
	David Dajun Chen <david.chen@...semi.com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Support Opensource <Support.Opensource@...semi.com>
Subject: Re: [PATCH RFC V1 2/3] rtc: da9063: Add DA9062 RTC capability to
 DA9063 RTC driver

On 20/07/2015 at 17:57:50 +0000, Opensource [Steve Twiss] wrote :
> On 18 July 2015 00:45, Alexandre Belloni wrote:
> > On 09/07/2015 at 08:45:53 +0100, S Twiss wrote :
> > > - Addition of a re-try when reading the RTC inside da9063_rtc_read_time()
> > 
> > Can you separate that change in another patch as it is a change in the
> > behaviour of the driver? And maybe give a word or two on why this is
> > needed.
> 
> Sure.
> 
> I will separate this into a new patch and add it at a later date. It is a recommendation
> to have this in for both DA9062/3 now, although I have not seen this happen during
> my testing -- it should be added.
> Something similar happened with the RTC previously -- everything tested fine 
> with earlier kernels until the RTC core functions were improved and I picked up a
> synchronisation problem with my testing:
> 
> http://lxr.free-electrons.com/source/drivers/rtc/rtc-da9063.c#L129
> 
> [...]
> 
> > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> > > index 7ffc570..e94fb6d 100644
> > > --- a/drivers/rtc/rtc-da9063.c
> > > +++ b/drivers/rtc/rtc-da9063.c
> > > @@ -1,127 +1,274 @@
> > > -/* rtc-da9063.c - Real time clock device driver for DA9063
> > > - * Copyright (C) 2013-14  Dialog Semiconductor Ltd.
> > > +/*
> > > + * Real time clock device driver for DA9063/DA9062
> > > + * Copyright (C) 2013-15  Dialog Semiconductor Ltd.
> > >   *
> > > - * This library is free software; you can redistribute it and/or
> > > - * modify it under the terms of the GNU Library General Public
> > > - * License as published by the Free Software Foundation; either
> > > - * version 2 of the License, or (at your option) any later version.
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License
> > > + * as published by the Free Software Foundation; either version 2
> > > + * of the License, or (at your option) any later version.
> > >   *
> > > - * This library is distributed in the hope that it will be useful,
> > > + * This program is distributed in the hope that it will be useful,
> > >   * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > > - * Library General Public License for more details.
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > >   */
> > >
> > 
> > Please also list that license change in the commit log. It should also
> > probably be separated in another patch.
> > 
> 
> I can add this to a different patch and change it at a later date.
> It is to fix a minor wording problem with the GPL header so it matches the correct
> MODULE_LICENSE(""); macro. 
> 
> https://lkml.org/lkml/2015/4/18/19
> 
> The info at the top of the file needs to remove the LGPL worded text where appropriate:
> words such as "Library" from the line "GNU Library General Public License"; and replace
> the word "library" with "program" in several places
> 

Yeah, I understood the change, it can go in as soon as you send the
patch.

> 
> > > +		return -ENXIO;
> > > +
> > > +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> > > +	if (!rtc)
> > > +		return -ENOMEM;
> > > +
> > > +	if (strncmp(match->name, "dlg,da9063-rtc", 14) == 0) {
> > 
> > You must not do that.
> > You should add a new compatible and change the of_compatible string of
> > the mfd_cell in drivers/mfd/da9063-core.c onc you know the variant.
> 
> I can check for a binary comparison against the address of the 
> static const struct of_device_id da9063_compatible_reg_id_table[] = {}
> entry for DA9063.

You also must not compare pointers that way. You can use
of_device_is_compatible().

> The DA9063 driver already associates "dlg,da9063-rtc" with both BB and AD
> register maps. I think that altering the string at this point would break backwards
> compatibility in the device tree for the DA9063.

I'm not fond of the backward compatibility exactly for that kind of
issue. The proper way to handle it is to have tow different compatible
strings because obviously, the BB and AD variants are not compatible.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/