Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 14 Nov 2022 23:17:30 +0100
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     glasveze@delta.nl
Cc:     linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
        a.zummo@towertech.it, robh+dt@kernel.org,
        linux-kernel@vger.kernel.org,
        Mike Looijmans <mike.looijmans@topic.nl>
Subject: Re: [PATCH 2/2] rtc: ds1307: Add support for Epson RX8111
Message-ID: <Y3K+ehjZAA4AIXie@mail.local>
References: <20221028144041.11067-1-glasveze@delta.nl>
 <20221028144041.11067-2-glasveze@delta.nl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221028144041.11067-2-glasveze@delta.nl>
Precedence: bulk

Hello,

On 28/10/2022 16:40:41+0200, glasveze@delta.nl wrote:
> +static u8 do_trickle_setup_rx8111(struct ds1307 *ds1307, u32 ohms, bool diode)
> +{
> +	/* make sure that the backup battery is enabled */
> +	u8 setup = RX8111_PSC_SMP_INIEN;

You can't do that, else it will never be possible to remove that without
breaking existing users. Instead, you should use
RTC_PARAM_BACKUP_SWITCH_MODE. Honestly, I would really like stopping
adding support for more RTCs in this driver and instead split them all
out.

> +
> +	if (diode)
> +		setup |= RX8111_PSC_SMP_CHGEN;
> +
> +	return setup;
> +}
> +
>  static u8 do_trickle_setup_rx8130(struct ds1307 *ds1307, u32 ohms, bool diode)
>  {
>  	/* make sure that the backup battery is enabled */
> @@ -1012,6 +1078,16 @@ static const struct chip_desc chips[last_ds_type] = {
>  		.century_bit	= DS1337_BIT_CENTURY,
>  		.bbsqi_bit	= DS3231_BIT_BBSQW,
>  	},
> +	[rx_8111] = {
> +		.alarm		= 1,
> +		.nvram_offset	= 0x40,
> +		.nvram_size	= 0x40,	/* 0x40 .. 0x7F is user RAM */
> +		.offset		= 0x10,
> +		.irq_handler = rx8130_irq,
> +		.rtc_ops = &rx8130_rtc_ops,
> +		.trickle_charger_reg = RX8111_PWR_SWITCH_CTRL,
> +		.do_trickle_setup = &do_trickle_setup_rx8111,
> +	},
>  	[rx_8130] = {
>  		.alarm		= 1,
>  		/* this is battery backed SRAM */
> @@ -1063,6 +1139,7 @@ static const struct i2c_device_id ds1307_id[] = {
>  	{ "pt7c4338", ds_1307 },
>  	{ "rx8025", rx_8025 },
>  	{ "isl12057", ds_1337 },
> +	{ "rx8111", rx_8111 },
>  	{ "rx8130", rx_8130 },
>  	{ }
>  };
> @@ -1137,6 +1214,10 @@ static const struct of_device_id ds1307_of_match[] = {
>  		.compatible = "isil,isl12057",
>  		.data = (void *)ds_1337
>  	},
> +	{
> +		.compatible = "epson,rx8111",
> +		.data = (void *)rx_8111
> +	},
>  	{
>  		.compatible = "epson,rx8130",
>  		.data = (void *)rx_8130
> @@ -1880,6 +1961,10 @@ static int ds1307_probe(struct i2c_client *client,
>  				     DS1307_REG_HOUR << 4 | 0x08, hour);
>  		}
>  		break;
> +	case rx_8111:
> +		/* Use memory as user RAM */
> +		regmap_write(ds1307->regmap, RX8111_TIME_STAMP_BUF_CTRL, 0);
> +		break;
>  	case ds_1388:
>  		err = regmap_read(ds1307->regmap, DS1388_REG_CONTROL, &tmp);
>  		if (err) {
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com