Re: [PATCH] Add Dallas DS1390 RTC chip

[Mark Jackson]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/