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
