Re: [PATCH anybus v2 1/5] misc: support the Arcx anybus bridge.

[Sven Van Asbroeck]

Hi Linus,

> This is fun :)

It sure is ! It's fascinating to see how the kernel abstractions are designed,
and how code is reviewed here.

>> +static DEVICE_ATTR_RO(version);
>
> Do you need this in userspace really?
>
>> +static DEVICE_ATTR_RO(design_number);
>
> And this?

Unfortunately, I do :(
The application software reads these out and displays them in an UI. It's
important to be able to see these on a running device.
Perhaps there is another kernel abstraction I could use?

> This:
>>
>> +       cd->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
>> +       if (!gpio_is_valid(cd->reset_gpio)) {
>> +               dev_err(dev, "reset-gpios not found\n");
>> +               return -EINVAL;
>> +       }
>> +       devm_gpio_request(dev, cd->reset_gpio, NULL);
>> +       gpio_direction_output(cd->reset_gpio, 0);
>
> Should be:
>
> cd->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(cd->reset_gpiod))
>     return PTR_ERR(cd->reset_gpiod);

That's actually pretty neat !
Now we have nothing that explicitly refers to the devicetree (except the
device binding, of course). I now see the point you made earlier:
resets, gpios, interrupts etc are associated with "struct device" and
not necessarily with the devicetree.

The rest of your suggestions have already gone in.

I hope you'll have some time later to look at the architecture description
in the bus driver. Looking forward to your feedback and to the next iteration !

Cheers,
Sven