Re: [PATCH v6 4/8] iio: dac: adi-axi-dac: extend features

[Jonathan Cameron <jic23@kernel.org>]

> > > >  static int axi_dac_probe(struct platform_device *pdev)
> > > >  {
> > > > -	const unsigned int *expected_ver;
> > > >  	struct axi_dac_state *st;
> > > >  	void __iomem *base;
> > > >  	unsigned int ver;
> > > > @@ -566,15 +793,26 @@ static int axi_dac_probe(struct platform_device *pdev)
> > > >  	if (!st)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	expected_ver = device_get_match_data(&pdev->dev);
> > > > -	if (!expected_ver)
> > > > +	st->info = device_get_match_data(&pdev->dev);
> > > > +	if (!st->info)
> > > >  		return -ENODEV;
> > > >  
> > > > -	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > > +	clk = devm_clk_get_enabled(&pdev->dev, "s_axi_aclk");  
> > > 
> > > This will break existing users that don't have clock-names
> > > in the DT. It should be fine to leave it as NULL in which
> > > case it will get the clock at index 0 in the clocks array
> > > even if there is more than one clock.
> > >   
> > 
> > mm, are there existing users except this hs driver right now ?
> > 
> > Clock names are actually described in the example, and if missing,
> > also retrieving "dac_clk" would fail.
> >   
> 
> There's already a frontend DAC using the generic DAC implementation. So, in theory,
> yes... We can already have users not setting clock-names in DT that would now fail to
> probe with this patch. David is only suggesting leaving this call to NULL. For
> dac_clk we do need the *id in devm_clk_get_enabled().
> 
> Maybe it would also be worth mentioning in the bindings that s_axi_aclk needs to be
> the first entry in clocks and clock-names for backward compatibility.

Usual trick for this is match on clk name first and then fallback to no name
to pick up old DT that didn't set clk names.

That way should be no need constrain the order when it is specified.
Slight hicup is new DT, old kernel. In which case maybe the wrong clock is started
but I think you only have the multiple clocks for new cases so this should be fine.

Just sprinkle some comments alongside the fallback code to say why it is there.


Jonathan

> 
> - Nuno Sá
> > >   
>