Re: [PATCH v2 07/32] swim: Enable clock divider only where appropriate

[Finn Thain <fthain@linux-m68k.org>]

On Mon, 31 Aug 2026, Geert Uytterhoeven wrote:

> On Mon, 17 Aug 2026 at 03:55, Finn Thain <fthain@linux-m68k.org> wrote:
> > Some models have a 16 MHz FCLK oscillator and others 32 MHz. Put this
> > information into the swim device platform_data so that the driver can
> > correctly enable the clock divider. When this is enabled incorrectly,
> > nothing can be read and failures from the Error Correction Machine are
> > flagged in the error register.
> >
> > This is chip initialization, so do this in swim_floppy_init() rather
> > than floppy_open(). Drop the udelay() which was apparently copied and
> > pasted from swim3.c, where it relates to interrupts (of which this chip
> > has none).
> 
> This move should be a separate patch.
> 

OK.

> > Cc: Joshua Thompson <funaho@jurai.org>
> > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> > Fixes: 8852ecd97488 ("m68k: mac - Add SWIM floppy support")
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> >  arch/m68k/mac/config.c | 31 ++++++++++++++++++++++++++++++-
> 
> For the m68k part:
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> > --- a/arch/m68k/mac/config.c
> > +++ b/arch/m68k/mac/config.c
> > @@ -984,8 +984,37 @@ static int __init mac_platform_init(void)
> >                         .start = swim_base,
> >                         .end   = swim_base + 0x1FFF,
> >                 };
> > +               unsigned int data = 0;
> 
> Nit: in the absence of a real platform_data structure, please use a
> more explanatory name.
> 
> Perhaps s/data/s_fclk/, and store S_FCLK_DIV2 or zero?
> Or s/data/setup/, and store the full register value?
> 

I looked again at this but I still can't see any uses for the other bits. 
So perhaps I should change it to:

+		bool fast_fclk = false;

> > --- a/drivers/block/swim.c
> > +++ b/drivers/block/swim.c
> 
> > @@ -807,8 +806,10 @@ static void swim_set_parameters(struct swim __iomem *base)
> >                 swim_write(base, parameter, mem[i]);
> >  }
> >
> > -static int swim_floppy_init(struct swim_priv *swd)
> > +static int swim_floppy_init(struct platform_device *pdev)
> >  {
> > +       struct swim_priv *swd = platform_get_drvdata(pdev);
> > +       unsigned int *data = pdev->dev.platform_data;
> 
> Likewise.
> 

... and change that to:

	bool *data = pdev->dev.platform_data;
	bool fast_fclk = data && *data;

I am unsure whether a NULL check is desirable here (?)

Interestingly, that NULL check itself is sufficient and so we don't really 
need the flag at all. Allocating memory for this bool is a completely 
pointless kmalloc.

> >         struct queue_limits lim = {
> >                 .features               = BLK_FEAT_ROTATIONAL,
> >         };
> > @@ -816,6 +817,8 @@ static int swim_floppy_init(struct swim_priv *swd)
> >         int drive;
> >         struct swim __iomem *base = swd->base;
> >
> > +       swim_write(base, setup, S_IBM_DRIVE | (*data ? S_FCLK_DIV2 : 0));
> > +

	swim_write(base, setup, S_IBM_DRIVE | (fast_fclk ? S_FCLK_DIV2 : 0));

Would the bool be more acceptable? TBH, I'm not sure it's an improvement.

Thanks for your review.

> >         swim_set_parameters(base);
> >
> >         /* scan floppy drives */
>