Re: [PATCH v2 1/2] fpga: Convert SYS_FPGA_CHECK_CTRLC to Kconfig

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2344 bytes --]

On Wed, Jul 13, 2022 at 03:20:46PM +0200, Alexander Dahl wrote:
> Hello Michal,
> 
> On Wed, Jul 13, 2022 at 02:56:08PM +0200, Michal Simek wrote:
[snip]
> > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > > index 76719517f5..53d91676e0 100644
> > > --- a/drivers/fpga/Kconfig
> > > +++ b/drivers/fpga/Kconfig
> > > @@ -91,4 +91,8 @@ config FPGA_ZYNQPL
> > >   	  Enable FPGA driver for loading bitstream in BIT and BIN format
> > >   	  on Xilinx Zynq devices.
> > > +config SYS_FPGA_CHECK_CTRLC
> > > +	bool "Allow Control-C to interrupt FPGA configuration"
> > > +	depends on FPGA
> > 
> > Please write help message.
> 
> Okay, I'll have to invent a new message here, if the prompt is not
> self explaining enough.  Since this is not conversion, but adding a
> new message we did not have before, should this go into a separate
> patch?

If you understand things well enough to add a line or two under "help",
that would be appreciated.  It may be a little redundant soundiing, and
if it's not long enough checkpatch might still complain (but can be
ignored).

> > And can you please remove this code from drivers/fpga/virtex2.c
> > 
> >  48 /*
> >  49  * Don't allow config cycle to be interrupted
> >  50  */
> >  51 #ifndef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  52 #undef CONFIG_SYS_FPGA_CHECK_CTRLC
> >  53 #endif
> > 
> > it doesn't make any sense.
> 
> I have no hardware to test this and this is out of scope of the
> conversion patch itself.

This kind of code logic needs to be enforced in Kconfig instead with
depends lines.  We can make sure it's size-neutral.

> > And with 2/2 please also remove
> > drivers/fpga/spartan2.c:18:#undef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:44:#ifndef CONFIG_SYS_FPGA_PROG_FEEDBACK
> > drivers/fpga/virtex2.c:45:#define CONFIG_SYS_FPGA_PROG_FEEDBACK
> > 
> > Thanks,
> > Michal
> 
> I may be able to add an additional patch or two, but those are all
> FPGAs I have no experience with and I can not test those.  This would
> be more or less guessing based on code reading.  I can try next week,
> not able to do this currently.

Thanks.  It's OK to just check the logic by inspection, one of the tests
I end up running is making sure the code size doesn't change so that'll
catch bad migrations.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]