On Mon, Aug 22, 2016 at 3:54 PM, Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
> On 22 August 2016 at 15:15, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>> On Fri, Aug 19, 2016 at 12:53 PM, Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
>>
>>> Add a pincontrol driver for BCM6328. BCM628 supports muxing 32 pins as
>>> GPIOs, as LEDs for the integrated LED controller, or various other
>>> functions. Its pincontrol mux registers also control other aspects, like
>>> switching the second USB port between host and device mode.
>>>
>>> Signed-off-by: Jonas Gorski <jonas.gorski@xxxxxxxxx>
>>
>> This looks good. Just thinking following the other patches
>> that maybe this should be a syscon child driver.
>>
>>> +#define BCM6328_NGPIO 32
>>
>> I wonder why the pin control driver cares about that?
>>
>>> +struct bcm6328_pinctrl {
>>> + struct pinctrl_dev *pctldev;
>>> + struct pinctrl_desc desc;
>>> +
>>> + void __iomem *mode;
>>> + void __iomem *mux[3];
>>> +
>>> + /* register access lock */
>>> + spinlock_t lock;
>>> +
>>> + struct gpio_chip gpio;
>>
>> Usually this should be the other way around: the gpio_chip
>> knows about the pin controller, the pin controller does not
>> know about the gpio_chip.
>>
>> I don't see it used in the code either? Artifact?
>
> That's because some of the drivers require it, and kept it here so to
> keep the drivers as similar as possible.
No dead code please. I think that is just confusing.
>> Why all this __raw_* accessors? What is wrong with readl_relaxed()
>> and writel_relaxed()? Or MIPS doesn't have them?
>
> BCM63XX is one of those weird targets that are usually big endian, and
> there are no native endian / big endian *_relaxed() accessors.
Aha OK.
>> Nice, but why not just add
>> depends on OF
>> to the Kconfig and get rid of the #ifdef?
>
> As mentioned for the generic part, I want to use it on a !OF target as well.
OK.
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mode");
>>> + mode = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(mode))
>>> + return PTR_ERR(mode);
>>> +
>>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mux");
>>> + mux = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(mux))
>>> + return PTR_ERR(mux);
>>
>> This mishmash of remap windows makes me prefer the syscon
>> design pattern.
>
> At least for this one I would still need to have multiple syscons
Having multiple syscons is not a problem.
> because there already is one OF enabled driver that uses a non-syscon
> access to drivers within this range.
I don't understand this. Can you explain? If another driver is using
a non-syscon pattern then surely it can be refactored due to being
done the wrong way in the first place. I have done so myself when
running into similar issues.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
