Re: [RFC/RFT v3 04/14] clk: meson: eeclk: add setup callback

[Jerome Brunet <jbrunet@xxxxxxxxxxxx>]

On Fri 26 Jul 2019 at 16:50, Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:

> On 03/07/2019 16:17, Jerome Brunet wrote:
>> On Mon 01 Jul 2019 at 11:12, Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>> 
>>> Add a setup() callback in the eeclk structure, to call an optional
>>> call() function at end of eeclk probe to setup clocks.
>>>
>>> It's used for the G12A clock controller to setup the CPU clock
>>> notifiers.
>> 
>> I'd prefer if you implement the probe function in the related controller
>> have this probe function call meson_eeclkc_probe() for the common part
>> 
>> In your case, I suppose it means implementing the g12a controller probe
>> to deal with the notifiers
>
> Sure, but with this eeclk setup callback I can provide a different setup() callback
> for g12a and g12b (and later sm1), without this means adding a top data struct
> containing a setup() callback pointer and the soc meson_eeclkc_data struct to be able
> to call a setup() for each family like done actually, but this will broke eeclk since
> the match_data data won't be a meson_eeclkc_data() struct anymore.

meson_eeclkc_probe is an helper we added to factorize common code out of the
clock controllers we had. I don't like the idea to now add callback in it
deal with the specifics of each SoCs. It feels like we are going in circles.

I think SoC/controller specific stuff should be dealt with in
related probe function of each clock controller which would then call
the 'common helper' if necessary.

If the common part interface needs to be reworked, maybe changing
the parameters, I'm ok with it.

>
> If you still prefer this, I can rework it like that.
>
> I'm rebasing all the stuff on v5.3-rc1 and plan to repost an updated version
> shortly, solving this would be easier.
>
> Neil
>
>> 
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>>> ---
>>>  drivers/clk/meson/meson-eeclk.c | 6 ++++++
>>>  drivers/clk/meson/meson-eeclk.h | 1 +
>>>  2 files changed, 7 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c
>>> index 6ba2094be257..81fd2abcd173 100644
>>> --- a/drivers/clk/meson/meson-eeclk.c
>>> +++ b/drivers/clk/meson/meson-eeclk.c
>>> @@ -61,6 +61,12 @@ int meson_eeclkc_probe(struct platform_device *pdev)
>>>  		}
>>>  	}
>>>  
>>> +	if (data->setup) {
>>> +		ret = data->setup(pdev);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>>  	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
>>>  					   data->hw_onecell_data);
>>>  }
>>> diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h
>>> index 9ab5d6fa7ccb..7fdf424f71a6 100644
>>> --- a/drivers/clk/meson/meson-eeclk.h
>>> +++ b/drivers/clk/meson/meson-eeclk.h
>>> @@ -20,6 +20,7 @@ struct meson_eeclkc_data {
>>>  	const struct reg_sequence	*init_regs;
>>>  	unsigned int			init_count;
>>>  	struct clk_hw_onecell_data	*hw_onecell_data;
>>> +	int				(*setup)(struct platform_device *pdev);
>>>  };
>>>  
>>>  int meson_eeclkc_probe(struct platform_device *pdev);
>>> -- 
>>> 2.21.0