linux-kernel - Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch MC33XS2410

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] [day] [month] [year] [list]

Message-ID: <f7imhfnhxt4mtasim7upuupyh2czt73coaa57aucswkzk4aflf@iiiymg422tj4>
Date: Fri, 2 Aug 2024 03:54:11 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, linux-pwm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] pwm: add support for NXPs high-side switch
 MC33XS2410

Hello Dimitri,

On Thu, Aug 01, 2024 at 04:28:02PM +0200, Dimitri Fedrau wrote:
> Am Thu, Aug 01, 2024 at 12:24:28AM +0200 schrieb Uwe Kleine-König:
> > > > > +	state->polarity = (val[2] & MC33XS2410_PWM_CTRL1_POL_INV(pwm->hwpwm)) ?
> > > > > +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> > > > > +
> > > > > +	state->enabled = !!(val[3] & MC33XS2410_PWM_CTRL3_EN(pwm->hwpwm));
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > [...]
> > > > > +static int mc33xs2410_probe(struct spi_device *spi)
> > > > > +{
> > > > > [...]
> > > > > +	/* Disable watchdog */
> > > > > +	ret = mc33xs2410_write_reg(spi, MC33XS2410_WDT, 0x0);
> > > > > +	if (ret < 0)
> > > > > +		return dev_err_probe(dev, ret, "Failed to disable watchdog\n");
> > > > 
> > > > Wouldn't the watchdog functionality better be handled by a dedicated
> > > > watchdog driver? Disabling it here unconditionally looks wrong.
> > > 
> > > Yes, would be better. I planned this after this patchset is accepted.
> > > Without disabling the watchdog, the device is not able to operate. So I
> > > would stick to it for now and come up with a patch later on.
> > 
> > How long is the default timeout? Don't you need to disable the watchdog
> > in the bootloader anyhow?
> 
> Default and also maximum timeout is 256ms. My hardware is configured to
> let the device stay in reset until the driver puts it out of reset by
> setting the associated GPIO. Datasheet refers to it as "Disable mode".
> Outputs are turned off.
> Without having such reset logic, the device would need to have the
> watchdog disabled in the bootloader and continue in "Normal mode" or it
> would go into "Safe mode" while booting the kernel almost sure violating
> the timeout. Outputs are then controlled by the INx input logic signals.
> I think there is no single solution but rather depends on the use case.
> There are three modes which the device can be in when the driver is probed:
> "Disable", "Safe" and "Normal". All three are handled by this driver,
> assuming the watchdog should be disabled.
> 
> [...]
> > Should this better be a mfd driver then?
> > 
> 
> I don't thinks so, the watchdog and the outputs belong somehow together.

Ah, so the watchdog doesn't trigger a reboot of the machine, just resets
the I/O lines to input? If yes, that's not what a watchdog driver is
about and so this isn't a hint to create an mfd driver.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)