Hi,
On 7/5/23 12:38, Sakari Ailus wrote:
> Hi Hans,
>
> On Tue, Jul 04, 2023 at 09:21:47PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 6/30/23 16:45, Andy Shevchenko wrote:
>>>> + sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
>>>> + if (sensor->lanes > IPU_MAX_LANES) {
>>>> + dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
>>>
>>> Yeah, I think we would be consistent in using the ACPI handle to print
>>> the messages from ACPI sensor devices.
>>
>> I do agree that we need to be consistent, but I regret having switched
>> to using the handle for this in the csi2-bridge code. The dmesg logs
>> this results in are much harder to read. Most devices typically have
>> 2 different sensors and normally it is quite easy to see in the logs
>> which GPIOs, etc. are being used for the sensor.
>>
>> But after the move to using the ACPI handle for logging the logs
>> show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
>> OVTI2680 and INT0310 making it much harder to figure on what
>> is going on (first need to do
>> "cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
>> to find out which path belongs to which sensor).
>
> In cases such as the above, the developer probably needs to address issues
> not in the sensor driver but in the ACPI tables (or in IPU bridge code). So
> for this reason I'd prefer printing the device path instead of the HID
> (which is also somewhat opaque).
>
>>
>> So I would rather get rid of the handle based logging, because it
>> is very cumbersome to use.
>
> The V4L2 async and fwnode frameworks use handles, too, for the same reason.
>
> That said, I don't mind printing device names either. AFAIR Laurent
> actually proposed that recently for the V4L2 fwnode and even promised to
> send a patch. :-)
Hmm, ok. I'll keep the acpi_handle logging then and add a " %s:", dev_name() to
the logs so that we log both the ACPI handle path and the dev-name / HID.
>> I'll add an extra patch to the next version of the set to switch all
>> the logging to using the acpi_device for logging.
So this extra patch is going to add logging of the dev_name() instead then.
Regards,
Hans
