Re: [PATCH 11/15] media: atomisp: Replace atomisp_drvfs attr with using driver.dev_groups attr

[Hans de Goede <hdegoede@redhat.com>]

Hi Andy,

Thank you for all the reviews.

I don't see anything really problematic in your review,
so I hope that Mauro will honor my pull-request and
then I'll fix the small remarks in some follow-up patches.

One remark regarding your review of this patch below:

On 1/2/24 01:33, Andy Shevchenko wrote:
> On Sun, Dec 31, 2023 at 12:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> sysfs attributes preferably should not be manually be registered but
>> instead the driver.groups / driver.dev_groups driver struct members
>> should be used to have the driver core handle this in a race free
>> manner.
>>
>> Using driver.groups would be the most direct replacement for
>> driver_[add|remove]_file, but some of the attributes actually need access
> 
> ..._file()
> 
>> to the struct atomisp_device (*), so as part of modernizing this part of
>> the atomisp driver this change also makes the sysfs attribute device
>> attributes instead of driver attributes.
>>
>> *) Before this change accessing these attributes without the driver having
>> bound would result in a NULL pointer deref, this commit fixes this.
> 
> ...
> 
>> +       if (dbglvl < 1 || dbglvl > 9)
> 
> in_range() ?

ack.

> 
>>                 return -ERANGE;
> 
> ...
> 
>> +static const struct attribute_group dbg_attr_group = {
>> +       .attrs = dbg_attrs,
>> +};
>>
>> +const struct attribute_group *dbg_attr_groups[] = {
>> +       &dbg_attr_group,
>> +       NULL
>> +};
> 
> ATTRIBUTE_GROUPS()

I deliberately wrote this out (had to write this out)
instead of using ATTRIBUTE_GROUPS() because ATTRIBUTE_GROUPS()
makes the groups variable static and here it gets used
in another file then where it is declared.

> 
> ...
> 
>> +#include <linux/sysfs.h>
> 
> But why? You can use forward declaration, no?

True, I'll fix this up in a follow-up patch.

> 
>> +extern const struct attribute_group *dbg_attr_groups[];
> 


Regards,

Hans