On 10/03/17 11:39, Sakari Ailus wrote:
> Hi Hans,
>
> Thanks! It's very nice to see one more driver converted to stand-alone one!
>
> Speaking of which --- this seems to be the second last one. The only
> remaining one is sh_mobile_ceu_camera.c!
>
> On Mon, Mar 06, 2017 at 03:56:08PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> This patch converts the atmel-isi driver from a soc-camera driver to a driver
>> that is stand-alone.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> drivers/media/i2c/soc_camera/ov2640.c | 23 +-
>> drivers/media/platform/soc_camera/Kconfig | 3 +-
>> drivers/media/platform/soc_camera/atmel-isi.c | 1223 +++++++++++++++----------
>> 3 files changed, 735 insertions(+), 514 deletions(-)
>>
<snip>
>> +static int isi_graph_parse(struct atmel_isi *isi,
>> + struct device_node *node)
>
> Fits on a single line.
>
>> +{
>> + struct device_node *remote;
>> + struct device_node *ep = NULL;
>> + struct device_node *next;
>> + int ret = 0;
>> +
>> + while (1) {
>> + next = of_graph_get_next_endpoint(node, ep);
>> + if (!next)
>> + break;
>
> You could make this a while loop condition.
>
>> +
>> + of_node_put(ep);
>
> ep is put by of_graph_get_next_endpoint(), no need to do it manually here.
>
>> + ep = next;
>> +
>> + remote = of_graph_get_remote_port_parent(ep);
>> + if (!remote) {
>
> of_node_put(ep);
>
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + /* Skip entities that we have already processed. */
>> + if (remote == isi->dev->of_node) {
>> + of_node_put(remote);
>> + continue;
>> + }
>> +
>> + /* Remote node to connect */
>> + if (!isi->entity.node) {
>
> There would have to be something wrong about the DT graph for the two above
> to happen. I guess one could just return an error if something is terribly
> wrong.
>
> I'm thinking this from the point of view of making this function generic,
> and moving it to the framework as most drivers to something very similar,
> but I'd prefer to get the fwnode patches in first.
>
>> + isi->entity.node = remote;
>
> You could use entity.asd.match.of.node instead, and drop the node field.
Slight problem with this. If I make this change, then the of_node_put below
changes as well:
@@ -1193,7 +1176,7 @@ static int isi_graph_init(struct atmel_isi *isi)
done:
if (ret < 0) {
v4l2_async_notifier_unregister(&isi->notifier);
- of_node_put(isi->entity.node);
+ of_node_put(isi->entity.asd.match.of.node);
}
And I get this compiler warning:
CC [M] drivers/media/platform/atmel/atmel-isi.o
drivers/media/platform/atmel/atmel-isi.c: In function ‘isi_graph_init’:
drivers/media/platform/atmel/atmel-isi.c:1179:15: warning: passing argument 1 of ‘of_node_put’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
of_node_put(isi->entity.asd.match.of.node);
^~~
In file included from drivers/media/platform/atmel/atmel-isi.c:25:0:
./include/linux/of.h:130:20: note: expected ‘struct device_node *’ but argument is of type ‘const struct device_node *’
static inline void of_node_put(struct device_node *node) { }
^~~~~~~~~~~
Any suggestions? Just keep the entity.node after all?
Regards,
Hans
>
>> + isi->entity.asd.match_type = V4L2_ASYNC_MATCH_OF;
>> + isi->entity.asd.match.of.node = remote;
>> + ret++;
>> + }
>> + }
>> +
>> + of_node_put(ep);
>> +
>> + return ret;
>> +}
