| Message ID | 1449568461-26016-1-git-send-email-s.trumtrar@pengutronix.de |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Am Dienstag, den 08.12.2015, 10:54 +0100 schrieb Steffen Trumtrar: > Add the binding documentation for the Epson RX6110 RTC. > > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> although I'd like a small change to be made below: > --- > Changes since v2: > - removed the size field in i2c binding > > .../devicetree/bindings/rtc/epson,rx6110.txt | 39 ++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.txt > > diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > new file mode 100644 > index 000000000000..71353542a59d > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > @@ -0,0 +1,39 @@ > +Epson RX6110 Real Time Clock > +============================ > + > +The Epson RX6110 can be used with SPI or I2C busses. The kind of > +bus depends on the SPISEL pin and can not be configured via software. > + > +I2C mode > +-------- > + > +Required properties: > + - compatible: should be: "epson,rx6110" > + - reg : offset of the register set of the device That should be the I2C address of the device, as in: git grep "reg.*I2C address" Documentation/ regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, den 08.12.2015, 10:54 +0100 schrieb Steffen Trumtrar: > Add the binding documentation for the Epson RX6110 RTC. > > Acked-by: Rob Herring <robh@kernel.org> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> although I'd like a small change to be made below: > --- > Changes since v2: > - removed the size field in i2c binding > > .../devicetree/bindings/rtc/epson,rx6110.txt | 39 ++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.txt > > diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > new file mode 100644 > index 000000000000..71353542a59d > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > @@ -0,0 +1,39 @@ > +Epson RX6110 Real Time Clock > +============================ > + > +The Epson RX6110 can be used with SPI or I2C busses. The kind of > +bus depends on the SPISEL pin and can not be configured via software. > + > +I2C mode > +-------- > + > +Required properties: > + - compatible: should be: "epson,rx6110" > + - reg : offset of the register set of the device That should be the I2C address of the device: git grep "reg.*I2C address" Documentation/ regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Dienstag, den 08.12.2015, 10:54 +0100 schrieb Steffen Trumtrar: > The RX6110 comes in two different variants: SPI and I2C. > This driver only supports the SPI variant. > > If the need ever arises to also support the I2C variant, this driver > could easily be refactored to support both cases. > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> I have a few suggestions: [...] > diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c > new file mode 100644 > index 000000000000..aa9d89d5d2de > --- /dev/null > +++ b/drivers/rtc/rtc-rx6110.c > @@ -0,0 +1,438 @@ > +/* > + * Driver for the Epson RTC module RX-6110 SA > + * > + * Copyright(C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de> > + * Copyright(C) SEIKO EPSON CORPORATION 2013. All rights reserved. > + * > + * This driver software is distributed as is, without any warranty of any kind, > + * either express or implied as further specified in the GNU Public License. > + * This software may be used and distributed according to the terms of the GNU > + * Public License, version 2 as published by the Free Software Foundation. > + * See the file COPYING in the main directory of this archive for more details. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/bcd.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> I think these three can be dropped. > +#include <linux/regmap.h> > +#include <linux/rtc.h> > +#include <linux/spi/spi.h> > + > +/* RX-6110 Register definitions */ [...] > + > +static struct spi_driver rx6110_driver; Several drivers do this. Instead of the forward declaration and using rx6110_driver.driver.name in devm_rtc_device_register below, why not just have a #define RX6110_DRIVER_NAME "rx6110-rtc" and reuse that? Actually, I think the "-rtc" suffix is superfluous in the rtc name. I'd just use "rx6110" as a parameter to devm_rtc_device_register. > +struct rx6110_data { > + struct rtc_device *rtc; > + struct regmap *regmap; > +}; [...] > +/** > + * rx6110_init - initialize the rx6110 registers > + * > + * @rx6110: pointer to the rx6110 struct in use > + * > + */ > +static int rx6110_init(struct rx6110_data *rx6110) > +{ > + struct rtc_device *rtc = rx6110->rtc; > + int ext; > + int flags; > + int ctrl; ctrl is unused, except in the dev_dbg to print 0 below. > + int ret; > + > + /* set reserved register 0x17 with specified value of 0xB8 */ > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES1, 0xB8); > + if (ret) > + return ret; > + > + /* set reserved register 0x30 with specified value of 0x00 */ > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES2, 0x00); > + if (ret) > + return ret; > + > + /* set reserved register 0x31 with specified value of 0x10 */ > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES3, 0x10); > + if (ret) > + return ret; > + > + ret = regmap_write(rx6110->regmap, RX6110_REG_IRQ, 0x0); > + if (ret) > + return ret; Maybe combine those using regmap_multi_reg_write? Not sure if the sequence can be reordered to also include the writes below. > + ret = regmap_update_bits(rx6110->regmap, RX6110_REG_EXT, > + RX6110_BIT_EXT_TE, 0); > + if (ret) > + return ret; > + > + /* get current extension, flag, control register values */ > + ret = regmap_read(rx6110->regmap, RX6110_REG_EXT, &ext); > + if (ret) > + return ret; > + > + ret = regmap_read(rx6110->regmap, RX6110_REG_FLAG, &flags); > + if (ret) > + return ret; > + > + /* clear ctrl register */ > + ret = regmap_write(rx6110->regmap, RX6110_REG_CTRL, 0); > + if (ret) > + return ret; > + > + ctrl = 0; This is unused ... > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALMIN, 0); > + if (ret) > + return ret; > + > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALHOUR, 0); > + if (ret) > + return ret; > + > + ret = regmap_write(rx6110->regmap, RX6110_REG_ALWDAY, 0); > + if (ret) > + return ret; > + > + dev_dbg(&rtc->dev, "ext: %x, flag: %x, ctrl: %x\n", ext, flags, ctrl); ... except here. > + > + /* check for VLF Flag (set at power-on) */ > + if ((flags & RX6110_BIT_FLAG_VLF)) > + dev_warn(&rtc->dev, "Voltage low, data loss detected.\n"); > + > + /* check for Alarm Flag */ > + if (flags & RX6110_BIT_FLAG_AF) > + dev_warn(&rtc->dev, "An alarm may have been missed.\n"); > + > + /* check for Periodic Timer Flag */ > + if (flags & RX6110_BIT_FLAG_TF) > + dev_warn(&rtc->dev, "Periodic timer was detected\n"); > + > + /* check for Update Timer Flag */ > + if (flags & RX6110_BIT_FLAG_UF) > + dev_warn(&rtc->dev, "Update timer was detected\n"); > + > + /* clear all flags BUT VLF */ > + ret = regmap_update_bits(rx6110->regmap, RX6110_REG_FLAG, > + RX6110_BIT_FLAG_AF | > + RX6110_BIT_FLAG_UF | > + RX6110_BIT_FLAG_TF, > + 0); > + if (ret) > + return ret; > + > + return 0; > +} [...] > +/** > + * rx6110_probe - initialize rtc driver > + * @spi: pointer to spi device > + */ > +static int rx6110_probe(struct spi_device *spi) > +{ > + struct rx6110_data *rx6110; > + int err; > + > + if ((spi->bits_per_word && spi->bits_per_word != 8) > + || (spi->max_speed_hz > 2000000) > + || (spi->mode != (SPI_CS_HIGH | SPI_CPOL | SPI_CPHA))) { > + dev_warn(&spi->dev, "SPI settings: bits_per_word: %d, max_speed_hz: %d, mode: %xh\n", > + spi->bits_per_word, spi->max_speed_hz, spi->mode); > + dev_warn(&spi->dev, "driving device in an unsupported mode"); > + } > + > + rx6110 = devm_kzalloc(&spi->dev, sizeof(*rx6110), GFP_KERNEL); > + if (!rx6110) > + return -ENOMEM; > + > + rx6110->regmap = devm_regmap_init_spi(spi, ®map_spi_config); > + if (IS_ERR(rx6110->regmap)) { > + dev_err(&spi->dev, "regmap init failed for rtc rx6110\n"); Maybe print the error code. > + return PTR_ERR(rx6110->regmap); > + } > + > + spi_set_drvdata(spi, rx6110); > + > + rx6110->rtc = devm_rtc_device_register(&spi->dev, > + rx6110_driver.driver.name, That could be just "rx6110" instead of "rx6110-rtc" in my opinion. > + &rx6110_rtc_ops, THIS_MODULE); > + > + if (IS_ERR(rx6110->rtc)) > + return PTR_ERR(rx6110->rtc); > + > + err = rx6110_init(rx6110); > + if (err) > + return err; > + > + rx6110->rtc->max_user_freq = 1; > + > + return 0; > +} regards Philipp -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi! On Tue, Dec 08, 2015 at 11:29:26AM +0100, Philipp Zabel wrote: > Am Dienstag, den 08.12.2015, 10:54 +0100 schrieb Steffen Trumtrar: > > Add the binding documentation for the Epson RX6110 RTC. > > > > Acked-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > although I'd like a small change to be made below: > > > --- > > Changes since v2: > > - removed the size field in i2c binding > > > > .../devicetree/bindings/rtc/epson,rx6110.txt | 39 ++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/rtc/epson,rx6110.txt > > > > diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > > new file mode 100644 > > index 000000000000..71353542a59d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt > > @@ -0,0 +1,39 @@ > > +Epson RX6110 Real Time Clock > > +============================ > > + > > +The Epson RX6110 can be used with SPI or I2C busses. The kind of > > +bus depends on the SPISEL pin and can not be configured via software. > > + > > +I2C mode > > +-------- > > + > > +Required properties: > > + - compatible: should be: "epson,rx6110" > > + - reg : offset of the register set of the device > > That should be the I2C address of the device, as in: > git grep "reg.*I2C address" Documentation/ > Sounds reasonable. Changed. Thanks, Steffen Trumtrar
Hi! On Tue, Dec 08, 2015 at 11:30:21AM +0100, Philipp Zabel wrote: > Am Dienstag, den 08.12.2015, 10:54 +0100 schrieb Steffen Trumtrar: > > The RX6110 comes in two different variants: SPI and I2C. > > This driver only supports the SPI variant. > > > > If the need ever arises to also support the I2C variant, this driver > > could easily be refactored to support both cases. > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > I have a few suggestions: > > [...] > > diff --git a/drivers/rtc/rtc-rx6110.c b/drivers/rtc/rtc-rx6110.c > > new file mode 100644 > > index 000000000000..aa9d89d5d2de > > --- /dev/null > > +++ b/drivers/rtc/rtc-rx6110.c > > @@ -0,0 +1,438 @@ > > +/* > > + * Driver for the Epson RTC module RX-6110 SA > > + * > > + * Copyright(C) 2015 Pengutronix, Steffen Trumtrar <kernel@pengutronix.de> > > + * Copyright(C) SEIKO EPSON CORPORATION 2013. All rights reserved. > > + * > > + * This driver software is distributed as is, without any warranty of any kind, > > + * either express or implied as further specified in the GNU Public License. > > + * This software may be used and distributed according to the terms of the GNU > > + * Public License, version 2 as published by the Free Software Foundation. > > + * See the file COPYING in the main directory of this archive for more details. > > + * > > + * You should have received a copy of the GNU General Public License along with > > + * this program. If not, see <http://www.gnu.org/licenses/>. > > + */ > > + > > +#include <linux/bcd.h> > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/of_gpio.h> > > I think these three can be dropped. > Okay. > > +#include <linux/regmap.h> > > +#include <linux/rtc.h> > > +#include <linux/spi/spi.h> > > + > > +/* RX-6110 Register definitions */ > [...] > > + > > +static struct spi_driver rx6110_driver; > > Several drivers do this. Instead of the forward declaration and using > rx6110_driver.driver.name in devm_rtc_device_register below, why not > just have a #define RX6110_DRIVER_NAME "rx6110-rtc" and reuse that? > Actually, I think the "-rtc" suffix is superfluous in the rtc name. > I'd just use "rx6110" as a parameter to devm_rtc_device_register. > Yeah, why not. The define seems to be a cleaner solution than the forward declaration. Changed. > > +struct rx6110_data { > > + struct rtc_device *rtc; > > + struct regmap *regmap; > > +}; > [...] > > +/** > > + * rx6110_init - initialize the rx6110 registers > > + * > > + * @rx6110: pointer to the rx6110 struct in use > > + * > > + */ > > +static int rx6110_init(struct rx6110_data *rx6110) > > +{ > > + struct rtc_device *rtc = rx6110->rtc; > > + int ext; > > + int flags; > > + int ctrl; > > ctrl is unused, except in the dev_dbg to print 0 below. > > > + int ret; > > + > > + /* set reserved register 0x17 with specified value of 0xB8 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES1, 0xB8); > > + if (ret) > > + return ret; > > + > > + /* set reserved register 0x30 with specified value of 0x00 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES2, 0x00); > > + if (ret) > > + return ret; > > + > > + /* set reserved register 0x31 with specified value of 0x10 */ > > + ret = regmap_write(rx6110->regmap, RX6110_REG_RES3, 0x10); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(rx6110->regmap, RX6110_REG_IRQ, 0x0); > > + if (ret) > > + return ret; > > Maybe combine those using regmap_multi_reg_write? Not sure if the > sequence can be reordered to also include the writes below. > Never heard of it, but it sounds like a good idea. (...) > > +/** > > + * rx6110_probe - initialize rtc driver > > + * @spi: pointer to spi device > > + */ > > +static int rx6110_probe(struct spi_device *spi) > > +{ > > + struct rx6110_data *rx6110; > > + int err; > > + > > + if ((spi->bits_per_word && spi->bits_per_word != 8) > > + || (spi->max_speed_hz > 2000000) > > + || (spi->mode != (SPI_CS_HIGH | SPI_CPOL | SPI_CPHA))) { > > + dev_warn(&spi->dev, "SPI settings: bits_per_word: %d, max_speed_hz: %d, mode: %xh\n", > > + spi->bits_per_word, spi->max_speed_hz, spi->mode); > > + dev_warn(&spi->dev, "driving device in an unsupported mode"); > > + } > > + > > + rx6110 = devm_kzalloc(&spi->dev, sizeof(*rx6110), GFP_KERNEL); > > + if (!rx6110) > > + return -ENOMEM; > > + > > + rx6110->regmap = devm_regmap_init_spi(spi, ®map_spi_config); > > + if (IS_ERR(rx6110->regmap)) { > > + dev_err(&spi->dev, "regmap init failed for rtc rx6110\n"); > > Maybe print the error code. > Done. Thanks, Steffen
diff --git a/Documentation/devicetree/bindings/rtc/epson,rx6110.txt b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt new file mode 100644 index 000000000000..71353542a59d --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/epson,rx6110.txt @@ -0,0 +1,39 @@ +Epson RX6110 Real Time Clock +============================ + +The Epson RX6110 can be used with SPI or I2C busses. The kind of +bus depends on the SPISEL pin and can not be configured via software. + +I2C mode +-------- + +Required properties: + - compatible: should be: "epson,rx6110" + - reg : offset of the register set of the device + +Example: + + rtc: rtc@32 { + compatible = "epson,rx6110" + reg = <0x32>; + }; + +SPI mode +-------- + +Required properties: + - compatible: should be: "epson,rx6110" + - reg: chip select number + - spi-cs-high: RX6110 needs chipselect high + - spi-cpha: RX6110 works with SPI shifted clock phase + - spi-cpol: RX6110 works with SPI inverse clock polarity + +Example: + + rtc: rtc@3 { + compatible = "epson,rx6110" + reg = <3> + spi-cs-high; + spi-cpha; + spi-cpol; + };