Message-ID: <20180515071036.GM5956@w540>
Date:   Tue, 15 May 2018 09:10:36 +0200
From:   jacopo mondi <jacopo@...ndi.org>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>, hans.verkuil@...co.com,
        mchehab@...nel.org, robh+dt@...nel.org,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] media: i2c: mt9t112: Add device tree support

Hi Sakari,

On Tue, May 15, 2018 at 12:50:04AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, May 14, 2018 at 04:30:44PM +0200, jacopo mondi wrote:
> > Hi Sakari,
> >
> > On Mon, May 07, 2018 at 12:32:19PM +0300, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Wed, Apr 25, 2018 at 01:00:14PM +0200, Jacopo Mondi wrote:
> >
> > [snip]
> >
> > > >  static int mt9t112_probe(struct i2c_client *client,
> > > >  			 const struct i2c_device_id *did)
> > > >  {
> > > >  	struct mt9t112_priv *priv;
> > > >  	int ret;
> > > >
> > > > -	if (!client->dev.platform_data) {
> > > > +	if (!client->dev.of_node && !client->dev.platform_data) {
> > > >  		dev_err(&client->dev, "mt9t112: missing platform data!\n");
> > > >  		return -EINVAL;
> > > >  	}
> > > > @@ -1081,23 +1118,39 @@ static int mt9t112_probe(struct i2c_client *client,
> > > >  	if (!priv)
> > > >  		return -ENOMEM;
> > > >
> > > > -	priv->info = client->dev.platform_data;
> > > >  	priv->init_done = false;
> > > > -
> > > > -	v4l2_i2c_subdev_init(&priv->subdev, client, &mt9t112_subdev_ops);
> > > > -
> > > > -	priv->clk = devm_clk_get(&client->dev, "extclk");
> > > > -	if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > +	priv->dev = &client->dev;
> > > > +
> > > > +	if (client->dev.platform_data) {
> > > > +		priv->info = client->dev.platform_data;
> > > > +
> > > > +		priv->clk = devm_clk_get(&client->dev, "extclk");
> > >
> > > extclk needs to be documented in DT binding documentation.
> > >
> > > > +		if (PTR_ERR(priv->clk) == -ENOENT) {
> > > > +			priv->clk = NULL;
> > > > +		} else if (IS_ERR(priv->clk)) {
> > > > +			dev_err(&client->dev,
> > > > +				"Unable to get clock \"extclk\"\n");
> > > > +			return PTR_ERR(priv->clk);
> > > > +		}
> > > > +	} else {
> > > > +		/*
> > > > +		 * External clock frequencies != 24MHz are only supported
> > > > +		 * for non-OF systems.
> > > > +		 */
> > >
> > > Shouldn't you actually set the frequency? Or perhaps even better to check
> > > it, and use assigned-clocks and assigned-clock-rates properties?
> > >
> >
> > I might be confused, but my intention was to use an external clock
> > reference, with a configurable frequency only in the platform data use
> > case. As you can see in this 'else' branch, in OF case, the priv->clk
> > field is null, and all the PLL and clock computations are performed
> > assuming a 24MHz input clock.
> >
> > In my opinion, as the driver when running on OF systems does not
> > get any reference to 'extclk' clock, it should not be documented in
> > bindings. Do you agree?
>
> Uh, isn't the clock generally controlled by the driver on OF-based systems?
> You could assign the frequency in DT though, and not in the driver, but
> that should be documented in binding documentation.
>
> The register configuration the driver does not appear to be dependent on
> the clock frequency, which suggests that it is only applicable to a
> particular frequency --- 24 MHz?

Correct.
That's what the comment above here states...

	/*
	 * External clock frequencies != 24MHz are only supported
	 * for non-OF systems.
	 */

That's how it works: the driver expects to receive the PLL dividers
from platform data. It's ugly, I agree, but that's how it was. I do
not have time atm to poke around with PLL configuration, and I'm not
even sure I have the right documentation, so I made a 'default PLL
configuration' for 24MHz clock, copied from the platform data supplied
to the driver by the only user in mainline. In general, I would like
to have the driver calculate the PLL dividers based on the input clock
frequency, that should be supplied from DT. As this is not possible,
at the moment I made the driver only provide a PLL configuration for
24MHz, and that's the only clock the driver accepts.

 +		priv->info = &mt9t112_default_pdata_24MHz;

Would you prefer I take a reference to an external clock, check it's
frequency and refuse it if != 24MHz?

By the way, I'm trying to run this driver with a camera module
connected to an ARM platform and so far I have not been able to
capture any image. The Ecovec I have (thanks Hans) has a camera module but the
cable is bad, so I can't test it on the platform the driver has
originally been developed on, but I assume on SH Ecovec it works properly.
Any confirmation from someone who has that board would be appreciate
though.

Thanks
   j

>
> >
> > Thanks
> >    j
> >
> > > >  		priv->clk = NULL;
> > > > -	} else if (IS_ERR(priv->clk)) {
> > > > -		dev_err(&client->dev, "Unable to get clock \"extclk\"\n");
> > > > -		return PTR_ERR(priv->clk);
> > > > +		priv->info = &mt9t112_default_pdata_24MHz;
> > > > +
> > > > +		ret = mt9t112_parse_dt(priv);
> > > > +		if (ret)
> > > > +			return ret;
> > > >  	}
> > > >
> > > > -	priv->standby_gpio = devm_gpiod_get_optional(&client->dev, "standby",
> > > > +	v4l2_i2c_subdev_init(&priv->subdev, client, &mt9t112_subdev_ops);
> > > > +
> > > > +	priv->standby_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",
> > > >  						     GPIOD_OUT_HIGH);
> > > >  	if (IS_ERR(priv->standby_gpio)) {
> > > > -		dev_err(&client->dev, "Unable to get gpio \"standby\"\n");
> > > > +		dev_err(&client->dev, "Unable to get gpio \"powerdown\"\n");
> > > >  		return PTR_ERR(priv->standby_gpio);
> > > >  	}
> > > >
> > > > @@ -1124,9 +1177,19 @@ static const struct i2c_device_id mt9t112_id[] = {
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, mt9t112_id);
> > > >
> > > > +#if IS_ENABLED(CONFIG_OF)
> > > > +static const struct of_device_id mt9t112_of_match[] = {
> > > > +	{ .compatible = "micron,mt9t111", },
> > > > +	{ .compatible = "micron,mt9t112", },
> > > > +	{ /* sentinel */ },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mt9t112_of_match);
> > > > +#endif
> > > > +
> > > >  static struct i2c_driver mt9t112_i2c_driver = {
> > > >  	.driver = {
> > > >  		.name = "mt9t112",
> > > > +		.of_match_table = of_match_ptr(mt9t112_of_match),
> > >
> > > No need to use of_match_ptr().
> > >
> > > >  	},
> > > >  	.probe    = mt9t112_probe,
> > > >  	.remove   = mt9t112_remove,
> > >
> > > --
> > > Sakari Ailus
> > > e-mail: sakari.ailus@....fi
>
>
>
> --
> Sakari Ailus
> e-mail: sakari.ailus@....fi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists