linux-kernel - Re: [PATCH v8 1/2] rtc: max31335: Add support for additional chips

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ce50b1bc-0786-454e-9498-0b5ddbea0e2a@alliedtelesis.co.nz>
Date: Tue, 27 Feb 2024 04:22:35 +0000
From: Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
CC: "antoniu.miclaus@...log.com" <antoniu.miclaus@...log.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "jdelvare@...e.com"
	<jdelvare@...e.com>, "linux@...ck-us.net" <linux@...ck-us.net>,
	"linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>, Ibrahim Tilki
	<Ibrahim.Tilki@...log.com>, Zeynep Arslanbenzer
	<Zeynep.Arslanbenzer@...log.com>
Subject: Re: [PATCH v8 1/2] rtc: max31335: Add support for additional chips

Hi Alexandre,

First off apologies if I've struck a nerve, that was never my intention. 
I thought cleaning up an inflight patch series to get it landed would be 
a straight forward thing.

On 27/02/24 15:29, Alexandre Belloni wrote:
> 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.

Yeah I'm only just getting to grips with this.

No idea about the MAX31335 itself but from the MAX31334 it looks like 
the alarm1 interrupt will move from INTA to INTB. I think rather than 
messing about with the clock stuff we'd be better off with a 
pin-6-function = "alarm1" | "clkout" property.

>
>> +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.

I don't think it does. It's just that based on the ENCLKO setting alarm1 
will be output on a different pin. I don't think there's really a proper 
way of disabling CLKOUT dynamically. Either it's enabled and all the 
interrupts are on the INTA pin or it's disabled and alarm1 is moved to 
the INTB. You can change the clkout frequency but not outright disable 
it (without having a side-effect that impacts the interrupt assignment).

So I think my suggestion of making it part of the hardware description 
is probably the sensible thing to do. It affects how the interrupts are 
physically connected so I think it does belong in the device tree.

>
>> +			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.

Yeah I don't understand that either. Not sure what clkin has to do with 
anything here. The clkout stuff affects which irq pins are in play.