Re: [PATCH v6 1/5] clk: Add a basic multiplier clock

[Michael Turquette <mturquette@baylibre.com>]

Quoting Maxime Ripard (2015-10-20 07:40:47)
> Hi Mike,
> =

> On Tue, Oct 20, 2015 at 06:43:43AM -0700, Michael Turquette wrote:
> > Hi Maxime,
> > =

> > Quoting Maxime Ripard (2015-10-20 00:36:45)
> > > +struct clk *clk_register_multiplier(struct device *dev, const char *=
name,
> > > +                                   const char *parent_name,
> > > +                                   unsigned long flags,
> > > +                                   void __iomem *reg, u8 shift, u8 w=
idth,
> > > +                                   u8 clk_mult_flags, spinlock_t *lo=
ck)
> > > +{
> > =

> > Patch looks good in general. However this is a good opportunity to stop
> > the madness around the registration functions in these basic clock
> > types.
> > =

> > clk_register is really all that we need since we've had struct
> > clk_init_data for a while. Initializing a multiplier should be as simple
> > as:
> > =

> >       struct clk_multiplier clk_foo =3D {
> >               .hw.init =3D &(struct clk_init_data){
> >                       .name =3D "foo",
> >                       .parent_names =3D (const char *[]){
> >                               "bar",
> >                       },
> >                       .num_parents =3D 1;
> >                       .ops =3D &clk_multiplier_ops,
> >               },
> >               .reg =3D 0xd34db33f,
> >               .shift =3D 1,
> >               .width =3D 2,
> >       };
> > =

> >       clk_register(dev, &clk_foo.hw);
> > =

> > This is nice since it turns these basic clocks into even more of a
> > library and less of a poor mans driver.
> > =

> > (I really hope the above works. I did not test it)
> > =

> > Is it possible you can convert to using this method, and if it is
> > correct for you then just remove clk_multiplier_register altogether? (In
> > fact you might not use the registration function at all since you use
> > the composite clock...)
> =

> This chunk of code has been here since v2, which has been first posted
> in May, two and half kernel releases ago.
> =

> In the meantime, we had a full-blown DMA driver and a quite unusual
> ASoC driver merged. For some reason, this is the only piece of the
> audio support that is missing for us, while at the same time it's the
> most trivial.
> =

> If that's the only issue you have with this patch, I'm fine with
> sending a subsequent patch this week. But I'd be really unhappy with
> sending yet another version for a single change, while you had 5
> monthes to review it, and we discussed it several times on IRC and
> face to face.

The change can go in later. It's not a prerequisite. I had a feeling
you'd be grumpy about me asking but I thought I'd try anyways. I won't
even ask if you got sign-off from Jim on whether this works for his
platforms ;-)

The copy/paste nature of these basic clock types really sucks and it is
one of many reasons that I am hesitant to accept them and slow to merge
them...

Anyways it seems that you are not using the registration function at all
so I might just follow up with a patch to remove it.

I can pick these 5 patches directly, or do you plan to send a PR?

Regards,
Mike

> =

> Maxime
> =

> -- =

> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com