From: Trent Piepho <tpiepho@impinj.com>
To: "alexandre.belloni@bootlin.com" <alexandre.belloni@bootlin.com>
Cc: "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
"a.zummo@towertech.it" <a.zummo@towertech.it>
Subject: Re: [PATCH 2/3] rtc: isl1208: Support more chip variations
Date: Tue, 12 Feb 2019 01:30:50 +0000 [thread overview]
Message-ID: <1549935049.3075.118.camel@impinj.com> (raw)
In-Reply-To: <20190210211239.GH23062@piout.net>
On Sun, 2019-02-10 at 22:12 +0100, Alexandre Belloni wrote:
> +};
> > +MODULE_DEVICE_TABLE(i2c, isl1208_id);
> > +
> > +static const struct of_device_id isl1208_of_match[] = {
> > + { .compatible = "isil,isl1208", .data = &isl1208_configs[TYPE_ISL1208] },
> > + { .compatible = "isil,isl1209", .data = &isl1208_configs[TYPE_ISL1209] },
>
> This compatible needs to be documented.
I see isil,isl1219 escaped documentation when it was added too. I'll
add both to rtc.txt.
> >
> > return handled ? IRQ_HANDLED : IRQ_NONE;
> > @@ -743,6 +778,19 @@ isl1208_probe(struct i2c_client *client, const struct i2c_device_id *id)
> > return -ENOMEM;
> > i2c_set_clientdata(client, isl1208);
> >
> > + /* Determine which chip we have */
> > + if (client->dev.of_node) {
> > + const struct of_device_id *match =
> > + of_match_node(isl1208_of_match, client->dev.of_node);
> > + if (!match)
>
> This will never happen, else the driver would not be probed.
If there was a driver_override ability with i2c, like PCI and SPI, then
it would be possible.
>
> > + return -EINVAL;
> > + isl1208->config = match->data;
>
> Maybe you should use of_device_get_match_data anyway.
Yes, of_device_get_match_data() would be better. Not sure why I didn't
find that.
Interestingly, I see could have just depended on id->driver_data being
set even in the OF case, as the i2c-core code always sets it. But it
look to be deprecated and i2c clients should provide a probe_new method
that does not provide an i2c_device_id. I'm not sure if that's
entirely better, as every i2c client that cares will need to re-
implement the code to look up by device name and/or OF node and/or
ACPI.
>
> > + } else {
> > + if (id->driver_data >= ISL_LAST_ID)
> > + return -EINVAL;
>
> This should be -ENODEV.
There's quite a few drivers that return EINVAL when the driver_data
appears to be incorrect. But some also return ENODEV.
next prev parent reply other threads:[~2019-02-12 1:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-01 19:42 [PATCH 1/3] rtc: isl1208: Introduce driver state struct Trent Piepho
2019-02-01 19:42 ` [PATCH 2/3] rtc: isl1208: Support more chip variations Trent Piepho
2019-02-10 21:12 ` Alexandre Belloni
2019-02-12 1:30 ` Trent Piepho [this message]
2019-02-01 19:42 ` [PATCH 3/3] rtc: isl1208: Add new style nvmem support to driver Trent Piepho
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=1549935049.3075.118.camel@impinj.com \
--to=tpiepho@impinj.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=linux-rtc@vger.kernel.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).