Received-SPF: pass (google.com: domain of linux-kernel+bounces-82610-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 27 Feb 2024 03:29:19 +0100
From: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Chris Packham <chris.packham@alliedtelesis.co.nz>
Cc: antoniu.miclaus@analog.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	jdelvare@suse.com, linux@roeck-us.net, linux-rtc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hwmon@vger.kernel.org,
	Ibrahim Tilki <Ibrahim.Tilki@analog.com>,
	Zeynep Arslanbenzer <Zeynep.Arslanbenzer@analog.com>
Subject: Re: [PATCH v8 1/2] rtc:  max31335: Add support for additional chips
Message-ID: <20240227022919856ffa1d@mail.local>
References: <20240227010312.3305966-1-chris.packham@alliedtelesis.co.nz>
 <20240227010312.3305966-2-chris.packham@alliedtelesis.co.nz>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240227010312.3305966-2-chris.packham@alliedtelesis.co.nz>

On 27/02/2024 14:03:09+1300, Chris Packham wrote:
> -	max31335->clkout.clk = devm_clk_get_enabled(dev, NULL);
> -	if (IS_ERR(max31335->clkout.clk))
> -		return dev_err_probe(dev, PTR_ERR(max31335->clkout.clk),
> -				     "cannot enable clkout\n");

This is so ugly and should have never passed. I went weak in front of
Antoniu's insistance but I'm very close from ripping out the whole
driver from the kernel now.

Seriously, read all the comments that have been made around the IRQ/clk
stuff and come back with a proper solution.

> +static int max31335_irq_init(struct device *dev, const char *devname)
> +{
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (max31335->id) {
> +	case ID_MAX31328:
> +		break;
> +	case ID_MAX31331:
> +	case ID_MAX31334:
> +		if (max31335->clkout.clk) {
> +			/*
> +			 * interrupt muxing depends on clkout so enable clkout
> +			 * if configured before requesting interrupt
> +			 */
> +			ret = clk_prepare_enable(max31335->clkout.clk);

Do I get this right that this is about enabling its own output clock?
Why would you need to do that? Something else must be the consumer, not
the provider itself.

> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "cannot enable clkout\n");
> +		}
> +		break;
> +	default:
> +		if (max31335->clkin) {
> +			if (max31335->clkout.clk && max31335->irq > 0)
> +				return dev_err_probe(dev, -EOPNOTSUPP,
> +						     "irq not possible when both clkin and clkout are configured\n");

This is not true, the RTC is always a clock provider. What is not
possible is muxing the clock on the pin in a few configurations.


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