Re: [PATCH net-next v2 3/3] net: phy: bcm54140: add hwmon support

[Michael Walle]

Am 2020-04-19 17:56, schrieb Andrew Lunn:
> On Sun, Apr 19, 2020 at 12:12:49PM +0200, Michael Walle wrote:
>
> Hi Michael
>
> You have an #if here...
>
> > +#if IS_ENABLED(CONFIG_HWMON)
> > +static umode_t bcm54140_hwmon_is_visible(const void *data,
> > +					 enum hwmon_sensor_types type,
> > +					 u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_in:
> > +		switch (attr) {
> > +		case hwmon_in_min:
> > +		case hwmon_in_max:
> > +			return 0644;
> > +		case hwmon_in_label:
> > +		case hwmon_in_input:
> > +		case hwmon_in_alarm:
> > +			return 0444;
> > +		default:
> > +			return 0;
> > +		}
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_min:
> > +		case hwmon_temp_max:
> > +			return 0644;
> > +		case hwmon_temp_input:
> > +		case hwmon_temp_alarm:
> > +			return 0444;
> > +		default:
> > +			return 0;
> > +		}
> > +	default:
> > +		return 0;
> > +	}
> > +}
>
> ...
>
>
> > +static const struct hwmon_chip_info bcm54140_chip_info = {
> > +	.ops = &bcm54140_hwmon_ops,
> > +	.info = bcm54140_hwmon_info,
> >  };
> >
> >  static int bcm54140_phy_base_read_rdb(struct phy_device *phydev, u16 
> > rdb)
> > @@ -203,6 +522,72 @@ static int bcm54140_get_base_addr_and_port(struct 
> > phy_device *phydev)
> >  	return 0;
> >  }
>
>
> Still inside the #if. Some original code is now inside the #if/#endif.
> Is this correct? Hard to see from just the patch.

Whoops you're correct, something got messed up here. Will
fix that in the next version.

-michael


> > +/* Check if one PHY has already done the init of the parts common to 
> > all PHYs
> > + * in the Quad PHY package.
> > + */
> > +static bool bcm54140_is_pkg_init(struct phy_device *phydev)
> > +{
> > +	struct bcm54140_phy_priv *priv = phydev->priv;
> > +	struct mii_bus *bus = phydev->mdio.bus;
> > +	int base_addr = priv->base_addr;
> > +	struct phy_device *phy;
> > +	int i;
> > +
>
> ...
>
> > +static int bcm54140_phy_probe_once(struct phy_device *phydev)
> > +{
> > +	struct device *hwmon;
> > +	int ret;
> > +
> > +	/* enable hardware monitoring */
> > +	ret = bcm54140_enable_monitoring(phydev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	hwmon = devm_hwmon_device_register_with_info(&phydev->mdio.dev,
> > +						     "BCM54140", phydev,
> > +						     &bcm54140_chip_info,
> > +						     NULL);
> > +	return PTR_ERR_OR_ZERO(hwmon);
> > +}
> > +#endif
>
>
> Thanks
>   Andrew