Re: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[Greg KH]

On Wed, Mar 15, 2023 at 09:07:12AM +0000, VaibhaavRam.TL@xxxxxxxxxxxxx wrote:
> 
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 
> Sent: Monday, March 13, 2023 9:47 PM
> To: VaibhaavRam TL - I69105 <VaibhaavRam.TL@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch
> 
> 
> On Mon, Mar 13, 2023 at 04:01:15PM +0000, VaibhaavRam.TL@xxxxxxxxxxxxx wrote:
> > > > Again, default group will handle this automatically, you should 
> > > > never need to call a sysfs_*() call from a driver.  Otherwise something is usually very wrong.
> > >
> > > Are you recommending similar to this snippet?
> > >
> > > static struct bin_attribute *pci1xxxx_bin_attributes[] = {
> > >       &pci1xxxx_otp_attr,
> > >       &pci1xxxx_eeprom_attr
> > >       NULL,
> > > };
> > >
> > > static const struct attribute_group pci1xxxx_bin_attributes_group = {
> > >       .bin_attrs = pci1xxxx_bin_attributes, }; ..
> > > ..
> > > auxiliary_device.device.attribute_group = 
> > > pci1xxxx_bin_attributes_group
> >
> > Yes.
> >
> > > This creates sysfs for both EEPROM and OTP at once and handles for its removal, right?
> > > But, In this case, I have to check whether EEPROM is responsive and only then create sysfs for it.
> > >
> > > Can you please provide some guidance, on how to handle this situation without using sysfs_*().
> >
> > Use the "is_visible" callback in your group to tell the driver core if the specific attribute needs to be created or not.
> 
> I have added "is_bin_visible" callback and it is working fine. Sysfs for EEPROM and OTP is created inside drivers folder

It should be for the device, not the driver, are you sure you are using
the right groups pointer?

> But I have used attribute group inside device_driver under auxiliary_driver structure.
> as opposed to what I have mentioned previously because , struct device_driver is exposed to me instead of struct device.
> Since there can be only one instance of driver for multiple devices, I cannot account for multiple instances of EEPROM/OTPs present in those devices.

Then your driver might be structured incorrectly, as the instance of the
device is what your driver binds to.

> Is it possible to use sysfs_create_group in this situation?

You should not need that at all, I really don't understand the problem,
sorry, can you post a patch that shows the issue?

thanks,

greg k-h