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

[Geert Uytterhoeven]

Hi Finn,

On Tue, 1 Sept 2026 at 09:56, Finn Thain <fthain@xxxxxxxxxxxxxx> wrote:
> On Mon, 31 Aug 2026, Geert Uytterhoeven wrote:
> > On Mon, 17 Aug 2026 at 03:55, Finn Thain <fthain@xxxxxxxxxxxxxx> 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).
> >
> > > Cc: Joshua Thompson <funaho@xxxxxxxxx>
> > > Reviewed-by: Laurent Vivier <laurent@xxxxxxxxx>
> > > Fixes: 8852ecd97488 ("m68k: mac - Add SWIM floppy support")
> > > Signed-off-by: Finn Thain <fthain@xxxxxxxxxxxxxx>

> > > --- 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;

That's another option.

> > > --- 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 (?)

In theory it cannot happen on a sane system. But recently people started
hardening drivers against misuse through sysfs' driver_override and
bind attributes, in which case the pointer would be NULL.

> 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.

Sure, but readability and maintainability also matter.  In the absence
of a proper driver-specific platform data structure, having a boolean
variable with an explanatory name is better than an obscure NULL
pointer check.
And what if you need to add more data later? At least the bool can be
safely replaced by a larger structure with a bool as its first member,
without the need for a single atomic patch.

> > >         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 */

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds