Re: [PATCH 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

["Pali Rohár" <pali@kernel.org>]

On Saturday 06 January 2024 13:13:01 Hans de Goede wrote:
> Hi,
> 
> On 1/5/24 19:26, Pali Rohár wrote:
> > On Friday 05 January 2024 17:31:32 Hans de Goede wrote:
> >> Hi Pali,
> >>
> >> On 12/24/23 22:55, Pali Rohár wrote:
> >>> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
> >>>> +static int smo8800_find_i801(struct device *dev, void *data)
> >>>> +{
> >>>> +	static const u16 i801_idf_pci_device_ids[] = {
> >>>> +		0x1d70, /* Patsburg (PCH) */
> >>>> +		0x1d71, /* Patsburg (PCH) */
> >>>> +		0x1d72, /* Patsburg (PCH) */
> >>>> +		0x8d7d, /* Wellsburg (PCH) */
> >>>> +		0x8d7e, /* Wellsburg (PCH) */
> >>>> +		0x8d7f, /* Wellsburg (PCH) */
> >>>> +	};
> >>>
> >>> I'm not happy with seeing another hardcoded list of device ids in the
> >>> driver. Are not we able to find compatible i801 adapter without need to
> >>> hardcode this list there in smo driver?
> >>
> >> I agree that having this hardcoded is not ideal.
> >>
> >> The problem is that for a couple of generations (Patsburg is for
> >> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell)
> >> intel had multiple i2c-i801 controllers / I2C-busses in the PCH
> >> and the i2c_client needs to be instantiated on the primary
> >> i2c-i801 (compatible) controller.
> >>
> >> Luckily Intel has only done this for these 2 generations PCH
> >> all older and newer PCHs only have 1 i2c-i801 I2C bus.
> >>
> >> So even though having this hardcoded is not ideal,
> >> the list should never change since it is only for
> >> this 2 somewhat old PCH generations and new generations
> >> are not impacted. So I believe that this is the best
> >> solution.
> > 
> > I see. Seems that this is the best solution which we have.
> > 
> > Anyway, is not possible to use pci_dev_driver() to find i801 driver and
> > from it takes that list of devices which have FEATURE_IDF flag? I have
> > looked at the code only quickly and maybe it could be possible. Just an
> > idea.
> 
> That is an interesting idea, but ...
> 
> that would mean interpreting the driver_data set by the i2c-i801
> driver inside the dell-smo8800 code, so this would basically rely on
> the meaning of that driver_data never changing. I would rather just
> duplicate the 6 PCI ids and decouple the 2 drivers.

It was just an alternative idea.

In case of code duplication I would suggest to write a comment on both
places (into i801 and smo drivers) that this information is duplicated
in both drivers and should be synchronized in case somebody would need
to modify it at either place.

> Regards,
> 
> Hans
> 
> 
> 
> 
> >>>> +	struct i2c_adapter *adap, **adap_ret = data;
> >>>> +	struct pci_dev *pdev;
> >>>> +	int i;
> >>>> +
> >>>> +	adap = i2c_verify_adapter(dev);
> >>>> +	if (!adap)
> >>>> +		return 0;
> >>>> +
> >>>> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> >>>> +		return 0;
> >>>> +
> >>>> +	/* The parent of an I801 adapter is always a PCI device */
> >>>> +	pdev = to_pci_dev(adap->dev.parent);
> >>>> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
> >>>> +		if (pdev->device == i801_idf_pci_device_ids[i])
> >>>> +			return 0; /* Only register client on main SMBus channel */
> >>>> +	}
> >>>> +
> >>>> +	*adap_ret = i2c_get_adapter(adap->nr);
> >>>> +	return 1;
> >>>> +}
> >>>
> >>
> > 
>