LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: jacopo mondi <jacopo@jmondi.org>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	hans.verkuil@cisco.com, mchehab@kernel.org, robh+dt@kernel.org,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] media: i2c: mt9t112: Add device tree support
Date: Tue, 15 May 2018 09:10:36 +0200	[thread overview]
Message-ID: <20180515071036.GM5956@w540> (raw)
In-Reply-To: <20180514215004.5oy6jr7f32jpfhx2@valkosipuli.retiisi.org.uk>

[-- Attachment #1: Type: text/plain, Size: 6333 bytes --]

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@iki.fi
>
>
>
> --
> Sakari Ailus
> e-mail: sakari.ailus@iki.fi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

      reply	other threads:[~2018-05-15  7:10 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25 11:00 [PATCH 0/2] media: i2c: mt9t112: Add OF " Jacopo Mondi
2018-04-25 11:00 ` [PATCH 1/2] dt-bindings: media: i2c: Add mt9t111 image sensor Jacopo Mondi
2018-05-01 14:28   ` Rob Herring
2018-05-07  9:17   ` Sakari Ailus
2018-04-25 11:00 ` [PATCH 2/2] media: i2c: mt9t112: Add device tree support Jacopo Mondi
2018-05-07  9:32   ` Sakari Ailus
2018-05-14 14:30     ` jacopo mondi
2018-05-14 21:50       ` Sakari Ailus
2018-05-15  7:10         ` jacopo mondi [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180515071036.GM5956@w540 \
    --to=jacopo@jmondi.org \
    --cc=devicetree@vger.kernel.org \
    --cc=hans.verkuil@cisco.com \
    --cc=jacopo+renesas@jmondi.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=sakari.ailus@iki.fi \
    --subject='Re: [PATCH 2/2] media: i2c: mt9t112: Add device tree support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).