Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Feedback-ID: i0ad843c9:Fastmail
Message-ID: <f5468eac-96ec-e8c9-bcc1-9ec219bf3352@sholland.org>
Date:   Sun, 29 Oct 2023 13:37:25 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux ppc64le; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Content-Language: en-US
From:   Samuel Holland <samuel@sholland.org>
To:     Lee Jones <lee@kernel.org>
Cc:     Pavel Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org,
        Chen-Yu Tsai <wens@csie.org>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        linux-kernel@vger.kernel.org, linux-sunxi@lists.linux.dev
References: <20221231235541.13568-1-samuel@sholland.org>
 <20221231235541.13568-3-samuel@sholland.org>
 <20230316133422.GM9667@google.com>
 <a1adbe50-3400-fd10-1856-8c1d0ed82276@sholland.org>
Subject: Re: [RESEND PATCH v7 2/5] leds: sun50i-a100: New driver for the A100
 LED controller
In-Reply-To: <a1adbe50-3400-fd10-1856-8c1d0ed82276@sholland.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Lee,

On 10/17/23 19:38, Samuel Holland wrote:
> On 3/16/23 08:34, Lee Jones wrote:
>> On Sat, 31 Dec 2022, Samuel Holland wrote:
>>> +	for_each_available_child_of_node(np, child) {
>>> +		struct sun50i_a100_ledc_led *led;
>>> +		struct led_classdev *cdev;
>>> +		u32 addr, color;
>>> +
>>> +		ret = of_property_read_u32(child, "reg", &addr);
>>> +		if (ret || addr >= count) {
>>> +			dev_err(dev, "LED 'reg' values must be from 0 to %d\n",
>>
>> Doesn't sounds like an address.
> 
> The one-wire protocol involves the first LED responding to the first 24
> bits of the transfer, then forwarding the rest to the next LED. The
> address is the ordinal position in the chain. So I don't think there is
> any reason to have gaps in the addresses--the LEDs would still have to
> physically be there, but you would not be able to control them. That
> said, the driver doesn't need to enforce this, so I can remove the check.

There's actually a reason for the driver enforcing that LED addresses
are contiguous. Removing this check decouples the largest address from
the number of LED class devices. Unfortunately, there's a register field
(LEDC_RESET_TIMING_CTRL_REG_LED_NUM) that must be set to the largest
address for transfers to work correctly.

This means the driver would need to iterate through the child nodes in
two passes inside the probe function: first to find the largest reg
value, and second to actually register the LED class devices.

Since I don't think having gaps in the addresses is a realistic use
case, I'd like to keep this restriction for now (with a comment). We
could always pay the complexity cost later if someone really does want
to relax this check.

Regards,
Samuel