| 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
| ||
|
Message-ID: <48F8467C.5010902@mimc.co.uk>
Date: Fri, 17 Oct 2008 09:02:04 +0100
From: Mark Jackson <mpfj@...c.co.uk>
To: dbrownell@...rs.sourceforge.net
CC: Alessandro Zummo <alessandro.zummo@...ertech.it>,
rtc-linux@...glegroups.com, lkml <linux-kernel@...r.kernel.org>,
spi-devel-general@...ts.sourceforge.net
Subject: Re: [PATCH] Add Dallas DS1390 RTC chip
David ... please see comments below regarding spi buffers ...
Regards
Mark
Alessandro Zummo wrote:
> On Tue, 14 Oct 2008 09:05:18 +0100
> Mark Jackson <mpfj@...c.co.uk> wrote:
>
> Hi Mark,
>
> a few notes below:
>
>> +#include <linux/module.h>
>> +#include <linux/version.h>
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/init.h>
>> +#include <linux/rtc.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/bcd.h>
>> +#include <linux/delay.h>
>
> Please check that you require all of those includes.
>
>
>> +struct ds1390
>> +{
>> + struct rtc_device *rtc;
>> + u8 buf[9]; /* cmd + 8 registers */
>> + u8 tx_buf[2];
>> + u8 rx_buf[2];
>> +};
>> +
>> +static void ds1390_set_reg(struct device *dev, unsigned char address,
>> + unsigned char data)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + unsigned char buf[2];
>> +
>> + /* MSB must be '1' to write */
>> + buf[0] = address | 0x80;
>> + buf[1] = data;
>> +
>> + spi_write(spi, buf, 2);
>> +}
>> +
>> +static int ds1390_get_reg(struct device *dev, unsigned char address,
>> + unsigned char *data)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + struct ds1390 *chip = dev_get_drvdata(dev);
>> + struct spi_message message;
>> + struct spi_transfer xfer;
>> + int status;
>> +
>> + if (!data)
>> + return -EINVAL;
>> +
>> + /* Build our spi message */
>> + spi_message_init(&message);
>> + memset(&xfer, 0, sizeof(xfer));
>> + xfer.len = 2;
>> + /* Can tx_buf and rx_buf be equal? The doc in spi.h is not sure... */
>> + xfer.tx_buf = chip->tx_buf;
>> + xfer.rx_buf = chip->rx_buf;
>
> you use the same buffer a few functions below. either
> one way or the other. please investigate with the spi subsystem maintainer.
David,
Just to double check (as per Alessandro's suggestion), is this okay use of the
spi buffers ?
I took the code from an existing RTC driver (rtc-max6902), so I am assuming that
this code has already passed judgement.
>
>
>> +
>> + /* Clear MSB to indicate read */
>> + chip->tx_buf[0] = address & 0x7f;
>> +
>> + spi_message_add_tail(&xfer, &message);
>> +
>> + /* do the i/o */
>> + status = spi_sync(spi, &message);
>> + if (status == 0)
>> + {
>> + status = message.status;
>> + }
>> + else
>> + {
>> + return status;
>> + }
>
> should be cleaner in this way:
>
> if (status != 0)
> return status;
>
> *data = chip->rx_buf[1];
>
> return message.status;
>
>
>> +static int ds1390_get_datetime(struct device *dev, struct rtc_time *dt)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + struct ds1390 *chip = dev_get_drvdata(dev);
>> + struct spi_message message;
>> + struct spi_transfer xfer;
>> + int status;
>> +
>> + /* build the message */
>> + spi_message_init(&message);
>> + memset(&xfer, 0, sizeof(xfer));
>> + xfer.len = 1 + 7; /* read command + 7 registers */
>> + xfer.tx_buf = chip->buf;
>> + xfer.rx_buf = chip->buf;
>
> the buffer mentioned above.
... see text above ...
>
>
>> + chip->buf[0] = DS1390_REG_SECONDS;
>> + spi_message_add_tail(&xfer, &message);
>> +
>> + /* do the i/o */
>> + status = spi_sync(spi, &message);
>> + if (status == 0)
>> + {
>> + status = message.status;
>> + }
>> + else
>> + {
>> + return status;
>> + }
>
> ditto.
>
>> + /* The chip sends data in this order:
>> + * Seconds, Minutes, Hours, Day, Date, Month / Century, Year */
>> + dt->tm_sec = BCD2BIN(chip->buf[1]);
>> + dt->tm_min = BCD2BIN(chip->buf[2]);
>> + dt->tm_hour = BCD2BIN(chip->buf[3]);
>> + dt->tm_wday = BCD2BIN(chip->buf[4]);
>> + dt->tm_mday = BCD2BIN(chip->buf[5]);
>> + /* mask off century bit */
>> + dt->tm_mon = BCD2BIN(chip->buf[6] & 0x7f) - 1;
>> + /* adjust for century bit */
>> + dt->tm_year = BCD2BIN(chip->buf[7]) + ((chip->buf[6] & 0x80) ? 100 : 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int ds1390_set_datetime(struct device *dev, struct rtc_time *dt)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + struct ds1390 *chip = dev_get_drvdata(dev);
>> + struct spi_message message;
>> + struct spi_transfer xfer;
>> + int status;
>> +
>> + /* build the message */
>> + spi_message_init(&message);
>> + memset(&xfer, 0, sizeof(xfer));
>> + xfer.len = 1 + 8; /* write command + 8 registers */
>> + xfer.tx_buf = chip->buf;
>> + xfer.rx_buf = chip->buf;
... and I guess here as well ... ???
>> + chip->buf[0] = DS1390_REG_SECONDS | 0x80;
>> + chip->buf[1] = BIN2BCD(dt->tm_sec);
>> + chip->buf[2] = BIN2BCD(dt->tm_min);
>> + chip->buf[3] = BIN2BCD(dt->tm_hour);
>> + chip->buf[4] = BIN2BCD(dt->tm_wday);
>> + chip->buf[5] = BIN2BCD(dt->tm_mday);
>> + chip->buf[6] = BIN2BCD(dt->tm_mon + 1) | ((dt->tm_year > 99) ? 0x80 : 0x00);
>> + chip->buf[7] = BIN2BCD(dt->tm_year % 100);
>> + spi_message_add_tail(&xfer, &message);
>> +
>> + /* do the i/o */
>> + status = spi_sync(spi, &message);
>> + if (status == 0)
>> + {
>> + status = message.status;
>> + }
>> + else
>> + {
>> + return status;
>> + }
>
> if status == 0, status is not used anymore, so you can
> just return if != 0;
>
>> + return 0;
>> +}
>> +
>> +static int ds1390_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + return ds1390_get_datetime(dev, tm);
>> +}
>> +
>> +static int ds1390_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + return ds1390_set_datetime(dev, tm);
>> +}
>> +
>> +static const struct rtc_class_ops ds1390_rtc_ops = {
>> + .read_time = ds1390_read_time,
>> + .set_time = ds1390_set_time,
>> +};
>> +
>> +static int __devinit ds1390_probe(struct spi_device *spi)
>> +{
>> + struct rtc_device *rtc;
>> + unsigned char tmp;
>> + struct ds1390 *chip;
>> + int res;
>> +
>> + printk("DS1390 SPI RTC driver\n");
>> +
>> + rtc = rtc_device_register("ds1390",
>> + &spi->dev, &ds1390_rtc_ops, THIS_MODULE);
>> + if (IS_ERR(rtc))
>> + {
>> + printk("RTC : unable to register device\n");
>> + return PTR_ERR(rtc);
>> + }
>> +
>> + spi->mode = SPI_MODE_3;
>> + spi->bits_per_word = 8;
>> + spi_setup(spi);
>> +
>> + chip = kzalloc(sizeof *chip, GFP_KERNEL);
>> + if (!chip) {
>> + printk("RTC : unable to allocate device memory\n");
>> + rtc_device_unregister(rtc);
>> + return -ENOMEM;
>> + }
>> + chip->rtc = rtc;
>> + dev_set_drvdata(&spi->dev, chip);
>> +
>> + res = ds1390_get_reg(&spi->dev, DS1390_REG_SECONDS, &tmp);
>> + if (res) {
>> + printk("RTC : unable to read device\n");
>> + rtc_device_unregister(rtc);
>> + return res;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __devexit ds1390_remove(struct spi_device *spi)
>> +{
>> + struct ds1390 *chip = platform_get_drvdata(spi);
>> + struct rtc_device *rtc = chip->rtc;
>> +
>> + if (rtc)
>> + rtc_device_unregister(rtc);
>> +
>> + kfree(chip);
>> +
>> + return 0;
>> +}
>> +
>> +static struct spi_driver ds1390_driver = {
>> + .driver = {
>> + .name = "rtc-ds1390",
>> + .bus = &spi_bus_type,
>> + .owner = THIS_MODULE,
>> + },
>> + .probe = ds1390_probe,
>> + .remove = __devexit_p(ds1390_remove),
>> +};
>> +
>> +static __init int ds1390_init(void)
>> +{
>> + return spi_register_driver(&ds1390_driver);
>> +}
>> +module_init(ds1390_init);
>> +
>> +static __exit void ds1390_exit(void)
>> +{
>> + spi_unregister_driver(&ds1390_driver);
>> +}
>> +module_exit(ds1390_exit);
>> +
>> +MODULE_DESCRIPTION ("DS1390 SPI RTC driver");
>> +MODULE_AUTHOR ("Mark Jackson");
>
> you email here please.
>
>> +MODULE_LICENSE ("GPL");
>
>
--
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/
Powered by blists - more mailing lists