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]
Date: Fri, 19 Jan 2024 18:01:38 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: broonie@...nel.org, lee@...nel.org, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	linus.walleij@...aro.org, vkoul@...nel.org, lgirdwood@...il.com, 
	yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com, 
	pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org, 
	patches@...nsource.cirrus.com, devicetree@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-spi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/6] mfd: cs42l43: Add support for cs42l43 core driver

On Fri, Jan 19, 2024 at 1:32 PM Charles Keepax
<ckeepax@...nsource.cirrus.com> wrote:
> On Thu, Jan 18, 2024 at 06:42:26PM +0200, andy.shevchenko@...il.com wrote:
> > Fri, Aug 04, 2023 at 11:45:59AM +0100, Charles Keepax kirjoitti:

..

> > > +#if IS_ENABLED(CONFIG_OF)
> >
> > We are trying hard to get rid of this ugly ifdefferies (ACPI as well) along
> > with respective macros that are often the PITA for CIs.
>
> Fair enough, but what is the expected alternative here? Is it now
> preferred to just always include both in the driver? That does
> come at a small cost in driver size, but it doesn't really bother
> me.

Yes. You may have noticed the pile of the "remove of_match_ptr()"
patches in the past cycles...

> > > +#endif

..

> > > +#define CS42L43_RESET_DELAY                        20
> > > +
> > > +#define CS42L43_SDW_ATTACH_TIMEOUT         500
> > > +#define CS42L43_SDW_DETACH_TIMEOUT         100
> > > +
> > > +#define CS42L43_MCU_POLL                   5000
> > > +#define CS42L43_MCU_CMD_TIMEOUT                    20000
> >
> > > +#define CS42L43_MCU_UPDATE_TIMEOUT         500000
> >
> > > +#define CS42L43_VDDP_DELAY                 50
> > > +#define CS42L43_VDDD_DELAY                 1000
> > > +
> > > +#define CS42L43_AUTOSUSPEND_TIME           250
> >
> > Usually we use units for the macro names as suffixes...
> > E.g., _US (for microseconds).
>
> Can add those, does make it clearer.

This is a nit-pick, but just to let you know the standard de facto in
several subsystems (which hold drivers of different devices). Not sure
if MFD or others related to this driver are really bothered with this
nuance.

..

> > > +   irq_flags = irqd_get_trigger_type(irq_data);
> > > +   switch (irq_flags) {
> > > +   case IRQF_TRIGGER_LOW:
> > > +   case IRQF_TRIGGER_HIGH:
> > > +   case IRQF_TRIGGER_RISING:
> > > +   case IRQF_TRIGGER_FALLING:
> > > +           break;
> > > +   case IRQ_TYPE_NONE:
> >
> > Are you sure it's a right place to interpret no type flags as a default?
>
> I mean... no... but I might need more to go on. The chip
> generates an active low IRQ by default so it seems reasonable if
> nothing is specified to assume the chip is doing what it normally
> would.

The problem is that if NONE comes here it might point to a mistake in
the initialisation / probe code somewhere else. Please, double check
that it's a valid case to have NONE here.

> > > +   default:
> > > +           irq_flags = IRQF_TRIGGER_LOW;
> > > +           break;
> > > +   }

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ