Re: [PATCH v7 3/6] mfd: cs42l43: Add support for cs42l43 core driver

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
	 krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	 linus.walleij@linaro.org, vkoul@kernel.org, lgirdwood@gmail.com,
	 yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
	 pierre-louis.bossart@linux.intel.com,
	alsa-devel@alsa-project.org,  patches@opensource.cirrus.com,
	devicetree@vger.kernel.org,  linux-gpio@vger.kernel.org,
	linux-spi@vger.kernel.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 3/6] mfd: cs42l43: Add support for cs42l43 core driver
Date: Fri, 19 Jan 2024 18:01:38 +0200	[thread overview]
Message-ID: <CAHp75Vco3+B_mcLRr7dcLx79601poLJtLt3Av6d-hAJQLYbe6Q@mail.gmail.com> (raw)
In-Reply-To: <20240119113203.GA16899@ediswmail.ad.cirrus.com>

On Fri, Jan 19, 2024 at 1:32 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Jan 18, 2024 at 06:42:26PM +0200, andy.shevchenko@gmail.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