Prev: [ANNOUNCE] MDB Merkey's Kernel Debugger x86_64 2.6.34 06-30-2010 Release 3
Next: [PATCH] sony-laptop: use platform_device_unregister in sony_pf_remove
From: Wan ZongShun on 30 Jun 2010 22:10
I don't get your patch in RTC mail list, but I google it.
Some minior comments:

2010/7/1 Mark Brown <broonie(a)opensource.wolfsonmicro.com>:
> On Tue, Jun 29, 2010 at 02:16:55PM +0100, David Dajun Chen wrote:
>> RTC module of the device driver for DA9052 PMIC device from Dialog
>> Semiconductor.
>
> Adding Alessandro and the RTC list.  As you have been reminded
> repeatedly you should CC all the relevant maintainers and lists on
> patches.
>
>> +config RTC_DRV_DA9052
>> +     tristate "Support RTC on Dialog Semiconductor DA9052 PMIC"
>> +     depends on PMIC_DA9052
>> +     help
>> +       Say y here to support the RTC found on
>> +       Dialog Semiconductor DA9052 PMIC.
>
> Kconfig and Makefile entries should be sorted.
>
>> +     if (msg.data & DA9052_ALARMMI_ALARMTYPE)
>> +             printk(KERN_INFO "RTC: TIMER ALARM\n");
>> +     else
>> +             printk(KERN_INFO "RTC: TICK ALARM\n");
>
> This logging seems inappropriate - you should be notifying the RTC
> subsystem of the events rather than logging them (loudly).
>
>> +static int da9052_rtc_validate_parameters(struct rtc_time *rtc_tm)
>> +{
>> +
>> +     if (rtc_tm->tm_sec > DA9052_RTC_SECONDS_LIMIT)
>> +             return DA9052_RTC_INVALID_SECONDS;
>
> Are these limits different to those enforced by rtc_valid_tm()?
> If there are any additional restrictions it'd be better to call that and
> only implement the additional checks that are required by the specific
> chip.  If checks apply to all times then adding them to rtc_valid_tm()
> would be better.

I have checked your validate_parameters codes, and why you limit
rtc_tm->tm_year
larger than '63'? , anyway, there is no need to implement this
function, except the years lmit.

In addition, in all the set time functions(set alarm or set time), you
don't need to
check your 'rtc_tm' again, since the checking action has done in rtc subsystem.
In all read time functions(read time or read alarm), it is much better
to hire 'rtc_valid_tm'
to ensure the correct returning time value for you.

In read_alarm function, tell your alarm->enabled to caller would be fine.

>
>> +     if (ENABLE == flag)
>> +             msg.data = msg.data | DA9052_ALARMY_ALARMON;
>> +     else if (DISABLE == flag)
>> +             msg.data = msg.data & ~(DA9052_ALARMY_ALARMON);
>
> This can just be written as
>
>        if (flag)
>                ...
>        else
>                ...
>
> which is much more idiomatic.
>
>> +     da9052_lock(da9052);
>> +     ret = da9052->read(da9052, &ssc_msg);
>> +     if (ret) {
>> +             da9052_unlock(da9052);
>> +             return ret;
>> +     }
>> +     da9052_unlock(da9052);
>> +
>> +     data = ret;
>> +
>> +     ssc_msg.data = data &= ~DA9052_IRQMASKA_MALRAM;
>> +
>> +     da9052_lock(da9052);
>> +     ret = da9052->write(da9052, &ssc_msg);
>> +     da9052_unlock(da9052);
>
> This has the locking problem with read/modify/write sequences I
> mentioned in reply to the regulator driver.
>
>> +static int da9052_rtc_class_ops_settime(struct device *dev, struct
>> rtc_time *tm)
>> +{
>> +     int ret;
>> +     struct da9052_rtc *priv = dev_get_drvdata(dev);
>> +
>> +     ret = da9052_rtc_settime(priv->da9052, tm);
>> +
>> +     return ret;
>> +}
>
> Separating the class operations and the actual implementations like this
> looks a bit odd - what does it add?
>
>> +/* Months */
>> +#define FEBRUARY                                     (2)
>> +#define APRIL                                                (4)
>> +#define JUNE                                         (6)
>> +#define      SEPTEMBER                                       (9)
>> +#define NOVEMBER                                     (11)
>
> These are *very* generic things to be defining in a driver specific
> header with no namespacing, and it seems odd that you'd need them at
> all.
>
>> +/* RTC error codes */
>> +#define DA9052_RTC_INVALID_SECONDS                   (3)
>> +#define DA9052_RTC_INVALID_MINUTES                   (4)
>> +#define DA9052_RTC_INVALID_HOURS                     (5)
>> +#define DA9052_RTC_INVALID_DAYS                              (6)
>> +#define DA9052_RTC_INVALID_MONTHS                    (7)
>> +#define DA9052_RTC_INVALID_YEARS                     (8)
>> +#define DA9052_RTC_INVALID_EVENT                     (9)
>> +#define DA9052_RTC_INVALID_IOCTL                     (10)
>> +#define DA9052_RTC_INVALID_SETTING                   (11)
>> +#define DA9052_RTC_EVENT_ALREADY_REGISTERED          (12)
>> +#define DA9052_RTC_EVENT_UNREGISTERED                        (13)
>> +#define DA9052_RTC_EVENT_REGISTRATION_FAILED         (14)
>> +#define DA9052_RTC_EVENT_UNREGISTRATION_FAILED               (15)
>
> Rather than defining custom error codes you should use standard Linux
> error codes.  These also overlap with the standard Linux codes.
>
> --
> You received this message because you are subscribed to "rtc-linux".
> Membership options at http://groups.google.com/group/rtc-linux .
> Please read http://groups.google.com/group/rtc-linux/web/checklist
> before submitting a driver.



--
*linux-arm-kernel mailing list
mail addr:linux-arm-kernel(a)lists.infradead.org
you can subscribe by:
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

* linux-arm-NUC900 mailing list
mail addr:NUC900(a)googlegroups.com
main web: https://groups.google.com/group/NUC900
you can subscribe it by sending me mail:
mcuos.com(a)gmail.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: [ANNOUNCE] MDB Merkey's Kernel Debugger x86_64 2.6.34 06-30-2010 Release 3
Next: [PATCH] sony-laptop: use platform_device_unregister in sony_pf_remove