Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 14 Apr 2014 18:35:19 +0100 | From | Mark Brown <> | Subject | Re: [rtc-linux] [PATCH] rtc: add support for maxim dallas rtc ds1343 and ds1344 |
| |
On Sun, Apr 13, 2014 at 08:12:57PM +0530, RAGHAVENDRA GANIGA wrote:
> +static const struct spi_device_id ds1343_id[] = { > + { "ds1343", 0 }, > + { "ds1344", 1 }, > + { } > +}; > +MODULE_DEVICE_TABLE(spi, ds1343_id);
If the 0 and 1 mean anything there they should have #defines, otherwise just omit them.
> +static int ds1343_get_reg(struct device *dev, unsigned char address, > + unsigned char *buf) > +{ > + struct spi_device *spi = to_spi_device(dev); > + > + /* MSB of the spi address _ > + in this rtc should be zero for read operation R/W */ > + *buf = address; > + > + return spi_write_then_read(spi, buf, 1, buf, 1); > +}
This and the set_reg() function look like you should be using regmap.
> +static int ds1343_ioctl(struct device *dev, unsigned int cmd, unsigned long arg) > +{ > + switch (cmd) { > +#ifdef RTC_SET_CHARGE > + case RTC_SET_CHARGE: > + { > + int val; > + > + if (copy_from_user(&val, (int __user *)arg, sizeof(int))) > + return -EFAULT; > + > + return ds1343_set_reg(dev, DS1343_TRICKLE_REG, val); > + } > + break; > +#endif > + }
What defines this? I notice that ds1302 also does this - is this device different enouh to need a separate driver?
> +static irqreturn_t ds1343_irq(int irq, void *dev_id) > +{ > + struct ds1343_priv *priv = dev_id; > + > + disable_irq_nosync(irq); > + schedule_work(&priv->work); > + return IRQ_HANDLED; > +}
Use request_threaded_irq() rather than open coding it. [unhandled content-type:application/pgp-signature] | ![\](/images/icornerr.gif) |